Re: [PATCH net] selftests/net: run tun tests in a dedicated network namespace

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 0

- [Medium] In the newly added main() in tools/testing/selftests/net/tun.c
  (lines 989-997), any failure of unshare(CLONE_NEWNET) is treated as a…

--

selftests/net: run tun tests in a dedicated network namespace

The patch replaces TEST_HARNESS_MAIN in tools/testing/selftests/net/tun.c
with a hand-written main() that calls unshare(CLONE_NEWNET) before running
the harness, so tap and geneve devices are created outside the init netns
that systemd-udevd watches.

Closes: https://bugs.launchpad.net/bugs/2158217

> diff --git a/tools/testing/selftests/net/tun.c b/tools/testing/selftests/net/tun.c
> index abe488bac50bb..7f118ba3ae0bb 100644
> --- a/tools/testing/selftests/net/tun.c
> +++ b/tools/testing/selftests/net/tun.c

[ ... ]

> @@ -985,4 +986,12 @@ XFAIL_ADD(tun_vnet_udptnl, 6in4_over_maxbytes, recv_gso_packet);
> XFAIL_ADD(tun_vnet_udptnl, 4in6_over_maxbytes, recv_gso_packet);
> XFAIL_ADD(tun_vnet_udptnl, 6in6_over_maxbytes, recv_gso_packet);
>
> -TEST_HARNESS_MAIN
> +int main(int argc, char **argv)
> +{
> +	if (unshare(CLONE_NEWNET) < 0) {
> +		perror("unshare(CLONE_NEWNET)");
> +		return 1;
> +	}
> +
> +	return test_harness_run(argc, argv);
> +}

[Severity: Medium]
Should an unshare() failure here be reported as FAIL for the whole binary?

Returning 1 from main() is KSFT_FAIL, and since test_harness_run() is never
reached, ksft_print_header() and ksft_set_plan() never run either, so the
binary emits no "TAP version 13" line, no "1..N" plan and no per-test result
lines at all.  Would a runner then be able to tell "network namespaces
unavailable" apart from a real tun/tap regression?

There appear to be two reachable ways to land in this path without any
actual tun/tap problem:

CONFIG_NET_NS=n, where the stub in include/net/net_namespace.h rejects the
flag outright:

  static inline struct net *copy_net_ns(u64 flags,
	struct user_namespace *user_ns, struct net *old_net)
  {
	if (flags & CLONE_NEWNET)
		return ERR_PTR(-EINVAL);
	return old_net;
  }

and running with CAP_NET_ADMIN but without CAP_SYS_ADMIN (for example a
container started with --cap-add=NET_ADMIN), where unshare(CLONE_NEWNET)
fails with EPERM.

In both cases the pre-existing tun fixture is aborted too, even though
FIXTURE_SETUP(tun) only does:

  self->fd = tun_alloc(self->ifname);
  ASSERT_GE(self->fd, 0);

which needs /dev/net/tun and CAP_NET_ADMIN, but no network namespace.  Was
losing that coverage intended?

Would returning KSFT_SKIP (4) fit the convention better here?  The precedent
named in the changelog, "This mirrors the approach used by
selftests/net/ipsec.c, which also unshares from main()", handles the same
failure differently in selftests/net/ipsec.c:main():

  if (init_namespaces())
	ksft_exit_skip("Failed to create namespaces\n");

Other harness call sites do the same, for example
selftests/arm64/gcs/gcs-locking.c:main():

  ksft_print_msg("GCS was enabled, test unsupported\n");
  return KSFT_SKIP;

and selftests/net/icmp_rfc4884.c keeps the unshare inside
FIXTURE_SETUP(rfc4884) so the failure stays attributable inside the TAP
stream:

  ret = unshare(CLONE_NEWNET);
  ASSERT_EQ(ret, 0) {
	TH_LOG("unshare(CLONE_NEWNET) failed: %s", strerror(errno));
  }

One more question on the changelog: the patch adds CAP_SYS_ADMIN in the
current user namespace and CONFIG_NET_NS as new prerequisites for the entire
binary.  Should that be mentioned in the commit message?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260905085318.3416670-1-edoardo.canepa%40canonical.com