Re: [PATCH v3 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: rxe_pkg_netkit.sh discards output and ignores dump errors
- [Medium] selftests/rdma: unguarded tracefs access under set -e in rxe_pkg_netkit.sh
- [Medium] selftests/rdma: broken INT/TERM trap and dump teardown in cleanup()
- [Medium] selftests/rdma: leaked background rping server process in cleanup()
- [Medium] selftests/rdma: out-of-bounds and uninitialized read in print_bpf_output()
- [Medium] selftests/rdma: signed char passed to isprint() and printf() in print_bpf_output()
--

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

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

This commit adds an automated selftest script (rxe_pkg_netkit.sh) for the
Soft-RoCE (rxe) driver over netkit interfaces. It configures an isolated
network namespace with a netkit link pair, attaches rxe devices, and runs an
rping client/server test alongside packet dump and ftrace collection.

> 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..181e2d85eed76
> --- /dev/null
> +++ b/tools/testing/selftests/rdma/rxe_pkg_netkit.sh
> @@ -0,0 +1,81 @@

[ ... ]

> +exec > /dev/null
> +
> +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
> +}
> +
> +trap cleanup EXIT INT TERM

[Severity: Medium]
What happens in tools/testing/selftests/rdma/rxe_pkg_netkit.sh when the
script receives an interrupt or termination signal (such as on a kselftest
runner timeout)?

When cleanup() runs for an interrupt or termination trap, it executes
set +e in the shell's global environment, tears down rxe1, nk0, and ns0,
and returns without calling exit. In POSIX /bin/sh, returning from an
interrupt or termination trap resumes execution at the command following
the interrupted one. Because set +e remains in effect globally, all
subsequent commands (rping -c, rdma link del, ip link del, ip netns delete)
fail silently against the already-removed devices and namespace, and if the
final cat /sys/kernel/debug/tracing/trace succeeds, the script exits with
status 0 (a false pass) before running cleanup() a second time on exit.

Also, on the normal execution path:

    # 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

the script deletes nk0 while rxe_netkit_dump is still running and attached
to nk0, duplicating the teardown in cleanup(), and then cleanup() kills
rxe_netkit_dump with kill -9 without waiting for it. Using kill -9 prevents
rxe_netkit_dump from catching the signal in sig_handler() and running its
cleanup path (ring_buffer__free() and rxe_pkg_kernel_dump__destroy()) or
flushing its standard output buffers.

Should cleanup() send a normal termination signal, wait for the child
process, and rely on the exit trap for teardown while having interrupt and
termination traps exit with a non-zero status?

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

[Severity: Medium]
Can this access to /sys/kernel/debug/tracing/trace in
tools/testing/selftests/rdma/rxe_pkg_netkit.sh cause a hard test failure on
systems where debugfs or tracefs is not mounted at /sys/kernel/debug/tracing
or where kernel tracing is disabled in the kernel configuration?

Earlier in the script, missing prerequisites (root privileges, ip, rdma,
rping, netkit, and rdma_rxe) cause the test to skip with exit code 4.
However, after set -e is enabled, writing to and reading from
/sys/kernel/debug/tracing/trace will abort the script with exit status 1 if
that path is unavailable. Should the script check whether
/sys/kernel/debug/tracing/trace is available and writable before running, or
skip the test with exit code 4 when tracefs is not present?

> +
> +# 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 launches rxe_netkit_dump here and runs rping over
the netkit and rxe links, can handle_event() -> print_bpf_output() in
tools/testing/selftests/rdma/rxe_pkg_user_dump.c read uninitialized ring
buffer memory and read out of bounds past the pkg_event buffer?

In tools/testing/selftests/rdma/rxe_pkg_kernel_dump.c,
handle_netkit_ingress() reserves a pkg_event entry in the ring buffer
without zeroing e->data and copies only len bytes:

    e = bpf_ringbuf_reserve(&rb, sizeof(*e), 0);
    ...
    e->len = len;

    if (bpf_skb_load_bytes(skb, 0, e->data, len) < 0) {

Then in tools/testing/selftests/rdma/rxe_pkg_user_dump.c, handle_event()
passes e->len - sizeof(struct ethhdr) directly to print_bpf_output() without
checking that e->len is at least sizeof(struct ethhdr) or at most
MAX_PKT_LEN:

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

Inside print_bpf_output(), the header loop checks only that the start offset
of each header is within size, without verifying that the header structure
itself fits within the remaining captured bytes:

    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;

And when handling the payload in print_bpf_output(), len is computed by
subtracting rxe_opcode[bth->opcode].length from ntohs(udp_hdr->len) without
subtracting sizeof(struct udphdr) (8 bytes) and without clamping len to the
captured buffer size:

            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 udp_hdr->len includes the 8-byte UDP header while cur_p starts at
(char *)bth + offset (after the UDP header and RoCE headers), len is always
8 bytes larger than the remaining packet bytes. On every packet with a
payload, cur_p[j] reads 8 bytes past the end of the captured packet into
uninitialized ring buffer memory (or past e->data[MAX_PKT_LEN] if the
packet length is near MAX_PKT_LEN).

[Severity: Medium]
Also in tools/testing/selftests/rdma/rxe_pkg_user_dump.c, can passing
cur_p[j] to isprint() and printf() in print_bpf_output() trigger undefined
behavior and sign-extension in the hex output?

In print_bpf_output(), cur_p is declared as a plain char pointer:

    char *cur_p = (char *)bth + offset;
    ...
    for (int j = 0; j < len; j++) {
        if (isprint(cur_p[j]))
            printf("%c", cur_p[j]);
        else
            printf("%02x", cur_p[j]);
    }

Because userspace tools under tools/testing/selftests/rdma/ are compiled
without -funsigned-char, char is signed on architectures like x86_64. Any
payload or trailing CRC byte with the high bit set (>= 0x80) is negative
when accessed through cur_p[j], which causes negative indexing in isprint()
and sign-extends to a 32-bit negative integer in printf("%02x", cur_p[j]),
printing 8 hex characters (such as ffffff80) instead of 2. Should cur_p[j]
be cast to unsigned char before calling isprint() and printf()?

> +
> +# 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 &
> +
> +# 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

[Severity: Medium]
Does tools/testing/selftests/rdma/rxe_pkg_netkit.sh leak the background
rping server process if rping -c fails or if the script exits early?

When rping -s is started in the background inside ns0, its process ID is
not saved, and cleanup() neither kills it nor deletes rxe0 inside ns0:

    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
    }

If rping -c fails to connect, the background rping -s process remains
blocked waiting for a connection, pinning the ns0 network namespace and
rxe0 device in the kernel even after ip netns delete ns0 unlinks the
namespace mount, and keeping inherited file descriptors open (which can
cause the kselftest runner pipeline to hang until timeout). Even when
rping -c succeeds, ip netns exec ns0 rdma link del rxe0 runs immediately
without waiting for the background rping -s process to finish disconnecting.

Should the script record the rping server PID, wait for it after rping -c
completes, and ensure cleanup() terminates the server and deletes 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]
Does tools/testing/selftests/rdma/rxe_pkg_netkit.sh verify the packet dump
or tracing output, or does redirecting standard output to /dev/null cause
dump failures to go unnoticed?

Near the top of rxe_pkg_netkit.sh, standard output is redirected for the
entire script and all child processes:

    exec > /dev/null

As a result, both the RoCEv2 packet dump printed by rxe_netkit_dump and the
cat /sys/kernel/debug/tracing/trace output at the end of the script are
discarded to /dev/null without checking that any packets or trace entries
were captured. In addition, when rxe_netkit_dump is started in the
background:

    # Start dump utility in background
    "$CUR_WD/rxe_netkit_dump" &
    DUMP_PID=$!

the script never checks whether rxe_netkit_dump existed, succeeded in
loading and attaching its BPF program to nk0 before rping starts, or exited
with an error. Even if rxe_netkit_dump fails immediately or captures zero
packets, the script still exits with 0. Should the script verify that
rxe_netkit_dump attaches and captures the expected RoCEv2 packets and trace
output?

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