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

[sashiko-bot@kernel.org]

Sashiko AI review found 3 potential issue(s):
- [High] Double byte-swap and endian-unsafe bitwise arithmetic in IPv4/IPv6 encapsulation helpers.
- [High] Uninitialized `atime` during LRU pre-population causes immediate UDP cache eviction.
- [Medium] The computed IP header checksum explicitly ignores appended IP options bytes.
--

commit 58cc34d2b388ed8d16360bc2fcd6b05fe083e6ef
Author: Puranjay Mohan <puranjay@kernel.org>

selftests/bpf: Add XDP load-balancer benchmark driver

This commit adds a userspace driver for an XDP load-balancer benchmark.
It covers 24 scenarios testing various protocols, flags, and cache
patterns, and validates output packets and BPF map counters before
benchmarking.

> 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 0000000000000..f5c85b027d1c5
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/benchs/bench_xdp_lb.c
[ ... ]
> +static void build_packet(int idx)
> +{
[ ... ]
> +	} 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 checksum calculation exclude the IP options? Since iph is just
the 20-byte local stack structure, it seems the 4 bytes of options added to
the packet buffer are not covered by the checksum. This might cause the BPF
program to drop the packet prematurely due to an invalid checksum instead
of processing the intended options logic.

> +		memcpy(p + ip_off, &iph, sizeof(iph));
> +	}
> +
> +	pkt_len[idx] = off;
> +}
[ ... ]
> +static void populate_lru(const struct test_scenario *sc, __u32 real_idx)
> +{
> +	struct real_pos_lru lru = { .pos = real_idx };

Can this zero-initialized atime cause immediate UDP cache evictions? If the
machine running the selftest has an uptime greater than 30 seconds
(LRU_UDP_TIMEOUT), any lookups against this pre-populated entry might
see an age of bpf_ktime_get_ns() - 0 and immediately evict it. This could
lead to unintended LRU misses and validation failures.

> +	struct flow_key fk;
> +	int i, err;
[ ... ]
> +static void build_expected_packet(int idx)
> +{
[ ... ]
> +	if (sc->encap_v6_outer) {
> +		struct ipv6hdr ip6h = {};
> +		__u8 nexthdr = sc->is_v6 ? IPPROTO_IPV6 : IPPROTO_IPIP;
> +
> +		ip6h.version     = 6;
> +		ip6h.nexthdr     = nexthdr;
> +		ip6h.payload_len = htons(inner_len);
> +		ip6h.hop_limit   = 64;
> +
> +		create_encap_ipv6_src(htons(sc->src_port),

Since create_encap_ipv6_src() internally uses lb_htons(port), will passing
the port through htons() here cause a double byte-swap on little-endian
architectures? The same applies below for create_encap_ipv4_src().

Also, looking at the implementations of those encapsulation helpers, they
appear to use bitwise shifts and XORs on host integers (like IPIP_V4_PREFIX)
directly into network-order types. On big-endian architectures, could this
place the prefix in the lowest bytes instead of the highest, altering IP
semantics and causing validation failures?

> +				      sc->is_v6 ? htonl(sc->src_addr_v6[0])
> +						: htonl(sc->src_addr),
> +				      (__be32 *)&ip6h.saddr);
> +		htonl_v6((__be32 *)&ip6h.daddr, sc->tunnel_dst_v6);
> +
> +		memcpy(p + off, &ip6h, sizeof(ip6h));
> +		off += sizeof(ip6h);
> +	} else {
> +		struct iphdr iph = {};
> +
> +		iph.version  = 4;
> +		iph.ihl      = sizeof(iph) >> 2;
> +		iph.protocol = IPPROTO_IPIP;
> +		iph.tot_len  = htons(inner_len + sizeof(iph));
> +		iph.ttl      = 64;
> +		iph.saddr    = create_encap_ipv4_src(htons(sc->src_port),
> +						     htonl(sc->src_addr));
> +		iph.daddr    = htonl(sc->tunnel_dst);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260420111726.2118636-1-puranjay@kernel.org?part=5