netdev - Re: Potential memory leak in htab_map_update

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Fri, 11 Jan 2019 10:20:58 -0500
From:   Kris Van Hees <kris.van.hees@...cle.com>
To:     Daniel Borkmann <daniel@...earbox.net>
Cc:     Kris Van Hees <kris.van.hees@...cle.com>, netdev@...r.kernel.org,
        ast@...nel.org
Subject: Re: Potential memory leak in htab_map_update_elem?

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