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