From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kris Van Hees Subject: Re: Potential memory leak in htab_map_update_elem? Date: Fri, 11 Jan 2019 10:20:58 -0500 Message-ID: <20190111152058.GC11821@oracle.com> References: <20190111050804.GB11821@oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Kris Van Hees , netdev@vger.kernel.org, ast@kernel.org To: Daniel Borkmann Return-path: Received: from aserp2130.oracle.com ([141.146.126.79]:41752 "EHLO aserp2130.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727821AbfAKPVV (ORCPT ); Fri, 11 Jan 2019 10:21:21 -0500 Content-Disposition: inline In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: On Fri, Jan 11, 2019 at 09:12:57AM +0100, Daniel Borkmann wrote: > Hi Kris, > > On 01/11/2019 06:08 AM, Kris Van Hees wrote: > > Maybe I am missing something trivial here, but it looks to me that there is > > a leak of htab elements in htab_map_update_elem when you are updating an > > existing element. After the new element is linked into the bucket list, the > > following code snippet is found: > > > > if (l_old) { > > hlist_nulls_del_rcu(&l_old->hash_node); > > if (!htab_is_prealloc(htab)) > > free_htab_elem(htab, l_old); > > } > > > > Nothing is done with l_old in the remainder of the function, and to me this > > looks like that element is be leaked if the htab is preallocated because we > > never add it to the free list. In fact, free_htab_elem() contains the very > > conditional that handles the two cases (preallocated vs non-preallocated. > > In this case in alloc_htab_elem() we are swapping out the per-cpu extra element > with the existing one to avoid freelist_pop/push combination in order to have > a fast replace, meaning we cannot call free_htab_elem() on it like in plain > htab_map_delete_elem() case. Aha, and since it is a per-cpu extra element, you never run into a problem with two updates using this trick because only one update can ever be ongoing per cpu. Nifty! Thanks for explaining! Kris