Re: [PATCH 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 6 potential issue(s) to consider:
- [Medium] selftests/rdma: packet length modulo mask in handle_netkit_ingress()
- [Medium] selftests/rdma: out-of-bounds read and NULL deref in print_bpf_output()
- [Medium] selftests/rdma: unpacked rxe_atmeth and 32-bit ntohl() on 64-bit fields
- [Medium] selftests/rdma: wrong SEC(tcx/ingress) for bpf_program__attach_netkit()
- [Medium] selftests/rdma: unchecked ring_buffer__new() and error handling in main()
- [Low] selftests/rdma: missing -I$(OUTPUT) for skel header in out-of-tree build
--

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

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

This commit adds eBPF kernel and user-space packet dump utilities to capture
and parse Soft-RoCE (RXE) traffic over netkit devices in the RDMA selftests.

> diff --git a/tools/testing/selftests/rdma/Makefile b/tools/testing/selftests/rdma/Makefile
> index a91c14c45006e..6e099cabf90b2 100644
> --- a/tools/testing/selftests/rdma/Makefile
> +++ b/tools/testing/selftests/rdma/Makefile
> @@ -4,6 +4,27 @@ 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
> +
> +INCLUDES	:= -I../../../../usr/include -I../../../../include/uapi -I../bpf
> +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_x86 $(INCLUDES) -c $< -o $@
> +
> +$(OUTPUT)/rxe_pkg_kernel.skel.h: $(OUTPUT)/rxe_pkg_kernel_dump.o
> +	$(BPFTOOL) gen skeleton $< > $@

[Severity: Low]
Will out-of-tree selftest builds (with O=... or OUTPUT=...) fail to locate
rxe_pkg_kernel.skel.h when compiling rxe_pkg_user_dump.c?

The rule generates $(OUTPUT)/rxe_pkg_kernel.skel.h, and rxe_pkg_user_dump.c
includes it with #include "rxe_pkg_kernel.skel.h", but INCLUDES does not
contain -I$(OUTPUT).

Also, should the $(OUTPUT)/rxe_pkg_kernel_dump.o rule derive the target
architecture instead of hardcoding -D__TARGET_ARCH_x86?

[ ... ]

> 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..54dfc8028e904
> --- /dev/null
> +++ b/tools/testing/selftests/rdma/rxe_pkg_kernel_dump.c
> @@ -0,0 +1,67 @@
> +// 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>
> +
> +struct {
> +	__uint(type, BPF_MAP_TYPE_RINGBUF);
> +	__uint(max_entries, 256 * 1024);/* Shared by all CPUs */
> +} rb SEC(".maps");
> +
> +SEC("tcx/ingress")
> +int handle_netkit_ingress(struct __sk_buff *skb)

[Severity: Medium]
Should handle_netkit_ingress() use SEC("netkit/primary") or SEC("netkit/peer")
instead of SEC("tcx/ingress")?

In main() in rxe_pkg_user_dump.c, the program is attached with
bpf_program__attach_netkit(), which uses bpf_program__expected_attach_type().
Because SEC("tcx/ingress") sets the expected attach type to BPF_TCX_INGRESS,
the kernel link_create path dispatches to tcx_link_attach() rather than
netkit_link_attach().

> +{
> +	/* 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)) {
> +		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 & 0x1FF;
> +
> +			if (len == 0) {
> +				bpf_printk("len is 0\n");
> +				return BPF_OK;
> +			}
> +
> +			void *reserve = bpf_ringbuf_reserve(&rb, 512, 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]
Does masking skb->len with 0x1FF in handle_netkit_ingress() truncate larger
packets and leave part of the ring buffer record uninitialized?

Using skb->len & 0x1FF computes skb->len modulo 512 rather than clamping the
length to 512. Any packet whose length is a multiple of 512 bytes results in
len == 0 and is skipped, while packets larger than 512 bytes (such as a
520-byte or 1082-byte RDMA frame) only copy the remainder (8 or 58 bytes).

In addition, bpf_ringbuf_reserve(&rb, 512, 0) always reserves and submits a
512-byte record to userspace, but bpf_skb_load_bytes() only writes len bytes,
leaving the trailing 512 - len bytes of reserve uninitialized.

[ ... ]

> 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..39747ae1e06a3
> --- /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;
> +} __packed;

[Severity: Medium]
Is __packed defined in this userspace program?

Because __packed is a kernel macro not provided by the included userspace
headers, this definition declares a global variable named __packed of type
struct rxe_atmeth rather than applying __attribute__((packed)). On 64-bit
architectures, 4 bytes of padding are inserted after rkey, shifting swap_add
and comp by 4 bytes and changing RXE_ATMETH_BYTES from 28 to 32.

[ ... ]

> +static inline void print_reth(struct rxe_reth *reth)
> +{
> +	printf("struct rxe_reth {\n");
> +	printf("\t__be64		va;\n");
> +	printf("\t__be32		rkey;\n");
> +	printf("\t__be32		len;\n");
> +	printf("};\n");
> +}
> +
> +#define AETH_MSN_MASK		(0x00ffffff)
> +
> +static inline u32 aeth_msn(struct rxe_aeth *aeth)
> +{
> +	return AETH_MSN_MASK & ntohl(aeth->smsn);
> +}
> +
> +static inline void print_aeth(struct rxe_aeth *aeth)
> +{
> +	printf("struct rxe_aeth {\n");
> +	printf("\t___be32		smsn; 0x%x\n", aeth_msn(aeth));
> +	printf("};\n");
> +}
> +
> +static inline u64 atmeth_va(struct rxe_atmeth *atmeth)
> +{
> +	return ntohl(atmeth->va);
> +}
> +
> +static inline u32 atmeth_rkey(struct rxe_atmeth *atmeth)
> +{
> +	return ntohl(atmeth->rkey);
> +}
> +
> +static inline u64 atmeth_swap_add(struct rxe_atmeth *atmeth)
> +{
> +	return ntohl(atmeth->swap_add);
> +}
> +
> +static inline u64 atmeth_comp(struct rxe_atmeth *atmeth)
> +{
> +	return ntohl(atmeth->comp);
> +}
> +
> +static inline void print_atmeth(struct rxe_atmeth *atmeth)
> +{
> +	printf("struct rxe_atmeth {\n");
> +	printf("\t__be64		va;       0x%llx\n", atmeth_va(atmeth));
> +	printf("\t__be32		rkey;     0x%x\n", atmeth_rkey(atmeth));
> +	printf("\t__be64		swap_add; 0x%llx\n", atmeth_swap_add(atmeth));
> +	printf("\t__be64		comp;     0x%llx\n", atmeth_comp(atmeth));
> +	printf("} __packed;\n");
> +}
> +
> +static inline u64 atmack_orig(struct rxe_atmack *atmack)
> +{
> +	return ntohl(atmack->orig);
> +}

[Severity: Medium]
Don't atmeth_va(), atmeth_swap_add(), atmeth_comp(), and atmack_orig()
truncate 64-bit fields by passing __be64 values to 32-bit ntohl() instead of
be64toh() or __be64_to_cpu()?

[ ... ]

> +static inline __be32 immdt_imm(struct rxe_immdt *immdt)
> +{
> +	return immdt->imm;
> +}
> +
> +static inline void print_immdt(struct rxe_immdt *immdt)
> +{
> +	printf("struct rxe_immdt {\n");
> +	printf("\t__be32		imm; 0x%x\n", immdt_imm(immdt));
> +	printf("};\n");
> +}

[Severity: Medium]
Should immdt_imm() also convert immdt->imm with ntohl() to match the other
header helpers?

[ ... ]

> +static void print_bpf_output(void *data, __u32 size)
> +{
> +	struct iphdr *iph = (struct iphdr *)data;
> +	struct udphdr *udp_hdr;
> +	struct rxe_bth *bth;
> +
> +	print_ipaddr(iph);
> +	udp_hdr = (struct udphdr *)(data + sizeof(struct iphdr));
> +
> +	printf("udp hdr:\n");
> +	print_udphdr(udp_hdr);
> +	bth = (struct rxe_bth *)(data + sizeof(struct iphdr) + sizeof(struct udphdr));
> +
> +	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) {
> +			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");
> +}

[Severity: Medium]
Can print_bpf_output() dereference a NULL opcode name or read past the end of
the ring buffer sample?

First, neither handle_netkit_ingress() nor print_bpf_output() verifies that
iph->protocol == IPPROTO_UDP, that the UDP destination port is 4791 (RoCEv2),
or that size covers the headers. Because only 58 of the 256 entries in
rxe_opcode[] are populated, non-RoCEv2 IPv4 packets (such as TCP or ICMP
during rping setup) or unpopulated opcodes have rxe_opcode[bth->opcode].name
== NULL, which crashes when passed to printf("%s\n", ...).

Second, in the RXE_PAYLOAD case:

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

this calculation does not subtract sizeof(struct udphdr), BTH padding, or the
4-byte ICRC, and does not clamp len against size. When ntohs(udp_hdr->len)
exceeds the 512-byte ring buffer record, the loop reads cur_p[j] past the end
of the buffer.

Third, cur_p is a signed char pointer on x86_64, so bytes >= 0x80 pass
negative values to isprint() and sign-extend in printf("%02x", cur_p[j]).
Also, print_reth() above prints the field names of struct rxe_reth without
printing the values of va, rkey, and len.

> +
> +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;
> +}
> +
> +int main(void)
> +{
> +	struct rxe_pkg_kernel_dump *skel;
> +	int err;
> +	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("nk1");
> +
> +	if (ifindex == 0) {
> +		perror("if_nametoindex nk1 failed");
> +		return 1;
> +	}
> +
> +	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 nk1\n");
> +		goto cleanup;
> +	}
> +
> +	err = rxe_pkg_kernel_dump__attach(skel);
> +	if (err) {
> +		fprintf(stderr, "Failed to attach BPF skeleton\n");
> +		goto cleanup;
> +	}
> +
> +	ringbuf = ring_buffer__new(bpf_map__fd(skel->maps.rb),
> +				   handle_event, NULL, NULL);
> +
> +	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]
What happens in main() if ring_buffer__new() returns NULL?

ringbuf is passed directly to ring_buffer__poll(ringbuf, 100) without a NULL
check, which dereferences rb->epoll_fd and crashes.

Also, if if_nametoindex("nk1") fails, main() returns 1 without calling
rxe_pkg_kernel_dump__destroy(skel), whereas the later error paths that jump to
cleanup return 0 instead of a non-zero exit code. Finally, since
handle_netkit_ingress is already attached via bpf_program__attach_netkit(), is
the subsequent call to rxe_pkg_kernel_dump__attach(skel) needed?

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