From: Song Liu <songliubraving@meta.com>
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: Song Liu <songliubraving@meta.com>, Song Liu <song@kernel.org>,
bpf <bpf@vger.kernel.org>,
"fsverity@lists.linux.dev" <fsverity@lists.linux.dev>,
Alexei Starovoitov <ast@kernel.org>,
Daniel Borkmann <daniel@iogearbox.net>,
Andrii Nakryiko <andrii@kernel.org>,
Martin KaFai Lau <martin.lau@kernel.org>,
Kernel Team <kernel-team@meta.com>,
Eric Biggers <ebiggers@kernel.org>,
"tytso@mit.edu" <tytso@mit.edu>,
"roberto.sassu@huaweicloud.com" <roberto.sassu@huaweicloud.com>
Subject: Re: [PATCH bpf-next 1/5] bpf: Add kfunc bpf_get_file_xattr
Date: Tue, 17 Oct 2023 22:46:30 +0000 [thread overview]
Message-ID: <4072463E-332B-40BE-BC8B-D284FB88D143@fb.com> (raw)
In-Reply-To: <CAEf4BzZXPMqLG95f5XXayvbTUOOJmzUKjyaurnjdMrt46S5d1Q@mail.gmail.com>
> On Oct 17, 2023, at 3:40 PM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>
> 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.
AFAICT, kfuncs can call other kfuncs, for example bpf_dynptr_slice_rdwr()
calls bpf_dynptr_slice(). This is limited to the same file at the moment,
since we do not expose kfuncs in headers.
Thanks,
Song
next prev parent reply other threads:[~2023-10-17 22:46 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20231013182644.2346458-1-song@kernel.org>
[not found] ` <20231013182644.2346458-3-song@kernel.org>
2023-10-15 7:07 ` [PATCH bpf-next 2/5] bpf, fsverity: Add kfunc bpf_get_fsverity_digest Eric Biggers
2023-10-16 20:10 ` Song Liu
2023-10-17 3:12 ` Eric Biggers
2023-10-17 5:35 ` Song Liu
2023-10-17 5:46 ` Eric Biggers
2023-10-17 14:16 ` Song Liu
2023-10-17 19:50 ` Andrii Nakryiko
[not found] ` <20231013182644.2346458-6-song@kernel.org>
2023-10-17 19:08 ` [PATCH bpf-next 5/5] selftests/bpf: Add test that use fsverity and xattr to sign a file Alexei Starovoitov
2023-10-17 20:36 ` Song Liu
[not found] ` <20231013182644.2346458-2-song@kernel.org>
2023-10-17 18:58 ` [PATCH bpf-next 1/5] bpf: Add kfunc bpf_get_file_xattr Andrii Nakryiko
2023-10-17 20:31 ` Song Liu
2023-10-17 21:52 ` Andrii Nakryiko
2023-10-17 22:16 ` Song Liu
2023-10-17 22:40 ` Andrii Nakryiko
2023-10-17 22:46 ` Song Liu [this message]
2023-10-18 1:42 ` Hou Tao
2023-10-17 19:10 ` Alexei Starovoitov
2023-11-02 1:19 ` KP Singh
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4072463E-332B-40BE-BC8B-D284FB88D143@fb.com \
--to=songliubraving@meta.com \
--cc=andrii.nakryiko@gmail.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=ebiggers@kernel.org \
--cc=fsverity@lists.linux.dev \
--cc=kernel-team@meta.com \
--cc=martin.lau@kernel.org \
--cc=roberto.sassu@huaweicloud.com \
--cc=song@kernel.org \
--cc=tytso@mit.edu \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).