On 24/12/04 09:58AM, Andrii Nakryiko wrote: > On Wed, Dec 4, 2024 at 2:39 AM Anton Protopopov <aspsk@xxxxxxxxxxxxx> wrote: > > > > 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. > > 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 > > > > > > > >