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

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

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

Also bump flow_dissector_classification's TEST_PACKET_LEN from 100 to
99 so this actually gets exercised instead of staying latent.

Without the fix and with TEST_PACKET_LEN=99, flow_dissector_classification
fails 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 just drops the packet over a bad checksum. With the fix, both
flow_dissector_classification and xdp_metadata pass.

v1 -> v2:
- comment style: opening /* on its own line, per BPF selftests style
- Retarget the Fixes tag to bcc00987bc56. commit f4504af68575
  ("selftests/bpf: move ip checksum helper to network helpers") 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(). csum_partial() first
  gets called with a length that isn't guaranteed even in
  bcc00987bc56, via build_udp_v4_csum()/build_udp_v6_csum().
- TEST_PACKET_LEN 100 -> 99 so an existing test catches this instead
  of the bug staying unexercised

Fixes: bcc00987bc56 ("selftests/bpf: add network helpers to generate udp checksums")
Link: https://lore.kernel.org/bpf/DLG5TIIHIMC2.2YCCY0UX86YQF@bootlin.com/T/#t
Signed-off-by: Madhav Khosla <madhav.khoslaa@gmail.com>
---
 tools/testing/selftests/bpf/network_helpers.h     | 15 +++++++++++++--
 .../prog_tests/flow_dissector_classification.c    |  2 +-
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/bpf/network_helpers.h b/tools/testing/selftests/bpf/network_helpers.h
index 75133119c04a..878c9fc5c37c 100644
--- a/tools/testing/selftests/bpf/network_helpers.h
+++ b/tools/testing/selftests/bpf/network_helpers.h
@@ -129,12 +129,23 @@ 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;
 }
diff --git a/tools/testing/selftests/bpf/prog_tests/flow_dissector_classification.c b/tools/testing/selftests/bpf/prog_tests/flow_dissector_classification.c
index 80b153d3ddec..421dfa6c4ea3 100644
--- 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"
-- 
2.55.0

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

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 3805 bytes --]

> diff --git a/tools/testing/selftests/bpf/network_helpers.h b/tools/testing/selftests/bpf/network_helpers.h
> index 75133119c04a3..878c9fc5c37ca 100644
> --- a/tools/testing/selftests/bpf/network_helpers.h
> +++ b/tools/testing/selftests/bpf/network_helpers.h
> @@ -129,12 +129,23 @@ 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;
>  }
> diff --git a/tools/testing/selftests/bpf/prog_tests/flow_dissector_classification.c b/tools/testing/selftests/bpf/prog_tests/flow_dissector_classification.c
> index 80b153d3ddecf..421dfa6c4ea36 100644
> --- 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"

The commit message body includes:

>     v1 -> v2:
>     - comment style: opening /* on its own line, per BPF selftests style
>     - Retarget the Fixes tag to bcc00987bc56. commit f4504af68575
>       ("selftests/bpf: move ip checksum helper to network helpers") 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(). csum_partial() first
>       gets called with a length that isn't guaranteed even in
>       bcc00987bc56, via build_udp_v4_csum()/build_udp_v6_csum().
>     - TEST_PACKET_LEN 100 -> 99 so an existing test catches this instead
>       of the bug staying unexercised

This isn't a bug, but could the 'v1 -> v2:' block move below the '---'
line so it stays out of the committed changelog? Documentation/process/
submitting-patches.rst asks for inter-version changelogs to go below the
'---' separator so they are dropped when the patch is applied. The
content itself is useful review material, especially the reasoning for
retargeting the Fixes tag.

The commit message also includes this test failure excerpt:

>     Without the fix and with TEST_PACKET_LEN=99, flow_dissector_classification
>     fails 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

This isn't a bug, but would it be clearer to paste the full failure
output or mark the elision? The summary line says six subtests failed
(1/6 FAILED) while only the ipv6 one is shown, and test_progs.c runs
three ASSERT_EQ calls per subtest ('test first port', 'test second port',
'test third port'), so 'test first port' would have failed too. And
'1/0 PASSED' reports one passing test, which cannot come from a run whose
only named test failed.


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

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

[Madhav Khosla]

> This isn't a bug, but would it be clearer to paste the full failure
> output or mark the elision? The summary line says six subtests failed
> (1/6 FAILED) while only the ipv6 one is shown, and test_progs.c runs
> three ASSERT_EQ calls per subtest ('test first port', 'test second port',
> 'test third port'), so 'test first port' would have failed too. And
> '1/0 PASSED' reports one passing test, which cannot come from a run whose
> only named test failed.

Not a bug. That log is the test failing on the TEST_PACKET_LEN=99 case
against the old (pre-fix) csum_partial(), not my patch. My change is
what makes the test catch it; it passes with the fix applied.

Madhav

On Wed, Sep 16, 2026 at 12:13 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..878c9fc5c37ca 100644
> > --- a/tools/testing/selftests/bpf/network_helpers.h
> > +++ b/tools/testing/selftests/bpf/network_helpers.h
> > @@ -129,12 +129,23 @@ 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;
> >  }
> > diff --git a/tools/testing/selftests/bpf/prog_tests/flow_dissector_classification.c b/tools/testing/selftests/bpf/prog_tests/flow_dissector_classification.c
> > index 80b153d3ddecf..421dfa6c4ea36 100644
> > --- 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"
>
> The commit message body includes:
>
> >     v1 -> v2:
> >     - comment style: opening /* on its own line, per BPF selftests style
> >     - Retarget the Fixes tag to bcc00987bc56. commit f4504af68575
> >       ("selftests/bpf: move ip checksum helper to network helpers") 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(). csum_partial() first
> >       gets called with a length that isn't guaranteed even in
> >       bcc00987bc56, via build_udp_v4_csum()/build_udp_v6_csum().
> >     - TEST_PACKET_LEN 100 -> 99 so an existing test catches this instead
> >       of the bug staying unexercised
>
> This isn't a bug, but could the 'v1 -> v2:' block move below the '---'
> line so it stays out of the committed changelog? Documentation/process/
> submitting-patches.rst asks for inter-version changelogs to go below the
> '---' separator so they are dropped when the patch is applied. The
> content itself is useful review material, especially the reasoning for
> retargeting the Fixes tag.
>
> The commit message also includes this test failure excerpt:
>
> >     Without the fix and with TEST_PACKET_LEN=99, flow_dissector_classification
> >     fails 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
>
> This isn't a bug, but would it be clearer to paste the full failure
> output or mark the elision? The summary line says six subtests failed
> (1/6 FAILED) while only the ipv6 one is shown, and test_progs.c runs
> three ASSERT_EQ calls per subtest ('test first port', 'test second port',
> 'test third port'), so 'test first port' would have failed too. And
> '1/0 PASSED' reports one passing test, which cannot come from a run whose
> only named test failed.
>
>
> ---
> 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/35091050280