[bpf-next,2/3] bpf/helpers: introduce bpf_dynptr_copy kfunc

Commit Message

Mykyta Yatsenko Feb. 18, 2025, 7 p.m. UTC

From: Mykyta Yatsenko <[e-mail]>

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 <[e-mail]>
---
 kernel/bpf/helpers.c  | 37 +++++++++++++++++++++++++++++++++++++
 kernel/bpf/verifier.c |  3 +++
 2 files changed, 40 insertions(+)

Comments

Eduard Zingerman Feb. 19, 2025, 1:01 a.m. UTC | #1

On Tue, 2025-02-18 at 19:00 +0000, Mykyta Yatsenko wrote:
> From: Mykyta Yatsenko <[e-mail]>
> 
> 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 <[e-mail]>
> ---

Acked-by: Eduard Zingerman <[e-mail]>

>  kernel/bpf/helpers.c  | 37 +++++++++++++++++++++++++++++++++++++
>  kernel/bpf/verifier.c |  3 +++
>  2 files changed, 40 insertions(+)
> 
> 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;
>  }

Nit: it would be nice to have a docstring here, as for other dynptr functions.

> +__bpf_kfunc int bpf_dynptr_copy(struct bpf_dynptr *dst_ptr, u32 dst_off,
> +				struct bpf_dynptr *src_ptr, u32 src_off, u32 size)

[...]

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

Nit: There is no need to add bpf_dynptr_copy to special kfuncs list.
     This list is maintained to get BTF ids of several kfuncs w/o lookup,
     like so: 'special_kfunc_list[KF_bpf_dynptr_slice]'.
     Which is useful when writing custom semantic rules for such functions.
     There are no special rules for bpf_dynptr_copy, hence entry in the
     special_kfunc_list is not needed.

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

Andrii Nakryiko Feb. 21, 2025, 12:42 a.m. UTC | #2

On Tue, Feb 18, 2025 at 11:01 AM Mykyta Yatsenko
<[e-mail]> wrote:
>
> From: Mykyta Yatsenko <[e-mail]>
>
> 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 <[e-mail]>
> ---
>  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
>

Patch

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;
+	int err = 0;
+
+	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);
+	} else if (src_slice) {
+		err = __bpf_dynptr_write(dst, dst_off, src_slice, size, 0);
+	} else if (dst_slice) {
+		err = __bpf_dynptr_read(dst_slice, size, src, src_off, 0);
+	} 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;
+
+		while (err == 0 && off < size) {
+			u32 chunk_sz = min(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);
+			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)