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

[Anton Protopopov <aspsk@isovalent.com>]

On 24/12/04 09:58AM, Andrii Nakryiko wrote:
> On Wed, Dec 4, 2024 at 2:39 AM Anton Protopopov <aspsk@isovalent.com> wrote:
> >
> > 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.
> 
> we don't enforce the, or adjust to, reverse xmas tree styling, so please don't

Ok, thanks, good to know. I've removed this diff.

> >
> > >
> > >
> > > >
> > > > -       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
> > > >
> > > >