Re: [PATCH v2 2/2] selftests/rdma: Add netkit-based automated test script for RXE

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Medium] selftests/rdma: cleanup() in rxe_pkg_netkit.sh deletes compiled test binary
- [Medium] selftests/rdma: rxe_pkg_netkit.sh leaks background rping server and rxe0 link
- [Medium] selftests/rdma: rxe_pkg_netkit.sh fails if debugfs trace path is unavailable
- [Medium] selftests/rdma: rxe_pkg_netkit.sh ignores rxe_netkit_dump failure and output
- [Medium] selftests/rdma: cleanup() trap on INT/TERM resumes execution with set +e
- [Medium] selftests/rdma: overread in print_bpf_output() when dumping RXE_PAYLOAD
--

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

selftests/rdma: Add netkit-based automated test script for RXE

This commit adds a shell script to automate functional and tracing tests
for the Soft-RoCE (RXE) driver over netkit network interfaces using rping
and rxe_netkit_dump.

> diff --git a/tools/testing/selftests/rdma/rxe_pkg_netkit.sh b/tools/testing/selftests/rdma/rxe_pkg_netkit.sh
> new file mode 100755
> index 0000000000000..a876bd9d57efb
> --- /dev/null
> +++ b/tools/testing/selftests/rdma/rxe_pkg_netkit.sh
> @@ -0,0 +1,83 @@
[ ... ]
> +set -e
> +
> +CUR_WD="$(dirname "$0")"
> +[ "$CUR_WD" = "." ] && CUR_WD="$(pwd)"
> +
> +cleanup() {
> +    set +e
> +    [ -n "$DUMP_PID" ] && kill -9 "$DUMP_PID" 2>/dev/null
> +    unset DUMP_PID
> +
> +    rdma link del rxe1 2>/dev/null
> +    ip link del nk0 2>/dev/null
> +    ip netns delete ns0 2>/dev/null
> +
> +    rm -f "$CUR_WD/rxe_netkit_dump" "$CUR_WD/rxe_pkg_kernel_dump.o" "$CUR_WD/rxe_pkg_kernel.skel.h"

[Severity: Medium]
Should cleanup() really remove rxe_netkit_dump, rxe_pkg_kernel_dump.o, and
rxe_pkg_kernel.skel.h here?

tools/testing/selftests/rdma/Makefile builds these files during compilation
and cleans them via TEST_GEN_PROGS_EXTENDED and EXTRA_CLEAN. Because
cleanup() is registered via trap on EXIT, running rxe_pkg_netkit.sh once
deletes the compiled rxe_netkit_dump binary from the test or installed
kselftest directory, causing all subsequent runs of the script to fail when
invoking "$CUR_WD/rxe_netkit_dump".

> +}
> +
> +trap cleanup EXIT INT TERM

[Severity: Medium]
When SIGINT or SIGTERM is delivered to rxe_pkg_netkit.sh (for example, on a
kselftest timeout), cleanup() runs set +e, tears down the test interfaces,
and returns without exiting the shell.

Won't the script then resume execution on the torn-down interfaces with
error checking disabled, run cleanup() a second time on EXIT, and
potentially exit with status 0 after being interrupted by a signal?

> +
> +# Clear tracing buffer
> +echo > /sys/kernel/debug/tracing/trace

[Severity: Medium]
What happens on systems where tracefs is mounted standalone at
/sys/kernel/tracing without debugfs at /sys/kernel/debug/tracing, or when
kernel tracing is not enabled in the config?

With set -e active, writing to /sys/kernel/debug/tracing/trace will abort
the script with a failure exit code rather than falling back to
/sys/kernel/tracing/trace or skipping with exit 4.

> +
> +# Create network namespace and netkit peer links
> +ip netns add ns0
> +ip link add nk0 type netkit mode l2 peer name nk1
> +ip link set nk0 up
> +ip link set nk1 up
> +
> +# Configure namespace interface
> +ip link set nk1 netns ns0
> +ip netns exec ns0 ip addr add 10.0.0.1/24 dev nk1
> +ip netns exec ns0 ip link set nk1 up
> +
> +# Configure host interface
> +ip addr add 10.0.0.2/24 dev nk0
> +
> +# Start dump utility in background
> +"$CUR_WD/rxe_netkit_dump" &
> +DUMP_PID=$!

[Severity: Medium]
When rxe_pkg_netkit.sh starts rxe_netkit_dump and generates RoCEv2 rping
traffic, can print_bpf_output() in rxe_pkg_user_dump.c read past the end of
the captured packet into uninitialized ring buffer memory, or out-of-bounds
past the ring buffer record?

In tools/testing/selftests/rdma/rxe_pkg_kernel_dump.c:
handle_netkit_ingress() reserves a fixed MAX_PKT_LEN (512) bytes in the BPF
ring buffer, but only copies len (skb->len) bytes:

    void *reserve = bpf_ringbuf_reserve(&rb, MAX_PKT_LEN, 0);
    ...
    if (bpf_skb_load_bytes(skb, 0, reserve, len) < 0) {

Then in tools/testing/selftests/rdma/rxe_pkg_user_dump.c, handle_event()
passes data_sz - sizeof(struct ethhdr) (which is always 512 - 14 = 498) to
print_bpf_output(), and the RXE_PAYLOAD handler computes:

    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]);
        }

Because cur_p starts at (char *)bth + offset (after the UDP and RDMA
headers), and len does not subtract sizeof(struct udphdr) (8 bytes) or
check against the captured packet size, doesn't this loop read 8 bytes past
skb->len into uninitialized ring buffer memory on every RoCEv2 packet, and
read past the 512-byte ring buffer record when skb->len > 504?

> +
> +# Add RXE links over netkit devices
> +ip netns exec ns0 rdma link add rxe0 type rxe netdev nk1
> +rdma link add rxe1 type rxe netdev nk0
> +
> +# Start rping server in namespace
> +ip netns exec ns0 rping -s -a 10.0.0.1 -C 3 &

[Severity: Medium]
Does cleanup() leak the background rping server process and the rxe0 RDMA
link if the rping client fails or the script is interrupted?

Unlike DUMP_PID, the PID of this background rping -s process is not saved,
and cleanup() neither kills it nor runs:

    ip netns exec ns0 rdma link del rxe0 2>/dev/null

If rping -c fails under set -e, ip netns delete ns0 in cleanup() only
unlinks the netns mount point while the orphaned rping -s process stays
alive holding ns0, nk1, rxe0, and the inherited stdout/stderr pipe file
descriptors open (which can also cause the kselftest runner to hang waiting
for pipe EOF).

> +
> +# Add wait time
> +sleep 1
> +
> +# Run rping client on host
> +rping -c -a 10.0.0.1 -C 3
> +
> +# Tear down RDMA links and netkit devices
> +ip netns exec ns0 rdma link del rxe0
> +rdma link del rxe1
> +ip link del nk0
> +ip netns delete ns0
> +
> +# Display kernel trace output
> +cat /sys/kernel/debug/tracing/trace

[Severity: Medium]
Will this test pass even if rxe_netkit_dump fails to start, fails to attach
its BPF program to nk0, or captures no packets?

Launching "$CUR_WD/rxe_netkit_dump" & in the background succeeds even if
the binary is missing or exits immediately (and main() in
rxe_pkg_user_dump.c returns 0 on its goto cleanup error paths). In
addition, cleanup() terminates DUMP_PID with kill -9, which prevents
rxe_netkit_dump from flushing block-buffered stdout when running under
non-interactive kselftest pipes or cleaning up its BPF resources, and the
final cat /sys/kernel/debug/tracing/trace does not check that any expected
trace events or packets were recorded.

Would it be better to verify that DUMP_PID is alive and ready before
running rping, signal it with SIGINT or SIGTERM instead of SIGKILL, and
check that the expected trace output was actually captured?

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