| Message ID | 20250125-bpf_dynptr_probe-v2-0-c42c87f97afe@outlook.com |
|---|---|
| Headers | show |
| Series | bpf: Add probe_read_{kernel,user}_dynptr and copy_from_user_dynptr | expand |
On Sat, Jan 25, 2025 at 12:30 AM Levi Zim via B4 Relay <devnull+rsworktech.outlook.com@kernel.org> wrote: > > From: Levi Zim <rsworktech@outlook.com> > > This patch add a helper function bpf_probe_read_kernel_dynptr: > > long bpf_probe_read_kernel_dynptr(const struct bpf_dynptr *dst, > u32 offset, u32 size, const void *unsafe_ptr, u64 flags); We stopped adding helpers years ago. Only new kfuncs are allowed. This particular one doesn't look useful as-is. The same logic can be expressed with - create dynptr - dynptr_slice - copy_from_kernel pw-bot: cr
On 2025/1/26 00:58, Alexei Starovoitov wrote: > On Sat, Jan 25, 2025 at 12:30 AM Levi Zim via B4 Relay > <devnull+rsworktech.outlook.com@kernel.org> wrote: >> From: Levi Zim <rsworktech@outlook.com> >> >> This patch add a helper function bpf_probe_read_kernel_dynptr: >> >> long bpf_probe_read_kernel_dynptr(const struct bpf_dynptr *dst, >> u32 offset, u32 size, const void *unsafe_ptr, u64 flags); > We stopped adding helpers years ago. > Only new kfuncs are allowed. Sorry, I didn't know that. Just asking, is there any documentation/discussion about stopping adding helpers? I will switch the implementation to kfuncs in v3. > This particular one doesn't look useful as-is. > The same logic can be expressed with > - create dynptr > - dynptr_slice > - copy_from_kernel By copy_from_kernel I assume you mean bpf_probe_read_kernel. The problem with dynptr_slice_rdwr and probe_read_kernel is that they only support a compile-time constant size [1]. But in order to best utilize the space on a BPF ringbuf, it is possible to reserve a variable length of space as dynptr on a ringbuf with bpf_ringbuf_reserve_dynptr. Then currently we have no way to read a variable length of kernel memory into this dynptr, except doing it chunk by chunk[2], which is kinda awkward. That's the problem the new helpers trying to solve. And I am not the only one needing this kind of feature [3]. Andrii said it would be a straightforward addition as it is a super thin wrapper around existing functionality (we are just avoiding fixed buffer size restrictions of existing probe/copy_from APIs) [1]: https://elixir.bootlin.com/linux/v6.12.6/source/kernel/bpf/helpers.c#L2600-L2601 [2]: https://github.com/libbpf/libbpf-bootstrap/commit/046fad60df3e39540937b5ec6ee86054f33d3f28 [3]: https://github.com/libbpf/libbpf-rs/issues/1041 [4]: https://lore.kernel.org/bpf/CAEf4BzZctXJsR+TwMhmXNWnR0_BV802-3KJw226ZZt8St4xNkw@mail.gmail.com/ > pw-bot: cr
On Sat, Jan 25, 2025 at 5:05 PM Levi Zim <rsworktech@outlook.com> wrote: > > On 2025/1/26 00:58, Alexei Starovoitov wrote: > > On Sat, Jan 25, 2025 at 12:30 AM Levi Zim via B4 Relay > > <devnull+rsworktech.outlook.com@kernel.org> wrote: > >> From: Levi Zim <rsworktech@outlook.com> > >> > >> This patch add a helper function bpf_probe_read_kernel_dynptr: > >> > >> long bpf_probe_read_kernel_dynptr(const struct bpf_dynptr *dst, > >> u32 offset, u32 size, const void *unsafe_ptr, u64 flags); > > We stopped adding helpers years ago. > > Only new kfuncs are allowed. > > Sorry, I didn't know that. Just asking, is there any > documentation/discussion > about stopping adding helpers? > > I will switch the implementation to kfuncs in v3. > > > This particular one doesn't look useful as-is. > > The same logic can be expressed with > > - create dynptr > > - dynptr_slice > > - copy_from_kernel > > By copy_from_kernel I assume you mean bpf_probe_read_kernel. The problem > with dynptr_slice_rdwr and probe_read_kernel is that they only support a > compile-time constant size [1]. > > But in order to best utilize the space on a BPF ringbuf, it is possible > to reserve a > variable length of space as dynptr on a ringbuf with > bpf_ringbuf_reserve_dynptr. That makes sense. The commit log didn't call it out. Please spell out the motivation clearly. Also why bpf_probe_read_kernel_common ? Do we need to memset() it on failure?
On Mon, Jan 27, 2025 at 5:04 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Sat, Jan 25, 2025 at 5:05 PM Levi Zim <rsworktech@outlook.com> wrote: > > > > On 2025/1/26 00:58, Alexei Starovoitov wrote: > > > On Sat, Jan 25, 2025 at 12:30 AM Levi Zim via B4 Relay > > > <devnull+rsworktech.outlook.com@kernel.org> wrote: > > >> From: Levi Zim <rsworktech@outlook.com> > > >> > > >> This patch add a helper function bpf_probe_read_kernel_dynptr: > > >> > > >> long bpf_probe_read_kernel_dynptr(const struct bpf_dynptr *dst, > > >> u32 offset, u32 size, const void *unsafe_ptr, u64 flags); > > > We stopped adding helpers years ago. > > > Only new kfuncs are allowed. > > > > Sorry, I didn't know that. Just asking, is there any > > documentation/discussion > > about stopping adding helpers? > > > > I will switch the implementation to kfuncs in v3. > > > > > This particular one doesn't look useful as-is. > > > The same logic can be expressed with > > > - create dynptr > > > - dynptr_slice > > > - copy_from_kernel > > > > By copy_from_kernel I assume you mean bpf_probe_read_kernel. The problem > > with dynptr_slice_rdwr and probe_read_kernel is that they only support a > > compile-time constant size [1]. > > > > But in order to best utilize the space on a BPF ringbuf, it is possible > > to reserve a > > variable length of space as dynptr on a ringbuf with > > bpf_ringbuf_reserve_dynptr. For our uprobes, we've run into similar issues around doing variable-sized bpf_probe_read_user() into ring buffers for our debugger [1]. Our use case is that we generate uprobes that recursively read data structures until we fill up a buffer. The verifier's insistence on knowing statically that a read fits into the buffer makes for awkward code, and makes it hard to pack the buffer fully; we have to split our reads into a couple of static size classes. Any chance there'd be interest in taking the opportunity to support dynamically-sized reads from userspace too? :) [1] https://side-eye.io > > That makes sense. The commit log didn't call it out. > Please spell out the motivation clearly. > Also why bpf_probe_read_kernel_common ? > Do we need to memset() it on failure? >
On Mon, Jan 27, 2025 at 2:54 PM Andrei Matei <andreimatei1@gmail.com> wrote: > > On Mon, Jan 27, 2025 at 5:04 PM Alexei Starovoitov > <alexei.starovoitov@gmail.com> wrote: > > > > On Sat, Jan 25, 2025 at 5:05 PM Levi Zim <rsworktech@outlook.com> wrote: > > > > > > On 2025/1/26 00:58, Alexei Starovoitov wrote: > > > > On Sat, Jan 25, 2025 at 12:30 AM Levi Zim via B4 Relay > > > > <devnull+rsworktech.outlook.com@kernel.org> wrote: > > > >> From: Levi Zim <rsworktech@outlook.com> > > > >> > > > >> This patch add a helper function bpf_probe_read_kernel_dynptr: > > > >> > > > >> long bpf_probe_read_kernel_dynptr(const struct bpf_dynptr *dst, > > > >> u32 offset, u32 size, const void *unsafe_ptr, u64 flags); > > > > We stopped adding helpers years ago. > > > > Only new kfuncs are allowed. > > > > > > Sorry, I didn't know that. Just asking, is there any > > > documentation/discussion > > > about stopping adding helpers? > > > > > > I will switch the implementation to kfuncs in v3. > > > > > > > This particular one doesn't look useful as-is. > > > > The same logic can be expressed with > > > > - create dynptr > > > > - dynptr_slice > > > > - copy_from_kernel > > > > > > By copy_from_kernel I assume you mean bpf_probe_read_kernel. The problem > > > with dynptr_slice_rdwr and probe_read_kernel is that they only support a > > > compile-time constant size [1]. > > > > > > But in order to best utilize the space on a BPF ringbuf, it is possible > > > to reserve a > > > variable length of space as dynptr on a ringbuf with > > > bpf_ringbuf_reserve_dynptr. > > For our uprobes, we've run into similar issues around doing variable-sized > bpf_probe_read_user() into ring buffers for our debugger [1]. Our use case > is that we generate uprobes that recursively read data structures until we > fill up a buffer. The verifier's insistence on knowing statically that a read > fits into the buffer makes for awkward code, and makes it hard to pack the > buffer fully; we have to split our reads into a couple of static size classes. > > Any chance there'd be interest in taking the opportunity to support > dynamically-sized reads from userspace too? :) That's bpf_probe_read_user_dynptr() from patch #2, no? But generally speaking, here's a list of new APIs that we'd need to cover all existing fixed buffer versions: - non-sleepable probe reads: bpf_probe_read_kernel_dynptr() bpf_probe_read_user_dynptr() bpf_probe_read_kernel_str_dynptr() bpf_probe_read_user_str_dynptr() - sleepable probe reads (copy_from_user): bpf_copy_from_user_dynptr() bpf_copy_from_user_str_dynptr() - and then we have complementary task-based APIs for non-current process: bpf_probe_read_user_task_dynptr() bpf_probe_read_user_str_task_dynptr() bpf_copy_from_user_task_dynptr() bpf_copy_from_user_str_task_dynptr() Jordan is working on non-dynptr version of bpf_copy_from_user_str_task(), once he's done with that, we'll add dynptr version, probably. > > [1] https://side-eye.io > > > > > That makes sense. The commit log didn't call it out. > > Please spell out the motivation clearly. > > Also why bpf_probe_read_kernel_common ? > > Do we need to memset() it on failure? > >
On 2025-01-28 07:09, Andrii Nakryiko wrote: > On Mon, Jan 27, 2025 at 2:54 PM Andrei Matei <andreimatei1@gmail.com> wrote: >> On Mon, Jan 27, 2025 at 5:04 PM Alexei Starovoitov >> <alexei.starovoitov@gmail.com> wrote: >>> On Sat, Jan 25, 2025 at 5:05 PM Levi Zim <rsworktech@outlook.com> wrote: >>>> On 2025/1/26 00:58, Alexei Starovoitov wrote: >>>> > On Sat, Jan 25, 2025 at 12:30 AM Levi Zim via B4 Relay >>>> > <devnull+rsworktech.outlook.com@kernel.org> wrote: >>>> >> From: Levi Zim <rsworktech@outlook.com> >>>> >> >>>> >> This patch add a helper function bpf_probe_read_kernel_dynptr: >>>> >> >>>> >> long bpf_probe_read_kernel_dynptr(const struct bpf_dynptr *dst, >>>> >> u32 offset, u32 size, const void *unsafe_ptr, u64 flags); >>>> > We stopped adding helpers years ago. >>>> > Only new kfuncs are allowed. >>>> >>>> Sorry, I didn't know that. Just asking, is there any >>>> documentation/discussion >>>> about stopping adding helpers? >>>> >>>> I will switch the implementation to kfuncs in v3. >>>> >>>> > This particular one doesn't look useful as-is. >>>> > The same logic can be expressed with >>>> > - create dynptr >>>> > - dynptr_slice >>>> > - copy_from_kernel >>>> >>>> By copy_from_kernel I assume you mean bpf_probe_read_kernel. The problem >>>> with dynptr_slice_rdwr and probe_read_kernel is that they only support a >>>> compile-time constant size [1]. >>>> >>>> But in order to best utilize the space on a BPF ringbuf, it is possible >>>> to reserve a >>>> variable length of space as dynptr on a ringbuf with >>>> bpf_ringbuf_reserve_dynptr. >> For our uprobes, we've run into similar issues around doing variable-sized >> bpf_probe_read_user() into ring buffers for our debugger [1]. Our use case >> is that we generate uprobes that recursively read data structures until we >> fill up a buffer. The verifier's insistence on knowing statically that a read >> fits into the buffer makes for awkward code, and makes it hard to pack the >> buffer fully; we have to split our reads into a couple of static size classes. >> >> Any chance there'd be interest in taking the opportunity to support >> dynamically-sized reads from userspace too? :) > That's bpf_probe_read_user_dynptr() from patch #2, no? > > But generally speaking, here's a list of new APIs that we'd need to > cover all existing fixed buffer versions: > > - non-sleepable probe reads: > > bpf_probe_read_kernel_dynptr() > bpf_probe_read_user_dynptr() > bpf_probe_read_kernel_str_dynptr() I think the _str_dynptr versions are probably not worth adding. For example, when we use probe_read_kernel_str, the length of the str is usually not known and we usually allocate a fixed size buffer for it. If we do know the length of the str beforehand, we can just use probe_read_kernel_dynptr. > bpf_probe_read_user_str_dynptr() > > - sleepable probe reads (copy_from_user): > > bpf_copy_from_user_dynptr() > bpf_copy_from_user_str_dynptr() > > - and then we have complementary task-based APIs for non-current process: > > bpf_probe_read_user_task_dynptr() > bpf_probe_read_user_str_task_dynptr() > bpf_copy_from_user_task_dynptr() > bpf_copy_from_user_str_task_dynptr() > > Jordan is working on non-dynptr version of > bpf_copy_from_user_str_task(), once he's done with that, we'll add > dynptr version, probably. > >> [1] https://side-eye.io >> >>> That makes sense. The commit log didn't call it out. >>> Please spell out the motivation clearly. >>> Also why bpf_probe_read_kernel_common ? >>> Do we need to memset() it on failure? >>>
On 2025/1/28 06:04, Alexei Starovoitov wrote: > On Sat, Jan 25, 2025 at 5:05 PM Levi Zim <rsworktech@outlook.com> wrote: >> On 2025/1/26 00:58, Alexei Starovoitov wrote: >> > On Sat, Jan 25, 2025 at 12:30 AM Levi Zim via B4 Relay >> > <devnull+rsworktech.outlook.com@kernel.org> wrote: >> >> From: Levi Zim <rsworktech@outlook.com> >> >> >> >> This patch add a helper function bpf_probe_read_kernel_dynptr: >> >> >> >> long bpf_probe_read_kernel_dynptr(const struct bpf_dynptr *dst, >> >> u32 offset, u32 size, const void *unsafe_ptr, u64 flags); >> > We stopped adding helpers years ago. >> > Only new kfuncs are allowed. >> >> Sorry, I didn't know that. Just asking, is there any >> documentation/discussion >> about stopping adding helpers? >> >> I will switch the implementation to kfuncs in v3. >> >> > This particular one doesn't look useful as-is. >> > The same logic can be expressed with >> > - create dynptr >> > - dynptr_slice >> > - copy_from_kernel >> >> By copy_from_kernel I assume you mean bpf_probe_read_kernel. The problem >> with dynptr_slice_rdwr and probe_read_kernel is that they only support a >> compile-time constant size [1]. >> >> But in order to best utilize the space on a BPF ringbuf, it is possible >> to reserve a >> variable length of space as dynptr on a ringbuf with >> bpf_ringbuf_reserve_dynptr. > That makes sense. The commit log didn't call it out. > Please spell out the motivation clearly. Thanks for the advice! I will include it in v3. > Also why bpf_probe_read_kernel_common ? > Do we need to memset() it on failure? Since the current patch is basically a thin wrapper around bpf_probe_read_kernel, I think we'd better not deviate from the wrapped function.
This series introduce the dynptr counterpart of the bpf_probe_read_{kernel,user} helpers and bpf_copy_from_user helper. These helpers are helpful for reading variable-length data from kernel memory into dynptr without going through an intermediate buffer. Link: https://lore.kernel.org/bpf/MEYP282MB2312CFCE5F7712FDE313215AC64D2@MEYP282MB2312.AUSP282.PROD.OUTLOOK.COM/ Suggested-by: Andrii Nakryiko <andrii.nakryiko@gmail.com> Signed-off-by: Levi Zim <rsworktech@outlook.com> --- Changes in v2: - Add missing bpf-next prefix. I forgot it in the initial series. Sorry about that. - Link to v1: https://lore.kernel.org/r/20250125-bpf_dynptr_probe-v1-0-c3cb121f6951@outlook.com --- Levi Zim (7): bpf: Implement bpf_probe_read_kernel_dynptr helper bpf: Implement bpf_probe_read_user_dynptr helper bpf: Implement bpf_copy_from_user_dynptr helper tools headers UAPI: Update tools's copy of bpf.h header selftests/bpf: probe_read_kernel_dynptr test selftests/bpf: probe_read_user_dynptr test selftests/bpf: copy_from_user_dynptr test include/linux/bpf.h | 3 + include/uapi/linux/bpf.h | 49 ++++++++++ kernel/bpf/helpers.c | 53 ++++++++++- kernel/trace/bpf_trace.c | 72 ++++++++++++++ tools/include/uapi/linux/bpf.h | 49 ++++++++++ tools/testing/selftests/bpf/prog_tests/dynptr.c | 45 ++++++++- tools/testing/selftests/bpf/progs/dynptr_success.c | 106 +++++++++++++++++++++ 7 files changed, 374 insertions(+), 3 deletions(-) --- base-commit: d0d106a2bd21499901299160744e5fe9f4c83ddb change-id: 20250124-bpf_dynptr_probe-ab483c554f1a Best regards,