Re: [PATCH net-next v2 4/4] selftests: net: adopt harness for flow label mgr

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
selftests: net: adopt harness for flow label mgr

This converts tools/testing/selftests/net/ipv6_flowlabel_mgr.c from the
local expect_pass/expect_fail/explain helpers to kselftest_harness.h,
splitting run_tests() into individual TEST() cases. It also removes
parse_opts(), so the two previously -l gated checks now always run.

> diff --git a/tools/testing/selftests/net/ipv6_flowlabel_mgr.c b/tools/testing/selftests/net/ipv6_flowlabel_mgr.c
> index af87eec799c80..482921f7ee119 100644
> --- a/tools/testing/selftests/net/ipv6_flowlabel_mgr.c
> +++ b/tools/testing/selftests/net/ipv6_flowlabel_mgr.c

[ ... ]

> @@ -159,230 +131,335 @@ static void tcp_connect(int listener, uint32_t flowlabel, int *client, int *acce

[ ... ]

>  	pid = fork();
> -	if (pid == -1)
> -		error(1, errno, "fork");
> +	ASSERT_NE(-1, pid) TH_LOG("fork failed");
>  	if (!pid) {
> -		expect_pass(flowlabel_get(fd, 3, IPV6_FL_S_USER, 0));
> -		if (setuid(USHRT_MAX))
> +		err = flowlabel_get(fd, 3, IPV6_FL_S_USER, 0);
> +		ASSERT_TRUE(!err) TH_LOG("child failed to get the user-private label");
> +
> +		if (setuid(USHRT_MAX)) {
>  			fprintf(stderr, "[INFO] skip setuid child test\n");
> -		else
> -			expect_fail(flowlabel_get(fd, 3, IPV6_FL_S_USER, 0));
> +			exit(0);
> +		}

[Severity: Medium]
Should this path report a skip rather than a pass?  In
TEST(user_private_label_share), when setuid(USHRT_MAX) fails the child
prints to stderr and calls exit(0).

The parent then runs:

	ASSERT_EQ(pid, wait(&wstatus)) TH_LOG("wait failed");
	ASSERT_TRUE(WIFEXITED(wstatus)) TH_LOG("child did not exit normally");
	ASSERT_EQ(0, WEXITSTATUS(wstatus)) TH_LOG("child reported unexpected result");

All three succeed, so the harness emits "ok user_private_label_share" even
though the check the test is named for, that an IPV6_FL_S_USER label cannot
be obtained after switching uid, never executed.

setuid(USHRT_MAX) fails when the binary runs without CAP_SETUID, or in a
user namespace where uid 65535 is unmapped, which is reachable since
ipv6_flowlabel.sh drives this through ./in_netns.sh.

Would propagating KSFT_SKIP work better here, for example the child doing
exit(KSFT_SKIP) and the parent turning that into SKIP(return, ...)?

The silent-skip logic itself predates this patch, but before the conversion
there were no per-test TAP results; now the test individually reports "ok"
instead of "# SKIP".

The raw fprintf(stderr, "[INFO] ...") is also the only diagnostic left in
the converted file that does not go through TH_LOG, which prefixes output
with "# ".

[ ... ]

> +TEST(reflect_flag)
> +{
[ ... ]
> +	if (flowlabel_consistency_enabled())
> +		SKIP(return,
> +		     "net.ipv6.flowlabel_consistency must be 0 (run via ipv6_flowlabel.sh)");

This is the idiom the setuid path above could use: SKIP() sets
_metadata->exit_code = KSFT_SKIP and prints a "# SKIP" diagnostic, so an
unmet precondition is not reported as a pass.  Could both unmet-precondition
sites in this file use it consistently?

[ ... ]