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