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

[Song Liu <songliubraving@meta.com>]

> 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. 

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

Thanks,
Song

> 
>> +
>> +       return vfs_getxattr(mnt_idmap(file->f_path.mnt), file_dentry(file), name_ptr->data,
>> +                           value_ptr->data, __bpf_dynptr_size(value_ptr));
>> +}