Re: [PATCH bpf-next 1/5] bpf: Add kfunc bpf_get_file_xattr

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Fri, Oct 13, 2023 at 11:29 AM Song Liu <song@kernel.org> wrote:
>
> This kfunc can be used to read xattr of a file.
>
> Since vfs_getxattr() requires null-terminated string as input "name", a new
> helper bpf_dynptr_is_string() is added to check the input before calling
> vfs_getxattr().
>
> Signed-off-by: Song Liu <song@kernel.org>
> ---
>  include/linux/bpf.h      | 12 +++++++++++
>  kernel/trace/bpf_trace.c | 44 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 56 insertions(+)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 61bde4520f5c..f14fae45e13d 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -2472,6 +2472,13 @@ static inline bool has_current_bpf_ctx(void)
>         return !!current->bpf_ctx;
>  }
>
> +static inline bool bpf_dynptr_is_string(struct bpf_dynptr_kern *ptr)

is_zero_terminated would be more accurate? though there is nothing
really dynptr-specific here...

> +{
> +       char *str = ptr->data;
> +
> +       return str[__bpf_dynptr_size(ptr) - 1] == '\0';
> +}
> +
>  void notrace bpf_prog_inc_misses_counter(struct bpf_prog *prog);
>
>  void bpf_dynptr_init(struct bpf_dynptr_kern *ptr, void *data,
> @@ -2708,6 +2715,11 @@ static inline bool has_current_bpf_ctx(void)
>         return false;
>  }
>
> +static inline bool bpf_dynptr_is_string(struct bpf_dynptr_kern *ptr)
> +{
> +       return false;
> +}
> +
>  static inline void bpf_prog_inc_misses_counter(struct bpf_prog *prog)
>  {
>  }
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index df697c74d519..946268574e05 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -24,6 +24,7 @@
>  #include <linux/key.h>
>  #include <linux/verification.h>
>  #include <linux/namei.h>
> +#include <linux/fileattr.h>
>
>  #include <net/bpf_sk_storage.h>
>
> @@ -1429,6 +1430,49 @@ static int __init bpf_key_sig_kfuncs_init(void)
>  late_initcall(bpf_key_sig_kfuncs_init);
>  #endif /* CONFIG_KEYS */
>
> +/* filesystem kfuncs */
> +__diag_push();
> +__diag_ignore_all("-Wmissing-prototypes",
> +                 "kfuncs which will be used in BPF programs");
> +
> +/**
> + * bpf_get_file_xattr - get xattr of a file
> + * @name_ptr: name of the xattr
> + * @value_ptr: output buffer of the xattr value
> + *
> + * Get xattr *name_ptr* of *file* and store the output in *value_ptr*.
> + *
> + * Return: 0 on success, a negative value on error.
> + */
> +__bpf_kfunc int bpf_get_file_xattr(struct file *file, struct bpf_dynptr_kern *name_ptr,
> +                                  struct bpf_dynptr_kern *value_ptr)
> +{
> +       if (!bpf_dynptr_is_string(name_ptr))
> +               return -EINVAL;

so dynptr can be invalid and name_ptr->data will be NULL, you should
account for that

and there could also be special dynptrs that don't have contiguous
memory region, so somehow you'd need to take care of that as well

> +
> +       return vfs_getxattr(mnt_idmap(file->f_path.mnt), file_dentry(file), name_ptr->data,
> +                           value_ptr->data, __bpf_dynptr_size(value_ptr));
> +}
> +
> +__diag_pop();
> +
> +BTF_SET8_START(fs_kfunc_set)
> +BTF_ID_FLAGS(func, bpf_get_file_xattr, KF_SLEEPABLE)
> +BTF_SET8_END(fs_kfunc_set)
> +
> +const struct btf_kfunc_id_set bpf_fs_kfunc_set = {
> +       .owner = THIS_MODULE,
> +       .set = &fs_kfunc_set,
> +};
> +
> +static int __init bpf_fs_kfuncs_init(void)
> +{
> +       return register_btf_kfunc_id_set(BPF_PROG_TYPE_TRACING,
> +                                        &bpf_fs_kfunc_set);
> +}
> +
> +late_initcall(bpf_fs_kfuncs_init);
> +
>  static const struct bpf_func_proto *
>  bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
>  {
> --
> 2.34.1
>