Re: [PATCH v2 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: uninit ringbuf read and OOB payload dump in print_bpf_output()
- [Medium] selftests/rdma: missing packed attribute on struct rxe_atmeth
- [Medium] selftests/rdma: main() in rxe_pkg_user_dump.c masks errors by returning 0
- [Low] selftests/rdma: broken include paths and system bpf deps in Makefile
--

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

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

This commit adds eBPF-based kernel and user-space packet dump utilities
(rxe_pkg_kernel_dump.c and rxe_pkg_user_dump.c) to tools/testing/selftests/rdma/
to capture and parse Soft-RoCE (RXE) traffic over netkit devices.

> 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 $< > $@

[Severity: Low]
Will these build rules work for out-of-tree builds or on systems without a
recent system-wide libbpf and bpftool installed?

There appear to be a few issues with the build configuration here:

1. INCLUDES hardcodes -I../../../../usr/include and -I../../../../include/uapi
   instead of using $(KHDR_INCLUDES) from ../lib.mk. In out-of-tree builds
   (make O=...), exported headers are placed under $(O)/usr/include rather
   than ../../../../usr/include. Falling back to raw unexported kernel headers
   in ../../../../include/uapi lacks generated asm headers, triggers the
   "Attempt to use kernel headers from user space" warning in
   include/uapi/linux/types.h, and uses raw _UAPI_* header guards that
   conflict with glibc headers such as net/ethernet.h. In addition, invoking
   $(CLANG) -target bpf without host multiarch system include paths (such as
   get_sys_includes in tools/testing/selftests/net/bpf.mk) can fail to locate
   architecture headers like sys/cdefs.h, gnu/stubs.h, and asm/types.h.

2. The Makefile links against a system-installed -lbpf and invokes a system
   bpftool instead of building the in-tree tools/lib/bpf and
   tools/bpf/bpftool (as done in tools/testing/selftests/net/bpf.mk and
   tools/testing/selftests/hid/Makefile). Note that -I../bpf does not provide
   bpf/bpf_helpers.h or bpf/libbpf.h, and rxe_pkg_user_dump.c requires
   recent libbpf support for bpf_program__attach_netkit() and
   SEC("netkit/primary").

3. The rule for $(OUTPUT)/rxe_pkg_kernel.skel.h redirects stdout directly to
   $@ without .DELETE_ON_ERROR or temporary file cleanup on failure, so a
   missing or failing bpftool leaves behind an empty header file that
   subsequent make invocations treat as up to date.

> +
> +EXTRA_CLEAN := $(OUTPUT)/rxe_pkg_kernel_dump.o $(OUTPUT)/rxe_pkg_kernel.skel.h
> 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..7227fc4a6f57e
> --- /dev/null
> +++ b/tools/testing/selftests/rdma/rxe_pkg_kernel_dump.c

[ ... ]

> +SEC("netkit/primary")
> +int handle_netkit_ingress(struct __sk_buff *skb)
> +{

[ ... ]

> +			__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;
> +			}
> +
> +			/* The len is fixed to MAX_PKT_LEN */
> +			void *reserve = bpf_ringbuf_reserve(&rb, MAX_PKT_LEN, 0);
> +
> +			if (!reserve) {
> +				bpf_printk("bpf_ringbuf_reserve fails\n");
> +				return BPF_OK;
> +			}
> +
> +			if (bpf_skb_load_bytes(skb, 0, reserve, len) < 0) {
> +				bpf_ringbuf_discard(reserve, 0);
> +				bpf_printk("bpf_skb_load_bytes fails\n");
> +				return BPF_OK;
> +			}
> +
> +			bpf_ringbuf_submit(reserve, 0);

[Severity: Medium]
In handle_netkit_ingress(), bpf_ringbuf_reserve() always reserves MAX_PKT_LEN
(512) bytes, while bpf_skb_load_bytes() only writes len (skb->len) bytes into
reserve without zeroing the trailing MAX_PKT_LEN - len bytes or passing the
actual captured packet length to user space.

When handle_event() in rxe_pkg_user_dump.c receives this record, data_sz is
always 512 rather than skb->len, so print_bpf_output() cannot tell how many
bytes were actually initialized by bpf_skb_load_bytes() (see the comment on
print_bpf_output() below).

> +		}
> +	}
> +
> +	/* 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..b98d77b1dcf2c
> --- /dev/null
> +++ b/tools/testing/selftests/rdma/rxe_pkg_user_dump.c

[ ... ]

> +/******************************************************************************
> + * Atomic Extended Transport Header
> + ******************************************************************************/
> +struct rxe_atmeth {
> +	__be64			va;
> +	__be32			rkey;
> +	__be64			swap_add;
> +	__be64			comp;
> +};

[Severity: Medium]
Should struct rxe_atmeth be marked with __attribute__((packed))?

In drivers/infiniband/sw/rxe/rxe_hdr.h, struct rxe_atmeth is defined with
__packed so that va (offset 0), rkey (offset 8), swap_add (offset 12), and
comp (offset 20) match the 28-byte RoCEv2 wire layout. Even print_atmeth()
below prints "} __packed;\n".

Without the packed attribute here, the compiler inserts 4 bytes of alignment
padding between rkey and swap_add, increasing RXE_ATMETH_BYTES from 28 to 32
bytes, shifting swap_add and comp by 4 bytes in print_atmeth(), and
miscalculating the header lengths and RXE_PAYLOAD offsets for
IB_OPCODE_RC_COMPARE_SWAP, IB_OPCODE_RC_FETCH_ADD, IB_OPCODE_RD_COMPARE_SWAP,
and IB_OPCODE_RD_FETCH_ADD.

[ ... ]

> +static void print_bpf_output(void *data, __u32 size)
> +{

[ ... ]

> +	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_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)
> +{
> +	print_bpf_output(data+sizeof(struct ethhdr),
> +			 data_sz-sizeof(struct ethhdr));
> +	return 0;
> +}

[Severity: Medium]
Can print_bpf_output() read uninitialized ring buffer memory or read out of
bounds past the end of data[] when dumping headers and RXE_PAYLOAD?

Looking at handle_event() -> print_bpf_output():

1. Because handle_netkit_ingress() reserves a fixed 512-byte ring buffer
   entry without passing skb->len, size is always 498 (512 - sizeof(struct
   ethhdr)) rather than the captured packet length. Furthermore, the loop
   condition (size >= sizeof(struct iphdr) + sizeof(struct udphdr) + offset)
   only checks that the start offset is within size, not that the full
   extended header (offset + sizeof(header)) fits within the captured packet.

2. Under case RXE_PAYLOAD, len is computed as:

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

   udp_hdr->len includes sizeof(struct udphdr) (8 bytes), the RoCE transport
   headers, the payload, BTH padding (0..3 bytes), and the 4-byte RoCEv2
   ICRC. Because sizeof(struct udphdr), padding, and the 4-byte ICRC are not
   subtracted, len is at least 12 bytes larger than the actual payload (and
   8 bytes larger than the entire UDP datagram). In addition,
   rxe_opcode[bth->opcode].offset[RXE_PAYLOAD] is populated even for
   header-only opcodes such as IB_OPCODE_RC_ACKNOWLEDGE and
   IB_OPCODE_RC_RDMA_READ_REQUEST, so RXE_PAYLOAD is entered unconditionally.

3. Because len is never clamped to the remaining bytes in data[], cur_p[j]
   reads at least 8 bytes of uninitialized ring buffer memory past the end of
   every packet shorter than 512 bytes, and reads out of bounds past the end
   of the 512-byte ring buffer record when a packet is 505..512 bytes long
   (or if udp_hdr->len is larger than the buffer).

4. cur_p is a signed char *, so passing cur_p[j] directly to isprint() and
   printf("%02x", cur_p[j]) without casting to unsigned char invokes
   undefined behavior in isprint() for bytes >= 0x80 and sign-extends them to
   8 hex digits.

> +
> +int main(void)
> +{
> +	struct rxe_pkg_kernel_dump *skel;
> +	int err = 1;
> +	struct ring_buffer *ringbuf = NULL;
> +	struct rlimit rlim = { .rlim_cur = RLIM_INFINITY, .rlim_max = RLIM_INFINITY };
> +
> +	if (setrlimit(RLIMIT_MEMLOCK, &rlim)) {
> +		perror("setrlimit failed");
> +		return 1;
> +	}
> +
> +	skel = rxe_pkg_kernel_dump__open_and_load();
> +	if (!skel) {
> +		fprintf(stderr, "Failed to open and load BPF skeleton\n");
> +		return 1;
> +	}
> +
> +	int ifindex = if_nametoindex("nk0");
> +
> +	if (ifindex == 0) {
> +		perror("if_nametoindex nk0 failed");
> +		goto cleanup;
> +	}
> +
> +	skel->links.handle_netkit_ingress = bpf_program__attach_netkit(
> +		skel->progs.handle_netkit_ingress, ifindex, NULL);
> +
> +	if (!skel->links.handle_netkit_ingress) {
> +		fprintf(stderr, "Failed to attach to netkit nk0\n");
> +		goto cleanup;
> +	}
> +
> +	ringbuf = ring_buffer__new(bpf_map__fd(skel->maps.rb),
> +				   handle_event, NULL, NULL);
> +	if (!ringbuf) {
> +		fprintf(stderr, "Failed to create ring buffer\n");
> +		goto cleanup;
> +	}
> +
> +	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 0;
> +}

[Severity: Medium]
Should main() return a non-zero exit status on error, and handle termination
signals cleanly?

If if_nametoindex("nk0"), bpf_program__attach_netkit(), or ring_buffer__new()
fails, execution jumps to cleanup with err == 1; similarly, if
ring_buffer__poll() fails with an error other than -EINTR, the loop breaks to
cleanup with err < 0. However, the cleanup path ignores err and
unconditionally returns 0, masking failures from the calling test script.

Also, because the while (1) loop has no signal handler for SIGINT/SIGTERM and
print_bpf_output() never flushes stdout, running rxe_netkit_dump in the
background with stdout redirected to a file or pipe and killing it from a
test script will lose block-buffered stdout output and skip the cleanup
label.

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