Re: [PATCH v3 1/2] selftests/rdma: Introduce eBPF kernel and user dump utilities for RXE

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] selftests/rdma: out-of-bounds read in print_bpf_output() packet parsing
- [Medium] selftests/rdma: invalid userspace header and build flags for BPF dump object
- [Medium] selftests/rdma: missing signal handling and stdout flush in main()
- [Low] selftests/rdma: conflicting __u64 typedef in rxe_pkg_user_dump.c
--

commit 812649f812ece426007a23045167b73c8aff9b63
Author: Zhu Yanjun <yanjun.zhu@linux.dev>

selftests/rdma: Introduce eBPF kernel and user dump utilities for RXE

This commit adds an eBPF program and a user-space ring-buffer consumer utility
to capture and dump Soft-RoCE (rxe) packets over netkit devices in the rdma
selftests.

> diff --git a/tools/testing/selftests/rdma/Makefile b/tools/testing/selftests/rdma/Makefile
> index a91c14c45006e..ba225e580787d 100644
> --- a/tools/testing/selftests/rdma/Makefile
> +++ b/tools/testing/selftests/rdma/Makefile
> @@ -4,6 +4,28 @@ TEST_PROGS := rxe_rping_between_netns.sh \
>  		rxe_socket_with_netns.sh \
>  		rxe_test_NETDEV_UNREGISTER.sh \
>  		rxe_sent_rcvd_bytes.sh \
> -		rxe_netns_names.sh
> +		rxe_netns_names.sh \
> +		rxe_pkg_netkit.sh
>  
> +TEST_GEN_PROGS_EXTENDED := rxe_netkit_dump
>  include ../lib.mk
> +
> +CLANG	?= clang
> +LLC	?= llc
> +BPFTOOL	?= bpftool
> +ARCH ?= $(shell uname -m | sed 's/x86_64/x86/' | sed 's/aarch64/arm64/' | sed 's/ppc64le/powerpc/')
> +
> +INCLUDES	:= -I../../../../usr/include -I../../../../include/uapi -I../bpf -I$(OUTPUT)
> +CFLAGS		+= -g -Wall $(INCLUDES)
> +LDFLAGS		+= -lbpf -lelf -lz
> +
> +$(OUTPUT)/rxe_netkit_dump: rxe_pkg_user_dump.c $(OUTPUT)/rxe_pkg_kernel.skel.h
> +	$(CC) $(CFLAGS) $< $(LDFLAGS) -o $@
> +
> +$(OUTPUT)/rxe_pkg_kernel_dump.o: rxe_pkg_kernel_dump.c
> +	$(CLANG) -g -O2 -target bpf -D__TARGET_ARCH_$(ARCH) $(INCLUDES) -c $< -o $@
> +
> +$(OUTPUT)/rxe_pkg_kernel.skel.h: $(OUTPUT)/rxe_pkg_kernel_dump.o
> +	$(BPFTOOL) gen skeleton $< > $@
> +
> +EXTRA_CLEAN := $(OUTPUT)/rxe_pkg_kernel_dump.o $(OUTPUT)/rxe_pkg_kernel.skel.h

[ ... ]

> diff --git a/tools/testing/selftests/rdma/rxe_pkg_common.h b/tools/testing/selftests/rdma/rxe_pkg_common.h
> new file mode 100644
> index 0000000000000..0dca4147d5f9a
> --- /dev/null
> +++ b/tools/testing/selftests/rdma/rxe_pkg_common.h
> @@ -0,0 +1,17 @@
> +// SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB
> +/*
> + * Copyright (c) 2026 Zhu Yanjun <yanjun.zhu@linux.dev>
> + */
> +
> +// rxe_pkg_common.h
> +#ifndef RXE_PKG_COMMON_H
> +#define RXE_PKG_COMMON_H
> +
> +#define MAX_PKT_LEN 512
> +
> +struct pkg_event {
> +	__u32 len;
> +	__u8 data[MAX_PKT_LEN];
> +};
> +
> +#endif
> diff --git a/tools/testing/selftests/rdma/rxe_pkg_kernel_dump.c b/tools/testing/selftests/rdma/rxe_pkg_kernel_dump.c
> new file mode 100644
> index 0000000000000..f465b52e7366c
> --- /dev/null
> +++ b/tools/testing/selftests/rdma/rxe_pkg_kernel_dump.c
> @@ -0,0 +1,77 @@
> +// SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB
> +/*
> + * Copyright (c) 2026 Zhu Yanjun <yanjun.zhu@linux.dev>
> + */
> +
> +#include <linux/bpf.h>
> +#include <bpf/bpf_helpers.h>
> +#include <linux/if_ether.h>
> +#include <linux/ip.h>
> +#include <arpa/inet.h>
> +
> +#include "rxe_pkg_common.h"
> +
> +struct {
> +	__uint(type, BPF_MAP_TYPE_RINGBUF);
> +	__uint(max_entries, 256 * 1024);/* Shared by all CPUs */
> +} rb SEC(".maps");
> +
> +SEC("netkit/primary")
> +int handle_netkit_ingress(struct __sk_buff *skb)
> +{
> +	/* Get the header pointer of skb */
> +	void *data_end = (void *)(long)skb->data_end;
> +	void *data = (void *)(long)skb->data;
> +
> +	struct ethhdr *eth = data;
> +
> +	/* Verify the data */
> +	if (data + sizeof(*eth) > data_end)
> +		return BPF_OK;
> +
> +	/* Check if it is IP skb */
> +	if (eth->h_proto == htons(ETH_P_IP)) {

[Severity: Medium]
Will compiling rxe_pkg_kernel_dump.c with -target bpf and including the
userspace <arpa/inet.h> header break the build or fail verification on
standard multiarch and 64-bit distributions?

When compiling with -target bpf in tools/testing/selftests/rdma/Makefile,
clang does not define host architecture macros such as __x86_64__. Including
glibc's <arpa/inet.h> pulls in <features.h> and <gnu/stubs.h>, which falls
back to <gnu/stubs-32.h> and fails on 64-bit hosts without 32-bit multilib
headers, or fails to find multiarch headers such as <sys/cdefs.h> and
<asm/types.h> without clang system include paths. In addition, calling
htons() instead of bpf_htons() from <bpf/bpf_endian.h> leaves an unresolved
external symbol call in handle_netkit_ingress() that libbpf and the bpf
verifier reject when loading the skeleton.

Also in tools/testing/selftests/rdma/Makefile, the build rules pass raw
unexported kernel headers via -I../../../../include/uapi, link against the
host -lbpf instead of building the in-tree tools/lib/bpf (which provides
bpf_program__attach_netkit()), omit rxe_pkg_common.h from the prerequisites,
and redirect bpftool output directly to $@ without cleaning up a partial
rxe_pkg_kernel.skel.h on failure. Should rxe_pkg_kernel_dump.c use
<bpf/bpf_endian.h> and bpf_htons(), and should the makefile build against
the in-tree libbpf and exported uapi headers?

> +		struct iphdr *iph = data + sizeof(*eth);
> +
> +		if ((void *)iph + sizeof(*iph) <= data_end) {
> +			bpf_printk("Got IP packet from netkit ingress, %d\n", skb->len);
> +
> +			__u32 len = skb->len;
> +
> +			/* It is a rping test, the size of packets should be
> +			 * less than MAX_PKT_LEN
> +			 */
> +			if (len == 0 || len > MAX_PKT_LEN) {
> +				bpf_printk("len is not in 0..MAX_PKT_LEN\n");
> +				return BPF_OK;
> +			}
> +
> +			struct pkg_event *e;
> +
> +			/* The len is fixed to MAX_PKT_LEN */
> +			e = bpf_ringbuf_reserve(&rb, sizeof(*e), 0);
> +
> +			if (!e) {
> +				bpf_printk("bpf_ringbuf_reserve fails\n");
> +				return BPF_OK;
> +			}
> +
> +			e->len = len;
> +
> +			if (bpf_skb_load_bytes(skb, 0, e->data, len) < 0) {
> +				bpf_ringbuf_discard(e, 0);
> +				bpf_printk("bpf_skb_load_bytes fails\n");
> +				return BPF_OK;
> +			}
> +
> +			bpf_ringbuf_submit(e, 0);
> +		}
> +	}
> +
> +	/* BPF_OK:	Pass
> +	 * BPF_DROP:	Drop skb
> +	 */
> +	return BPF_OK;
> +}
> +
> +char _license[] SEC("license") = "GPL";
> diff --git a/tools/testing/selftests/rdma/rxe_pkg_user_dump.c b/tools/testing/selftests/rdma/rxe_pkg_user_dump.c
> new file mode 100644
> index 0000000000000..0fe6d00c198e3
> --- /dev/null
> +++ b/tools/testing/selftests/rdma/rxe_pkg_user_dump.c
> @@ -0,0 +1,1342 @@
> +// SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB
> +/*
> + * Copyright (c) 2026 Zhu Yanjun <yanjun.zhu@linux.dev>
> + */
> +
> +#include <stdio.h>
> +#include <sys/resource.h>
> +#include <net/if.h>
> +#include "rxe_pkg_kernel.skel.h"
> +
> +#include <linux/ip.h>
> +#include <linux/udp.h>
> +#include <ctype.h>
> +
> +#include <net/ethernet.h>
> +#include <arpa/inet.h>
> +
> +#include "rxe_pkg_common.h"
> +
> +typedef unsigned char __u8;
> +typedef unsigned short __u16;
> +typedef unsigned int __u32;
> +typedef unsigned long long __u64;

[Severity: Low]
Does redefining __u64 in tools/testing/selftests/rdma/rxe_pkg_user_dump.c
cause a conflicting type error on ppc64le?

Because <linux/ip.h> and <linux/udp.h> are included above, <linux/types.h>
is already pulled in. On 64-bit powerpc (ppc64le, which the makefile
explicitly supports) as well as mips64 and alpha, the userspace uapi headers
use <asm-generic/int-l64.h> and define __u64 as unsigned long, conflicting
with this unsigned long long typedef. Could these duplicate __u8 through
__u64 typedefs be dropped in favor of <linux/types.h>?

[ ... ]

> +static void print_bpf_output(const void *data, __u32 size)
> +{
> +	/* Validate size covers IP and UDP headers */
> +	if (size < sizeof(struct iphdr) + sizeof(struct udphdr))
> +		return;
> +
> +	struct iphdr *iph = (struct iphdr *)data;
> +
> +	if (iph->protocol != IPPROTO_UDP)
> +		return;
> +
> +	struct udphdr *udp_hdr = (struct udphdr *)(data + sizeof(struct iphdr));
> +
> +	if (ntohs(udp_hdr->dest) != ROCEV2_PORT)
> +		return;
> +
> +	print_ipaddr(iph);
> +	printf("udp hdr:\n");
> +	print_udphdr(udp_hdr);
> +
> +	size_t base_hdr_size = sizeof(struct iphdr) + sizeof(struct udphdr)
> +				+ sizeof(struct rxe_bth);
> +
> +	if (size < base_hdr_size)
> +		return;
> +
> +	struct rxe_bth *bth = (struct rxe_bth *)(data + sizeof(struct iphdr)
> +				+ sizeof(struct udphdr));
> +
> +	/* Validate opcode bounds and ensure name exists to prevent NULL dereference */
> +	if (bth->opcode >= 256 || !rxe_opcode[bth->opcode].name) {
> +		printf("Unknown or unpopulated opcode: 0x%x\n", bth->opcode);
> +		return;
> +	}
> +
> +	printf("%s\n", rxe_opcode[bth->opcode].name);
> +	print_bth(bth);
> +
> +	for (int i = RXE_BTH + 1; i < NUM_HDR_TYPES; i++) {
> +		int offset = rxe_opcode[bth->opcode].offset[i];
> +
> +		if (offset && (size >= sizeof(struct iphdr) + sizeof(struct udphdr) + offset)) {
> +			char *cur_p = (char *)bth + offset;
> +
> +			switch (i) {
> +			case	RXE_RETH:
> +				printf("RXE_RETH\n");
> +				struct rxe_reth *reth = (struct rxe_reth *)cur_p;
> +
> +				print_reth(reth);
> +				break;

[ ... ]

> +			case	RXE_PAYLOAD:
> +				printf("RXE_PAYLOAD\n");
> +				int len = ntohs(udp_hdr->len) - rxe_opcode[bth->opcode].length;
> +
> +				for (int j = 0; j < len; j++) {
> +					if (isprint(cur_p[j]))
> +						printf("%c", cur_p[j]);
> +					else
> +						printf("%02x", cur_p[j]);
> +				}
> +				printf("\n");
> +				break;
> +			default:
> +				break;
> +			}
> +		}
> +	}
> +
> +	printf("\n");
> +}
> +
> +static int handle_event(void *ctx, void *data, unsigned long data_sz)
> +{
> +	const struct pkg_event *e = data;
> +
> +	if (data_sz < sizeof(*e))
> +		return -1;
> +
> +	print_bpf_output(e->data + sizeof(struct ethhdr),
> +				e->len - sizeof(struct ethhdr));
> +
> +	return 0;
> +}

[Severity: Medium]
Can handle_event() -> print_bpf_output() read uninitialized ring-buffer
memory and overflow past e->data[]?

In handle_netkit_ingress(), bpf_ringbuf_reserve() allocates a struct
pkg_event without zeroing e->data, and bpf_skb_load_bytes() only writes the
first e->len bytes. Then in handle_event(), e->len is not checked against
sizeof(struct ethhdr) or the capacity of e->data before subtracting
sizeof(struct ethhdr) and calling print_bpf_output().

Inside print_bpf_output(), there are several bounds issues:

1. The header loop check:

	if (offset && (size >= sizeof(struct iphdr) + sizeof(struct udphdr) + offset))

only verifies that the start pointer cur_p = (char *)bth + offset is within
size, without checking that the full extended header struct at cur_p (such as
struct rxe_reth or struct rxe_atmeth) fits within size.

2. In the payload case:

	int len = ntohs(udp_hdr->len) - rxe_opcode[bth->opcode].length;

ntohs(udp_hdr->len) includes the 8-byte struct udphdr as well as the 4-byte
rocev2 icrc (and any bth pad count), and len is never clamped to the
remaining captured bytes in size. Because sizeof(struct udphdr) is not
subtracted, cur_p + len always extends at least 8 bytes past data + size,
reading uninitialized ring-buffer memory on every packet and reading
out-of-bounds past struct pkg_event whenever e->len is between 505 and 512
(or up to 64 kilobytes past e->data if udp_hdr->len is larger than the
captured size).

3. In rxe_opcode[], header-only opcodes such as acknowledge and rdma read
request define a non-zero payload offset, so the 4-byte trailing icrc
satisfies size >= sizeof(struct iphdr) + sizeof(struct udphdr) + offset and
causes print_bpf_output() to print the icrc plus 8 uninitialized or
out-of-bounds bytes as payload.

4. Because cur_p is a signed char *, passing cur_p[j] with bit 7 set is
undefined behavior in isprint() and sign-extends to a negative int in
printf("%02x", cur_p[j]). Should cur_p be unsigned char * (or cast to
unsigned char)?

> +
> +int main(void)
> +{

[ ... ]

> +	err = 0;
> +
> +	while (1) {
> +		err = ring_buffer__poll(ringbuf, 100 /* timeout */);
> +		if (err < 0 && err != -EINTR) {
> +			fprintf(stderr, "poll error\n");
> +			break;
> +		}
> +	}
> +
> +cleanup:
> +	ring_buffer__free(ringbuf);
> +	rxe_pkg_kernel_dump__destroy(skel);
> +	return err;
> +}

[Severity: Medium]
What happens when rxe_netkit_dump is run in the background with stdout
redirected to a file or pipe and is then stopped by a signal?

In main(), the while (1) loop around ring_buffer__poll() has no signal
handler for termination signals and only breaks when err < 0 && err != -EINTR,
making the cleanup label unreachable on normal termination. In addition,
print_bpf_output() writes via printf() without flushing stdout (or setting
line buffering via setvbuf()), so when stdout is redirected to a non-tty file
or pipe, stdio block-buffers the output and discards captured packet dumps
when the process is killed. Could main() install a signal handler to break
out of the poll loop and flush stdout after dumping each packet?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021203115.54560-1-yanjun.zhu@linux.dev?part=1