Re: [PATCH v4 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 2 potential issue(s) to consider:
- [Medium] selftests/rdma: missing bounds checks on headers in print_bpf_output()
- [Medium] selftests/rdma: non-zero exit status on signal termination in main()
--

commit 4272376de8af3a4ff5f669a42f1584996bddd230
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
to tools/testing/selftests/rdma/ to capture and parse Soft-RoCE (RXE)
traffic over netkit devices.

> 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..5d51e7d1af66a
> --- /dev/null
> +++ b/tools/testing/selftests/rdma/rxe_pkg_user_dump.c
> @@ -0,0 +1,1365 @@

[ ... ]

> +static void print_bpf_output(const 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)) {
> +			const unsigned char *cur_p = (const unsigned char *)bth + offset;
> +			size_t remaining_size = size - ((const void *)cur_p - data);
> +
> +			switch (i) {
> +			case	RXE_RETH:
> +				if (remaining_size < sizeof(struct rxe_reth))
> +					break;
> +				printf("RXE_RETH\n");
> +				print_reth((const struct rxe_reth *)cur_p);
> +				break;
> +
> +			case	RXE_AETH:
> +				printf("RXE_AETH\n");
> +				struct rxe_aeth *aeth = (struct rxe_aeth *)cur_p;
> +
> +				print_aeth(aeth);
> +				break;
> +
> +			case	RXE_ATMETH:
> +				printf("RXE_ATMETH\n");
> +				struct rxe_atmeth *atmeth = (struct rxe_atmeth *)cur_p;
> +
> +				print_atmeth(atmeth);
> +				break;
> +
> +			case	RXE_ATMACK:
> +				printf("RXE_ATMACK\n");
> +				struct rxe_atmack *atmack = (struct rxe_atmack *)cur_p;
> +
> +				print_atmack(atmack);
> +				break;
> +
> +			case	RXE_IETH:
> +				printf("RXE_IETH\n");
> +				struct rxe_ieth *ieth = (struct rxe_ieth *)cur_p;
> +
> +				print_ieth(ieth);
> +				break;
> +
> +			case	RXE_RDETH:
> +				printf("RXE_RDETH\n");
> +				struct rxe_rdeth *rdeth = (struct rxe_rdeth *)cur_p;
> +
> +				print_rdeth(rdeth);
> +				break;
> +
> +			case	RXE_DETH:
> +				printf("RXE_DETH\n");
> +				struct rxe_deth *deth = (struct rxe_deth *)cur_p;
> +
> +				print_deth(deth);
> +				break;
> +
> +			case	RXE_IMMDT:
> +				printf("RXE_IMMDT\n");
> +				struct rxe_immdt *immdt = (struct rxe_immdt *)cur_p;
> +
> +				print_immdt(immdt);
> +				break;

[Severity: Medium]
Can print_bpf_output() read past the captured packet length in e->data[]
when parsing extended transport headers?

In handle_netkit_ingress() in tools/testing/selftests/rdma/rxe_pkg_kernel_dump.c,
bpf_ringbuf_reserve() allocates an uninitialized struct pkg_event, and
bpf_skb_load_bytes() only populates the first e->len bytes of e->data:

	e = bpf_ringbuf_reserve(&rb, sizeof(*e), 0);
	...
	e->len = len;
	if (bpf_skb_load_bytes(skb, 0, e->data, len) < 0) {

In print_bpf_output(), the condition:

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

only ensures that cur_p does not start past data + size (meaning
remaining_size >= 0), not that remaining_size is large enough to hold the
header structure at cur_p.

While the RXE_RETH case checks whether remaining_size < sizeof(struct rxe_reth),
the cases for RXE_AETH, RXE_ATMETH, RXE_ATMACK, RXE_IETH, RXE_RDETH, RXE_DETH,
and RXE_IMMDT dereference cur_p without checking remaining_size against their
respective struct sizes. If a truncated UDP packet arrives on port 4791,
won't these cases read uninitialized ring buffer memory past the end of the
captured packet (or past the end of e->data[MAX_PKT_LEN] if e->len is near
MAX_PKT_LEN)?

[ ... ]

> +int main(void)
> +{

[ ... ]

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

[Severity: Medium]
Does main() return a non-zero exit code when terminated normally by SIGINT or
SIGTERM?

When a signal interrupts epoll_wait() inside ring_buffer__poll(),
ring_buffer__poll() returns -EINTR. The loop checks for -EINTR and breaks
without resetting err to 0.

Similarly, if the signal arrives while ring_buffer__poll() is processing
events, ring_buffer__poll() returns the positive number of consumed records
and the while (!exiting) loop terminates with err > 0.

In both cases, main() reaches cleanup and returns err directly instead of 0.

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