Re: [PATCH net-next v2 1/4] selftests: net: test IPV6_FL_A_RENEW

[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: test IPV6_FL_A_RENEW

This adds selftest coverage for the IPV6_FL_A_RENEW flow label action: a
new flowlabel_renew() helper, an expect_fail_errno() assertion macro, and
tests that renew fails with ESRCH for a nonexistent label and succeeds for
an existing one. A long-running case verifies that renewing extends the
linger period so a new label cannot be created after the original linger
time.

A couple of questions below.

> diff --git a/tools/testing/selftests/net/ipv6_flowlabel_mgr.c b/tools/testing/selftests/net/ipv6_flowlabel_mgr.c
> index af95b48acea90..01fab414895c5 100644
> --- a/tools/testing/selftests/net/ipv6_flowlabel_mgr.c
> +++ b/tools/testing/selftests/net/ipv6_flowlabel_mgr.c
> @@ -27,6 +27,7 @@
>  
>  /* from net/ipv6/ip6_flowlabel.c */
>  #define FL_MIN_LINGER		6
> +#define FL_MAX_LINGER	150

[Severity: Low]
Is FL_MAX_LINGER used anywhere?  Grepping the file, the identifier appears
only on this definition line, both in this commit and at the end of the
series.  The only linger values the test passes are
2 * (FL_MIN_LINGER * 2 + 1) and the implicit 0.

The kernel path this constant mirrors is the one RENEW error case the new
tests do not exercise:

net/ipv6/ip6_flowlabel.c:check_linger() {
	if (ttl > FL_MAX_LINGER && !capable(CAP_NET_ADMIN))
		return 0;
	...
}

and fl6_renew() turns that 0 into -EPERM.  Since the selftest is driven
through in_netns.sh as root, an over-max linger value would not reach that
branch anyway.

Would it make sense to either drop the define, or add an unprivileged
over-max linger case that expects EPERM?  As it stands an unreferenced
object-like macro produces no warning, so it suggests coverage that is not
there.

Also, this isn't a bug, but the new line uses a single tab while the
adjacent #define FL_MIN_LINGER uses two, so the values no longer line up.

[ ... ]

> @@ -71,6 +84,17 @@ static int flowlabel_put(int fd, uint32_t label)
>  	return setsockopt(fd, SOL_IPV6, IPV6_FLOWLABEL_MGR, &req, sizeof(req));
>  }
>  
> +static int flowlabel_renew(int fd, uint32_t label, uint16_t linger)
> +{
> +	struct in6_flowlabel_req req = {
> +		.flr_action = IPV6_FL_A_RENEW,
> +		.flr_label = htonl(label),
> +		.flr_linger = linger,
> +	};
> +
> +	return setsockopt(fd, SOL_IPV6, IPV6_FLOWLABEL_MGR, &req, sizeof(req));
> +}

[Severity: Low]
Should flr_share be set explicitly here?  Leaving it out zeroes it, and 0
is IPV6_FL_S_NONE, which selects a second renew path in the kernel:

net/ipv6/ip6_flowlabel.c:ipv6_flowlabel_renew() {
	rcu_read_lock();
	for_each_sk_fl_rcu(sk, sfl) {
		if (sfl->fl->label == freq->flr_label) {
			err = fl6_renew(sfl->fl, freq->flr_linger,
					freq->flr_expires);
	...
	if (freq->flr_share == IPV6_FL_S_NONE &&
	    ns_capable(net->user_ns, CAP_NET_ADMIN)) {
		struct ip6_flowlabel *fl = fl_lookup(net, freq->flr_label);

		if (fl) {
			err = fl6_renew(fl, freq->flr_linger,
					freq->flr_expires);
	...
	return -ESRCH;
}

The test runs as root via ipv6_flowlabel.sh -> in_netns.sh, so the
CAP_NET_ADMIN fallback is always available, and labels 5 and 6 are also
present in the per-netns hash because the same socket created them.

If the for_each_sk_fl_rcu() matching branch ever stopped matching, would
fl_lookup() not still find the label, so fl6_renew() returns 0 and both
the "renew succeeds" and the long-running EPERM linger-extension
assertions still pass?

Would setting flr_share to the label's share (IPV6_FL_S_EXCL for these
labels), or adding a case using a label the socket does not own, separate
the two branches?

Cross-instance finding from sashiko-gemini (2a95a5b15b2d5ec9fd64306a937e0876383a2116fb10cbee36476a293b84adb4):
[Severity: Medium]
The `expect_fail_errno` macro incorrectly formats the error message when the tested function unexpectedly succeeds, printing a stale `errno` value rather than clearly indicating unexpected success.
-- 
pw-bot: cr