Re: [PATCH 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.
---
--- Patch [4]: [PATCH 4/4] selftests: net: adopt harness for flow label mgr ---
commit 645c864c25dbeeb65888e7e3525b6298beb1ae4c
Author: Marcelo Mendes Spessoto Junior <marcelomspessoto@gmail.com>

selftests: net: adopt harness for flow label mgr

This converts tools/testing/selftests/net/ipv6_flowlabel_mgr.c from the
hand-rolled explain()/expect_pass()/expect_fail() helpers to
kselftest_harness.h, splitting run_tests() into individual TEST() cases.
It also removes parse_opts() and the cfg_long_running/cfg_verbose options.

A few questions about the conversion below.

> Author: Marcelo Mendes Spessoto Junior <marcelomspessoto@gmail.com>
> Signed-off-by: Marcelo Mendes Spessoto Junior <marcelospe@proton.me>

[Severity: Low]
The From: address (marcelomspessoto@gmail.com) and the Signed-off-by:
address (marcelospe@proton.me) differ, so the sign-off chain cannot be
matched to the author by tooling.

checkpatch.pl reports this as "From:/Signed-off-by: email address
mismatch". Could either the sign-off address be changed to match the
author address, or an explicit From: line be added at the top of the
patch body?

> Another change made to the structure of ipv6_flowlabel_mgr test file
> was the removal of parse_opts. The supported opts were already not
> used on TEST_GEN_PROGS and are not impactful for development. Opt (-l)
> skips long tests that are not time consuming at all, while the TH_LOG
> and code comments can cover the information obtainable from (-v).

[Severity: Medium]
Is this description accurate on two points?

First, the binary is listed in TEST_GEN_FILES in
tools/testing/selftests/net/Makefile, not TEST_GEN_PROGS; it is driven by
the TEST_PROGS script ipv6_flowlabel.sh, whose only invocation is:

	./in_netns.sh ./ipv6_flowlabel_mgr

Second, the -l tests are described as "not time consuming at all", but
FL_MIN_LINGER is 6 in this file, so each sleep(FL_MIN_LINGER * 2 + 1) is
13 seconds. Dropping cfg_long_running turns both previously gated blocks
into unconditional tests, adding roughly 26 seconds of wall time to every
run of ipv6_flowlabel.sh.

Since no in-tree invocation ever passed -l, these assertions have also
never been exercised by an automated run before. Should the changelog say
that the long-running checks are now always enabled, rather than
describing them as inexpensive?

> diff --git a/tools/testing/selftests/net/ipv6_flowlabel_mgr.c b/tools/testing/selftests/net/ipv6_flowlabel_mgr.c
> index da253dff2cfd4..9aa5c8146f171 100644
> --- a/tools/testing/selftests/net/ipv6_flowlabel_mgr.c
> +++ b/tools/testing/selftests/net/ipv6_flowlabel_mgr.c

[ ... ]

> @@ -39,23 +40,6 @@
>  		.sin6_port	= htons(8888),				\
>  	}
>  
> -#define explain(x)							\
> -	do { if (cfg_verbose) fprintf(stderr, "       " x "\n"); } while (0)
> -
> -#define __expect(x)							\
> -	do {								\
> -		if (!(x))						\
> -			fprintf(stderr, "[OK]   " #x "\n");		\
> -		else							\
> -			error(1, 0, "[ERR]  " #x " (line %d)", __LINE__); \
> -	} while (0)
> -
> -#define expect_pass(x)	__expect(x)
> -#define expect_fail(x)	__expect(!(x))
> -
> -static bool cfg_long_running;
> -static bool cfg_verbose;
> -
>  static int flowlabel_get(int fd, uint32_t label, uint8_t share, uint16_t flags)

[ ... ]

> @@ -141,6 +125,10 @@ static void tcp_connect(int listener, uint32_t flowlabel, int *client, int *acce
>  
>  static void set_flowlabel_consistency(bool enable)
>  {
> +	/* flowlabel_consistency must be disable to use the IPV6_FL_F_REFLECT
> +	 * flag. This helper function is required for setting up and tearing
> +	 * down test cases for this flag.
> +	 */

[Severity: Low]
This isn't a bug, but "must be disable" reads as a typo for "must be
disabled".

Also, does the second sentence match the code? set_flowlabel_consistency()
is called inline twice from reflect_flag(), and there is no
FIXTURE_SETUP()/FIXTURE_TEARDOWN() anywhere in the file, so nothing
performs the "tearing down" role described here.

[Severity: Low]
This isn't a bug introduced by this patch, but the helpers reached from the
new harness bodies still abort the process instead of emitting a TAP
result. set_flowlabel_consistency() calls error(1, errno, "open
flowlabel_consistency") when /proc/sys/net/ipv6/flowlabel_consistency
cannot be opened, and tcp_listen()/tcp_connect() do the same on bind,
listen, connect or accept failure.

>  	int fd;
>  
>  	fd = open("/proc/sys/net/ipv6/flowlabel_consistency", O_WRONLY);

An unprivileged run therefore exits the harness child with status 1, and
the harness prints a bare "not ok" with no TAP diagnostic. Given the
changelog says the conversion "enforces TAP standard", would SKIP() from
kselftest_harness.h be a better fit for these environment preconditions?

[ ... ]

> @@ -152,187 +140,270 @@ static void set_flowlabel_consistency(bool enable)

[ ... ]

> +TEST(exclusive_label_share)
> +{

[ ... ]

> +	err = flowlabel_get(fd, 2, IPV6_FL_S_ANY, IPV6_FL_F_CREATE);
> +	ASSERT_TRUE(err) TH_LOG("expected reuse to fail, due to linger");
> +
> +	sleep(FL_MIN_LINGER * 2 + 1);

[Severity: Medium]
These two assertions plus the 13 second sleep were previously behind
if (cfg_long_running) and never ran in CI. Was enabling them
unconditionally intended?

The other side effect is state left behind in the per-netns label table:
this test finishes holding label 2, and renew_label_linger() below
deliberately extends label 5's linger to 26 seconds. A repeated run of the
binary in the same namespace within that window, or a single filtered test
run, would fail the initial IPV6_FL_S_EXCL create.

Related to that, splitting run_tests() into separate TEST() cases suggests
per-test independence, but cannot_get_non_existent_label() and
cannot_put_non_existent_label() still assume label 1 does not exist while
can_create_and_get_and_put_labels() creates it. Is the ordering dependency
between these cases worth a comment, or could each case use a distinct
label?

> +
> +	err = flowlabel_get(fd, 2, IPV6_FL_S_ANY, IPV6_FL_F_CREATE);
> +	ASSERT_TRUE(!err) TH_LOG("expected reuse to succeed after linger");
> +
> +	ASSERT_EQ(0, close(fd));
> +}
> +
> +TEST(user_private_label_share)
> +{

[ ... ]

>  	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))
> +		if (flowlabel_get(fd, 3, IPV6_FL_S_USER, 0))
> +			exit(1);
> +		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);
> +		}
> +		if (!flowlabel_get(fd, 3, IPV6_FL_S_USER, 0))
> +			exit(1);
>  		exit(0);
>  	}

[Severity: Low]
Does this lose the per-check diagnostic that expect_pass()/expect_fail()
used to print? Both child expectations, "child can get the user-private
label" and "child cannot get it after setuid(nobody)", now collapse to the
same exit(1), so the parent can only report:

	ASSERT_EQ(0, WEXITSTATUS(wstatus)) TH_LOG("child reported unexpected result");

with no indication of which sub-check failed. The same applies to the
child in process_private_label_share(). Would distinct exit codes, or a
TH_LOG-carrying variant, keep the information the removed macros printed?

Separately, when setuid(USHRT_MAX) fails on a non-root run the child
prints "[INFO] skip setuid child test" and exits 0, so the harness reports
the whole case as "ok" while a sub-check was skipped. This matches the old
behaviour, but SKIP() is now available.

[ ... ]

> +TEST(renew_label_linger)
> +{
> +	/* After a label with EXCL share is put and lingered, it must be
> +	 * possible to create a new one. Check if RENEW action extends
> +	 * the linger period of put label, blocking creation after previous
> +	 * linger time.
> +	 */

[Severity: Low]
Does the first sentence describe this test? The body does
get(5, S_EXCL, CREATE) -> renew(5, 26) -> put(5) -> sleep(13) and then only
asserts that creation still fails; there is no assertion that creation
succeeds once the extended linger elapses. The positive case is covered by
exclusive_label_share() instead.

> +	int fd, err;

[ ... ]

> +	sleep(FL_MIN_LINGER * 2 + 1);
> +
> +	err = flowlabel_get(fd, 5, IPV6_FL_S_ANY, IPV6_FL_F_CREATE);
> +	ASSERT_TRUE(err) TH_LOG("expected reuse to fail, new linger time not over yet");
> +
> +	ASSERT_EQ(0, close(fd));
> +}

[ ... ]

> +TEST(reflect_flag)
> +{

[ ... ]

> +	set_flowlabel_consistency(false);
> +
> +	listener = tcp_listen();
> +	err = setsockopt(listener, SOL_IPV6, IPV6_FLOWLABEL_MGR, &reflect_on, sizeof(reflect_on));
> +	ASSERT_TRUE(!err) TH_LOG("failed to enable REFLECT on the listener");
> +
> +	tcp_connect(listener, 7, &cfd, &afd);
> +
> +	err = getsockopt(afd, SOL_IPV6, IPV6_FLOWLABEL_MGR, &reflect_query, &reflect_query_len);
> +	ASSERT_TRUE(!err) TH_LOG("failed to query the accepted socket's outgoing label");
> +	ASSERT_EQ(7, ntohl(reflect_query.flr_label)) TH_LOG("accepted socket did not reflect client's label");

[Severity: Medium]
Can this leave net.ipv6.flowlabel_consistency disabled?

set_flowlabel_consistency(false) is called at the top, and the matching
set_flowlabel_consistency(true) only runs on the straight-line success
path at the end. Every ASSERT_* in between ends the test child
immediately:

	ASSERT_*  ->  OPTIONAL_HANDLER(1)  ->  __bail(1, _metadata)

tools/testing/selftests/kselftest_harness.h:__bail() {
	if (for_realz) {
		if (t->teardown_fn)
			t->teardown_fn(false, t, t->self, t->variant);
		abort();
	}

A plain TEST() is registered by __TEST_IMPL, which never assigns
.teardown_fn, so teardown_fn is NULL and the child just abort()s without
running the restore at the end of the body. The error() paths inside
tcp_listen()/tcp_connect() and a harness timeout kill behave the same way.

Since each test runs in a forked child, the parent continues, so any test
running after reflect_flag (and anything else in that namespace or on the
host) proceeds with flow label consistency validation off. The most likely
trigger is exactly the regression this test exists to catch:

	ASSERT_EQ(7, ntohl(reflect_query.flr_label))

The in-tree path runs the binary under ./in_netns.sh, and the sysctl is
per-netns, so the leak is normally confined to a temporary namespace; but
the binary is also run directly by developers, where the host setting is
left disabled.

Would FIXTURE_SETUP()/FIXTURE_TEARDOWN() be a better home for this, since
teardown_fn is the only callback the harness still runs after a failed
assertion? That would also make the new comment on
set_flowlabel_consistency() about "setting up and tearing down" accurate.

> +
> +	err = setsockopt(afd, SOL_IPV6, IPV6_FLOWLABEL_MGR, &reflect_off, sizeof(reflect_off));
> +	ASSERT_TRUE(!err) TH_LOG("failed to disable REFLECT on the accepted socket");
> +
> +	err = setsockopt(afd, SOL_IPV6, IPV6_FLOWLABEL_MGR, &reflect_off, sizeof(reflect_off));
> +	ASSERT_TRUE(err) TH_LOG("expected disabling REFLECT twice to fail");
> +
> +	set_flowlabel_consistency(true);
> +
> +	ASSERT_EQ(0, close(afd));
> +	ASSERT_EQ(0, close(cfd));
> +	ASSERT_EQ(0, close(listener));
>  }

For reference, the earlier commit adding the IPV6_FL_F_REFLECT test also
skipped the restore on its error() paths, so the missing restore is not
new; this patch increases the number of abort points between the two calls.