Re: [PATCH bpf-next v2 2/2] selftests/bpf: add test cases for bpf_dynptr_memset()

[Ihor Solodrai <ihor.solodrai@linux.dev>]

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
>