On Sat, Jan 25, 2025 at 5:05 PM Levi Zim <rsworktech@xxxxxxxxxxx> 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@xxxxxxxxxx> wrote: > >> From: Levi Zim <rsworktech@xxxxxxxxxxx> > >> > >> 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. you might want to look at Documentation/bpf/kfuncs.rst as well > > > 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) Yep, exactly. All the probe/copy APIs we have require statically known dst buffer and, worst still, statically known read size, which makes it very hard and cumbersome to request data which size is known at runtime (e.g., process argv/environ data). So, in short, I think these new dynptr-based APIs are a good addition, but yeah, they'll have to be added as kfuncs. One thing that we need to figure out is how to deal with SKB/XDP dynptrs, we can't just reject them as destination buffers for all possible APIs. I think we'll need to develop a set of internal helpers that would handle skb/xdp dynptr cases almost transparently: - if skb/xdp memory is linear, we just memcpy/probe_read into it directly; - if it's not linear, we use on-the-stack small buffer (128 or 256 bytes, something like that), to do desired operation (memcpy or probe) into that buffer, and then using generic dynptr write functionality from that buffer into skb/xdp. We probably can start simple with excluding xdp/skb, but this should be the second step. > > [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@xxxxxxxxxxxxxx/ > > > pw-bot: cr
