Re: [RFC PATCH bpf-next 3/6] selftests/bpf: Add XDP load-balancer common definitions

[sashiko-bot@kernel.org]

Sashiko AI review found 3 potential issue(s):
- [Medium] Endianness and byte-order bugs in encapsulation source-address helpers.
- [Medium] Missing `union` in `struct real_definition` wastes memory and breaks structural layout.
- [Medium] Implicit padding in UAPI-like shared data structures breaks 32-bit vs 64-bit ABI.
--

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

selftests/bpf: Add XDP load-balancer common definitions
    
This commit adds a shared header for the XDP load-balancer benchmark, defining
the data structures used by both the BPF program and userspace. It also
provides encapsulation source-address helpers.

> diff --git a/tools/testing/selftests/bpf/xdp_lb_bench_common.h b/tools/testing/selftests/bpf/xdp_lb_bench_common.h
> new file mode 100644
> index 0000000000000..aed20a9637017
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/xdp_lb_bench_common.h
[ ... ]
> +LB_INLINE __be32 create_encap_ipv4_src(__u16 port, __be32 src)
> +{
> +	__u32 ip_suffix = lb_htons(port);
> +
> +	ip_suffix <<= 16;
> +	ip_suffix ^= src;
> +	return (0xFFFF0000 & ip_suffix) | IPIP_V4_PREFIX;
> +}

Does this mix host-endian arithmetic with network-order types?

Shifting and masking __be32 values using host operations modifies different IP
octets depending on the architecture. Also, on big-endian systems, will the
host-order constant IPIP_V4_PREFIX (4268) map to 0.0.16.172 instead of
172.16.0.0?

> +LB_INLINE void create_encap_ipv6_src(__u16 port, __be32 src, __be32 *saddr)
> +{
> +	saddr[0] = IPIP_V6_PREFIX1;
> +	saddr[1] = IPIP_V6_PREFIX2;
> +	saddr[2] = IPIP_V6_PREFIX3;
> +	saddr[3] = src ^ port;
> +}

Are we also mixing endianness here by applying a host-endian XOR operation
between a __be32 source and a __u16 port?

[ ... ]
> +struct real_pos_lru {
> +	__u32 pos;
> +	__u64 atime;
> +};

Could this struct layout cause mismatches between a 32-bit userspace binary
and a 64-bit BPF program?

Having a __u32 immediately followed by a __u64 introduces 4 bytes of implicit
padding on 64-bit architectures, but potentially no padding on 32-bit
architectures (where __u64 can be 4-byte aligned). Should explicit padding
be added here?

> +struct real_definition {
> +	__be32 dst;
> +	__be32 dstv6[4];
> +	__u8   flags;
> +};

Should dst and dstv6 be wrapped in an anonymous union?

The other dual-stack IP structures in this header (flow_key and
vip_definition) use a union to save memory in BPF maps and maintain structural
layout.

Also, does this structure need explicit trailing padding? There are currently
3 bytes of implicit trailing padding which might cause the same UAPI ABI
mismatch issues mentioned above.

[ ... ]

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