| Message ID | 20241203135052.3380721-2-aspsk@isovalent.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | Add fd_array_cnt attribute for BPF_PROG_LOAD | expand |
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? > #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); > + ? > 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 > > - 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 > >
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 > > > >
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 > > > > > > > > > > > - 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 > > > > > >
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 > > > > > > > >
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> #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); + 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; - 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; }
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(-)