From owner-freebsd-net@freebsd.org Fri May 18 06:53:24 2018 Return-Path: Delivered-To: freebsd-net@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 00814EEC858 for ; Fri, 18 May 2018 06:53:24 +0000 (UTC) (envelope-from lstewart@freebsd.org) Received: from lauren.room52.net (lauren.room52.net [45.63.28.37]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 4320169D6E; Fri, 18 May 2018 06:53:23 +0000 (UTC) (envelope-from lstewart@freebsd.org) Received: by lauren.room52.net (Postfix) with ESMTPSA id 4F7D2FF2; Fri, 18 May 2018 16:46:19 +1000 (AEST) Subject: Re: Bug: Newreno; Seems Memory leak in newreno_cb_init From: Lawrence Stewart To: Tom Jones , Harsh Jain Cc: freebsd-net@freebsd.org, Navdeep Parhar , sonyarpitad@chelsio.com, John Baldwin References: <58ab6c13-f9c4-b4d7-7f2c-eade3749457f@chelsio.com> <20180508200439.GA32339@tom-desk.erg.abdn.ac.uk> <6c84c94a-9d16-db30-f8d5-c6b364cd9469@freebsd.org> Message-ID: <6e4d875e-15db-51f9-2031-82b2bb631f08@freebsd.org> Date: Fri, 18 May 2018 16:46:18 +1000 User-Agent: Not your concern MIME-Version: 1.0 In-Reply-To: <6c84c94a-9d16-db30-f8d5-c6b364cd9469@freebsd.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 18 May 2018 06:53:24 -0000 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