* Re: [PATCH] selftests: net: forwarding: cleanup veth peers created via NUM_NETIFS
2026-01-20 23:09 [PATCH] selftests: net: forwarding: cleanup veth peers created via NUM_NETIFS Aleksei Oladko
@ 2026-01-21 1:31 ` Bobby Eshleman
2026-01-21 1:41 ` Bobby Eshleman
2026-01-21 13:39 ` Petr Machata
1 sibling, 1 reply; 4+ messages in thread
From: Bobby Eshleman @ 2026-01-21 1:31 UTC (permalink / raw)
To: Aleksei Oladko
Cc: David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Simon Horman, Shuah Khan, Petr Machata, netdev, linux-kselftest,
linux-kernel
On Tue, Jan 20, 2026 at 11:09:05PM +0000, Aleksei Oladko wrote:
> Some net/forwarding kselftests set NUM_NETIFS, causing lib.sh to create
> the requested number of veth peer interfaces.
>
> These interfaces are not removed when the tests finish, leaving stale
> veth devices in the system. This can cause subsequent tests to fail,
> for example min_max_mtu.sh.
>
> Ensure that veth peers created via NUM_NETIFS are properly removed at
> the end of the tests to avoid interference between test runs.
>
> Signed-off-by: Aleksei Oladko <aleksey.oladko@virtuozzo.com>
> ---
> tools/testing/selftests/net/forwarding/lib.sh | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
> index a9034f0bb58b..ae7699c0c7e5 100644
> --- a/tools/testing/selftests/net/forwarding/lib.sh
> +++ b/tools/testing/selftests/net/forwarding/lib.sh
> @@ -597,6 +597,10 @@ vrf_cleanup()
> ip -6 rule del pref 32765
> ip -4 rule add pref 0 table local
> ip -4 rule del pref 32765
> +
> + for ((i = 1; i <= NUM_NETIFS; i=$i+2)); do
I'd suggest 'i = i + 2' here instead of "i=$i+2", as it seems closer to the rest of lib.sh
style.
> + ip link delete dev ${NETIFS[p$i]} 2>/dev/null || true
> + done
> }
>
> adf_vrf_prepare()
> --
> 2.43.0
>
I see that some tests that set NUM_NETIFS do not seem to call
vrf_cleanup() (e.g. bridge_activity_notify.sh), so I think those tests
will still leak?
Maybe lib.sh needs a remove_netif() to go along with create_netif(), and
then tests can call it from their 'cleanup()' EXIT handlers? This avoids
the mismatch between vrf_prepare() not creating devices but
vrf_cleanup() removing them.
Thanks,
Bobby
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] selftests: net: forwarding: cleanup veth peers created via NUM_NETIFS
2026-01-20 23:09 [PATCH] selftests: net: forwarding: cleanup veth peers created via NUM_NETIFS Aleksei Oladko
2026-01-21 1:31 ` Bobby Eshleman
@ 2026-01-21 13:39 ` Petr Machata
1 sibling, 0 replies; 4+ messages in thread
From: Petr Machata @ 2026-01-21 13:39 UTC (permalink / raw)
To: Aleksei Oladko
Cc: David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Simon Horman, Shuah Khan, Petr Machata, netdev, linux-kselftest,
linux-kernel
Aleksei Oladko <aleksey.oladko@virtuozzo.com> writes:
> Some net/forwarding kselftests set NUM_NETIFS, causing lib.sh to create
> the requested number of veth peer interfaces.
>
> These interfaces are not removed when the tests finish, leaving stale
> veth devices in the system. This can cause subsequent tests to fail,
> for example min_max_mtu.sh.
What in particular makes it fail? I tried a bridge_vlan_unaware.sh +
min_max_mtu.sh combination at random, and that seems to work fine.
> Ensure that veth peers created via NUM_NETIFS are properly removed at
> the end of the tests to avoid interference between test runs.
>
> Signed-off-by: Aleksei Oladko <aleksey.oladko@virtuozzo.com>
> ---
> tools/testing/selftests/net/forwarding/lib.sh | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
> index a9034f0bb58b..ae7699c0c7e5 100644
> --- a/tools/testing/selftests/net/forwarding/lib.sh
> +++ b/tools/testing/selftests/net/forwarding/lib.sh
> @@ -597,6 +597,10 @@ vrf_cleanup()
> ip -6 rule del pref 32765
> ip -4 rule add pref 0 table local
> ip -4 rule del pref 32765
> +
> + for ((i = 1; i <= NUM_NETIFS; i=$i+2)); do
> + ip link delete dev ${NETIFS[p$i]} 2>/dev/null || true
> + done
This would also erase interfaces that existed before the test was run,
which seems worse than leaving garbage behind, which I agree is not
ideal either. Although yeah, existing tests create all sorts of ad-hoc
interfaces assuming the namespace is available, and then presumably
deleting them. It's all around fairly hacky.
Nevertheless, I think if the test should erase what it created, then it
needs to also track what it created. Then the 2>/dev/null || true
business can go away, because it's supposed to work.
Hmm, that's basically reinventing defer. But we can't use defer, because
few tests are defer-enabled. Or maybe...?
Subject: selftests: lib: Delete created veth pairs after the test
Currently the created veth pairs are not removed after the test finishes.
Ideally we would just use defer to selectively delete exactly what the test
created, but most tests are not defer ready in that they use the EXIT trap
for their own cleanup management.
So instead invoke the cleanup from vrf_cleanup(). Almost all tests use VRFs
and typically call vrf_prepare() first, which makes vrf_cleanup() a good
place to do it.
Defer-enabled tests however schedule vrf_cleanup() by way of
adf_vrf_prepare(). Invoking scope cleanup from within scope cleanup is
obviously not a great idea, so instead add a __-prefixed helper to do the
actual work, have it scheduled by adf_vrf_prepare(), and only invoke the
scope cleanup from a sans-__ wrapper.
This is all admittedly a bit of a hack, but perhaps less so than
handrolling a list of interfaces to delete, when we have a perfectly good
library for exactly that purpose. (And the hand-rolled cleanup would end up
being invoked from vrf_cleanup() anyway.)
Signed-off-by: Petr Machata <petrm@nvidia.com>
---
tools/testing/selftests/net/forwarding/lib.sh | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
index a9034f0bb58b..729d6bedf97b 100644
--- a/tools/testing/selftests/net/forwarding/lib.sh
+++ b/tools/testing/selftests/net/forwarding/lib.sh
@@ -392,6 +392,7 @@ create_netif_veth()
echo "Failed to create netif"
exit 1
fi
+ defer ip link del "${NETIFS[p$i]}"
fi
i=$j
done
@@ -591,7 +592,7 @@ vrf_prepare()
ip -6 rule del pref 0
}
-vrf_cleanup()
+__vrf_cleanup()
{
ip -6 rule add pref 0 table local
ip -6 rule del pref 32765
@@ -599,10 +600,16 @@ vrf_cleanup()
ip -4 rule del pref 32765
}
+vrf_cleanup()
+{
+ __vrf_cleanup
+ defer_scopes_cleanup
+}
+
adf_vrf_prepare()
{
vrf_prepare
- defer vrf_cleanup
+ defer __vrf_cleanup
}
__last_tb_id=0
Like your patch, this doesn't fix tests that don't use vrf_cleanup().
There's at least bridge_sticky_fdb.sh, but there might be more in
driver-specific directories, I didn't look. It should be pretty easy to
deferify those tests FWIW, the gist of it is:
diff --git a/tools/testing/selftests/net/forwarding/bridge_sticky_fdb.sh b/tools/testing/selftests/net/forwarding/bridge_sticky_fdb.sh
index 1f8ef0eff862..7d51676c886e 100755
--- a/tools/testing/selftests/net/forwarding/bridge_sticky_fdb.sh
+++ b/tools/testing/selftests/net/forwarding/bridge_sticky_fdb.sh
@@ -40,7 +40,7 @@ setup_prepare()
switch_create
}
-cleanup()
+setup_cleanup()
{
pre_cleanup
switch_destroy
@@ -62,6 +62,8 @@ sticky()
trap cleanup EXIT
setup_prepare
+defer setup_cleanup
+
setup_wait
tests_run
^ permalink raw reply related [flat|nested] 4+ messages in thread