* [PATCH bpf-next v2 0/2] bpf: add bpf_dynptr_memset() kfunc
@ 2025-06-24 20:52 Ihor Solodrai
2025-06-24 20:52 ` [PATCH bpf-next v2 1/2] " Ihor Solodrai
2025-06-24 20:52 ` [PATCH bpf-next v2 2/2] selftests/bpf: add test cases for bpf_dynptr_memset() Ihor Solodrai
0 siblings, 2 replies; 11+ messages in thread
From: Ihor Solodrai @ 2025-06-24 20:52 UTC (permalink / raw)
To: bpf; +Cc: andrii, ast, eddyz87, mykyta.yatsenko5, mykolal, kernel-team
Implement bpf_dynptr_memset() kfunc and add tests for it.
v1->v2:
* handle non-linear buffers with bpf_dynptr_write()
* change function signature to include offset arg
* add more test cases
v1: https://lore.kernel.org/bpf/20250618223310.3684760-1-isolodrai@meta.com/
Ihor Solodrai (2):
bpf: add bpf_dynptr_memset() kfunc
selftests/bpf: add test cases for bpf_dynptr_memset()
kernel/bpf/helpers.c | 48 +++++
.../testing/selftests/bpf/prog_tests/dynptr.c | 8 +
.../selftests/bpf/progs/dynptr_success.c | 164 ++++++++++++++++++
3 files changed, 220 insertions(+)
--
2.47.1
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH bpf-next v2 1/2] bpf: add bpf_dynptr_memset() kfunc
2025-06-24 20:52 [PATCH bpf-next v2 0/2] bpf: add bpf_dynptr_memset() kfunc Ihor Solodrai
@ 2025-06-24 20:52 ` Ihor Solodrai
2025-06-24 21:50 ` Andrii Nakryiko
2025-06-25 11:27 ` Mykyta Yatsenko
2025-06-24 20:52 ` [PATCH bpf-next v2 2/2] selftests/bpf: add test cases for bpf_dynptr_memset() Ihor Solodrai
1 sibling, 2 replies; 11+ messages in thread
From: Ihor Solodrai @ 2025-06-24 20:52 UTC (permalink / raw)
To: bpf; +Cc: andrii, ast, eddyz87, mykyta.yatsenko5, mykolal, kernel-team
Currently there is no straightforward way to fill dynptr memory with a
value (most commonly zero). One can do it with bpf_dynptr_write(), but
a temporary buffer is necessary for that.
Implement bpf_dynptr_memset() - an analogue of memset() from libc.
Signed-off-by: Ihor Solodrai <isolodrai@meta.com>
---
kernel/bpf/helpers.c | 48 ++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 48 insertions(+)
diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index b71e428ad936..b8a7dbc971b4 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -2906,6 +2906,53 @@ __bpf_kfunc int bpf_dynptr_copy(struct bpf_dynptr *dst_ptr, u32 dst_off,
return 0;
}
+/**
+ * bpf_dynptr_memset() - Fill dynptr memory with a constant byte.
+ * @ptr: Destination dynptr - where data will be filled
+ * @ptr_off: Offset into the dynptr to start filling from
+ * @size: Number of bytes to fill
+ * @val: Constant byte to fill the memory with
+ *
+ * Fills the size bytes of the memory area pointed to by ptr
+ * at offset ptr_off with the constant byte val.
+ * Returns 0 on success; negative error, otherwise.
+ */
+ __bpf_kfunc int bpf_dynptr_memset(struct bpf_dynptr *ptr, u32 ptr_off, u32 size, u8 val)
+ {
+ struct bpf_dynptr_kern *p = (struct bpf_dynptr_kern *)ptr;
+ char buf[256];
+ u32 chunk_sz;
+ void* slice;
+ u32 offset;
+ int err;
+
+ if (__bpf_dynptr_is_rdonly(p))
+ return -EINVAL;
+
+ err = bpf_dynptr_check_off_len(p, ptr_off, size);
+ if (err)
+ return err;
+
+ slice = bpf_dynptr_slice_rdwr(ptr, ptr_off, NULL, size);
+ if (likely(slice)) {
+ memset(slice, val, size);
+ return 0;
+ }
+
+ /* Non-linear data under the dynptr, write from a local buffer */
+ chunk_sz = min_t(u32, sizeof(buf), size);
+ memset(buf, val, chunk_sz);
+
+ for (offset = ptr_off; offset < ptr_off + size; offset += chunk_sz) {
+ chunk_sz = min_t(u32, sizeof(buf), size - offset);
+ err = __bpf_dynptr_write(p, offset, buf, chunk_sz, 0);
+ if (err)
+ return err;
+ }
+
+ return 0;
+}
+
__bpf_kfunc void *bpf_cast_to_kern_ctx(void *obj)
{
return obj;
@@ -3364,6 +3411,7 @@ 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)
+BTF_ID_FLAGS(func, bpf_dynptr_memset)
#ifdef CONFIG_NET
BTF_ID_FLAGS(func, bpf_modify_return_test_tp)
#endif
--
2.47.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH bpf-next v2 2/2] selftests/bpf: add test cases for bpf_dynptr_memset()
2025-06-24 20:52 [PATCH bpf-next v2 0/2] bpf: add bpf_dynptr_memset() kfunc Ihor Solodrai
2025-06-24 20:52 ` [PATCH bpf-next v2 1/2] " Ihor Solodrai
@ 2025-06-24 20:52 ` Ihor Solodrai
2025-06-25 11:45 ` Mykyta Yatsenko
1 sibling, 1 reply; 11+ messages in thread
From: Ihor Solodrai @ 2025-06-24 20:52 UTC (permalink / raw)
To: bpf; +Cc: andrii, ast, eddyz87, mykyta.yatsenko5, mykolal, kernel-team
Add tests to verify the behavior of bpf_dynptr_memset():
* normal memset 0
* normal memset non-0
* memset with an offset
* memset in dynptr that was adjusted
* error: size overflow
* error: offset+size overflow
* error: readonly dynptr
* memset into non-linear xdp dynptr
Signed-off-by: Ihor Solodrai <isolodrai@meta.com>
---
.../testing/selftests/bpf/prog_tests/dynptr.c | 8 +
.../selftests/bpf/progs/dynptr_success.c | 164 ++++++++++++++++++
2 files changed, 172 insertions(+)
diff --git a/tools/testing/selftests/bpf/prog_tests/dynptr.c b/tools/testing/selftests/bpf/prog_tests/dynptr.c
index 62e7ec775f24..f2b65398afce 100644
--- a/tools/testing/selftests/bpf/prog_tests/dynptr.c
+++ b/tools/testing/selftests/bpf/prog_tests/dynptr.c
@@ -21,6 +21,14 @@ static struct {
{"test_dynptr_data", SETUP_SYSCALL_SLEEP},
{"test_dynptr_copy", SETUP_SYSCALL_SLEEP},
{"test_dynptr_copy_xdp", SETUP_XDP_PROG},
+ {"test_dynptr_memset_zero", SETUP_SYSCALL_SLEEP},
+ {"test_dynptr_memset_notzero", SETUP_SYSCALL_SLEEP},
+ {"test_dynptr_memset_zero_offset", SETUP_SYSCALL_SLEEP},
+ {"test_dynptr_memset_zero_adjusted", SETUP_SYSCALL_SLEEP},
+ {"test_dynptr_memset_overflow", SETUP_SYSCALL_SLEEP},
+ {"test_dynptr_memset_overflow_offset", SETUP_SYSCALL_SLEEP},
+ {"test_dynptr_memset_readonly", SETUP_SKB_PROG},
+ {"test_dynptr_memset_xdp_chunks", SETUP_XDP_PROG},
{"test_ringbuf", SETUP_SYSCALL_SLEEP},
{"test_skb_readonly", SETUP_SKB_PROG},
{"test_dynptr_skb_data", SETUP_SKB_PROG},
diff --git a/tools/testing/selftests/bpf/progs/dynptr_success.c b/tools/testing/selftests/bpf/progs/dynptr_success.c
index a0391f9da2d4..61d9ae2c6a52 100644
--- a/tools/testing/selftests/bpf/progs/dynptr_success.c
+++ b/tools/testing/selftests/bpf/progs/dynptr_success.c
@@ -681,6 +681,170 @@ int test_dynptr_copy_xdp(struct xdp_md *xdp)
return XDP_DROP;
}
+char memset_zero_data[] = "data to be zeroed";
+
+SEC("?tp/syscalls/sys_enter_nanosleep")
+int test_dynptr_memset_zero(void *ctx)
+{
+ __u32 data_sz = sizeof(memset_zero_data);
+ char zeroes[32] = {'\0'};
+ struct bpf_dynptr ptr;
+
+ err = bpf_dynptr_from_mem(memset_zero_data, data_sz, 0, &ptr);
+ err = err ?: bpf_dynptr_memset(&ptr, 0, data_sz, 0);
+ err = err ?: bpf_memcmp(zeroes, memset_zero_data, data_sz);
+
+ return 0;
+}
+
+#define DYNPTR_MEMSET_VAL 42
+
+char memset_notzero_data[] = "data to be overwritten";
+
+SEC("?tp/syscalls/sys_enter_nanosleep")
+int test_dynptr_memset_notzero(void *ctx)
+{
+ u32 data_sz = sizeof(memset_notzero_data);
+ struct bpf_dynptr ptr;
+ char expected[32];
+
+ __builtin_memset(expected, DYNPTR_MEMSET_VAL, data_sz);
+
+ err = bpf_dynptr_from_mem(memset_notzero_data, data_sz, 0, &ptr);
+ err = err ?: bpf_dynptr_memset(&ptr, 0, data_sz, DYNPTR_MEMSET_VAL);
+ err = err ?: bpf_memcmp(expected, memset_notzero_data, data_sz);
+
+ return 0;
+}
+
+char memset_zero_offset_data[] = "data to be zeroed partially";
+
+SEC("?tp/syscalls/sys_enter_nanosleep")
+int test_dynptr_memset_zero_offset(void *ctx)
+{
+ char expected[] = "data to \0\0\0\0eroed partially";
+ __u32 data_sz = sizeof(memset_zero_offset_data);
+ struct bpf_dynptr ptr;
+
+ err = bpf_dynptr_from_mem(memset_zero_offset_data, data_sz, 0, &ptr);
+ err = err ?: bpf_dynptr_memset(&ptr, 8, 4, 0);
+ err = err ?: bpf_memcmp(expected, memset_zero_offset_data, data_sz);
+
+ return 0;
+}
+
+char memset_zero_adjusted_data[] = "data to be zeroed partially";
+
+SEC("?tp/syscalls/sys_enter_nanosleep")
+int test_dynptr_memset_zero_adjusted(void *ctx)
+{
+ char expected[] = "data\0\0\0\0be zeroed partially";
+ __u32 data_sz = sizeof(memset_zero_adjusted_data);
+ struct bpf_dynptr ptr;
+
+ err = bpf_dynptr_from_mem(memset_zero_adjusted_data, data_sz, 0, &ptr);
+ err = err ?: bpf_dynptr_adjust(&ptr, 4, 8);
+ err = err ?: bpf_dynptr_memset(&ptr, 0, bpf_dynptr_size(&ptr), 0);
+ err = err ?: bpf_memcmp(expected, memset_zero_adjusted_data, data_sz);
+
+ return 0;
+}
+
+char memset_overflow_data[] = "memset overflow data";
+
+SEC("?tp/syscalls/sys_enter_nanosleep")
+int test_dynptr_memset_overflow(void *ctx)
+{
+ __u32 data_sz = sizeof(memset_overflow_data);
+ struct bpf_dynptr ptr;
+ int ret;
+
+ err = bpf_dynptr_from_mem(memset_overflow_data, data_sz, 0, &ptr);
+ ret = bpf_dynptr_memset(&ptr, 0, data_sz + 1, 0);
+ if (ret != -E2BIG)
+ err = 1;
+
+ return 0;
+}
+
+SEC("?tp/syscalls/sys_enter_nanosleep")
+int test_dynptr_memset_overflow_offset(void *ctx)
+{
+ __u32 data_sz = sizeof(memset_overflow_data);
+ struct bpf_dynptr ptr;
+ int ret;
+
+ err = bpf_dynptr_from_mem(memset_overflow_data, data_sz, 0, &ptr);
+ ret = bpf_dynptr_memset(&ptr, 1, data_sz, 0);
+ if (ret != -E2BIG)
+ err = 1;
+
+ return 0;
+}
+
+SEC("?cgroup_skb/egress")
+int test_dynptr_memset_readonly(struct __sk_buff *skb)
+{
+ struct bpf_dynptr ptr;
+ int ret;
+
+ err = bpf_dynptr_from_skb(skb, 0, &ptr);
+
+ /* cgroup skbs are read only, memset should fail */
+ ret = bpf_dynptr_memset(&ptr, 0, bpf_dynptr_size(&ptr), 0);
+ if (ret != -EINVAL)
+ err = 1;
+
+ return 0;
+}
+
+SEC("xdp")
+int test_dynptr_memset_xdp_chunks(struct xdp_md *xdp)
+{
+ const int max_chunks = 200;
+ struct bpf_dynptr ptr_xdp;
+ u32 data_sz, offset = 0;
+ char expected_buf[32];
+ char buf[32];
+ int i;
+
+ __builtin_memset(expected_buf, DYNPTR_MEMSET_VAL, sizeof(expected_buf));
+
+ /* ptr_xdp is backed by non-contiguous memory */
+ bpf_dynptr_from_xdp(xdp, 0, &ptr_xdp);
+ data_sz = bpf_dynptr_size(&ptr_xdp);
+
+ err = bpf_dynptr_memset(&ptr_xdp, 0, data_sz, DYNPTR_MEMSET_VAL);
+ if (err)
+ goto out;
+
+ bpf_for(i, 0, max_chunks) {
+ offset = i * sizeof(buf);
+ err = bpf_dynptr_read(&buf, sizeof(buf), &ptr_xdp, offset, 0);
+ switch (err) {
+ case 0:
+ break;
+ case -E2BIG:
+ goto handle_tail;
+ default:
+ goto out;
+ }
+ err = bpf_memcmp(buf, expected_buf, sizeof(buf));
+ if (err)
+ goto out;
+ }
+
+handle_tail:
+ if (data_sz - offset < sizeof(buf)) {
+ err = bpf_dynptr_read(&buf, data_sz - offset, &ptr_xdp, offset, 0);
+ if (err)
+ goto out;
+ err = bpf_memcmp(buf, expected_buf, data_sz - offset);
+ }
+out:
+ return XDP_DROP;
+}
+
void *user_ptr;
/* Contains the copy of the data pointed by user_ptr.
* Size 384 to make it not fit into a single kernel chunk when copying
--
2.47.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH bpf-next v2 1/2] bpf: add bpf_dynptr_memset() kfunc
2025-06-24 20:52 ` [PATCH bpf-next v2 1/2] " Ihor Solodrai
@ 2025-06-24 21:50 ` Andrii Nakryiko
2025-06-25 11:27 ` Mykyta Yatsenko
1 sibling, 0 replies; 11+ messages in thread
From: Andrii Nakryiko @ 2025-06-24 21:50 UTC (permalink / raw)
To: ihor.solodrai
Cc: bpf, andrii, ast, eddyz87, mykyta.yatsenko5, mykolal, kernel-team
On Tue, Jun 24, 2025 at 1:53 PM Ihor Solodrai <isolodrai@meta.com> wrote:
>
> Currently there is no straightforward way to fill dynptr memory with a
> value (most commonly zero). One can do it with bpf_dynptr_write(), but
> a temporary buffer is necessary for that.
>
> Implement bpf_dynptr_memset() - an analogue of memset() from libc.
>
> Signed-off-by: Ihor Solodrai <isolodrai@meta.com>
> ---
> kernel/bpf/helpers.c | 48 ++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 48 insertions(+)
>
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index b71e428ad936..b8a7dbc971b4 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -2906,6 +2906,53 @@ __bpf_kfunc int bpf_dynptr_copy(struct bpf_dynptr *dst_ptr, u32 dst_off,
> return 0;
> }
>
> +/**
> + * bpf_dynptr_memset() - Fill dynptr memory with a constant byte.
> + * @ptr: Destination dynptr - where data will be filled
> + * @ptr_off: Offset into the dynptr to start filling from
> + * @size: Number of bytes to fill
> + * @val: Constant byte to fill the memory with
> + *
> + * Fills the size bytes of the memory area pointed to by ptr
> + * at offset ptr_off with the constant byte val.
> + * Returns 0 on success; negative error, otherwise.
> + */
> + __bpf_kfunc int bpf_dynptr_memset(struct bpf_dynptr *ptr, u32 ptr_off, u32 size, u8 val)
nit: ptr_off -> offset, let's keep consistent naming with other APIs
(as much as possible)
> + {
> + struct bpf_dynptr_kern *p = (struct bpf_dynptr_kern *)ptr;
> + char buf[256];
> + u32 chunk_sz;
> + void* slice;
> + u32 offset;
nit: combine chunk_sz and offset on single line
> + int err;
> +
> + if (__bpf_dynptr_is_rdonly(p))
> + return -EINVAL;
> +
> + err = bpf_dynptr_check_off_len(p, ptr_off, size);
> + if (err)
> + return err;
> +
> + slice = bpf_dynptr_slice_rdwr(ptr, ptr_off, NULL, size);
> + if (likely(slice)) {
> + memset(slice, val, size);
> + return 0;
> + }
> +
> + /* Non-linear data under the dynptr, write from a local buffer */
> + chunk_sz = min_t(u32, sizeof(buf), size);
> + memset(buf, val, chunk_sz);
> +
> + for (offset = ptr_off; offset < ptr_off + size; offset += chunk_sz) {
> + chunk_sz = min_t(u32, sizeof(buf), size - offset);
you have offset = ptr_off + chunk offset, so size - offset seems
wrong, it should be `size - offset + ptr_off` to "neutralize" ptr_off
itself. I'd probably write the for loop using
for (offset = 0; offset < size; offset += chunk_sz) {
chunk_sz = min_t(u32, sizeof(buf), size - offset);
err = __bpf_dynptr_write(p, ptr_off + offset, buf, chink_sz, 0);
...
seems simpler to just add that ptr_off in dynptr_write call
pw-bot: cr
> + err = __bpf_dynptr_write(p, offset, buf, chunk_sz, 0);
> + if (err)
> + return err;
> + }
> +
> + return 0;
> +}
> +
> __bpf_kfunc void *bpf_cast_to_kern_ctx(void *obj)
> {
> return obj;
> @@ -3364,6 +3411,7 @@ 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)
> +BTF_ID_FLAGS(func, bpf_dynptr_memset)
> #ifdef CONFIG_NET
> BTF_ID_FLAGS(func, bpf_modify_return_test_tp)
> #endif
> --
> 2.47.1
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH bpf-next v2 1/2] bpf: add bpf_dynptr_memset() kfunc
2025-06-24 20:52 ` [PATCH bpf-next v2 1/2] " Ihor Solodrai
2025-06-24 21:50 ` Andrii Nakryiko
@ 2025-06-25 11:27 ` Mykyta Yatsenko
2025-06-30 21:05 ` Ihor Solodrai
1 sibling, 1 reply; 11+ messages in thread
From: Mykyta Yatsenko @ 2025-06-25 11:27 UTC (permalink / raw)
To: ihor.solodrai, bpf; +Cc: andrii, ast, eddyz87, mykolal, kernel-team
On 6/24/25 21:52, Ihor Solodrai wrote:
> Currently there is no straightforward way to fill dynptr memory with a
> value (most commonly zero). One can do it with bpf_dynptr_write(), but
> a temporary buffer is necessary for that.
>
> Implement bpf_dynptr_memset() - an analogue of memset() from libc.
>
> Signed-off-by: Ihor Solodrai <isolodrai@meta.com>
> ---
> kernel/bpf/helpers.c | 48 ++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 48 insertions(+)
>
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index b71e428ad936..b8a7dbc971b4 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -2906,6 +2906,53 @@ __bpf_kfunc int bpf_dynptr_copy(struct bpf_dynptr *dst_ptr, u32 dst_off,
> return 0;
> }
>
> +/**
> + * bpf_dynptr_memset() - Fill dynptr memory with a constant byte.
> + * @ptr: Destination dynptr - where data will be filled
> + * @ptr_off: Offset into the dynptr to start filling from
> + * @size: Number of bytes to fill
> + * @val: Constant byte to fill the memory with
> + *
> + * Fills the size bytes of the memory area pointed to by ptr
> + * at offset ptr_off with the constant byte val.
> + * Returns 0 on success; negative error, otherwise.
> + */
> + __bpf_kfunc int bpf_dynptr_memset(struct bpf_dynptr *ptr, u32 ptr_off, u32 size, u8 val)
> + {
> + struct bpf_dynptr_kern *p = (struct bpf_dynptr_kern *)ptr;
> + char buf[256];
> + u32 chunk_sz;
> + void* slice;
> + u32 offset;
> + int err;
> +
> + if (__bpf_dynptr_is_rdonly(p))
> + return -EINVAL;
> +
> + err = bpf_dynptr_check_off_len(p, ptr_off, size);
> + if (err)
> + return err;
> +
> + slice = bpf_dynptr_slice_rdwr(ptr, ptr_off, NULL, size);
> + if (likely(slice)) {
> + memset(slice, val, size);
> + return 0;
> + }
bpf_dynptr_slice_rdwr is doing rdonly and off_len checks anyways, so perhaps we can
avoid calling __bpf_dynptr_is_rdonly and bpf_dynptr_check_off_len before bpf_dynptr_slice_rdwr,
that'll make fast path a little faster.
> +
> + /* Non-linear data under the dynptr, write from a local buffer */
> + chunk_sz = min_t(u32, sizeof(buf), size);
> + memset(buf, val, chunk_sz);
> +
> + for (offset = ptr_off; offset < ptr_off + size; offset += chunk_sz) {
> + chunk_sz = min_t(u32, sizeof(buf), size - offset);
> + err = __bpf_dynptr_write(p, offset, buf, chunk_sz, 0);
> + if (err)
> + return err;
> + }
> +
> + return 0;
> +}
> +
> __bpf_kfunc void *bpf_cast_to_kern_ctx(void *obj)
> {
> return obj;
> @@ -3364,6 +3411,7 @@ 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)
> +BTF_ID_FLAGS(func, bpf_dynptr_memset)
> #ifdef CONFIG_NET
> BTF_ID_FLAGS(func, bpf_modify_return_test_tp)
> #endif
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH bpf-next v2 2/2] selftests/bpf: add test cases for bpf_dynptr_memset()
2025-06-24 20:52 ` [PATCH bpf-next v2 2/2] selftests/bpf: add test cases for bpf_dynptr_memset() Ihor Solodrai
@ 2025-06-25 11:45 ` Mykyta Yatsenko
2025-06-26 1:25 ` Ihor Solodrai
0 siblings, 1 reply; 11+ messages in thread
From: Mykyta Yatsenko @ 2025-06-25 11:45 UTC (permalink / raw)
To: ihor.solodrai, bpf; +Cc: andrii, ast, eddyz87, mykolal, kernel-team
On 6/24/25 21:52, Ihor Solodrai wrote:
> Add tests to verify the behavior of bpf_dynptr_memset():
> * normal memset 0
> * normal memset non-0
> * memset with an offset
> * memset in dynptr that was adjusted
> * error: size overflow
> * error: offset+size overflow
> * error: readonly dynptr
> * memset into non-linear xdp dynptr
>
> Signed-off-by: Ihor Solodrai <isolodrai@meta.com>
> ---
> .../testing/selftests/bpf/prog_tests/dynptr.c | 8 +
> .../selftests/bpf/progs/dynptr_success.c | 164 ++++++++++++++++++
> 2 files changed, 172 insertions(+)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/dynptr.c b/tools/testing/selftests/bpf/prog_tests/dynptr.c
> index 62e7ec775f24..f2b65398afce 100644
> --- a/tools/testing/selftests/bpf/prog_tests/dynptr.c
> +++ b/tools/testing/selftests/bpf/prog_tests/dynptr.c
> @@ -21,6 +21,14 @@ static struct {
> {"test_dynptr_data", SETUP_SYSCALL_SLEEP},
> {"test_dynptr_copy", SETUP_SYSCALL_SLEEP},
> {"test_dynptr_copy_xdp", SETUP_XDP_PROG},
> + {"test_dynptr_memset_zero", SETUP_SYSCALL_SLEEP},
> + {"test_dynptr_memset_notzero", SETUP_SYSCALL_SLEEP},
> + {"test_dynptr_memset_zero_offset", SETUP_SYSCALL_SLEEP},
> + {"test_dynptr_memset_zero_adjusted", SETUP_SYSCALL_SLEEP},
> + {"test_dynptr_memset_overflow", SETUP_SYSCALL_SLEEP},
> + {"test_dynptr_memset_overflow_offset", SETUP_SYSCALL_SLEEP},
> + {"test_dynptr_memset_readonly", SETUP_SKB_PROG},
> + {"test_dynptr_memset_xdp_chunks", SETUP_XDP_PROG},
> {"test_ringbuf", SETUP_SYSCALL_SLEEP},
> {"test_skb_readonly", SETUP_SKB_PROG},
> {"test_dynptr_skb_data", SETUP_SKB_PROG},
> diff --git a/tools/testing/selftests/bpf/progs/dynptr_success.c b/tools/testing/selftests/bpf/progs/dynptr_success.c
> index a0391f9da2d4..61d9ae2c6a52 100644
> --- a/tools/testing/selftests/bpf/progs/dynptr_success.c
> +++ b/tools/testing/selftests/bpf/progs/dynptr_success.c
> @@ -681,6 +681,170 @@ int test_dynptr_copy_xdp(struct xdp_md *xdp)
> return XDP_DROP;
> }
>
> +char memset_zero_data[] = "data to be zeroed";
> +
> +SEC("?tp/syscalls/sys_enter_nanosleep")
> +int test_dynptr_memset_zero(void *ctx)
> +{
> + __u32 data_sz = sizeof(memset_zero_data);
> + char zeroes[32] = {'\0'};
> + struct bpf_dynptr ptr;
> +
> + err = bpf_dynptr_from_mem(memset_zero_data, data_sz, 0, &ptr);
> + err = err ?: bpf_dynptr_memset(&ptr, 0, data_sz, 0);
> + err = err ?: bpf_memcmp(zeroes, memset_zero_data, data_sz);
> +
> + return 0;
> +}
> +
> +#define DYNPTR_MEMSET_VAL 42
> +
> +char memset_notzero_data[] = "data to be overwritten";
> +
> +SEC("?tp/syscalls/sys_enter_nanosleep")
> +int test_dynptr_memset_notzero(void *ctx)
> +{
> + u32 data_sz = sizeof(memset_notzero_data);
> + struct bpf_dynptr ptr;
> + char expected[32];
> +
> + __builtin_memset(expected, DYNPTR_MEMSET_VAL, data_sz);
> +
> + err = bpf_dynptr_from_mem(memset_notzero_data, data_sz, 0, &ptr);
> + err = err ?: bpf_dynptr_memset(&ptr, 0, data_sz, DYNPTR_MEMSET_VAL);
> + err = err ?: bpf_memcmp(expected, memset_notzero_data, data_sz);
> +
> + return 0;
> +}
> +
> +char memset_zero_offset_data[] = "data to be zeroed partially";
> +
> +SEC("?tp/syscalls/sys_enter_nanosleep")
> +int test_dynptr_memset_zero_offset(void *ctx)
> +{
> + char expected[] = "data to \0\0\0\0eroed partially";
> + __u32 data_sz = sizeof(memset_zero_offset_data);
> + struct bpf_dynptr ptr;
> +
> + err = bpf_dynptr_from_mem(memset_zero_offset_data, data_sz, 0, &ptr);
> + err = err ?: bpf_dynptr_memset(&ptr, 8, 4, 0);
> + err = err ?: bpf_memcmp(expected, memset_zero_offset_data, data_sz);
> +
> + return 0;
> +}
> +
> +char memset_zero_adjusted_data[] = "data to be zeroed partially";
> +
> +SEC("?tp/syscalls/sys_enter_nanosleep")
> +int test_dynptr_memset_zero_adjusted(void *ctx)
> +{
> + char expected[] = "data\0\0\0\0be zeroed partially";
> + __u32 data_sz = sizeof(memset_zero_adjusted_data);
> + struct bpf_dynptr ptr;
> +
> + err = bpf_dynptr_from_mem(memset_zero_adjusted_data, data_sz, 0, &ptr);
> + err = err ?: bpf_dynptr_adjust(&ptr, 4, 8);
> + err = err ?: bpf_dynptr_memset(&ptr, 0, bpf_dynptr_size(&ptr), 0);
> + err = err ?: bpf_memcmp(expected, memset_zero_adjusted_data, data_sz);
> +
> + return 0;
> +}
> +
> +char memset_overflow_data[] = "memset overflow data";
> +
> +SEC("?tp/syscalls/sys_enter_nanosleep")
> +int test_dynptr_memset_overflow(void *ctx)
> +{
> + __u32 data_sz = sizeof(memset_overflow_data);
> + struct bpf_dynptr ptr;
> + int ret;
> +
> + err = bpf_dynptr_from_mem(memset_overflow_data, data_sz, 0, &ptr);
> + ret = bpf_dynptr_memset(&ptr, 0, data_sz + 1, 0);
> + if (ret != -E2BIG)
> + err = 1;
> +
> + return 0;
> +}
> +
> +SEC("?tp/syscalls/sys_enter_nanosleep")
> +int test_dynptr_memset_overflow_offset(void *ctx)
> +{
> + __u32 data_sz = sizeof(memset_overflow_data);
> + struct bpf_dynptr ptr;
> + int ret;
> +
> + err = bpf_dynptr_from_mem(memset_overflow_data, data_sz, 0, &ptr);
> + ret = bpf_dynptr_memset(&ptr, 1, data_sz, 0);
> + if (ret != -E2BIG)
> + err = 1;
> +
> + return 0;
> +}
> +
> +SEC("?cgroup_skb/egress")
> +int test_dynptr_memset_readonly(struct __sk_buff *skb)
> +{
> + struct bpf_dynptr ptr;
> + int ret;
> +
> + err = bpf_dynptr_from_skb(skb, 0, &ptr);
> +
> + /* cgroup skbs are read only, memset should fail */
> + ret = bpf_dynptr_memset(&ptr, 0, bpf_dynptr_size(&ptr), 0);
> + if (ret != -EINVAL)
> + err = 1;
> +
> + return 0;
> +}
> +
> +SEC("xdp")
> +int test_dynptr_memset_xdp_chunks(struct xdp_md *xdp)
> +{
> + const int max_chunks = 200;
> + struct bpf_dynptr ptr_xdp;
> + u32 data_sz, offset = 0;
> + char expected_buf[32];
nit: expected_buf[32] = {DYNPTR_MEMSET_VAL};
> + char buf[32];
> + int i;
> +
> + __builtin_memset(expected_buf, DYNPTR_MEMSET_VAL, sizeof(expected_buf));
> +
> + /* ptr_xdp is backed by non-contiguous memory */
> + bpf_dynptr_from_xdp(xdp, 0, &ptr_xdp);
> + data_sz = bpf_dynptr_size(&ptr_xdp);
> +
> + err = bpf_dynptr_memset(&ptr_xdp, 0, data_sz, DYNPTR_MEMSET_VAL);
> + if (err)
> + goto out;
> +
Maybe we can calculate max_chunks instead of hardcoding, something like:
max_chunks = data_sz / sizeof(expected_buf) + (data_sz %
sizeof(expected_buf) ? 1 : 0);
> + bpf_for(i, 0, max_chunks) {
> + offset = i * sizeof(buf);
> + err = bpf_dynptr_read(&buf, sizeof(buf), &ptr_xdp, offset, 0);
handle_tail seems unnecessary, maybe handle tail in the main loop:
__u32 sz = min_t(data_sz - offset : sizeof(buf));
bpf_dynptr_read(&buf, sz, &ptr_xdp, offset, 0);
> + switch (err) {
> + case 0:
> + break;
> + case -E2BIG:
> + goto handle_tail;
> + default:
> + goto out;
> + }
> + err = bpf_memcmp(buf, expected_buf, sizeof(buf));
> + if (err)
> + goto out;
> + }
> +
> +handle_tail:
> + if (data_sz - offset < sizeof(buf)) {
> + err = bpf_dynptr_read(&buf, data_sz - offset, &ptr_xdp, offset, 0);
> + if (err)
> + goto out;
> + err = bpf_memcmp(buf, expected_buf, data_sz - offset);
> + }
> +out:
> + return XDP_DROP;
> +}
> +
> void *user_ptr;
> /* Contains the copy of the data pointed by user_ptr.
> * Size 384 to make it not fit into a single kernel chunk when copying
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH bpf-next v2 2/2] selftests/bpf: add test cases for bpf_dynptr_memset()
2025-06-25 11:45 ` Mykyta Yatsenko
@ 2025-06-26 1:25 ` Ihor Solodrai
2025-06-26 13:34 ` Mykyta Yatsenko
0 siblings, 1 reply; 11+ messages in thread
From: Ihor Solodrai @ 2025-06-26 1:25 UTC (permalink / raw)
To: Mykyta Yatsenko, bpf; +Cc: andrii, ast, eddyz87, mykolal, kernel-team
On 6/25/25 4:45 AM, Mykyta Yatsenko wrote:
> On 6/24/25 21:52, Ihor Solodrai wrote:
>> Add tests to verify the behavior of bpf_dynptr_memset():
>> * normal memset 0
>> * normal memset non-0
>> * memset with an offset
>> * memset in dynptr that was adjusted
>> * error: size overflow
>> * error: offset+size overflow
>> * error: readonly dynptr
>> * memset into non-linear xdp dynptr
>>
>> Signed-off-by: Ihor Solodrai <isolodrai@meta.com>
>> ---
>> .../testing/selftests/bpf/prog_tests/dynptr.c | 8 +
>> .../selftests/bpf/progs/dynptr_success.c | 164 ++++++++++++++++++
>> 2 files changed, 172 insertions(+)
>>
>> diff --git a/tools/testing/selftests/bpf/prog_tests/dynptr.c b/tools/
>> testing/selftests/bpf/prog_tests/dynptr.c
>> index 62e7ec775f24..f2b65398afce 100644
>> --- a/tools/testing/selftests/bpf/prog_tests/dynptr.c
>> +++ b/tools/testing/selftests/bpf/prog_tests/dynptr.c
>> @@ -21,6 +21,14 @@ static struct {
>> [...]
>> +
>> +SEC("xdp")
>> +int test_dynptr_memset_xdp_chunks(struct xdp_md *xdp)
>> +{
>> + const int max_chunks = 200;
>> + struct bpf_dynptr ptr_xdp;
>> + u32 data_sz, offset = 0;
A question not directly to Mykyta.
So noalu32 version of this test was failing to verify with this:
118: (85) call bpf_dynptr_read#201
R2 min value is negative, either use unsigned or 'var &= const'
Where R2 refers to `data_sz - offset`
Full log here:
https://github.com/kernel-patches/bpf/actions/runs/15861036149/job/44718289284
I tried various conditions unsuccessfully. But changing u32 to u64
made it work. If handle_tail part is removed, as Mykyta suggested,
this doesn't matter, so I will probably leave u32 in v3.
However I am curious if u32->u64 change is an appropriate workaround
in general for noalu32 problems? AFAIU verifier might get confused by
all the added shifts, and "noalu32" is a backward compatibility thing.
>> + char expected_buf[32];
> nit: expected_buf[32] = {DYNPTR_MEMSET_VAL};
I tried that at the beginning. As it turns out, this doesn't work in
BPF the way you'd expect:
Here is a piece of llvm-objdump with explicit memset:
0000000000000968 <test_dynptr_memset_xdp_chunks>:
301: 18 02 00 00 2a 2a 2a 2a 00 00 00 00 2a 2a 2a 2a r2 =
0x2a2a2a2a2a2a2a2a ll
303: 7b 2a c8 ff 00 00 00 00 *(u64 *)(r10 - 0x38) = r2
304: 7b 2a d0 ff 00 00 00 00 *(u64 *)(r10 - 0x30) = r2
305: 7b 2a d8 ff 00 00 00 00 *(u64 *)(r10 - 0x28) = r2
306: 7b 2a e0 ff 00 00 00 00 *(u64 *)(r10 - 0x20) = r2
307: bf a7 00 00 00 00 00 00 r7 = r10
308: 07 07 00 00 e8 ff ff ff r7 += -0x18
309: b7 02 00 00 00 00 00 00 r2 = 0x0
310: bf 73 00 00 00 00 00 00 r3 = r7
311: 85 10 00 00 ff ff ff ff call -0x1
...
You can clearly see a piece of stack filling up with 0x2a
After applying this diff:
diff --git a/tools/testing/selftests/bpf/progs/dynptr_success.c
b/tools/testing/selftests/bpf/progs/dynptr_success.c
index 5120acb8b15a..5b351f6fe07c 100644
--- a/tools/testing/selftests/bpf/progs/dynptr_success.c
+++ b/tools/testing/selftests/bpf/progs/dynptr_success.c
@@ -809,12 +809,10 @@ int test_dynptr_memset_xdp_chunks(struct xdp_md *xdp)
const int max_chunks = 200;
struct bpf_dynptr ptr_xdp;
u32 data_sz, chunk_sz, offset = 0;
- char expected_buf[32];
+ char expected_buf[32] = { DYNPTR_MEMSET_VAL };
char buf[32];
int i;
- __builtin_memset(expected_buf, DYNPTR_MEMSET_VAL,
sizeof(expected_buf));
-
/* ptr_xdp is backed by non-contiguous memory */
bpf_dynptr_from_xdp(xdp, 0, &ptr_xdp);
data_sz = bpf_dynptr_size(&ptr_xdp);
We get the following:
0000000000000968 <test_dynptr_memset_xdp_chunks>:
301: bf a7 00 00 00 00 00 00 r7 = r10
302: 07 07 00 00 e8 ff ff ff r7 += -0x18
303: b7 02 00 00 00 00 00 00 r2 = 0x0
304: bf 73 00 00 00 00 00 00 r3 = r7
305: 85 10 00 00 ff ff ff ff call -0x1
...
The stack allocated array is not initialized.
Could be an LLVM bug/incompleteness? I used LLVM 19 while developing.
>> + char buf[32];
>> + int i;
>> +
>> + __builtin_memset(expected_buf, DYNPTR_MEMSET_VAL,
>> sizeof(expected_buf));
>> +
>> + /* ptr_xdp is backed by non-contiguous memory */
>> + bpf_dynptr_from_xdp(xdp, 0, &ptr_xdp);
>> + data_sz = bpf_dynptr_size(&ptr_xdp);
>> +
>> + err = bpf_dynptr_memset(&ptr_xdp, 0, data_sz, DYNPTR_MEMSET_VAL);
>> + if (err)
>> + goto out;
>> +
> Maybe we can calculate max_chunks instead of hardcoding, something like:
> max_chunks = data_sz / sizeof(expected_buf) + (data_sz %
> sizeof(expected_buf) ? 1 : 0);
I don't see a point of doing it for this test. max_chunks is just a
big enough arbitrary constant that works. We do a similar thing in
other tests.
>> + bpf_for(i, 0, max_chunks) {
>> + offset = i * sizeof(buf);
>> + err = bpf_dynptr_read(&buf, sizeof(buf), &ptr_xdp, offset, 0);
>
> handle_tail seems unnecessary, maybe handle tail in the main loop:
> __u32 sz = min_t(data_sz - offset : sizeof(buf));
> bpf_dynptr_read(&buf, sz, &ptr_xdp, offset, 0);
>
Yeah, you're right.
It ended up like this because I've been fighting the verifier while
writing the test, and this version worked eventually. The critical
piece to uncofuse it was changing:
offset += sizeof(buf)
to
offset = i * sizeof(buf)
I will have to add min_t macro locally though.
>> + switch (err) {
>> + case 0:
>> + break;
>> + case -E2BIG:
>> + goto handle_tail;
>> + default:
>> + goto out;
>> + }
>> + err = bpf_memcmp(buf, expected_buf, sizeof(buf));
>> + if (err)
>> + goto out;
>> + }
>> +
>> +handle_tail:
>> + if (data_sz - offset < sizeof(buf)) {
>> + err = bpf_dynptr_read(&buf, data_sz - offset, &ptr_xdp,
>> offset, 0);
>> + if (err)
>> + goto out;
>> + err = bpf_memcmp(buf, expected_buf, data_sz - offset);
>> + }
>> +out:
>> + return XDP_DROP;
>> +}
>> +
>> void *user_ptr;
>> /* Contains the copy of the data pointed by user_ptr.
>> * Size 384 to make it not fit into a single kernel chunk when copying
>
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH bpf-next v2 2/2] selftests/bpf: add test cases for bpf_dynptr_memset()
2025-06-26 1:25 ` Ihor Solodrai
@ 2025-06-26 13:34 ` Mykyta Yatsenko
2025-06-30 21:02 ` Ihor Solodrai
0 siblings, 1 reply; 11+ messages in thread
From: Mykyta Yatsenko @ 2025-06-26 13:34 UTC (permalink / raw)
To: Ihor Solodrai, bpf; +Cc: andrii, ast, eddyz87, mykolal, kernel-team
On 6/26/25 02:25, Ihor Solodrai wrote:
> On 6/25/25 4:45 AM, Mykyta Yatsenko wrote:
>> On 6/24/25 21:52, Ihor Solodrai wrote:
>>> Add tests to verify the behavior of bpf_dynptr_memset():
>>> * normal memset 0
>>> * normal memset non-0
>>> * memset with an offset
>>> * memset in dynptr that was adjusted
>>> * error: size overflow
>>> * error: offset+size overflow
>>> * error: readonly dynptr
>>> * memset into non-linear xdp dynptr
>>>
>>> Signed-off-by: Ihor Solodrai <isolodrai@meta.com>
>>> ---
>>> .../testing/selftests/bpf/prog_tests/dynptr.c | 8 +
>>> .../selftests/bpf/progs/dynptr_success.c | 164
>>> ++++++++++++++++++
>>> 2 files changed, 172 insertions(+)
>>>
>>> diff --git a/tools/testing/selftests/bpf/prog_tests/dynptr.c
>>> b/tools/ testing/selftests/bpf/prog_tests/dynptr.c
>>> index 62e7ec775f24..f2b65398afce 100644
>>> --- a/tools/testing/selftests/bpf/prog_tests/dynptr.c
>>> +++ b/tools/testing/selftests/bpf/prog_tests/dynptr.c
>>> @@ -21,6 +21,14 @@ static struct {
>>> [...]
>>> +
>>> +SEC("xdp")
>>> +int test_dynptr_memset_xdp_chunks(struct xdp_md *xdp)
>>> +{
>>> + const int max_chunks = 200;
>>> + struct bpf_dynptr ptr_xdp;
>>> + u32 data_sz, offset = 0;
>
> A question not directly to Mykyta.
>
> So noalu32 version of this test was failing to verify with this:
>
> 118: (85) call bpf_dynptr_read#201
> R2 min value is negative, either use unsigned or 'var &= const'
>
> Where R2 refers to `data_sz - offset`
>
> Full log here:
> https://github.com/kernel-patches/bpf/actions/runs/15861036149/job/44718289284
>
> I tried various conditions unsuccessfully. But changing u32 to u64
> made it work. If handle_tail part is removed, as Mykyta suggested,
> this doesn't matter, so I will probably leave u32 in v3.
>
> However I am curious if u32->u64 change is an appropriate workaround
> in general for noalu32 problems? AFAIU verifier might get confused by
> all the added shifts, and "noalu32" is a backward compatibility thing.
>
>
>>> + char expected_buf[32];
>> nit: expected_buf[32] = {DYNPTR_MEMSET_VAL};
My bad, it's actually should be `char expected_buf[32] = {[0 ... 31] =
DYNPTR_MEMSET_VAL}`;
Otherwise it initializes just the first element of the expected_buf and
places that array into the .rodata.cst32 section.
>
> I tried that at the beginning. As it turns out, this doesn't work in
> BPF the way you'd expect:
>
> Here is a piece of llvm-objdump with explicit memset:
>
> 0000000000000968 <test_dynptr_memset_xdp_chunks>:
> 301: 18 02 00 00 2a 2a 2a 2a 00 00 00 00 2a 2a 2a 2a r2 =
> 0x2a2a2a2a2a2a2a2a ll
> 303: 7b 2a c8 ff 00 00 00 00 *(u64 *)(r10 - 0x38) = r2
> 304: 7b 2a d0 ff 00 00 00 00 *(u64 *)(r10 - 0x30) = r2
> 305: 7b 2a d8 ff 00 00 00 00 *(u64 *)(r10 - 0x28) = r2
> 306: 7b 2a e0 ff 00 00 00 00 *(u64 *)(r10 - 0x20) = r2
> 307: bf a7 00 00 00 00 00 00 r7 = r10
> 308: 07 07 00 00 e8 ff ff ff r7 += -0x18
> 309: b7 02 00 00 00 00 00 00 r2 = 0x0
> 310: bf 73 00 00 00 00 00 00 r3 = r7
> 311: 85 10 00 00 ff ff ff ff call -0x1
> ...
>
> You can clearly see a piece of stack filling up with 0x2a
>
> After applying this diff:
>
> diff --git a/tools/testing/selftests/bpf/progs/dynptr_success.c
> b/tools/testing/selftests/bpf/progs/dynptr_success.c
> index 5120acb8b15a..5b351f6fe07c 100644
> --- a/tools/testing/selftests/bpf/progs/dynptr_success.c
> +++ b/tools/testing/selftests/bpf/progs/dynptr_success.c
> @@ -809,12 +809,10 @@ int test_dynptr_memset_xdp_chunks(struct xdp_md
> *xdp)
> const int max_chunks = 200;
> struct bpf_dynptr ptr_xdp;
> u32 data_sz, chunk_sz, offset = 0;
> - char expected_buf[32];
> + char expected_buf[32] = { DYNPTR_MEMSET_VAL };
> char buf[32];
> int i;
>
> - __builtin_memset(expected_buf, DYNPTR_MEMSET_VAL,
> sizeof(expected_buf));
> -
> /* ptr_xdp is backed by non-contiguous memory */
> bpf_dynptr_from_xdp(xdp, 0, &ptr_xdp);
> data_sz = bpf_dynptr_size(&ptr_xdp);
>
> We get the following:
>
> 0000000000000968 <test_dynptr_memset_xdp_chunks>:
> 301: bf a7 00 00 00 00 00 00 r7 = r10
> 302: 07 07 00 00 e8 ff ff ff r7 += -0x18
> 303: b7 02 00 00 00 00 00 00 r2 = 0x0
> 304: bf 73 00 00 00 00 00 00 r3 = r7
> 305: 85 10 00 00 ff ff ff ff call -0x1
> ...
>
> The stack allocated array is not initialized.
> Could be an LLVM bug/incompleteness? I used LLVM 19 while developing.
>
>
>>> + char buf[32];
>>> + int i;
>>> +
>>> + __builtin_memset(expected_buf, DYNPTR_MEMSET_VAL,
>>> sizeof(expected_buf));
>>> +
>>> + /* ptr_xdp is backed by non-contiguous memory */
>>> + bpf_dynptr_from_xdp(xdp, 0, &ptr_xdp);
>>> + data_sz = bpf_dynptr_size(&ptr_xdp);
>>> +
>>> + err = bpf_dynptr_memset(&ptr_xdp, 0, data_sz, DYNPTR_MEMSET_VAL);
>>> + if (err)
>>> + goto out;
>>> +
>> Maybe we can calculate max_chunks instead of hardcoding, something like:
>> max_chunks = data_sz / sizeof(expected_buf) + (data_sz %
>> sizeof(expected_buf) ? 1 : 0);
>
> I don't see a point of doing it for this test. max_chunks is just a
> big enough arbitrary constant that works. We do a similar thing in
> other tests.
>
>>> + bpf_for(i, 0, max_chunks) {
>>> + offset = i * sizeof(buf);
>>> + err = bpf_dynptr_read(&buf, sizeof(buf), &ptr_xdp, offset, 0);
>>
>> handle_tail seems unnecessary, maybe handle tail in the main loop:
>> __u32 sz = min_t(data_sz - offset : sizeof(buf));
>> bpf_dynptr_read(&buf, sz, &ptr_xdp, offset, 0);
>>
>
> Yeah, you're right.
>
> It ended up like this because I've been fighting the verifier while
> writing the test, and this version worked eventually. The critical
> piece to uncofuse it was changing:
> offset += sizeof(buf)
> to
> offset = i * sizeof(buf)
>
> I will have to add min_t macro locally though.
>
>
>>> + switch (err) {
>>> + case 0:
>>> + break;
>>> + case -E2BIG:
>>> + goto handle_tail;
>>> + default:
>>> + goto out;
>>> + }
>>> + err = bpf_memcmp(buf, expected_buf, sizeof(buf));
>>> + if (err)
>>> + goto out;
>>> + }
>>> +
>>> +handle_tail:
>>> + if (data_sz - offset < sizeof(buf)) {
>>> + err = bpf_dynptr_read(&buf, data_sz - offset, &ptr_xdp,
>>> offset, 0);
>>> + if (err)
>>> + goto out;
>>> + err = bpf_memcmp(buf, expected_buf, data_sz - offset);
>>> + }
>>> +out:
>>> + return XDP_DROP;
>>> +}
>>> +
>>> void *user_ptr;
>>> /* Contains the copy of the data pointed by user_ptr.
>>> * Size 384 to make it not fit into a single kernel chunk when
>>> copying
>>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH bpf-next v2 2/2] selftests/bpf: add test cases for bpf_dynptr_memset()
2025-06-26 13:34 ` Mykyta Yatsenko
@ 2025-06-30 21:02 ` Ihor Solodrai
0 siblings, 0 replies; 11+ messages in thread
From: Ihor Solodrai @ 2025-06-30 21:02 UTC (permalink / raw)
To: Mykyta Yatsenko, bpf; +Cc: andrii, ast, eddyz87, mykolal, kernel-team
On 6/26/25 6:34 AM, Mykyta Yatsenko wrote:
> On 6/26/25 02:25, Ihor Solodrai wrote:
>> On 6/25/25 4:45 AM, Mykyta Yatsenko wrote:
>>> On 6/24/25 21:52, Ihor Solodrai wrote:
>>>> Add tests to verify the behavior of bpf_dynptr_memset():
>>>> * normal memset 0
>>>> * normal memset non-0
>>>> * memset with an offset
>>>> * memset in dynptr that was adjusted
>>>> * error: size overflow
>>>> * error: offset+size overflow
>>>> * error: readonly dynptr
>>>> * memset into non-linear xdp dynptr
>>>>
>>>> Signed-off-by: Ihor Solodrai <isolodrai@meta.com>
>>>> ---
>>>> .../testing/selftests/bpf/prog_tests/dynptr.c | 8 +
>>>> .../selftests/bpf/progs/dynptr_success.c | 164 ++++++++++++++
>>>> ++++
>>>> 2 files changed, 172 insertions(+)
>>>>
>>>> diff --git a/tools/testing/selftests/bpf/prog_tests/dynptr.c b/
>>>> tools/ testing/selftests/bpf/prog_tests/dynptr.c
>>>> index 62e7ec775f24..f2b65398afce 100644
>>>> --- a/tools/testing/selftests/bpf/prog_tests/dynptr.c
>>>> +++ b/tools/testing/selftests/bpf/prog_tests/dynptr.c
>>>> @@ -21,6 +21,14 @@ static struct {
>>>> [...]
>>>> +
>>>> +SEC("xdp")
>>>> +int test_dynptr_memset_xdp_chunks(struct xdp_md *xdp)
>>>> +{
>>>> + const int max_chunks = 200;
>>>> + struct bpf_dynptr ptr_xdp;
>>>> + u32 data_sz, offset = 0;
>>
>> A question not directly to Mykyta.
>>
>> So noalu32 version of this test was failing to verify with this:
>>
>> 118: (85) call bpf_dynptr_read#201
>> R2 min value is negative, either use unsigned or 'var &= const'
>>
>> Where R2 refers to `data_sz - offset`
>>
>> Full log here: https://github.com/kernel-patches/bpf/actions/
>> runs/15861036149/job/44718289284
>>
>> I tried various conditions unsuccessfully. But changing u32 to u64
>> made it work. If handle_tail part is removed, as Mykyta suggested,
>> this doesn't matter, so I will probably leave u32 in v3.
>>
>> However I am curious if u32->u64 change is an appropriate workaround
>> in general for noalu32 problems? AFAIU verifier might get confused by
>> all the added shifts, and "noalu32" is a backward compatibility thing.
>>
>>
>>>> + char expected_buf[32];
>>> nit: expected_buf[32] = {DYNPTR_MEMSET_VAL};
> My bad, it's actually should be `char expected_buf[32] = {[0 ... 31] =
> DYNPTR_MEMSET_VAL}`;
> Otherwise it initializes just the first element of the expected_buf and
> places that array into the .rodata.cst32 section.
If I knew C, I'd pointed out that one can only do that with zero,
instead of objdumping bpf.
Anyways I think it's cleaner and more aligned with the style of these
tests to use memset and not bother with a hairy expression.
>>
>> I tried that at the beginning. As it turns out, this doesn't work in
>> BPF the way you'd expect:
>>
>> Here is a piece of llvm-objdump with explicit memset:
>>
>> 0000000000000968 <test_dynptr_memset_xdp_chunks>:
>> 301: 18 02 00 00 2a 2a 2a 2a 00 00 00 00 2a 2a 2a 2a r2 =
>> 0x2a2a2a2a2a2a2a2a ll
>> 303: 7b 2a c8 ff 00 00 00 00 *(u64 *)(r10 - 0x38) = r2
>> 304: 7b 2a d0 ff 00 00 00 00 *(u64 *)(r10 - 0x30) = r2
>> 305: 7b 2a d8 ff 00 00 00 00 *(u64 *)(r10 - 0x28) = r2
>> 306: 7b 2a e0 ff 00 00 00 00 *(u64 *)(r10 - 0x20) = r2
>> 307: bf a7 00 00 00 00 00 00 r7 = r10
>> 308: 07 07 00 00 e8 ff ff ff r7 += -0x18
>> 309: b7 02 00 00 00 00 00 00 r2 = 0x0
>> 310: bf 73 00 00 00 00 00 00 r3 = r7
>> 311: 85 10 00 00 ff ff ff ff call -0x1
>> ...
>>
>> You can clearly see a piece of stack filling up with 0x2a
>>
>> After applying this diff:
>>
>> diff --git a/tools/testing/selftests/bpf/progs/dynptr_success.c b/
>> tools/testing/selftests/bpf/progs/dynptr_success.c
>> index 5120acb8b15a..5b351f6fe07c 100644
>> --- a/tools/testing/selftests/bpf/progs/dynptr_success.c
>> +++ b/tools/testing/selftests/bpf/progs/dynptr_success.c
>> @@ -809,12 +809,10 @@ int test_dynptr_memset_xdp_chunks(struct xdp_md
>> *xdp)
>> const int max_chunks = 200;
>> struct bpf_dynptr ptr_xdp;
>> u32 data_sz, chunk_sz, offset = 0;
>> - char expected_buf[32];
>> + char expected_buf[32] = { DYNPTR_MEMSET_VAL };
>> char buf[32];
>> int i;
>>
>> - __builtin_memset(expected_buf, DYNPTR_MEMSET_VAL,
>> sizeof(expected_buf));
>> -
>> /* ptr_xdp is backed by non-contiguous memory */
>> bpf_dynptr_from_xdp(xdp, 0, &ptr_xdp);
>> data_sz = bpf_dynptr_size(&ptr_xdp);
>>
>> We get the following:
>>
>> 0000000000000968 <test_dynptr_memset_xdp_chunks>:
>> 301: bf a7 00 00 00 00 00 00 r7 = r10
>> 302: 07 07 00 00 e8 ff ff ff r7 += -0x18
>> 303: b7 02 00 00 00 00 00 00 r2 = 0x0
>> 304: bf 73 00 00 00 00 00 00 r3 = r7
>> 305: 85 10 00 00 ff ff ff ff call -0x1
>> ...
>>
>> The stack allocated array is not initialized.
>> Could be an LLVM bug/incompleteness? I used LLVM 19 while developing.
>>
>>
>>>> + char buf[32];
>>>> + int i;
>>>> +
>>>> + __builtin_memset(expected_buf, DYNPTR_MEMSET_VAL,
>>>> sizeof(expected_buf));
>>>> +
>>>> + /* ptr_xdp is backed by non-contiguous memory */
>>>> + bpf_dynptr_from_xdp(xdp, 0, &ptr_xdp);
>>>> + data_sz = bpf_dynptr_size(&ptr_xdp);
>>>> +
>>>> + err = bpf_dynptr_memset(&ptr_xdp, 0, data_sz, DYNPTR_MEMSET_VAL);
>>>> + if (err)
>>>> + goto out;
>>>> +
>>> Maybe we can calculate max_chunks instead of hardcoding, something like:
>>> max_chunks = data_sz / sizeof(expected_buf) + (data_sz %
>>> sizeof(expected_buf) ? 1 : 0);
>>
>> I don't see a point of doing it for this test. max_chunks is just a
>> big enough arbitrary constant that works. We do a similar thing in
>> other tests.
>>
>>>> + bpf_for(i, 0, max_chunks) {
>>>> + offset = i * sizeof(buf);
>>>> + err = bpf_dynptr_read(&buf, sizeof(buf), &ptr_xdp, offset, 0);
>>>
>>> handle_tail seems unnecessary, maybe handle tail in the main loop:
>>> __u32 sz = min_t(data_sz - offset : sizeof(buf));
>>> bpf_dynptr_read(&buf, sz, &ptr_xdp, offset, 0);
>>>
>>
>> Yeah, you're right.
>>
>> It ended up like this because I've been fighting the verifier while
>> writing the test, and this version worked eventually. The critical
>> piece to uncofuse it was changing:
>> offset += sizeof(buf)
>> to
>> offset = i * sizeof(buf)
>>
>> I will have to add min_t macro locally though.
>>
>>
>>>> + switch (err) {
>>>> + case 0:
>>>> + break;
>>>> + case -E2BIG:
>>>> + goto handle_tail;
>>>> + default:
>>>> + goto out;
>>>> + }
>>>> + err = bpf_memcmp(buf, expected_buf, sizeof(buf));
>>>> + if (err)
>>>> + goto out;
>>>> + }
>>>> +
>>>> +handle_tail:
>>>> + if (data_sz - offset < sizeof(buf)) {
>>>> + err = bpf_dynptr_read(&buf, data_sz - offset, &ptr_xdp,
>>>> offset, 0);
>>>> + if (err)
>>>> + goto out;
>>>> + err = bpf_memcmp(buf, expected_buf, data_sz - offset);
>>>> + }
>>>> +out:
>>>> + return XDP_DROP;
>>>> +}
>>>> +
>>>> void *user_ptr;
>>>> /* Contains the copy of the data pointed by user_ptr.
>>>> * Size 384 to make it not fit into a single kernel chunk when
>>>> copying
>>>
>>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH bpf-next v2 1/2] bpf: add bpf_dynptr_memset() kfunc
2025-06-25 11:27 ` Mykyta Yatsenko
@ 2025-06-30 21:05 ` Ihor Solodrai
2025-06-30 21:27 ` Ihor Solodrai
0 siblings, 1 reply; 11+ messages in thread
From: Ihor Solodrai @ 2025-06-30 21:05 UTC (permalink / raw)
To: Mykyta Yatsenko, bpf; +Cc: andrii, ast, eddyz87, mykolal, kernel-team
On 6/25/25 4:27 AM, Mykyta Yatsenko wrote:
> On 6/24/25 21:52, Ihor Solodrai wrote:
>> Currently there is no straightforward way to fill dynptr memory with a
>> value (most commonly zero). One can do it with bpf_dynptr_write(), but
>> a temporary buffer is necessary for that.
>>
>> Implement bpf_dynptr_memset() - an analogue of memset() from libc.
>>
>> Signed-off-by: Ihor Solodrai <isolodrai@meta.com>
>> ---
>> kernel/bpf/helpers.c | 48 ++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 48 insertions(+)
>>
>> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
>> index b71e428ad936..b8a7dbc971b4 100644
>> --- a/kernel/bpf/helpers.c
>> +++ b/kernel/bpf/helpers.c
>> @@ -2906,6 +2906,53 @@ __bpf_kfunc int bpf_dynptr_copy(struct
>> bpf_dynptr *dst_ptr, u32 dst_off,
>> return 0;
>> }
>> +/**
>> + * bpf_dynptr_memset() - Fill dynptr memory with a constant byte.
>> + * @ptr: Destination dynptr - where data will be filled
>> + * @ptr_off: Offset into the dynptr to start filling from
>> + * @size: Number of bytes to fill
>> + * @val: Constant byte to fill the memory with
>> + *
>> + * Fills the size bytes of the memory area pointed to by ptr
>> + * at offset ptr_off with the constant byte val.
>> + * Returns 0 on success; negative error, otherwise.
>> + */
>> + __bpf_kfunc int bpf_dynptr_memset(struct bpf_dynptr *ptr, u32
>> ptr_off, u32 size, u8 val)
>> + {
>> + struct bpf_dynptr_kern *p = (struct bpf_dynptr_kern *)ptr;
>> + char buf[256];
>> + u32 chunk_sz;
>> + void* slice;
>> + u32 offset;
>> + int err;
>> +
>> + if (__bpf_dynptr_is_rdonly(p))
>> + return -EINVAL;
>> +
>> + err = bpf_dynptr_check_off_len(p, ptr_off, size);
>> + if (err)
>> + return err;
>> +
>> + slice = bpf_dynptr_slice_rdwr(ptr, ptr_off, NULL, size);
>> + if (likely(slice)) {
>> + memset(slice, val, size);
>> + return 0;
>> + }
>
> bpf_dynptr_slice_rdwr is doing rdonly and off_len checks anyways, so
> perhaps we can
> avoid calling __bpf_dynptr_is_rdonly and bpf_dynptr_check_off_len before
> bpf_dynptr_slice_rdwr,
> that'll make fast path a little faster.
Yes, there are quite a bit of repetitive checks.
Same in bpf_dynptr_write() in the slow path.
In case of bpf_dynptr_slice_rdwr() the issue is that it returns a
pointer, and if we only check for null pointer we lose the information
about the error cause (is it readonly or size?). So I think it's
better to leave explicit checks.
>
>> +
>> + /* Non-linear data under the dynptr, write from a local buffer */
>> + chunk_sz = min_t(u32, sizeof(buf), size);
>> + memset(buf, val, chunk_sz);
>> +
>> + for (offset = ptr_off; offset < ptr_off + size; offset +=
>> chunk_sz) {
>> + chunk_sz = min_t(u32, sizeof(buf), size - offset);
>> + err = __bpf_dynptr_write(p, offset, buf, chunk_sz, 0);
>> + if (err)
>> + return err;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> __bpf_kfunc void *bpf_cast_to_kern_ctx(void *obj)
>> {
>> return obj;
>> @@ -3364,6 +3411,7 @@ 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)
>> +BTF_ID_FLAGS(func, bpf_dynptr_memset)
>> #ifdef CONFIG_NET
>> BTF_ID_FLAGS(func, bpf_modify_return_test_tp)
>> #endif
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH bpf-next v2 1/2] bpf: add bpf_dynptr_memset() kfunc
2025-06-30 21:05 ` Ihor Solodrai
@ 2025-06-30 21:27 ` Ihor Solodrai
0 siblings, 0 replies; 11+ messages in thread
From: Ihor Solodrai @ 2025-06-30 21:27 UTC (permalink / raw)
To: Mykyta Yatsenko, bpf; +Cc: andrii, ast, eddyz87, mykolal, kernel-team
On 6/30/25 2:05 PM, Ihor Solodrai wrote:
> On 6/25/25 4:27 AM, Mykyta Yatsenko wrote:
>> On 6/24/25 21:52, Ihor Solodrai wrote:
>>> Currently there is no straightforward way to fill dynptr memory with a
>>> value (most commonly zero). One can do it with bpf_dynptr_write(), but
>>> a temporary buffer is necessary for that.
>>>
>>> Implement bpf_dynptr_memset() - an analogue of memset() from libc.
>>>
>>> Signed-off-by: Ihor Solodrai <isolodrai@meta.com>
>>> ---
>>> kernel/bpf/helpers.c | 48 ++++++++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 48 insertions(+)
>>>
>>> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
>>> index b71e428ad936..b8a7dbc971b4 100644
>>> --- a/kernel/bpf/helpers.c
>>> +++ b/kernel/bpf/helpers.c
>>> @@ -2906,6 +2906,53 @@ __bpf_kfunc int bpf_dynptr_copy(struct
>>> bpf_dynptr *dst_ptr, u32 dst_off,
>>> return 0;
>>> }
>>> +/**
>>> + * bpf_dynptr_memset() - Fill dynptr memory with a constant byte.
>>> + * @ptr: Destination dynptr - where data will be filled
>>> + * @ptr_off: Offset into the dynptr to start filling from
>>> + * @size: Number of bytes to fill
>>> + * @val: Constant byte to fill the memory with
>>> + *
>>> + * Fills the size bytes of the memory area pointed to by ptr
>>> + * at offset ptr_off with the constant byte val.
>>> + * Returns 0 on success; negative error, otherwise.
>>> + */
>>> + __bpf_kfunc int bpf_dynptr_memset(struct bpf_dynptr *ptr, u32
>>> ptr_off, u32 size, u8 val)
>>> + {
>>> + struct bpf_dynptr_kern *p = (struct bpf_dynptr_kern *)ptr;
>>> + char buf[256];
>>> + u32 chunk_sz;
>>> + void* slice;
>>> + u32 offset;
>>> + int err;
>>> +
>>> + if (__bpf_dynptr_is_rdonly(p))
>>> + return -EINVAL;
>>> +
>>> + err = bpf_dynptr_check_off_len(p, ptr_off, size);
>>> + if (err)
>>> + return err;
>>> +
>>> + slice = bpf_dynptr_slice_rdwr(ptr, ptr_off, NULL, size);
>>> + if (likely(slice)) {
>>> + memset(slice, val, size);
>>> + return 0;
>>> + }
>>
>> bpf_dynptr_slice_rdwr is doing rdonly and off_len checks anyways, so
>> perhaps we can
>> avoid calling __bpf_dynptr_is_rdonly and bpf_dynptr_check_off_len
>> before bpf_dynptr_slice_rdwr,
>> that'll make fast path a little faster.
>
> Yes, there are quite a bit of repetitive checks.
> Same in bpf_dynptr_write() in the slow path.
>
> In case of bpf_dynptr_slice_rdwr() the issue is that it returns a
> pointer, and if we only check for null pointer we lose the information
> about the error cause (is it readonly or size?). So I think it's
> better to leave explicit checks.
I just realized that I might have misunderstood your comment.
Did you mean to put the checks after bpf_dynptr_slice_rdwr()
call instead of before?
>
>>
>>> +
>>> + /* Non-linear data under the dynptr, write from a local buffer */
>>> + chunk_sz = min_t(u32, sizeof(buf), size);
>>> + memset(buf, val, chunk_sz);
>>> +
>>> + for (offset = ptr_off; offset < ptr_off + size; offset +=
>>> chunk_sz) {
>>> + chunk_sz = min_t(u32, sizeof(buf), size - offset);
>>> + err = __bpf_dynptr_write(p, offset, buf, chunk_sz, 0);
>>> + if (err)
>>> + return err;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> __bpf_kfunc void *bpf_cast_to_kern_ctx(void *obj)
>>> {
>>> return obj;
>>> @@ -3364,6 +3411,7 @@ 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)
>>> +BTF_ID_FLAGS(func, bpf_dynptr_memset)
>>> #ifdef CONFIG_NET
>>> BTF_ID_FLAGS(func, bpf_modify_return_test_tp)
>>> #endif
>>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2025-06-30 21:27 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-06-24 20:52 [PATCH bpf-next v2 0/2] bpf: add bpf_dynptr_memset() kfunc Ihor Solodrai
2025-06-24 20:52 ` [PATCH bpf-next v2 1/2] " Ihor Solodrai
2025-06-24 21:50 ` Andrii Nakryiko
2025-06-25 11:27 ` Mykyta Yatsenko
2025-06-30 21:05 ` Ihor Solodrai
2025-06-30 21:27 ` Ihor Solodrai
2025-06-24 20:52 ` [PATCH bpf-next v2 2/2] selftests/bpf: add test cases for bpf_dynptr_memset() Ihor Solodrai
2025-06-25 11:45 ` Mykyta Yatsenko
2025-06-26 1:25 ` Ihor Solodrai
2025-06-26 13:34 ` Mykyta Yatsenko
2025-06-30 21:02 ` Ihor Solodrai
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.