Re: [PATCH bpf-next v3 09/13] bpf: Wire up freeing of referenced kptr

[Kumar Kartikeya Dwivedi <memxor@gmail.com>]

On Wed, Mar 23, 2022 at 02:21:56AM IST, Andrii Nakryiko wrote:
> On Sun, Mar 20, 2022 at 8:55 AM Kumar Kartikeya Dwivedi
> <memxor@gmail.com> wrote:
> >
> > A destructor kfunc can be defined as void func(type *), where type may
> > be void or any other pointer type as per convenience.
> >
> > In this patch, we ensure that the type is sane and capture the function
> > pointer into off_desc of ptr_off_tab for the specific pointer offset,
> > with the invariant that the dtor pointer is always set when 'kptr_ref'
> > tag is applied to the pointer's pointee type, which is indicated by the
> > flag BPF_MAP_VALUE_OFF_F_REF.
> >
> > Note that only BTF IDs whose destructor kfunc is registered, thus become
> > the allowed BTF IDs for embedding as referenced kptr. Hence it serves
> > the purpose of finding dtor kfunc BTF ID, as well acting as a check
> > against the whitelist of allowed BTF IDs for this purpose.
> >
> > Finally, wire up the actual freeing of the referenced pointer if any at
> > all available offsets, so that no references are leaked after the BPF
> > map goes away and the BPF program previously moved the ownership a
> > referenced pointer into it.
> >
> > The behavior is similar to BPF timers, where bpf_map_{update,delete}_elem
> > will free any existing referenced kptr. The same case is with LRU map's
> > bpf_lru_push_free/htab_lru_push_free functions, which are extended to
> > reset unreferenced and free referenced kptr.
> >
> > Note that unlike BPF timers, kptr is not reset or freed when map uref
> > drops to zero.
> >
> > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> > ---
> >  include/linux/bpf.h   |  4 ++
> >  include/linux/btf.h   |  2 +
> >  kernel/bpf/arraymap.c | 14 ++++++-
> >  kernel/bpf/btf.c      | 86 ++++++++++++++++++++++++++++++++++++++++++-
> >  kernel/bpf/hashtab.c  | 29 ++++++++++-----
> >  kernel/bpf/syscall.c  | 57 +++++++++++++++++++++++++---
> >  6 files changed, 173 insertions(+), 19 deletions(-)
> >
>
> [...]
>
> > +                       /* This call also serves as a whitelist of allowed objects that
> > +                        * can be used as a referenced pointer and be stored in a map at
> > +                        * the same time.
> > +                        */
> > +                       dtor_btf_id = btf_find_dtor_kfunc(off_btf, id);
> > +                       if (dtor_btf_id < 0) {
> > +                               ret = dtor_btf_id;
> > +                               btf_put(off_btf);
>
> do btf_put() in end section instead of copy/pasting it in every single
> branch here and below?
>

Ok.

> > +                               goto end;
> > +                       }
> > +
> > +                       dtor_func = btf_type_by_id(off_btf, dtor_btf_id);
> > +                       if (!dtor_func || !btf_type_is_func(dtor_func)) {
> > +                               ret = -EINVAL;
> > +                               btf_put(off_btf);
> > +                               goto end;
> > +                       }
> > +
>
> [...]
>
> > -       while (tab->nr_off--)
> > +       while (tab->nr_off--) {
> >                 btf_put(tab->off[tab->nr_off].btf);
> > +               if (tab->off[tab->nr_off].module)
> > +                       module_put(tab->off[tab->nr_off].module);
> > +       }
> >         kfree(tab);
> >         return ERR_PTR(ret);
> >  }
> > diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
> > index 65877967f414..fa4a0a8754c5 100644
> > --- a/kernel/bpf/hashtab.c
> > +++ b/kernel/bpf/hashtab.c
> > @@ -725,12 +725,16 @@ static int htab_lru_map_gen_lookup(struct bpf_map *map,
> >         return insn - insn_buf;
> >  }
> >
> > -static void check_and_free_timer(struct bpf_htab *htab, struct htab_elem *elem)
> > +static void check_and_free_timer_and_kptr(struct bpf_htab *htab,
>
> we'll need to rename this to
> check_and_free_timer_and_kptrs_and_dynptrs() pretty soon, so let's
> better figure out more generic name now? :)
>
> Don't know, something like "release_fields" or something?
>

Ok, will change.

> > +                                         struct htab_elem *elem,
> > +                                         bool free_kptr)
> >  {
> > +       void *map_value = elem->key + round_up(htab->map.key_size, 8);
> > +
> >         if (unlikely(map_value_has_timer(&htab->map)))
> > -               bpf_timer_cancel_and_free(elem->key +
> > -                                         round_up(htab->map.key_size, 8) +
> > -                                         htab->map.timer_off);
> > +               bpf_timer_cancel_and_free(map_value + htab->map.timer_off);
> > +       if (unlikely(map_value_has_kptr(&htab->map)) && free_kptr)
> > +               bpf_map_free_kptr(&htab->map, map_value);
>
> kptrs (please use plural consistently for functions that actually
> handle multiple kptrs).
>

Ok, will audit all other places as well.

> >  }
> >
> >  /* It is called from the bpf_lru_list when the LRU needs to delete
>
> [...]
>
> >  static void htab_lru_push_free(struct bpf_htab *htab, struct htab_elem *elem)
> >  {
> > -       check_and_free_timer(htab, elem);
> > +       check_and_free_timer_and_kptr(htab, elem, true);
> >         bpf_lru_push_free(&htab->lru, &elem->lru_node);
> >  }
> >
> > @@ -1420,7 +1424,10 @@ static void htab_free_malloced_timers(struct bpf_htab *htab)
> >                 struct htab_elem *l;
> >
> >                 hlist_nulls_for_each_entry(l, n, head, hash_node)
> > -                       check_and_free_timer(htab, l);
> > +                       /* We don't reset or free kptr on uref dropping to zero,
> > +                        * hence set free_kptr to false.
> > +                        */
> > +                       check_and_free_timer_and_kptr(htab, l, false);
>
> ok, now reading this, I wonder if it's better to keep timer and kptrs
> clean ups separate? And then dynptrs separate still? Instead of adding
> all these flags.

Right, in case of array map, we directly called bpf_timer_cancel_and_free,
instead of going through the function named like this, I guess it makes sense to
do the same for hash map, since I assume both kptrs and dynptrs wouldn't want to
be freed on map_release_uref, unlike timers.

>
> >                 cond_resched_rcu();
> >         }
> >         rcu_read_unlock();
> > @@ -1430,6 +1437,7 @@ static void htab_map_free_timers(struct bpf_map *map)
> >  {
> >         struct bpf_htab *htab = container_of(map, struct bpf_htab, map);
> >
> > +       /* We don't reset or free kptr on uref dropping to zero. */
> >         if (likely(!map_value_has_timer(&htab->map)))
> >                 return;
> >         if (!htab_is_prealloc(htab))
>
> [...]

--
Kartikeya