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

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

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