Bug: Newreno; Seems Memory leak in newreno_cb_init

[Lawrence Stewart]

On 09/05/2018 11:00, Lawrence Stewart wrote:
> On 09/05/2018 06:04, Tom Jones wrote:
>> On Tue, May 08, 2018 at 05:14:49PM +0530, Harsh Jain wrote:
>>> Hi All,
>>>
>>> We have observed memory leak with TCP network traffic in "newreno".
>>>
>>> Output of vmstat -m
>>>
>>>    in_mfilter     3     3K       -        3  1024
>>>      in_multi     4     1K       -        4  256
>>>   ip_moptions     6     1K       -        6  64,256
>>> encap_export_host     2     2K       -        2  1024
>>>  newreno data 394849273 6169520K       - 394849273  16
>>>     sctp_a_it     0     0K       -        5  16
>>>      sctp_vrf     1     1K       -        1  64
>>>      sctp_ifa     7     1K       -        7  128
>>>      sctp_ifn     4     1K       -        4  128
>>>
>>> There is 1 malloc in "newreno_cb_init" whose pointer is not saved in any global structure to free the same.
>>>
>>> Is this a BUG?
>>
>>
>> Hi Harsh,
>>
>> Adding Lawrence in cc
>>
>> It looks like it, running nc in a loop I can watch MemUse grow. 
>>
>> I think this should address the leak
>>
>> https://reviews.freebsd.org/D15358
> 
> I'm not clear why yet, but the patch I ultimately committed as r331214
> is deeply flawed on account of missing memory allocation and other
> changes that never ended up in the working copy I committed from. The
> cb_destroy() change for example exists in the D11616 Phabricator review
> though. I think I may have refined the final patch and committed from a
> working copy that started with an older stale version of the patch. Ugh.
> 
> Mea culpa, thanks for the bug report, and apologies for the oversight.
> Will work with Tom to get this fixed.


Forgot to mention, this should be fixed with the commit of r333699:


> Author: lstewart
> Date: Thu May 17 02:46:27 2018
> New Revision: 333699
> URL: https://svnweb.freebsd.org/changeset/base/333699
> 
> Log:
>   Plug a memory leak and potential NULL-pointer dereference introduced in r331214.
>   
>   Each TCP connection that uses the system default cc_newreno(4) congestion
>   control algorithm module leaks a "struct newreno" (8 bytes of memory) at
>   connection initialisation time. The NULL-pointer dereference is only germane
>   when using the ABE feature, which is disabled by default.
>   
>   While at it:
>   
>   - Defer the allocation of memory until it is actually needed given that ABE is
>     optional and disabled by default.
>   
>   - Document the ENOMEM errno in getsockopt(2)/setsockopt(2).
>   
>   - Document ENOMEM and ENOBUFS in tcp(4) as being synonymous given that they are
>     used interchangeably throughout the code.
>   
>   - Fix a few other nits also accidentally omitted from the original patch.
>   
>   Reported by:	Harsh Jain on freebsd-net@
>   Tested by:	tjh@
>   Differential Revision:	https://reviews.freebsd.org/D15358
> 
> Modified:
>   head/lib/libc/sys/getsockopt.2
>   head/share/man/man4/tcp.4
>   head/sys/netinet/cc/cc_newreno.c