Re: [PATCH bpf] selftests/bpf: Fix csum_partial() dropping trailing byte on odd length

["Alexis Lothoré" <alexis.lothore@bootlin.com>]

Hi Madhav,
thanks for the fix.

On Tue Sep 15, 2026 at 6:43 PM CEST, Madhav Khosla wrote:
> Note: Resending as plain text

[...]

Could you please keep your answers interleaved in the previous messages
next time ?

> 1. checkpatch --strict doesn't flag it, and it's not actually an
>    outlier in the tree; bpftool_helpers.c, cap_helpers.c, disasm.c,
>    btf_helpers.c, netlink_helpers.c, jit_disasm_helpers.c,
>    trace_helpers.c, uprobe_multi.c and bench.c all do the same thing.
>    I'll leave it as is.

Checkpatch will (unfortunately) not raise any warning for this. And yes,
there are discrepancies in the tree, but adding more will just increase
the noise on each review. The check is voluntarily enforced in the
review prompts, so new comments should comply with it:

https://github.com/masoncl/review-prompts/blob/main/kernel/subsystem/bpf.md#bpf-comment-style

> 2. A second pointer would result in the same code either way, the cast just
>    moves from inside the loop to the declaration. I see no reason to
>    carry two pointers into the same buffer for that.
>    And for __builtin_memcpy(), it's because this header doesn't pull
>    in <string.h>, and I didn't want to add that just for a 1-byte copy.
>    It's also what a bunch of the progs/ files already do, so it's not
> exactly out of place here either.

[...]

IMHO the provided Fixes tag does not make sense here because commit
f4504af68575 ("selftests/bpf: move ip checksum helper to network
helpers") just moved the helper without changing the computation, it
was already dividing len by two. The Fixes tag should either target
the actual commit introducing ip_csum in xdp_metadata.c (I guess it
would be commit e2a46d54d7a1 ("selftests/bpf: Verify xdp_metadata
xdp->af_xdp path") then), or just be dropped completely (I feel like
the latter is the proper option, but I'll let maintainers conclude)

Ensuring that selftests would fail without your fix would be nice
though, maybe by ensuring that at least one of csum_partial's user uses
an odd data size ? eg:

--- a/tools/testing/selftests/bpf/prog_tests/flow_dissector_classification.c
+++ b/tools/testing/selftests/bpf/prog_tests/flow_dissector_classification.c
@@ -27,7 +27,7 @@
 #define TEST_NAME_MAX_LEN (32 + SUBTEST_NAME_MAX_LEN)
 #define MAX_SOURCE_PORTS 3
 #define TEST_PACKETS_COUNT 10
-#define TEST_PACKET_LEN 100
+#define TEST_PACKET_LEN 99
 #define TEST_PACKET_PATTERN 'a'
 #define TEST_IPV4 "192.168.0.1/32"
 #define TEST_IPV6 "100::a/128"

Thanks,

Alexis

>
> Madhav
>
>
> On Tue, Sep 15, 2026 at 9:26 PM <bot+bpf-ci@kernel.org> wrote:
>>
>> > diff --git a/tools/testing/selftests/bpf/network_helpers.h b/tools/testing/selftests/bpf/network_helpers.h
>> > index 75133119c04a3..f6d28a2de7888 100644
>> > --- a/tools/testing/selftests/bpf/network_helpers.h
>> > +++ b/tools/testing/selftests/bpf/network_helpers.h
>> > @@ -129,12 +129,22 @@ static __u16 csum_fold(__u32 csum)
>> >
>> >  static __wsum csum_partial(const void *buf, int len, __wsum sum)
>> >  {
>> > -     __u16 *p = (__u16 *)buf;
>> > +     const __u8 *p = buf;
>> >       int num_u16 = len >> 1;
>> >       int i;
>> >
>> >       for (i = 0; i < num_u16; i++)
>> > -             sum += p[i];
>> > +             sum += ((const __u16 *)p)[i];
>> > +
>> > +     /* RFC 1071: an odd-length buffer's trailing byte is paired with
>> > +      * a zero pad byte to form the final 16-bit word.
>> > +      */
>>
>> A subsystem pattern flags this as potentially concerning: the BPF
>> comment-style guide states that multi-line comments under
>> tools/testing/selftests/bpf/ must have the opening '/*' on its own line,
>> with the comment text starting on the next line:
>>
>>         /*
>>          * RFC 1071: an odd-length buffer's trailing byte is paired with
>>          * a zero pad byte to form the final 16-bit word.
>>          */
>>
>> The comment text itself is accurate and matches RFC 1071. Is the current
>> style acceptable given that the surrounding file has no other inline
>> multi-line comments in this format?
>>
>> > +     if (len & 1) {
>> > +             __u16 tail = 0;
>> > +
>> > +             __builtin_memcpy(&tail, p + len - 1, 1);
>> > +             sum += tail;
>> > +     }
>> >
>> >       return sum;
>> >  }
>>
>> This isn't a bug, but would keeping a separate '__u16 *' pointer
>> alongside the byte pointer let the loop body stay as it was, rather than
>> casting inside the subscript? Also, is there a reason for using
>> '__builtin_memcpy()' here instead of plain 'memcpy()' like the rest of
>> the selftests tree?
>>
>>
>> ---
>> AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
>> See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md
>>
>> CI run summary: https://github.com/kernel-patches/bpf/actions/runs/34986038731




-- 
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com