Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.
Critical: 0 · High: 0 · Medium: 2 (1) · Low: 1 (1)
If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).
- [Medium] cleanup() runs `trap - EXIT` (line 61) before `trap '' HUP INT
QUIT ABRT TERM` (line 62).
- [Medium] The liveness check in wait_for_dut_server() uses `jobs -pr
%%`.
- [Low] cleanup() deletes a namespace only if peer_created or dut_created
is true, and each flag is set only after its `ip netns add` returns 0.
Pre-existing issues:
- [Medium] test_xdp_features.sh decides pass or fail only from the
tester's exit status.
- [Low] Each xdp_features process attaches global tp_btf/xdp_exception
and tp_btf/xdp_cpumap_kthread programs.
--
selftests/bpf: Track XDP test processes and isolate namespaces
This patch reworks test_xdp_features.sh so the DUT xdp_features server
runs as a tracked shell job in its own network namespace, separate from
the tester's namespace. The pidof based kill in cleanup() is replaced by
job based termination. EXIT and signal traps are added, and signals that
arrive during setup are deferred.
> diff --git a/tools/testing/selftests/bpf/test_xdp_features.sh
> b/tools/testing/selftests/bpf/test_xdp_features.sh
> index 0aa71c4455c05..7b96222b690a6 100755
> --- a/tools/testing/selftests/bpf/test_xdp_features.sh
> +++ b/tools/testing/selftests/bpf/test_xdp_features.sh
> @@ -2,105 +2,173 @@
> # SPDX-License-Identifier: GPL-2.0
>
> readonly NS="ns1-$(mktemp -u XXXXXX)"
> +readonly DUT_NS="dut-${NS}"
[Severity: Low]
This is a pre-existing issue, not one introduced by this patch. The
per-run namespaces don't isolate the tracepoint programs that each
xdp_features instance attaches. In
tools/testing/selftests/bpf/progs/xdp_features.c:
SEC("tp_btf/xdp_exception")
int BPF_PROG(xdp_exception, const struct net_device *dev,
const struct bpf_prog *xdp, __u32 act)
{
...
val = bpf_map_lookup_elem(&dut_stats, &key);
if (val)
__sync_add_and_fetch(val, 1);
...
}
tp_xdp_cpumap_kthread does the same. Neither handler filters by ifindex
or netns. Any XDP exception or cpumap kthread event on the system
therefore increments this instance's dut_stats.
With concurrent runs or other XDP users on the host, can
tester_collect_detected_cap() print the wrong DETECTED or NOT DETECTED
result for a given run? The script's pass/fail doesn't change, because
tester_run() returns err < 0 ? err : 0 whatever detected_cap is.
[ ... ]
> setup() {
> {
> - ip netns add ${NS}
> + ip netns add "${NS}"
> + peer_created=true
> + ip netns add "${DUT_NS}"
> + dut_created=true
[Severity: Low]
What happens if a group-directed signal, such as Ctrl-C, kills
ip netns add after it has created and bind-mounted
/var/run/netns/<name> but before it returns?
set -e would exit setup() with peer_created or dut_created still false.
The EXIT trap would then skip the matching delete in cleanup():
if "$dut_created"; then
ip netns del "$DUT_NS" 2> /dev/null || true
fi
Does this leak an empty namespace? The old cleanup() deleted ${NS}
unconditionally.
A later commit in this series, "selftests/net: Move the XDP feature test
to drivers/net/hw", deletes test_xdp_features.sh, so this code is gone by
the end of the series.
[ ... ]
> cleanup() {
> - ip link del v1 2> /dev/null
> - ip netns del ${NS} 2> /dev/null
> - [ "$(pidof xdp_features)" = "" ] || kill $(pidof xdp_features) 2>
> /dev/null
> + trap - EXIT
> + trap '' HUP INT QUIT ABRT TERM
> + terminate_dut_server
> + if "$dut_created"; then
> + ip netns del "$DUT_NS" 2> /dev/null || true
> + fi
> + if "$peer_created"; then
> + ip netns del "$NS" 2> /dev/null || true
> + fi
> }
[Severity: Medium]
Can a trapped signal still cut this cleanup short? trap - EXIT runs
before the HUP/INT/QUIT/ABRT/TERM traps are set to '', and once setup has
finished handle_signal() always exits:
handle_signal() {
if "$setup_in_progress"; then
pending_signal=$1
else
exit "$1"
fi
}
Suppose a signal is already pending when cleanup() is entered from the
EXIT trap. For example, a harness sends INT and then TERM while the shell
waits on the foreground tester. A signal could also arrive just after
set -e starts the exit. Either way, handle_signal() runs exit from inside
the EXIT trap.
Bash then exits at once, and terminate_dut_server and both ip netns del
calls never run. Would that leak the DUT xdp_features process and both
namespaces?
A later commit in this series, "selftests/net: Move the XDP feature test
to drivers/net/hw", deletes test_xdp_features.sh, so this is not in the
final state of the series.
> wait_for_dut_server() {
> - while sleep 1; do
> - ss -tlp | grep -q xdp_features
> - [ $? -eq 0 ] && break
> + while true; do
> + if [ "$(jobs -pr %% 2> /dev/null)" != "$dut_pid" ]; then
> + echo "xdp_features server $dut_pid exited before
> readiness" >&2
> + return 1
> + fi
[Severity: Medium]
Does this check notice a DUT that has already exited? In bash, the -r
state filter of jobs only applies when no jobspec is given. With an
explicit %% jobspec, bash prints the job's PID whatever its state.
The jobs call also runs in a command substitution subshell. The parent
shell never reports or reaps the finished job, so %% keeps resolving to
it.
Suppose xdp_features exits normally before it starts listening, for
example because xdp_features__load() or xdp_features__attach() fails in
main(). This test then keeps comparing $dut_pid with itself and never
fires. ss never finds a socket with pid=$dut_pid, so the loop sleeps
forever.
Would the selftest then hang until an external timeout? That would not
match this part of the commit message:
Wait for readiness while the DUT is alive.
The later commit "selftests/net: Move the XDP feature test to
drivers/net/hw" deletes this script. The replacement xdp_features.py uses
bkg(dut_cmd, exit_wait=True, ksft_ready=True) instead.
> +
> + if ip netns exec "$DUT_NS" ss -tlp 2> /dev/null |
> + grep -q "pid=$dut_pid,"; then
> + return 0
> + fi
> +
> + sleep 1
> done
> }
[ ... ]
> test_xdp_features() {
> setup
>
> ## XDP_PASS
> - ./xdp_features -f XDP_PASS -D $V1_IP6 -T $V0_IP6 v1 &
> - wait_for_dut_server
> + start_dut_server -f XDP_PASS -D $V1_IP6 -T $V0_IP6 v1
> ip netns exec ${NS} ./xdp_features -t -f XDP_PASS \
> -D $V1_IP6 -C $V1_IP6 \
> -T $V0_IP6 v0
> [ $? -ne 0 ] && exit
[Severity: Medium]
This isn't a bug introduced by this patch, but the script only checks
the tester's exit status. tester_run() in
tools/testing/selftests/bpf/xdp_features.c computes detected_cap and the
advertised state and prints them. It then returns:
out:
bpf_xdp_detach(env.ifindex, flags, NULL);
close(sockfd);
return err < 0 ? err : 0;
So the selftest can pass even when every action is NOT DETECTED, or when
detection doesn't match what the driver advertises.
The later commit "selftests/net: Move the XDP feature test to
drivers/net/hw" fixes this. It replaces the script with
drivers/net/hw/xdp_features.py, which checks
ksft_eq(detected, advertised, ...).
> + reap_dut_server
[ ... ]
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261010-xdp-v5-send-20261008-v5-0-de687cb67c40%40gmail.com
