On 24/12/03 01:25PM, Andrii Nakryiko wrote:
> On Tue, Dec 3, 2024 at 5:48 AM Anton Protopopov <aspsk@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxx>
>
> > 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
> >
> >
