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

bot+bpf-ci
2026-04-28T00:05:12Z

[-- Attachment #1: Type: text/plain, Size: 3460 bytes --]

> 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
> @@ -0,0 +1,112 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright (c) 2026 Meta Platforms, Inc. and affiliates. */
> +
> +#ifndef XDP_LB_BENCH_COMMON_H
> +#define XDP_LB_BENCH_COMMON_H

[ ... ]

> +#ifdef __BPF__
> +#define lb_htons(x) bpf_htons(x)
> +#define LB_INLINE static __always_inline
> +#else
> +#define lb_htons(x) htons(x)
> +#define LB_INLINE static inline
> +#endif
> +
> +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?

This concern was raised in review of v1:

https://lore.kernel.org/bpf/20260420132625.95C0AC2BCB9@smtp.kernel.org/

> +
> +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 mixing endianness here by applying a host-endian XOR operation
between a __be32 source and a __u16 port?

Looking at the parallel IPv4 version above, it converts port with
lb_htons(port) before the XOR operation. Shouldn't this be:

saddr[3] = src ^ lb_htons(port);

to match the IPv4 version's byte order handling?

On little-endian architectures, when port=0x1234, the XOR will use
0x3412 (byte-swapped during 32-bit promotion) instead of the intended
value. This will cause packet validation failures and incorrect routing
behavior when these functions are eventually used.

This concern was raised in review of v1:

https://lore.kernel.org/bpf/20260420132625.95C0AC2BCB9@smtp.kernel.org/

> +
> +struct flow_key {
> + union {
> + __be32 src;
> + __be32 srcv6[4];
> + };
> + union {
> + __be32 dst;
> + __be32 dstv6[4];
> + };
> + union {
> + __u32 ports;
> + __u16 port16[2];
> + };
> + __u8 proto;
> + __u8 pad[3];
> +};
> +
> +struct vip_definition {
> + union {
> + __be32 vip;
> + __be32 vipv6[4];
> + };
> + __u16 port;
> + __u8 proto;
> + __u8 pad;
> +};

[ ... ]

> +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 UAPI
ABI mismatch issues between 32-bit and 64-bit architectures.

These concerns were raised in review of v1:

https://lore.kernel.org/bpf/20260420132625.95C0AC2BCB9@smtp.kernel.org/

[ ... ]

---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/25025533577