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@xxxxxxxxxx> 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