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

[bot+bpf-ci@kernel.org]

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