Re: [PATCH 1/2] selftests: bpf: run test_xdp_features in its own netns

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 4218 bytes --]

> diff --git a/tools/testing/selftests/bpf/test_xdp_features.sh b/tools/testing/selftests/bpf/test_xdp_features.sh
> index 0aa71c4455c0..1f6be4af6718 100755
> --- a/tools/testing/selftests/bpf/test_xdp_features.sh
> +++ b/tools/testing/selftests/bpf/test_xdp_features.sh
> @@ -1,6 +1,19 @@
>  #!/bin/bash
>  # SPDX-License-Identifier: GPL-2.0
>
> +# The device under test listens on the veth address of the namespace this
> +# script runs in and the tester connects to it, so a firewall on the
> +# machine can refuse the control connection:
> +#   Failed connecting to the Device Under Test control socket
> +# That side of the pair is also always called v1, and cleanup only runs
> +# on a signal, so a failed run leaves the device behind and every later
> +# run stops in setup.  A namespace of our own has no such rules, and
> +# takes the leftovers with it when the test ends.
> +if [ -z "${XDP_FEATURES_NETNS:-}" ]; then
> +	XDP_FEATURES_NETNS=1 export XDP_FEATURES_NETNS
> +	exec unshare -n sh -c 'ip link set lo up; exec "$0" "$@"' "$0" "$@"
> +fi

The comment and commit message both state "everything the test made goes
away with the namespace when it exits", but does the anonymous namespace
actually reclaim the persistent named namespace created by ip netns add?

unshare -n only unshares CLONE_NEWNET. The mount namespace remains the
host's, so when setup() later runs:

    ip netns add ${NS}
    ip link add v1 type veth peer name v0 netns ${NS}

ip netns add deliberately creates a persistent namespace by bind-mounting
/proc/self/ns/net onto /run/netns/${NS}. That bind mount is visible on
the host and pins ${NS} with the v0 veth inside it.

Exiting the anonymous netns from unshare -n doesn't remove the named
netns -- that's why cleanup() must explicitly call ip netns del ${NS}.

cleanup() is still only reachable from the signal trap or the fully
successful path. All failure arms are [ $? -ne 0 ] && exit, and there is
no trap cleanup EXIT. On a failing run after the patch:

  - v1 and the anonymous netns: reclaimed
  - /run/netns/ns1-XXXXXX and the netns holding v0: leaked on the host

The practical breakage described in the changelog (name collision
causing later runs to fail in setup) is fixed because ${NS} is
randomised.

But the claim of complete cleanup is not accurate. Should this either
narrow the claim to the device and firewall rules, or make cleanup
complete with trap cleanup EXIT (in addition to the signal trap), or use
unshare -n -m with a private /run/netns so the named namespace really
does die with the test?

> +if [ -z "${XDP_FEATURES_NETNS:-}" ]; then
> +	XDP_FEATURES_NETNS=1 export XDP_FEATURES_NETNS
> +	exec unshare -n sh -c 'ip link set lo up; exec "$0" "$@"' "$0" "$@"
> +fi

A subsystem pattern flags this as potentially concerning: the new code
hand-rolls network-namespace setup (re-exec under unshare -n plus
ip link set lo up) in a script that already hand-rolls ip netns add,
ip link del, and ip netns del.

tools/testing/selftests/net/lib.sh already provides this: setup_ns
generates a randomised namespace name, creates it, brings lo up and
normalises rp_filter; cleanup_ns and cleanup_all_ns tear the list down
and register teardown to run on normal exit as well as on a signal,
which is the gap the patch's changelog identifies.

Worth a maintainer's call on whether the shared helpers should be used
here instead of a second implementation.

The alternative view is that this may be defensible: bpf selftests do
not currently source tools/testing/selftests/net/lib.sh (no bpf/*.sh
does), pulling it in would add a TEST_FILES dependency, and setup_ns is
not a drop-in substitute for what this hunk does. setup_ns creates a
named peer namespace, whereas the hunk moves the script's own process
into an anonymous namespace so the DUT-side v1 device and the host
firewall rules stop mattering. The parts net/lib.sh would genuinely
replace are the pre-existing ip netns add and cleanup lines, not the new
ones.


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/35173519344