* [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
* 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 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
* [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 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
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.