Re: [PATCH v4 bpf-next 1/7] bpf: add a __btf_get_by_fd helper

[Anton Protopopov <aspsk@isovalent.com>]

On 24/12/03 01:25PM, Andrii Nakryiko wrote:
> On Tue, Dec 3, 2024 at 5:48 AM Anton Protopopov <aspsk@isovalent.com> wrote:
> >
> > Add a new helper to get a pointer to a struct btf from a file
> > descriptor. This helper doesn't increase a refcnt. Add a comment
> > explaining this and pointing to a corresponding function which
> > does take a reference.
> >
> > Signed-off-by: Anton Protopopov <aspsk@isovalent.com>
> > ---
> >  include/linux/bpf.h | 17 +++++++++++++++++
> >  include/linux/btf.h |  2 ++
> >  kernel/bpf/btf.c    | 13 ++++---------
> >  3 files changed, 23 insertions(+), 9 deletions(-)
> >
> 
> Minor (but unexplained and/or unnecessary) things I pointed out below,
> but overall looks good
> 
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
> 
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index eaee2a819f4c..ac44b857b2f9 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -2301,6 +2301,14 @@ void __bpf_obj_drop_impl(void *p, const struct btf_record *rec, bool percpu);
> >  struct bpf_map *bpf_map_get(u32 ufd);
> >  struct bpf_map *bpf_map_get_with_uref(u32 ufd);
> >
> > +/*
> > + * The __bpf_map_get() and __btf_get_by_fd() functions parse a file
> > + * descriptor and return a corresponding map or btf object.
> > + * Their names are double underscored to emphasize the fact that they
> > + * do not increase refcnt. To also increase refcnt use corresponding
> > + * bpf_map_get() and btf_get_by_fd() functions.
> > + */
> > +
> >  static inline struct bpf_map *__bpf_map_get(struct fd f)
> >  {
> >         if (fd_empty(f))
> > @@ -2310,6 +2318,15 @@ static inline struct bpf_map *__bpf_map_get(struct fd f)
> >         return fd_file(f)->private_data;
> >  }
> >
> > +static inline struct btf *__btf_get_by_fd(struct fd f)
> > +{
> > +       if (fd_empty(f))
> > +               return ERR_PTR(-EBADF);
> > +       if (unlikely(fd_file(f)->f_op != &btf_fops))
> > +               return ERR_PTR(-EINVAL);
> > +       return fd_file(f)->private_data;
> > +}
> > +
> >  void bpf_map_inc(struct bpf_map *map);
> >  void bpf_map_inc_with_uref(struct bpf_map *map);
> >  struct bpf_map *__bpf_map_inc_not_zero(struct bpf_map *map, bool uref);
> > diff --git a/include/linux/btf.h b/include/linux/btf.h
> > index 4214e76c9168..69159e649675 100644
> > --- a/include/linux/btf.h
> > +++ b/include/linux/btf.h
> > @@ -4,6 +4,7 @@
> >  #ifndef _LINUX_BTF_H
> >  #define _LINUX_BTF_H 1
> >
> > +#include <linux/file.h>
> 
> do we need this in linux/btf.h header?

Thanks, removed.

> >  #include <linux/types.h>
> >  #include <linux/bpfptr.h>
> >  #include <linux/bsearch.h>
> > @@ -143,6 +144,7 @@ void btf_get(struct btf *btf);
> >  void btf_put(struct btf *btf);
> >  const struct btf_header *btf_header(const struct btf *btf);
> >  int btf_new_fd(const union bpf_attr *attr, bpfptr_t uattr, u32 uattr_sz);
> > +
> 
> ?

Thanks, removed.

> >  struct btf *btf_get_by_fd(int fd);
> >  int btf_get_info_by_fd(const struct btf *btf,
> >                        const union bpf_attr *attr,
> > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > index e7a59e6462a9..ad5310fa1d3b 100644
> > --- a/kernel/bpf/btf.c
> > +++ b/kernel/bpf/btf.c
> > @@ -7743,17 +7743,12 @@ int btf_new_fd(const union bpf_attr *attr, bpfptr_t uattr, u32 uattr_size)
> >
> >  struct btf *btf_get_by_fd(int fd)
> >  {
> > -       struct btf *btf;
> >         CLASS(fd, f)(fd);
> > +       struct btf *btf;
> 
> nit: no need to just move this around

Ok, I can remove it. I moved it to form a reverse xmas tree,
as I was already editing this function.

> 
> 
> >
> > -       if (fd_empty(f))
> > -               return ERR_PTR(-EBADF);
> > -
> > -       if (fd_file(f)->f_op != &btf_fops)
> > -               return ERR_PTR(-EINVAL);
> > -
> > -       btf = fd_file(f)->private_data;
> > -       refcount_inc(&btf->refcnt);
> > +       btf = __btf_get_by_fd(f);
> > +       if (!IS_ERR(btf))
> > +               refcount_inc(&btf->refcnt);
> >
> >         return btf;
> >  }
> > --
> > 2.34.1
> >
> >