Re: [PATCH bpf-next 4/7] selftests/bpf: Add XDP load-balancer common definitions

[sashiko-bot@kernel.org]

Sashiko AI review found 3 potential issue(s):
- [Medium] Endianness bug and invalid arithmetic on network-order values in IP encapsulation helpers.
- [Medium] Implicit compiler padding in shared BPF/userspace structures causes cross-architecture ABI breakage.
- [Low] Missing anonymous union for IPv4/IPv6 addresses in `struct real_definition`.
--

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

selftests/bpf: Add XDP load-balancer common definitions

This commit adds the shared header for the XDP load-balancer benchmark.
It defines the data structures used by both the BPF program and userspace,
along with 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 000000000000..aed20a963701
> --- /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;
> +}

Is it safe to perform bitwise operations directly on network-order values
like this? Since lb_htons() returns a network-order port, left-shifting it
by 16 and masking with host-order constants like 0xFFFF0000 or combining
it with IPIP_V4_PREFIX (4268) seems to assume a little-endian architecture.
Could this generate corrupted packet headers on big-endian systems?

> +
> +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;
> +}

Will saddr[0] evaluate to the correct prefix on all architectures? Because
IPIP_V6_PREFIX1 is defined as 1, it evaluates to 0100:: on little-endian
but ::1 on big-endian.

Also, is it intended to XOR the network-order __be32 src with the
host-order __u16 port without a prior byte-order conversion?

[ ... ]

> +struct real_pos_lru {
> +	__u32 pos;
> +	__u64 atime;
> +};

Could this shared structure cause ABI breakage between 32-bit userspace
and 64-bit BPF environments? In 64-bit environments, the compiler will
insert 4 bytes of padding after pos, but 32-bit environments only require
4-byte alignment for __u64. Should an explicit padding field be added to
ensure identical memory layout?

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

This isn't a bug, but earlier structures like flow_key and vip_definition
use an anonymous union to overlay their IPv4 and IPv6 addresses. Should
dst and dstv6 be placed in a union here as well to save memory and
maintain consistency?

[ ... ]

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