Re: [PATCH net-next v6 2/5] selftests/bpf: Track XDP test processes and isolate namespaces

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] Concern about a signal race in cleanup() and handle_signal() in
  tools/testing/selftests/bpf/test_xdp_features.sh.
- [Low] Concern about a check-then-create race (TOCTOU) on namespace
  ownership in setup() of…

--

selftests/bpf: Track XDP test processes and isolate namespaces

This commit reworks test_xdp_features.sh so each run gets its own DUT and
tester namespaces. The DUT server is tracked as a shell job with a bounded
readiness wait, and signals are handled during setup and cleanup. It also
filters the tracepoint counters by this run's program and CPU map IDs, and
makes the tester fail when detected and advertised features differ.

> diff --git a/tools/testing/selftests/bpf/test_xdp_features.sh b/tools/testing/selftests/bpf/test_xdp_features.sh
> index 0aa71c4455c05..5b68b20d94ac9 100755
> --- a/tools/testing/selftests/bpf/test_xdp_features.sh
> +++ b/tools/testing/selftests/bpf/test_xdp_features.sh
> @@ -2,105 +2,179 @@
>  # SPDX-License-Identifier: GPL-2.0
>  
>  readonly NS="ns1-$(mktemp -u XXXXXX)"
> +readonly DUT_NS="dut-${NS}"

[ ... ]

>  setup() {
>  	{
> -		ip netns add ${NS}
> -
> -		ip link add v1 type veth peer name v0 netns ${NS}
> -
> -		ip link set v1 up
> -		ip addr add $V1_IP4/24 dev v1
> -		ip addr add $V1_IP6/64 nodad dev v1
> +		[ ! -e "/run/netns/${NS}" ]
> +		peer_created=true
> +		ip netns add "${NS}"
> +		[ ! -e "/run/netns/${DUT_NS}" ]
> +		dut_created=true
> +		ip netns add "${DUT_NS}"

[Severity: Low]
Can cleanup() end up deleting a namespace that belongs to a different run?

The existence check and the ip netns add are not atomic. The ownership
flag is also set before the create succeeds.

Suppose a concurrent run creates the same name after the [ ! -e ... ]
check but before ip netns add. The add fails and set -e exits. cleanup()
then sees peer_created=true (or dut_created=true):

cleanup() {
    ...
	if "$peer_created"; then
		ip netns del "$NS" 2> /dev/null || true
	fi
}

and deletes the other run's namespace.

This needs two runs to get the same suffix from mktemp -u XXXXXX and then
hit the window, so it is unlikely. Would setting peer_created and
dut_created only after each ip netns add succeeds close it?

The later patch in this series, "selftests/net: Move the XDP feature test
to drivers/net/hw", deletes test_xdp_features.sh entirely. So this only
affects the intermediate state 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 '' HUP INT QUIT ABRT TERM
> +	trap - EXIT
> +	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: Low]
Is there a window where a second signal can cut cleanup() short?

After the first HUP/INT/QUIT/ABRT/TERM, handle_signal() calls exit and
cleanup() starts as the EXIT trap. If a second signal arrives before the
trap '' HUP INT QUIT ABRT TERM line takes effect, handle_signal() still
runs:

handle_signal() {
	if "$setup_in_progress"; then
		pending_signal=$1
	else
		exit "$1"
	fi
}

That exit would then run from inside the EXIT trap. In that case,
terminate_dut_server() and both ip netns del calls would be skipped,
leaving the DUT xdp_features process and both namespaces behind.

The old script had no TERM or EXIT trap, so this is not worse than
before. The later patch "selftests/net: Move the XDP feature test to
drivers/net/hw" also removes this script.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261018-xdp-v5-send-20261008-v6-0-7336e1282ca3%40gmail.com