On Tue, Feb 18, 2025 at 11:01 AM Mykyta Yatsenko
<mykyta.yatsenko5@xxxxxxxxx> wrote:
>
> From: Mykyta Yatsenko <yatsenko@xxxxxxxx>
>
> Introducing bpf_dynptr_copy kfunc allowing copying data from one dynptr to
> another. This functionality is useful in scenarios such as capturing XDP
> data to a ring buffer.
> The implementation consists of 4 branches:
> * A fast branch for contiguous buffer capacity in both source and
> destination dynptrs
> * 3 branches utilizing __bpf_dynptr_read and __bpf_dynptr_write to copy
> data to/from non-contiguous buffer
>
> Signed-off-by: Mykyta Yatsenko <yatsenko@xxxxxxxx>
> ---
> kernel/bpf/helpers.c | 37 +++++++++++++++++++++++++++++++++++++
> kernel/bpf/verifier.c | 3 +++
> 2 files changed, 40 insertions(+)
This is surprisingly nicely compact and terse, I really like this! See
some nits below, and yes, let's add doc-comment as Eduard suggested.
pw-bot: cr
>
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index 2833558c3009..ac5fbdfc504d 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -2770,6 +2770,42 @@ __bpf_kfunc int bpf_dynptr_clone(const struct bpf_dynptr *p,
> return 0;
> }
>
> +__bpf_kfunc int bpf_dynptr_copy(struct bpf_dynptr *dst_ptr, u32 dst_off,
> + struct bpf_dynptr *src_ptr, u32 src_off, u32 size)
> +{
> + struct bpf_dynptr_kern *dst = (struct bpf_dynptr_kern *)dst_ptr;
> + struct bpf_dynptr_kern *src = (struct bpf_dynptr_kern *)src_ptr;
> + __u8 *src_slice, *dst_slice;
let's use `void *`, both memmove and bpf_dynptr_{write,read} uses
`void *` throughout
> + int err = 0;
I'm not sure we want to thread through this err, see below
> +
> + src_slice = bpf_dynptr_slice(src_ptr, src_off, NULL, size);
> + dst_slice = bpf_dynptr_slice_rdwr(dst_ptr, dst_off, NULL, size);
> +
> + if (src_slice && dst_slice) {
> + memmove(dst_slice, src_slice, size);
return 0;
> + } else if (src_slice) {
> + err = __bpf_dynptr_write(dst, dst_off, src_slice, size, 0);
return __bpf_dynptr_write(...)
> + } else if (dst_slice) {
> + err = __bpf_dynptr_read(dst_slice, size, src, src_off, 0);
ditto, direct return
> + } else {
> + u32 off = 0;
> + char buf[256];
> +
> + if (bpf_dynptr_check_off_len(dst, dst_off, size) ||
> + bpf_dynptr_check_off_len(src, src_off, size))
> + return -E2BIG;
see, you are doing direct return here (but inconsistent with the rest of logic)
> +
> + while (err == 0 && off < size) {
just while (off < size), because...
> + u32 chunk_sz = min(sizeof(buf), size - off);
this might trigger warning about type mismatch (size_t vs u32), so
probably best to be explicit: `min_t(u32, sizeof(buf), size - off);`
> +
> + err = err ?: __bpf_dynptr_read(buf, chunk_sz, src, src_off + off, 0);
> + err = err ?: __bpf_dynptr_write(dst, dst_off + off, buf, chunk_sz, 0);
I'd keep this a bit more conventional with just
err = __bpf_dynptr_read(...);
if (err)
return err;
err = __bpf_dynptr_write(...);
if (err)
return err;
off += chunk_sz;
Nothing wrong or incorrect with how you wrote it, but somehow feels a
bit more typical to do it this way.
> + off += chunk_sz;
> + }
> + }
> + return err;
> +}
> +
> __bpf_kfunc void *bpf_cast_to_kern_ctx(void *obj)
> {
> return obj;
> @@ -3174,6 +3210,7 @@ BTF_ID_FLAGS(func, bpf_dynptr_is_null)
> BTF_ID_FLAGS(func, bpf_dynptr_is_rdonly)
> BTF_ID_FLAGS(func, bpf_dynptr_size)
> BTF_ID_FLAGS(func, bpf_dynptr_clone)
> +BTF_ID_FLAGS(func, bpf_dynptr_copy)
> #ifdef CONFIG_NET
> BTF_ID_FLAGS(func, bpf_modify_return_test_tp)
> #endif
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index e7bc74171c99..3c567bfcc582 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -11781,6 +11781,7 @@ enum special_kfunc_type {
> KF_bpf_dynptr_slice,
> KF_bpf_dynptr_slice_rdwr,
> KF_bpf_dynptr_clone,
> + KF_bpf_dynptr_copy,
> KF_bpf_percpu_obj_new_impl,
> KF_bpf_percpu_obj_drop_impl,
> KF_bpf_throw,
> @@ -11819,6 +11820,7 @@ BTF_ID(func, bpf_dynptr_from_xdp)
> BTF_ID(func, bpf_dynptr_slice)
> BTF_ID(func, bpf_dynptr_slice_rdwr)
> BTF_ID(func, bpf_dynptr_clone)
> +BTF_ID(func, bpf_dynptr_copy)
> BTF_ID(func, bpf_percpu_obj_new_impl)
> BTF_ID(func, bpf_percpu_obj_drop_impl)
> BTF_ID(func, bpf_throw)
> @@ -11857,6 +11859,7 @@ BTF_ID_UNUSED
> BTF_ID(func, bpf_dynptr_slice)
> BTF_ID(func, bpf_dynptr_slice_rdwr)
> BTF_ID(func, bpf_dynptr_clone)
> +BTF_ID(func, bpf_dynptr_copy)
> BTF_ID(func, bpf_percpu_obj_new_impl)
> BTF_ID(func, bpf_percpu_obj_drop_impl)
> BTF_ID(func, bpf_throw)
> --
> 2.48.1
>
