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

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

On Tue, Oct 17, 2023 at 3:16 PM Song Liu <songliubraving@meta.com> wrote:
>
>
>
> > On Oct 17, 2023, at 2:52 PM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> >
> > On Tue, Oct 17, 2023 at 1:31 PM Song Liu <songliubraving@meta.com> wrote:
> >>
> >>
> >>
> >>> On Oct 17, 2023, at 11:58 AM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> >>>
> >>> 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...
> >>
> >> is_zero_terminated sounds better.
> >>
> >>>
> >>>> +{
> >>>> +       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
> >>
> >> We can add a NULL check (or size check) here.
> >
> > there must be some helper to check if dynptr is valid, let's use that
> > instead of NULL checks
>
> Yeah, we can use bpf_dynptr_is_null().
>
> >
> >>
> >>>
> >>> 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
> >>
> >> We can require the dynptr to be BPF_DYNPTR_TYPE_LOCAL. I don't think
> >> we need this for dynptr of skb or xdp. Would this be sufficient?
> >
> > well, to keep thing simple we can have a simple internal helper API
> > that will tell if it's safe to assume that dynptr memory is contiguous
> > and it's ok to use dynptr memory. But still, you shouldn't access data
> > pointer directly, there must be some helper for that. Please check. It
> > has to take into account offset and stuff like that.
>
> Yeah, we can use bpf_dynptr_write(), which is a helper (not kfunc).
>
> >
> > Also, and separately from that, we should think about providing a
> > bpf_dynptr_slice()-like helper that will accept a fixed-sized
> > temporary buffer and return pointer to either actual memory or copy
> > non-contiguous memory into that buffer. That will make sure you can
> > use any dynptr as a source of data, and only pay the price of memory
> > copy in rare cases where it's necessary
>
> I don't quite follow here. Currently, we have
>
> bpf_dynptr_data()
> bpf_dynptr_slice()
> bpf_dynptr_slice_rdwr()
> bpf_dynptr_write()
>
> AFAICT, they are sufficient to cover existing use cases (and the new
> use case we are adding in this set). What's the new kfunc are you
> thinking about?

I wasn't talking about kfuncs, but rather just internal helpers to be
used by other kfuncs when working with dynptrs as input arguments.

>
> Thanks,
> Song
>
>