Re: [PATCH bpf-next 6/7] selftests/bpf: Add XDP load-balancer benchmark driver

On Tue, Apr 28, 2026 at 2:29 AM <[email protected]> wrote:
>
> Sashiko AI review found 1 potential issue(s):
> - [Low] Incorrect IPv4 checksum calculation for packets with IP options in test packet generator
> --
>
> commit cc1cb16090fdc833cb82a1c6a168d6cb11005220
> Author: Puranjay Mohan <[email protected]>
> selftests/bpf: Add XDP load-balancer benchmark driver
>
> This commit wires up the userspace side of the XDP load-balancer benchmark.
> It covers 24 scenarios and validates correctness before benchmarking by
> comparing the output packet byte-for-byte against an expected packet.
>
> > diff --git a/tools/testing/selftests/bpf/benchs/bench_xdp_lb.c b/tools/testing/selftests/bpf/benchs/bench_xdp_lb.c
> > new file mode 100644
> > index 000000000000..0b6709a2b03c
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/benchs/bench_xdp_lb.c
> [ ... ]
> > +static void build_packet(int idx)
> > +{
> [ ... ]
> > +     if (sc->is_v6) {
> [ ... ]
> > +     } else {
> > +             struct iphdr iph = {};
> > +             __u32 ip_off = off;
> > +
> > +             iph.version  = 4;
> > +             iph.ihl      = sc->set_ip_options ? 6 : 5;
> > +             iph.ttl      = 64;
> > +             iph.protocol = sc->ip_proto;
> > +             iph.saddr    = htonl(sc->src_addr);
> > +             iph.daddr    = htonl(sc->vip_addr);
> > +             iph.frag_off = sc->set_frag ? htons(IP_MF) : 0;
> > +             off += sizeof(iph);
> > +
> > +             if (sc->set_ip_options) {
> > +                     /* NOP option padding (4 bytes = 1 word) */
> > +                     __u32 nop = htonl(0x01010101);
> > +
> > +                     memcpy(p + off, &nop, sizeof(nop));
> > +                     off += sizeof(nop);
> > +             }
> > +
> > +             build_l4(sc, p, &off);
> > +
> > +             memcpy(p + off, "bench___payload!", 16);
> > +             off += 16;
> > +
> > +             iph.tot_len = htons(off - ip_off);
> > +             iph.check   = ip_checksum(&iph, sizeof(iph));
>
> Does this calculation exclude the appended IP options from the checksum?
>
> When sc->set_ip_options is true, the header length is set to 6 words (24
> bytes) and 4 bytes of NOP options are copied directly into the packet
> buffer. However, ip_checksum() is only passed sizeof(iph), which is exactly
> 20 bytes, and computes the checksum over the local iph struct alone.
>
> While this logically malformed packet might not cause the current tests
> to fail because the BPF program validates iph->ihl == 5 before verifying
> the IP checksum, could this mask bugs or produce false test failures if
> the BPF program is later modified to validate L3 checksums?

the current BPF program drops on ihl != 5 and never validates checksums.