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
