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

On Tue, Apr 28, 2026 at 1:38 AM <[email protected]> wrote:
>
> 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 <[email protected]>
>
> 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?

It is safe, everything happens on the same host, this is a benchmark!

>
> > +
> > +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?

No, some scenarios use both.

>
> [ ... ]
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4