Potential memory leak in htab_map_update_elem?

Potential memory leak in htab_map_update_elem?

[Kris Van Hees]

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.

	Cheers,
	Kris

Re: Potential memory leak in htab_map_update_elem?

[Daniel Borkmann]

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.

Thanks,
Daniel

Re: Potential memory leak in htab_map_update_elem?

[Kris Van Hees]

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