* [PATCH bpf] selftests/bpf: Fix csum_partial() dropping trailing byte on odd length
@ 2026-09-15 14:50 Madhav Khosla
2026-09-15 15:56 ` bot+bpf-ci
0 siblings, 1 reply; 5+ messages in thread
From: Madhav Khosla @ 2026-09-15 14:50 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
Cc: bpf, Eduard Zingerman, Kumar Kartikeya Dwivedi, Martin KaFai Lau,
Song Liu, Yonghong Song, Jiri Olsa, Emil Tsalapatis,
Ihor Solodrai, Stanislav Fomichev, Alexis Lothoré,
Madhav Khosla
csum_partial() computes num_u16 = len >> 1 and only sums that many
16-bit words, so the last byte of an odd-length buffer never gets
added to the checksum. RFC 1071 says it should be padded with a zero
byte and summed as one more word, not dropped.
This backs build_ip_csum(), build_udp_v4_csum() and
build_udp_v6_csum(), used by flow_dissector_classification.c and
xdp_metadata.c to hand-build packets. No current caller builds an
odd-length payload, so nothing fails today, but a future one would
get a silently wrong checksum.
Verified against a reference implementation for even/odd/boundary
lengths, and ran flow_dissector_classification and xdp_metadata under
vmtest.sh; both pass.
Fixes: f4504af68575 ("selftests/bpf: move ip checksum helper to network helpers")
Signed-off-by: Madhav Khosla <madhav.khoslaa@gmail.com>
---
tools/testing/selftests/bpf/network_helpers.h | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/bpf/network_helpers.h b/tools/testing/selftests/bpf/network_helpers.h
index 75133119c04a..f6d28a2de788 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.
+ */
+ if (len & 1) {
+ __u16 tail = 0;
+
+ __builtin_memcpy(&tail, p + len - 1, 1);
+ sum += tail;
+ }
return sum;
}
--
2.55.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH bpf] selftests/bpf: Fix csum_partial() dropping trailing byte on odd length
2026-09-15 14:50 [PATCH bpf] selftests/bpf: Fix csum_partial() dropping trailing byte on odd length Madhav Khosla
@ 2026-09-15 15:56 ` bot+bpf-ci
2026-09-15 16:43 ` Madhav Khosla
0 siblings, 1 reply; 5+ messages in thread
From: bot+bpf-ci @ 2026-09-15 15:56 UTC (permalink / raw)
To: madhav.khoslaa, ast, daniel, andrii
Cc: bpf, eddyz87, memxor, martin.lau, song, yonghong.song, jolsa,
emil, ihor.solodrai, sdf, alexis.lothore, madhav.khoslaa, ast,
andrii, daniel, martin.lau, eddyz87, yonghong.song, mason,
ihor.solodrai
[-- Attachment #1: Type: text/plain, Size: 1938 bytes --]
> 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH bpf] selftests/bpf: Fix csum_partial() dropping trailing byte on odd length
2026-09-15 15:56 ` bot+bpf-ci
@ 2026-09-15 16:43 ` Madhav Khosla
2026-09-15 20:07 ` Alexis Lothoré
0 siblings, 1 reply; 5+ messages in thread
From: Madhav Khosla @ 2026-09-15 16:43 UTC (permalink / raw)
To: bot+bpf-ci
Cc: ast, daniel, andrii, bpf, eddyz87, memxor, martin.lau, song,
yonghong.song, jolsa, emil, ihor.solodrai, sdf, alexis.lothore,
martin.lau, mason
Note: Resending as plain text
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.
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.
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH bpf] selftests/bpf: Fix csum_partial() dropping trailing byte on odd length
2026-09-15 16:43 ` Madhav Khosla
@ 2026-09-15 20:07 ` Alexis Lothoré
2026-09-16 11:23 ` Madhav Khosla
0 siblings, 1 reply; 5+ messages in thread
From: Alexis Lothoré @ 2026-09-15 20:07 UTC (permalink / raw)
To: Madhav Khosla, bot+bpf-ci
Cc: ast, daniel, andrii, bpf, eddyz87, memxor, martin.lau, song,
yonghong.song, jolsa, emil, ihor.solodrai, sdf, alexis.lothore,
martin.lau, mason
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH bpf] selftests/bpf: Fix csum_partial() dropping trailing byte on odd length
2026-09-15 20:07 ` Alexis Lothoré
@ 2026-09-16 11:23 ` Madhav Khosla
0 siblings, 0 replies; 5+ messages in thread
From: Madhav Khosla @ 2026-09-16 11:23 UTC (permalink / raw)
To: Alexis Lothoré
Cc: bot+bpf-ci, ast, daniel, andrii, bpf, eddyz87, memxor, martin.lau,
song, yonghong.song, jolsa, emil, ihor.solodrai, sdf, martin.lau,
mason
Hi Alexis,
On Tue, 15 Sep 2026 20:07:00 UTC, Alexis Lothoré wrote:
> Could you please keep your answers interleaved in the previous messages
> next time ?
Sure, I'm doing that here.
> 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. [...] new comments should comply with it
Valid; fixed in the V2 patch.
> IMHO the provided Fixes tag does not make sense here because commit
> f4504af68575 [...] just moved the helper without changing the
> computation [...] The Fixes tag should either target the actual commit
> introducing ip_csum in xdp_metadata.c [...] or just be dropped
> completely
Retargeted it. f4504af68575 moved the helper but sizeof(iphdr) is
always a multiple of 32 bit words / 4 Bytes (iph->ihl counts in 4-byte
words), so the odd-length path was never reachable through
build_ip_csum().
> 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 ?
I tested on the edge case without my fix, and it failed under
vmtest.sh:
test_flow_dissector_classification:FAIL:test third port unexpected
test third port: actual 0 != expected 10
#137/6 flow_dissector_classification/ipv6:FAIL
#137 flow_dissector_classification:FAIL
Summary: 1/0 PASSED, 0 SKIPPED, 1/6 FAILED
kernel drops the packet over the bad checksum. I added the edge case
to be tested in the code, like this:
#define TEST_PACKET_LEN 99
With the fix applied, both flow_dissector_classification and
xdp_metadata pass in full.
v2 sent separately.
Madhav
On Tue, Sep 15, 2026 at 8:07 PM Alexis Lothoré
<alexis.lothore@bootlin.com> wrote:
>
> 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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2026-09-16 11:23 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-09-15 14:50 [PATCH bpf] selftests/bpf: Fix csum_partial() dropping trailing byte on odd length Madhav Khosla
2026-09-15 15:56 ` bot+bpf-ci
2026-09-15 16:43 ` Madhav Khosla
2026-09-15 20:07 ` Alexis Lothoré
2026-09-16 11:23 ` Madhav Khosla
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).