[PATCH net-next 0/4] net: selftests: adjustments to ipv6_flowlabel_mgr

[PATCH net-next 0/4] net: selftests: adjustments to ipv6_flowlabel_mgr

[Marcelo Mendes Spessoto Junior]

The ipv6_flowlabel_mgr test file was lacking coverage for the IPV6_FL_A_RENEW
action, and the IPV6_FL_F_REMOTE and IPV6_FL_F_REFLECT flags. The first
three patches from this set aim to add a proper test case for each.

The fourth patch proposes the adoption of "kselftest_harness.h" helpers,
improving code readability and conforming to the implementation of the most
recent selftests.

Marcelo Mendes Spessoto Junior (4):
  selftests: net: test IPV6_FL_A_RENEW
  selftests: net: test IPV6_FL_F_REMOTE
  selftests: net: test IPV6_FL_F_REFLECT
  selftests: net: adopt harness for flow label mgr

 .../selftests/net/ipv6_flowlabel_mgr.c        | 450 +++++++++++++-----
 1 file changed, 330 insertions(+), 120 deletions(-)

-- 
2.55.0

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

[Marcelo Mendes Spessoto Junior]

RENEW was the only flow label action without selftests coverage.

Based on the previously implemented EXCL share test, that demonstrates
that a new flow label with same label can be created after the linger
period, use renew to show that the flow label can last longer and
block a new flow label creation after the previous linger time.

Signed-off-by: Marcelo Mendes Spessoto Junior <marcelospe@proton.me>
---
 .../selftests/net/ipv6_flowlabel_mgr.c        | 23 +++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/tools/testing/selftests/net/ipv6_flowlabel_mgr.c b/tools/testing/selftests/net/ipv6_flowlabel_mgr.c
index af95b48acea9..cfa7e6270994 100644
--- a/tools/testing/selftests/net/ipv6_flowlabel_mgr.c
+++ b/tools/testing/selftests/net/ipv6_flowlabel_mgr.c
@@ -71,6 +71,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));
+}
+
 static void run_tests(int fd)
 {
 	int wstatus;
@@ -160,6 +171,18 @@ static void run_tests(int fd)
 		error(1, errno, "wait");
 	if (!WIFEXITED(wstatus) || WEXITSTATUS(wstatus) != 0)
 		error(1, errno, "wait: unexpected child result");
+
+	if (cfg_long_running) {
+		explain("create a new label with FL_MIN_LINGER linger time");
+		expect_pass(flowlabel_get(fd, 5, IPV6_FL_S_EXCL, IPV6_FL_F_CREATE));
+		explain("renew the label to increase its linger time and put it");
+		expect_pass(flowlabel_renew(fd, 5, 2 * (FL_MIN_LINGER * 2 + 1)));
+		expect_pass(flowlabel_put(fd, 5));
+		sleep(FL_MIN_LINGER * 2 + 1);
+		explain("The label cannot be created because the new linger time is not over yet");
+		expect_fail(flowlabel_get(fd, 5, IPV6_FL_S_ANY, IPV6_FL_F_CREATE));
+	}
+
 }
 
 static void parse_opts(int argc, char **argv)
-- 
2.55.0

[PATCH net-next 2/4] selftests: net: test IPV6_FL_F_REMOTE

[Marcelo Mendes Spessoto Junior]

This flag retrieves the latest label received by the socket with
a getsockopt query. Therefore, the validation of this flag requires
a brief connection setup (source code for flow label shows it must
be TCP).

The simple TCP connection logic was wrapped inside two simple helpers,
because there are other uncovered features of flow label mgr that
could benefit from it (such as IPV6_FL_F_REFLECT).

Signed-off-by: Marcelo Mendes Spessoto Junior <marcelospe@proton.me>
---
 .../selftests/net/ipv6_flowlabel_mgr.c        | 71 +++++++++++++++++++
 1 file changed, 71 insertions(+)

diff --git a/tools/testing/selftests/net/ipv6_flowlabel_mgr.c b/tools/testing/selftests/net/ipv6_flowlabel_mgr.c
index cfa7e6270994..383a84ea9385 100644
--- a/tools/testing/selftests/net/ipv6_flowlabel_mgr.c
+++ b/tools/testing/selftests/net/ipv6_flowlabel_mgr.c
@@ -24,10 +24,20 @@
 #ifndef IPV6_FLOWLABEL_MGR
 #define IPV6_FLOWLABEL_MGR	32
 #endif
+#ifndef IPV6_FLOWINFO_SEND
+#define IPV6_FLOWINFO_SEND	33
+#endif
 
 /* from net/ipv6/ip6_flowlabel.c */
 #define FL_MIN_LINGER		6
 
+#define INIT_SIN6_LOOPBACK(name)					\
+	struct sockaddr_in6 name = {					\
+		.sin6_family	= AF_INET6,				\
+		.sin6_addr	= IN6ADDR_LOOPBACK_INIT,		\
+		.sin6_port	= htons(8888),				\
+	}
+
 #define explain(x)							\
 	do { if (cfg_verbose) fprintf(stderr, "       " x "\n"); } while (0)
 
@@ -82,6 +92,52 @@ static int flowlabel_renew(int fd, uint32_t label, uint16_t linger)
 	return setsockopt(fd, SOL_IPV6, IPV6_FLOWLABEL_MGR, &req, sizeof(req));
 }
 
+static int tcp_listen(void)
+{
+	INIT_SIN6_LOOPBACK(addr);
+	const int one = 1;
+	int fd;
+
+	fd = socket(PF_INET6, SOCK_STREAM, 0);
+	if (fd == -1)
+		error(1, errno, "socket listener");
+	if (setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &one, sizeof(one)))
+		error(1, errno, "setsockopt SO_REUSEADDR");
+	if (bind(fd, (void *)&addr, sizeof(addr)))
+		error(1, errno, "bind");
+	if (listen(fd, 1))
+		error(1, errno, "listen");
+
+	return fd;
+}
+
+static void tcp_connect(int listener, uint32_t flowlabel, int *client, int *accepted)
+{
+	INIT_SIN6_LOOPBACK(addr);
+	const int one = 1;
+	int cfd, afd;
+
+	cfd = socket(PF_INET6, SOCK_STREAM, 0);
+	if (cfd == -1)
+		error(1, errno, "socket client");
+
+	if (flowlabel_get(cfd, flowlabel, IPV6_FL_S_EXCL, IPV6_FL_F_CREATE))
+		error(1, errno, "flowlabel_get");
+	if (setsockopt(cfd, SOL_IPV6, IPV6_FLOWINFO_SEND, &one, sizeof(one)))
+		error(1, errno, "setsockopt flowinfo_send");
+	addr.sin6_flowinfo = htonl(flowlabel);
+
+	if (connect(cfd, (void *)&addr, sizeof(addr)))
+		error(1, errno, "connect");
+
+	afd = accept(listener, NULL, NULL);
+	if (afd == -1)
+		error(1, errno, "accept");
+
+	*client = cfd;
+	*accepted = afd;
+}
+
 static void run_tests(int fd)
 {
 	int wstatus;
@@ -183,6 +239,21 @@ static void run_tests(int fd)
 		expect_fail(flowlabel_get(fd, 5, IPV6_FL_S_ANY, IPV6_FL_F_CREATE));
 	}
 
+	explain("Prepare TCP SYN for REMOTE flag validation");
+	int remote_listener = tcp_listen();
+	int remote_cfd, remote_afd;
+	tcp_connect(remote_listener, 6, &remote_cfd, &remote_afd);
+	struct in6_flowlabel_req freq = {
+		.flr_action = IPV6_FL_A_GET,
+		.flr_flags = IPV6_FL_F_REMOTE,
+	};
+	socklen_t freq_len = sizeof(freq);
+	explain("Query for label sent by client with IPV6_FL_F_REMOTE");
+	expect_pass(getsockopt(remote_afd, SOL_IPV6, IPV6_FLOWLABEL_MGR, &freq, &freq_len));
+	expect_pass(ntohl(freq.flr_label) != 6);
+	close(remote_afd);
+	close(remote_cfd);
+	close(remote_listener);
 }
 
 static void parse_opts(int argc, char **argv)
-- 
2.55.0

[PATCH net-next 3/4] selftests: net: test IPV6_FL_F_REFLECT

[Marcelo Mendes Spessoto Junior]

According to source code, flowlabel_consistency must be deactivated
for the IPV6_FL_F_REFLECT flag to work.

The previously defined tcp_listen and tcp_connect helpers were reused,
since the connection flow required for REFLECT validation is very
similar to REMOTE.

Signed-off-by: Marcelo Mendes Spessoto Junior <marcelospe@proton.me>
---
 .../selftests/net/ipv6_flowlabel_mgr.c        | 45 +++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/tools/testing/selftests/net/ipv6_flowlabel_mgr.c b/tools/testing/selftests/net/ipv6_flowlabel_mgr.c
index 383a84ea9385..da253dff2cfd 100644
--- a/tools/testing/selftests/net/ipv6_flowlabel_mgr.c
+++ b/tools/testing/selftests/net/ipv6_flowlabel_mgr.c
@@ -6,6 +6,7 @@
 #include <arpa/inet.h>
 #include <error.h>
 #include <errno.h>
+#include <fcntl.h>
 #include <limits.h>
 #include <linux/in6.h>
 #include <stdbool.h>
@@ -138,6 +139,19 @@ static void tcp_connect(int listener, uint32_t flowlabel, int *client, int *acce
 	*accepted = afd;
 }
 
+static void set_flowlabel_consistency(bool enable)
+{
+	int fd;
+
+	fd = open("/proc/sys/net/ipv6/flowlabel_consistency", O_WRONLY);
+	if (fd == -1)
+		error(1, errno, "open flowlabel_consistency");
+	if (write(fd, enable ? "1" : "0", 1) != 1)
+		error(1, errno, "write flowlabel_consistency");
+	if (close(fd))
+		error(1, errno, "close flowlabel_consistency");
+}
+
 static void run_tests(int fd)
 {
 	int wstatus;
@@ -254,6 +268,37 @@ static void run_tests(int fd)
 	close(remote_afd);
 	close(remote_cfd);
 	close(remote_listener);
+
+	explain("Prepare TCP SYN for REFLECT flag validation");
+	set_flowlabel_consistency(false);
+	int reflect_listener = tcp_listen();
+	struct in6_flowlabel_req reflect_on = {
+		.flr_action = IPV6_FL_A_GET,
+		.flr_flags = IPV6_FL_F_REFLECT,
+	};
+	explain("Enable REFLECT on the listener before the client connects");
+	expect_pass(setsockopt(reflect_listener, SOL_IPV6, IPV6_FLOWLABEL_MGR, &reflect_on, sizeof(reflect_on)));
+	int reflect_cfd, reflect_afd;
+	tcp_connect(reflect_listener, 7, &reflect_cfd, &reflect_afd);
+	struct in6_flowlabel_req reflect_query = {
+		.flr_action = IPV6_FL_A_GET,
+	};
+	socklen_t reflect_query_len = sizeof(reflect_query);
+	explain("Query the accepted socket's outgoing label, should be reflected");
+	expect_pass(getsockopt(reflect_afd, SOL_IPV6, IPV6_FLOWLABEL_MGR, &reflect_query, &reflect_query_len));
+	expect_pass(ntohl(reflect_query.flr_label) != 7);
+	struct in6_flowlabel_req reflect_off = {
+		.flr_action = IPV6_FL_A_PUT,
+		.flr_flags = IPV6_FL_F_REFLECT,
+	};
+	explain("PUT+REFLECT disables reflection on the accepted socket");
+	expect_pass(setsockopt(reflect_afd, SOL_IPV6, IPV6_FLOWLABEL_MGR, &reflect_off, sizeof(reflect_off)));
+	explain("cannot disable reflection twice");
+	expect_fail(setsockopt(reflect_afd, SOL_IPV6, IPV6_FLOWLABEL_MGR, &reflect_off, sizeof(reflect_off)));
+	set_flowlabel_consistency(true);
+	close(reflect_afd);
+	close(reflect_cfd);
+	close(reflect_listener);
 }
 
 static void parse_opts(int argc, char **argv)
-- 
2.55.0

[PATCH 4/4] selftests: net: adopt harness for flow label mgr

[Marcelo Mendes Spessoto Junior]

The kselftest_harness.h file contains modern helpers to build tests
for kselftest. Dropping current test helpers for ipv6_flowlabel_mgr
to use harness helps tests to be more legible and conform to the
structure of the latest tests. It also enforces TAP standard.

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).

Signed-off-by: Marcelo Mendes Spessoto Junior <marcelospe@proton.me>
---
 .../selftests/net/ipv6_flowlabel_mgr.c        | 397 +++++++++++-------
 1 file changed, 234 insertions(+), 163 deletions(-)

diff --git a/tools/testing/selftests/net/ipv6_flowlabel_mgr.c b/tools/testing/selftests/net/ipv6_flowlabel_mgr.c
index da253dff2cfd..9aa5c8146f17 100644
--- a/tools/testing/selftests/net/ipv6_flowlabel_mgr.c
+++ b/tools/testing/selftests/net/ipv6_flowlabel_mgr.c
@@ -20,6 +20,7 @@
 #include <sys/types.h>
 #include <sys/wait.h>
 #include <unistd.h>
+#include "kselftest_harness.h"
 
 /* uapi/glibc weirdness may leave this undefined */
 #ifndef IPV6_FLOWLABEL_MGR
@@ -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)
 {
 	struct in6_flowlabel_req req = {
@@ -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.
+	 */
 	int fd;
 
 	fd = open("/proc/sys/net/ipv6/flowlabel_consistency", O_WRONLY);
@@ -152,187 +140,270 @@ static void set_flowlabel_consistency(bool enable)
 		error(1, errno, "close flowlabel_consistency");
 }
 
-static void run_tests(int fd)
+TEST(cannot_get_non_existent_label)
+{
+	int fd, err;
+
+	fd = socket(PF_INET6, SOCK_DGRAM, 0);
+	ASSERT_GE(fd, 0) TH_LOG("socket failed");
+
+	err = flowlabel_get(fd, 1, IPV6_FL_S_ANY, 0);
+	ASSERT_TRUE(err) TH_LOG("expected get of a non-existent label to fail");
+
+	ASSERT_EQ(0, close(fd));
+}
+
+TEST(cannot_put_non_existent_label)
 {
-	int wstatus;
+	int fd, err;
+
+	fd = socket(PF_INET6, SOCK_DGRAM, 0);
+	ASSERT_GE(fd, 0) TH_LOG("socket failed");
+
+	err = flowlabel_put(fd, 1);
+	ASSERT_TRUE(err) TH_LOG("expected put of a non-existent label to fail");
+
+	ASSERT_EQ(0, close(fd));
+}
+
+TEST(cannot_create_label_greater_than_20_bits)
+{
+	int fd, err;
+
+	fd = socket(PF_INET6, SOCK_DGRAM, 0);
+	ASSERT_GE(fd, 0) TH_LOG("socket failed");
+
+	err = flowlabel_get(fd, 0x1FFFFF, IPV6_FL_S_ANY, IPV6_FL_F_CREATE);
+	ASSERT_TRUE(err) TH_LOG("expected label > 20 bits to be rejected");
+
+	ASSERT_EQ(0, close(fd));
+}
+
+TEST(can_create_and_get_and_put_labels)
+{
+	int fd, err;
+
+	fd = socket(PF_INET6, SOCK_DGRAM, 0);
+	ASSERT_GE(fd, 0) TH_LOG("socket failed");
+
+	err = flowlabel_get(fd, 1, IPV6_FL_S_ANY, IPV6_FL_F_CREATE);
+	ASSERT_TRUE(!err) TH_LOG("failed to create label (FL_F_CREATE)");
+
+	err = flowlabel_get(fd, 1, IPV6_FL_S_ANY, 0);
+	ASSERT_TRUE(!err) TH_LOG("failed to get the label without FL_F_CREATE");
+
+	err = flowlabel_get(fd, 1, IPV6_FL_S_ANY, IPV6_FL_F_CREATE);
+	ASSERT_TRUE(!err) TH_LOG("failed to get it again with create flag set, too");
+
+	err = flowlabel_get(fd, 1, IPV6_FL_S_ANY, IPV6_FL_F_CREATE | IPV6_FL_F_EXCL);
+	ASSERT_TRUE(err) TH_LOG("expected FL_F_EXCL to reject an already-existing label");
+
+	err = flowlabel_put(fd, 1);
+	ASSERT_TRUE(!err) TH_LOG("failed to put first reference");
+	err = flowlabel_put(fd, 1);
+	ASSERT_TRUE(!err) TH_LOG("failed to put second reference");
+	err = flowlabel_put(fd, 1);
+	ASSERT_TRUE(!err) TH_LOG("failed to put third reference");
+	err = flowlabel_put(fd, 1);
+	ASSERT_TRUE(err) TH_LOG("expected fourth put to fail, no references left");
+
+	ASSERT_EQ(0, close(fd));
+}
+
+TEST(exclusive_label_share)
+{
+	int fd, err;
+
+	fd = socket(PF_INET6, SOCK_DGRAM, 0);
+	ASSERT_GE(fd, 0) TH_LOG("socket failed");
+
+	err = flowlabel_get(fd, 2, IPV6_FL_S_EXCL, IPV6_FL_F_CREATE);
+	ASSERT_TRUE(!err) TH_LOG("failed to create a new exclusive label (FL_S_EXCL)");
+
+	err = flowlabel_get(fd, 2, IPV6_FL_S_ANY, IPV6_FL_F_CREATE);
+	ASSERT_TRUE(err) TH_LOG("expected reuse in non-exclusive mode to fail");
+
+	err = flowlabel_get(fd, 2, IPV6_FL_S_EXCL, IPV6_FL_F_CREATE);
+	ASSERT_TRUE(err) TH_LOG("expected reuse in exclusive mode to fail too");
+
+	err = flowlabel_put(fd, 2);
+	ASSERT_TRUE(!err) TH_LOG("failed to put the exclusive label");
+
+	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);
+
+	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)
+{
+	int fd, err, wstatus;
 	pid_t pid;
 
-	explain("cannot get non-existent label");
-	expect_fail(flowlabel_get(fd, 1, IPV6_FL_S_ANY, 0));
-
-	explain("cannot put non-existent label");
-	expect_fail(flowlabel_put(fd, 1));
-
-	explain("cannot create label greater than 20 bits");
-	expect_fail(flowlabel_get(fd, 0x1FFFFF, IPV6_FL_S_ANY,
-				  IPV6_FL_F_CREATE));
-
-	explain("create a new label (FL_F_CREATE)");
-	expect_pass(flowlabel_get(fd, 1, IPV6_FL_S_ANY, IPV6_FL_F_CREATE));
-	explain("can get the label (without FL_F_CREATE)");
-	expect_pass(flowlabel_get(fd, 1, IPV6_FL_S_ANY, 0));
-	explain("can get it again with create flag set, too");
-	expect_pass(flowlabel_get(fd, 1, IPV6_FL_S_ANY, IPV6_FL_F_CREATE));
-	explain("cannot get it again with the exclusive (FL_FL_EXCL) flag");
-	expect_fail(flowlabel_get(fd, 1, IPV6_FL_S_ANY,
-					 IPV6_FL_F_CREATE | IPV6_FL_F_EXCL));
-	explain("can now put exactly three references");
-	expect_pass(flowlabel_put(fd, 1));
-	expect_pass(flowlabel_put(fd, 1));
-	expect_pass(flowlabel_put(fd, 1));
-	expect_fail(flowlabel_put(fd, 1));
-
-	explain("create a new exclusive label (FL_S_EXCL)");
-	expect_pass(flowlabel_get(fd, 2, IPV6_FL_S_EXCL, IPV6_FL_F_CREATE));
-	explain("cannot get it again in non-exclusive mode");
-	expect_fail(flowlabel_get(fd, 2, IPV6_FL_S_ANY,  IPV6_FL_F_CREATE));
-	explain("cannot get it again in exclusive mode either");
-	expect_fail(flowlabel_get(fd, 2, IPV6_FL_S_EXCL, IPV6_FL_F_CREATE));
-	expect_pass(flowlabel_put(fd, 2));
-
-	if (cfg_long_running) {
-		explain("cannot reuse the label, due to linger");
-		expect_fail(flowlabel_get(fd, 2, IPV6_FL_S_ANY,
-					  IPV6_FL_F_CREATE));
-		explain("after sleep, can reuse");
-		sleep(FL_MIN_LINGER * 2 + 1);
-		expect_pass(flowlabel_get(fd, 2, IPV6_FL_S_ANY,
-					  IPV6_FL_F_CREATE));
-	}
+	fd = socket(PF_INET6, SOCK_DGRAM, 0);
+	ASSERT_GE(fd, 0) TH_LOG("socket failed");
+
+	err = flowlabel_get(fd, 3, IPV6_FL_S_USER, IPV6_FL_F_CREATE);
+	ASSERT_TRUE(!err) TH_LOG("failed to create a new user-private label (FL_S_USER)");
+
+	err = flowlabel_get(fd, 3, IPV6_FL_S_ANY, 0);
+	ASSERT_TRUE(err) TH_LOG("expected get in non-exclusive mode to fail");
+
+	err = flowlabel_get(fd, 3, IPV6_FL_S_EXCL, 0);
+	ASSERT_TRUE(err) TH_LOG("expected get in exclusive mode to fail");
+
+	err = flowlabel_get(fd, 3, IPV6_FL_S_USER, 0);
+	ASSERT_TRUE(!err) TH_LOG("failed to get it again in user mode");
 
-	explain("create a new user-private label (FL_S_USER)");
-	expect_pass(flowlabel_get(fd, 3, IPV6_FL_S_USER, IPV6_FL_F_CREATE));
-	explain("cannot get it again in non-exclusive mode");
-	expect_fail(flowlabel_get(fd, 3, IPV6_FL_S_ANY, 0));
-	explain("cannot get it again in exclusive mode");
-	expect_fail(flowlabel_get(fd, 3, IPV6_FL_S_EXCL, 0));
-	explain("can get it again in user mode");
-	expect_pass(flowlabel_get(fd, 3, IPV6_FL_S_USER, 0));
-	explain("child process can get it too, but not after setuid(nobody)");
 	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);
 	}
-	if (wait(&wstatus) == -1)
-		error(1, errno, "wait");
-	if (!WIFEXITED(wstatus) || WEXITSTATUS(wstatus) != 0)
-		error(1, errno, "wait: unexpected child result");
-
-	explain("create a new process-private label (FL_S_PROCESS)");
-	expect_pass(flowlabel_get(fd, 4, IPV6_FL_S_PROCESS, IPV6_FL_F_CREATE));
-	explain("can get it again");
-	expect_pass(flowlabel_get(fd, 4, IPV6_FL_S_PROCESS, 0));
-	explain("child process cannot can get it");
+	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");
+
+	ASSERT_EQ(0, close(fd));
+}
+
+TEST(process_private_label_share)
+{
+	int fd, err, wstatus;
+	pid_t pid;
+
+	fd = socket(PF_INET6, SOCK_DGRAM, 0);
+	ASSERT_GE(fd, 0) TH_LOG("socket failed");
+
+	err = flowlabel_get(fd, 4, IPV6_FL_S_PROCESS, IPV6_FL_F_CREATE);
+	ASSERT_TRUE(!err) TH_LOG("failed to create a new process-private label (FL_S_PROCESS)");
+
+	err = flowlabel_get(fd, 4, IPV6_FL_S_PROCESS, 0);
+	ASSERT_TRUE(!err) TH_LOG("failed to get it again");
+
 	pid = fork();
-	if (pid == -1)
-		error(1, errno, "fork");
+	ASSERT_NE(-1, pid) TH_LOG("fork failed");
 	if (!pid) {
-		expect_fail(flowlabel_get(fd, 4, IPV6_FL_S_PROCESS, 0));
+		if (!flowlabel_get(fd, 4, IPV6_FL_S_PROCESS, 0))
+			exit(1);
 		exit(0);
 	}
-	if (wait(&wstatus) == -1)
-		error(1, errno, "wait");
-	if (!WIFEXITED(wstatus) || WEXITSTATUS(wstatus) != 0)
-		error(1, errno, "wait: unexpected child result");
-
-	if (cfg_long_running) {
-		explain("create a new label with FL_MIN_LINGER linger time");
-		expect_pass(flowlabel_get(fd, 5, IPV6_FL_S_EXCL, IPV6_FL_F_CREATE));
-		explain("renew the label to increase its linger time and put it");
-		expect_pass(flowlabel_renew(fd, 5, 2 * (FL_MIN_LINGER * 2 + 1)));
-		expect_pass(flowlabel_put(fd, 5));
-		sleep(FL_MIN_LINGER * 2 + 1);
-		explain("The label cannot be created because the new linger time is not over yet");
-		expect_fail(flowlabel_get(fd, 5, IPV6_FL_S_ANY, IPV6_FL_F_CREATE));
-	}
+	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");
+
+	ASSERT_EQ(0, close(fd));
+}
+
+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.
+	 */
+	int fd, err;
+
+	fd = socket(PF_INET6, SOCK_DGRAM, 0);
+	ASSERT_GE(fd, 0) TH_LOG("socket failed");
+
+	err = flowlabel_get(fd, 5, IPV6_FL_S_EXCL, IPV6_FL_F_CREATE);
+	ASSERT_TRUE(!err) TH_LOG("failed to create a new label with FL_MIN_LINGER linger time");
+
+	err = flowlabel_renew(fd, 5, 2 * (FL_MIN_LINGER * 2 + 1));
+	ASSERT_TRUE(!err) TH_LOG("failed to renew the label to increase its linger time");
+
+	err = flowlabel_put(fd, 5);
+	ASSERT_TRUE(!err) TH_LOG("failed to put the label");
+
+	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));
+}
 
-	explain("Prepare TCP SYN for REMOTE flag validation");
-	int remote_listener = tcp_listen();
-	int remote_cfd, remote_afd;
-	tcp_connect(remote_listener, 6, &remote_cfd, &remote_afd);
+TEST(remote_flag)
+{
+	/* The REMOTE flag, used for getsockopt, is expected to retrieve the
+	 * label from the latest received header.
+	 */
 	struct in6_flowlabel_req freq = {
 		.flr_action = IPV6_FL_A_GET,
 		.flr_flags = IPV6_FL_F_REMOTE,
 	};
 	socklen_t freq_len = sizeof(freq);
-	explain("Query for label sent by client with IPV6_FL_F_REMOTE");
-	expect_pass(getsockopt(remote_afd, SOL_IPV6, IPV6_FLOWLABEL_MGR, &freq, &freq_len));
-	expect_pass(ntohl(freq.flr_label) != 6);
-	close(remote_afd);
-	close(remote_cfd);
-	close(remote_listener);
-
-	explain("Prepare TCP SYN for REFLECT flag validation");
-	set_flowlabel_consistency(false);
-	int reflect_listener = tcp_listen();
+	int listener, cfd, afd, err;
+
+	listener = tcp_listen();
+	tcp_connect(listener, 6, &cfd, &afd);
+
+	err = getsockopt(afd, SOL_IPV6, IPV6_FLOWLABEL_MGR, &freq, &freq_len);
+	ASSERT_TRUE(!err) TH_LOG("getsockopt with IPV6_FL_F_REMOTE failed");
+	ASSERT_EQ(6, ntohl(freq.flr_label)) TH_LOG("unexpected remote flow label");
+
+	ASSERT_EQ(0, close(afd));
+	ASSERT_EQ(0, close(cfd));
+	ASSERT_EQ(0, close(listener));
+}
+
+TEST(reflect_flag)
+{
+	/* The REFLECT flag acts as a trigger to the REPFLOW bit. When REPFLOW
+	 * is triggered for a socket, it adopts the label received from the
+	 * connected socket.
+	 */
 	struct in6_flowlabel_req reflect_on = {
 		.flr_action = IPV6_FL_A_GET,
 		.flr_flags = IPV6_FL_F_REFLECT,
 	};
-	explain("Enable REFLECT on the listener before the client connects");
-	expect_pass(setsockopt(reflect_listener, SOL_IPV6, IPV6_FLOWLABEL_MGR, &reflect_on, sizeof(reflect_on)));
-	int reflect_cfd, reflect_afd;
-	tcp_connect(reflect_listener, 7, &reflect_cfd, &reflect_afd);
 	struct in6_flowlabel_req reflect_query = {
 		.flr_action = IPV6_FL_A_GET,
 	};
-	socklen_t reflect_query_len = sizeof(reflect_query);
-	explain("Query the accepted socket's outgoing label, should be reflected");
-	expect_pass(getsockopt(reflect_afd, SOL_IPV6, IPV6_FLOWLABEL_MGR, &reflect_query, &reflect_query_len));
-	expect_pass(ntohl(reflect_query.flr_label) != 7);
 	struct in6_flowlabel_req reflect_off = {
 		.flr_action = IPV6_FL_A_PUT,
 		.flr_flags = IPV6_FL_F_REFLECT,
 	};
-	explain("PUT+REFLECT disables reflection on the accepted socket");
-	expect_pass(setsockopt(reflect_afd, SOL_IPV6, IPV6_FLOWLABEL_MGR, &reflect_off, sizeof(reflect_off)));
-	explain("cannot disable reflection twice");
-	expect_fail(setsockopt(reflect_afd, SOL_IPV6, IPV6_FLOWLABEL_MGR, &reflect_off, sizeof(reflect_off)));
-	set_flowlabel_consistency(true);
-	close(reflect_afd);
-	close(reflect_cfd);
-	close(reflect_listener);
-}
+	socklen_t reflect_query_len = sizeof(reflect_query);
+	int listener, cfd, afd, err;
 
-static void parse_opts(int argc, char **argv)
-{
-	int c;
-
-	while ((c = getopt(argc, argv, "lv")) != -1) {
-		switch (c) {
-		case 'l':
-			cfg_long_running = true;
-			break;
-		case 'v':
-			cfg_verbose = true;
-			break;
-		default:
-			error(1, 0, "%s: parse error", argv[0]);
-		}
-	}
-}
+	set_flowlabel_consistency(false);
 
-int main(int argc, char **argv)
-{
-	int fd;
+	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");
 
-	parse_opts(argc, argv);
+	tcp_connect(listener, 7, &cfd, &afd);
 
-	fd = socket(PF_INET6, SOCK_DGRAM, 0);
-	if (fd == -1)
-		error(1, errno, "socket");
+	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");
 
-	run_tests(fd);
+	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");
 
-	if (close(fd))
-		error(1, errno, "close");
+	err = setsockopt(afd, SOL_IPV6, IPV6_FLOWLABEL_MGR, &reflect_off, sizeof(reflect_off));
+	ASSERT_TRUE(err) TH_LOG("expected disabling REFLECT twice to fail");
 
-	return 0;
+	set_flowlabel_consistency(true);
+
+	ASSERT_EQ(0, close(afd));
+	ASSERT_EQ(0, close(cfd));
+	ASSERT_EQ(0, close(listener));
 }
+
+TEST_HARNESS_MAIN
-- 
2.55.0

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

[Jakub Kicinski]

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 [1]: [PATCH net-next 1/4] selftests: net: test IPV6_FL_A_RENEW ---
commit e6f2968aee0cf7d9f88007bffba4ffec657d1cd4
Author: Marcelo Mendes Spessoto Junior <marcelomspessoto@gmail.com>

selftests: net: test IPV6_FL_A_RENEW

This adds a flowlabel_renew() helper wrapping IPV6_FLOWLABEL_MGR with
IPV6_FL_A_RENEW, and a new sub-test in run_tests() that creates an
exclusive label, renews it with a longer linger, releases it, sleeps past
the default linger, and expects re-creation to fail.

> diff --git a/tools/testing/selftests/net/ipv6_flowlabel_mgr.c b/tools/testing/selftests/net/ipv6_flowlabel_mgr.c
> index af95b48acea90..cfa7e6270994a 100644
> --- a/tools/testing/selftests/net/ipv6_flowlabel_mgr.c
> +++ b/tools/testing/selftests/net/ipv6_flowlabel_mgr.c

[ ... ]

> @@ -160,6 +171,18 @@ static void run_tests(int fd)
>  		error(1, errno, "wait");
>  	if (!WIFEXITED(wstatus) || WEXITSTATUS(wstatus) != 0)
>  		error(1, errno, "wait: unexpected child result");
> +
> +	if (cfg_long_running) {

[Severity: Medium]
Does the new test ever run in an automated pass?  The whole block,
including the only call site of the new flowlabel_renew() helper, sits
inside if (cfg_long_running), and cfg_long_running is only set by the '-l'
option in parse_opts():

	case 'l':
		cfg_long_running = true;
		break;

The only in-tree invocation of this binary is in
tools/testing/selftests/net/ipv6_flowlabel.sh, which passes no arguments:

	echo "TEST management"
	./in_netns.sh ./ipv6_flowlabel_mgr

So under make run_tests the fl6_renew() path is not exercised, while the
changelog says "RENEW was the only flow label action without selftests
coverage".  Should the sub-test be unconditional, or should
ipv6_flowlabel.sh pass '-l'?

A sleep-free variant seems possible: renew of a label held by the socket
returns 0, renew of an unknown label returns -ESRCH, and renew with
linger > FL_MAX_LINGER without CAP_NET_ADMIN returns -EPERM.

> +		explain("create a new label with FL_MIN_LINGER linger time");
> +		expect_pass(flowlabel_get(fd, 5, IPV6_FL_S_EXCL, IPV6_FL_F_CREATE));
> +		explain("renew the label to increase its linger time and put it");
> +		expect_pass(flowlabel_renew(fd, 5, 2 * (FL_MIN_LINGER * 2 + 1)));
> +		expect_pass(flowlabel_put(fd, 5));
> +		sleep(FL_MIN_LINGER * 2 + 1);
> +		explain("The label cannot be created because the new linger time is not over yet");
> +		expect_fail(flowlabel_get(fd, 5, IPV6_FL_S_ANY, IPV6_FL_F_CREATE));

[Severity: Low]
This isn't a bug, but would it be worth bounding the renewed linger from
both sides?  The sequence only checks the negative case, and the label was
created with IPV6_FL_S_EXCL, so in net/ipv6/ip6_flowlabel.c the create at
t=13s fails as long as the entry is still hashed:

net/ipv6/ip6_flowlabel.c:ipv6_flowlabel_get() {
	...
	if (fl1->share == IPV6_FL_S_EXCL ||
	    fl1->share != fl->share ||
	...
		goto release;
}

Since expect_fail() cannot inspect errno, an "entry still present"
failure is indistinguishable from a share/permission failure, and no upper
bound on the renewed lifetime is tested.  A kernel change that made a
renewed label linger forever, or that clamped linger/expires too high in
fl6_renew(), would still pass.  Adding a second sleep past the renewed 26s
followed by expect_pass(flowlabel_get(fd, 5, ...)) would pin both ends.

The RENEW error paths are also untested: -ESRCH for an unknown label,
-EPERM for linger above FL_MAX_LINGER without CAP_NET_ADMIN, and the
IPV6_FL_S_NONE plus CAP_NET_ADMIN lookup branch in
ipv6_flowlabel_renew().

> +	}
> +
>  }

[Severity: Low]
There is a stray blank line between the closing brace of the new
if (cfg_long_running) block and the closing brace of run_tests().
scripts/checkpatch.pl reports:

	CHECK: Blank lines aren't necessary before a close brace '}'

>  
>  static void parse_opts(int argc, char **argv)

[Severity: Low]
The From: and Signed-off-by: identities use different addresses:

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

checkpatch reports WARNING: FROM_SIGN_OFF_MISMATCH for this.  Could the
two be made to match on resend?
-- 
pw-bot: cr

Re: [PATCH net-next 2/4] selftests: net: test IPV6_FL_F_REMOTE

[Jakub Kicinski]

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 [2]: [PATCH net-next 2/4] selftests: net: test IPV6_FL_F_REMOTE ---
commit bdc4decc572fa37fb4ef0e74f408cc6167e8342d
Author: Marcelo Mendes Spessoto Junior <marcelomspessoto@gmail.com>

selftests: net: test IPV6_FL_F_REMOTE

This adds coverage for the IPV6_FL_F_REMOTE flag of IPV6_FLOWLABEL_MGR by
setting up a loopback TCP connection with a flow label on the SYN and then
querying the accepted socket with getsockopt().  The connection setup is
factored into two new helpers, tcp_listen() and tcp_connect().

A few questions below.

[Severity: Low]
The commit message says:

  "This flag retrieves the latest label received by the socket with
   a getsockopt query."

Does the new test actually cover the "latest label" part?

ipv6_flowlabel_opt_get() in net/ipv6/ip6_flowlabel.c returns
np->rcv_flowinfo masked with IPV6_FLOWLABEL_MASK.  For TCP, rcv_flowinfo is
set unconditionally for the child socket from the SYN in
tcp_v6_syn_recv_sock():

  net/ipv6/tcp_ipv6.c:tcp_v6_syn_recv_sock() {
      ...
      newnp->rcv_flowinfo = ip6_flowinfo(ipv6_hdr(skb));
      ...
  }

and it is only refreshed later, in tcp_v6_do_rcv(), when rxflow or rxtclass
are enabled on the socket:

  net/ipv6/tcp_ipv6.c:tcp_v6_do_rcv() {
      ...
      if (np->rxopt.bits.rxflow || np->rxopt.bits.rxtclass)
              np->rcv_flowinfo = ip6_flowinfo(ipv6_hdr(opt_skb));
      ...
  }

The new test queries the accepted socket right after accept(), never sets
IPV6_FLOWINFO or IPV6_TCLASS on it, and never exchanges data afterwards, so
only the SYN-time value is observed and the refresh path above would remain
untested.  Would it make sense to either extend the test (enable
IPV6_FLOWINFO on the accepted socket and send data carrying a second label)
or reword the commit message to say the label seen at connection setup?

> diff --git a/tools/testing/selftests/net/ipv6_flowlabel_mgr.c b/tools/testing/selftests/net/ipv6_flowlabel_mgr.c
> index cfa7e6270994a..383a84ea9385c 100644
> --- a/tools/testing/selftests/net/ipv6_flowlabel_mgr.c
> +++ b/tools/testing/selftests/net/ipv6_flowlabel_mgr.c

[ ... ]

> @@ -82,6 +92,52 @@ static int flowlabel_renew(int fd, uint32_t label, uint16_t linger)

[ ... ]

> +static void tcp_connect(int listener, uint32_t flowlabel, int *client, int *accepted)
> +{
> +	INIT_SIN6_LOOPBACK(addr);
> +	const int one = 1;
> +	int cfd, afd;
> +
> +	cfd = socket(PF_INET6, SOCK_STREAM, 0);
> +	if (cfd == -1)
> +		error(1, errno, "socket client");
> +
> +	if (flowlabel_get(cfd, flowlabel, IPV6_FL_S_EXCL, IPV6_FL_F_CREATE))
> +		error(1, errno, "flowlabel_get");

[Severity: Low]
Should this get be paired with the existing flowlabel_put() helper?

Every other test in this file pairs get and put explicitly, but tcp_connect()
only releases the label implicitly through close(cfd).  That path goes
fl6_free_socklist() -> fl_release(), and fl_release() applies the linger
timeout rather than freeing the label right away:

  net/ipv6/ip6_flowlabel.c:fl_release() {
      ...
      fl->lastuse = jiffies;
      if (atomic_dec_and_test(&fl->users)) {
              unsigned long ttd = fl->lastuse + fl->linger;
      ...
  }

So the label value stays reserved for roughly FL_MIN_LINGER seconds after the
helper's socket is closed.  The commit message says the helpers exist so other
features such as IPV6_FL_F_REFLECT can reuse them; wouldn't a second
tcp_connect(listener, 6, ...) in the same run then fail in flowlabel_get()
with IPV6_FL_S_EXCL|IPV6_FL_F_CREATE and abort the whole binary via
error(1, ...)?  Adding flowlabel_put(cfd, flowlabel) before closing the client
socket, or making the share mode a parameter, would match the style used
elsewhere in this file.

> +	if (setsockopt(cfd, SOL_IPV6, IPV6_FLOWINFO_SEND, &one, sizeof(one)))
> +		error(1, errno, "setsockopt flowinfo_send");
> +	addr.sin6_flowinfo = htonl(flowlabel);
> +
> +	if (connect(cfd, (void *)&addr, sizeof(addr)))
> +		error(1, errno, "connect");
> +
> +	afd = accept(listener, NULL, NULL);
> +	if (afd == -1)
> +		error(1, errno, "accept");
> +
> +	*client = cfd;
> +	*accepted = afd;
> +}
> +
>  static void run_tests(int fd)
>  {
>  	int wstatus;
> @@ -183,6 +239,21 @@ static void run_tests(int fd)
>  		expect_fail(flowlabel_get(fd, 5, IPV6_FL_S_ANY, IPV6_FL_F_CREATE));
>  	}
>  
> +	explain("Prepare TCP SYN for REMOTE flag validation");
> +	int remote_listener = tcp_listen();
> +	int remote_cfd, remote_afd;

[Severity: Low]
This isn't a bug, but these declarations come after executable statements,
while run_tests() already declares wstatus and pid at the top of the function
and the rest of the file uses top-of-block declarations.  selftests/net
CFLAGS do not turn on -Wdeclaration-after-statement so it builds, but hoisting
remote_listener, remote_cfd, remote_afd, freq and freq_len to the top of
run_tests() (or putting the new test in its own block) would keep the style
consistent with no change in behaviour.

> +	tcp_connect(remote_listener, 6, &remote_cfd, &remote_afd);
> +	struct in6_flowlabel_req freq = {
> +		.flr_action = IPV6_FL_A_GET,
> +		.flr_flags = IPV6_FL_F_REMOTE,
> +	};
> +	socklen_t freq_len = sizeof(freq);
> +	explain("Query for label sent by client with IPV6_FL_F_REMOTE");
> +	expect_pass(getsockopt(remote_afd, SOL_IPV6, IPV6_FLOWLABEL_MGR, &freq, &freq_len));
> +	expect_pass(ntohl(freq.flr_label) != 6);
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: Low]
Is this assertion written the way it was intended?

expect_pass() succeeds when its argument evaluates to zero:

  #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))

So expect_pass(ntohl(freq.flr_label) != 6) passes exactly when the label is
6, which is the intended check, but it reads as the opposite and the log line
printed on success is "[OK]   ntohl(freq.flr_label) != 6".  Every other use of
expect_pass()/expect_fail() in this file passes a setsockopt()/getsockopt()
return code rather than a value comparison.

Would a plain check be clearer here, for example:

  if (ntohl(freq.flr_label) != REMOTE_LABEL)
	error(1, 0, "unexpected remote flowlabel %u", ntohl(freq.flr_label));

Also, the expected label appears twice as a bare literal, in
tcp_connect(remote_listener, 6, ...) and in the comparison against 6.  Could a
named constant be used at both sites so they cannot drift apart?

> +	close(remote_afd);
> +	close(remote_cfd);
> +	close(remote_listener);
>  }

Re: [PATCH net-next 3/4] selftests: net: test IPV6_FL_F_REFLECT

[Jakub Kicinski]

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 [3]: [PATCH net-next 3/4] selftests: net: test IPV6_FL_F_REFLECT ---
commit 9b79debc5b7eacef4cf0bdc8e171841fff374daf
Author: Marcelo Mendes Spessoto Junior <marcelomspessoto@gmail.com>

selftests: net: test IPV6_FL_F_REFLECT

This adds a REFLECT sub-test to tools/testing/selftests/net/ipv6_flowlabel_mgr.c,
reusing the existing tcp_listen()/tcp_connect() helpers to set up a connection.
A new set_flowlabel_consistency() helper toggles
/proc/sys/net/ipv6/flowlabel_consistency around the new test block, since the
kernel refuses IPV6_FL_F_REFLECT while that sysctl is enabled.

A few questions below.

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

[Severity: Low]
The author address in the commit header (marcelomspessoto@gmail.com) does not
match the Signed-off-by address (marcelospe@proton.me), which checkpatch
reports as "email address ... from: does not match Signed-off-by". Since the
Developer's Certificate of Origin is tied to the author identity, could the
From:/Signed-off-by be made consistent (or both addresses listed) so the patch
can be applied?

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

[ ... ]

> @@ -138,6 +139,19 @@ static void tcp_connect(int listener, uint32_t flowlabel, int *client, int *acce
>  	*accepted = afd;
>  }
>  
> +static void set_flowlabel_consistency(bool enable)
> +{
> +	int fd;
> +
> +	fd = open("/proc/sys/net/ipv6/flowlabel_consistency", O_WRONLY);
> +	if (fd == -1)
> +		error(1, errno, "open flowlabel_consistency");

[Severity: Low]
Should an unavailable or unwritable sysctl be a skip rather than a hard
failure? Any open()/write()/close() error here calls error(1, ...), which
exits the whole ipv6_flowlabel_mgr binary with status 1, so kselftest reports
FAIL rather than SKIP (exit code 4).

Before this patch the binary only issued setsockopt()/getsockopt() and needed
no privileges, and it already degrades gracefully elsewhere:

		if (setuid(USHRT_MAX)) {
			fprintf(stderr, "[INFO] skip setuid child test\n");
			exit(0);
		}

With this change, environments without a writable /proc/sys hierarchy
(CONFIG_SYSCTL=n, /proc not mounted or read-only, restricted container, no
CAP_NET_ADMIN over the netns) turn all the pre-existing, unrelated flowlabel
management assertions into a FAIL. Could only the REFLECT sub-test be skipped
in that case?

> +	if (write(fd, enable ? "1" : "0", 1) != 1)
> +		error(1, errno, "write flowlabel_consistency");
> +	if (close(fd))
> +		error(1, errno, "close flowlabel_consistency");
> +}

[Severity: Medium]
Does this leave net.ipv6.flowlabel_consistency permanently at 0 in the
namespace the test ran in?

Two things stand out. First, the helper never reads the current value, so
set_flowlabel_consistency(true) is a hardcoded write of "1" rather than a
restore, and an administrator's deliberate 0 is silently overwritten even on
success.

Second, everything between the disable and the "restore" in run_tests() aborts
the process on failure, so the restore is skipped entirely:

#define __expect(x)							\
	do {								\
		if (!(x))						\
			fprintf(stderr, "[OK]   " #x "\n");		\
		else							\
			error(1, 0, "[ERR]  " #x " (line %d)", __LINE__); \
	} while (0)

and tcp_listen()/tcp_connect() likewise call error(1, errno, ...) on
socket/bind/listen/connect/accept failure. There is no atexit() or signal
handler, so any failing assertion (or a signal) exits with the sysctl still 0.

That direction is fail-open, since ipv6_flowlabel_get() only rejects the flag
while the sysctl is set:

net/ipv6/ip6_flowlabel.c:ipv6_flowlabel_get() {
	if (freq->flr_flags & IPV6_FL_F_REFLECT) {
		if (READ_ONCE(net->ipv6.sysctl.flowlabel_consistency)) {
			net_info_ratelimited("Can not set IPV6_FL_F_REFLECT if flowlabel_consistency sysctl is enable\n");
			return -EPERM;
		}
}

Under ipv6_flowlabel.sh the throwaway netns from in_netns.sh absorbs the leak,
but ipv6_flowlabel_mgr is installed as a standalone TEST_GEN_FILES binary with
its own option parsing and is commonly run directly, in which case the writes
land in the caller's (typically init_net) namespace and persist after the test.

Would it fit the existing convention better to set the prerequisite from the
wrapper script inside the namespace, the way the other sysctls in this suite
are handled?

  ./in_netns.sh \
    sh -c 'sysctl -q -w net.ipv6.auto_flowlabels=0 && ./ipv6_flowlabel -l 1'

>  static void run_tests(int fd)
>  {
>  	int wstatus;

[ ... ]

> @@ -254,6 +268,37 @@ static void run_tests(int fd)
>  	close(remote_afd);
>  	close(remote_cfd);
>  	close(remote_listener);
> +
> +	explain("Prepare TCP SYN for REFLECT flag validation");
> +	set_flowlabel_consistency(false);
> +	int reflect_listener = tcp_listen();
> +	struct in6_flowlabel_req reflect_on = {
> +		.flr_action = IPV6_FL_A_GET,
> +		.flr_flags = IPV6_FL_F_REFLECT,
> +	};
> +	explain("Enable REFLECT on the listener before the client connects");
> +	expect_pass(setsockopt(reflect_listener, SOL_IPV6, IPV6_FLOWLABEL_MGR, &reflect_on, sizeof(reflect_on)));

[Severity: Low]
This isn't a bug, but three of the new assertion lines exceed the 100 column
limit checkpatch enforces (counting the leading tab as 8 columns): the
setsockopt() line above is around 113 columns, the getsockopt() line below is
around 111, and the two lines using &reflect_off are around 110. The
pre-existing REMOTE block stays under the limit. Wrapping the arguments or
shortening the variable names would address it.

> +	int reflect_cfd, reflect_afd;
> +	tcp_connect(reflect_listener, 7, &reflect_cfd, &reflect_afd);
> +	struct in6_flowlabel_req reflect_query = {
> +		.flr_action = IPV6_FL_A_GET,
> +	};
> +	socklen_t reflect_query_len = sizeof(reflect_query);
> +	explain("Query the accepted socket's outgoing label, should be reflected");
> +	expect_pass(getsockopt(reflect_afd, SOL_IPV6, IPV6_FLOWLABEL_MGR, &reflect_query, &reflect_query_len));
> +	expect_pass(ntohl(reflect_query.flr_label) != 7);

[Severity: Low]
Does this line assert the opposite of what it reads as? expect_pass() is
__expect(), which succeeds when the expression is false:

#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)

So this passes only when ntohl(reflect_query.flr_label) == 7, which is the
intended check, but the failure diagnostic prints
"[ERR]  ntohl(reflect_query.flr_label) != 7", i.e. the inverse of the real
expectation. Someone later "fixing" the condition to == 7 would silently
invert the test so it passes exactly when reflection is broken.

The macro was written for syscall return codes where 0 means pass; would an
explicit comparison spelled out as a return-code style check, or a separate
value-comparison macro, be clearer here? The same idiom was introduced by the
earlier REMOTE patch in this series
(expect_pass(ntohl(freq.flr_label) != 6)), so both could be adjusted together.

> +	struct in6_flowlabel_req reflect_off = {
> +		.flr_action = IPV6_FL_A_PUT,
> +		.flr_flags = IPV6_FL_F_REFLECT,
> +	};
> +	explain("PUT+REFLECT disables reflection on the accepted socket");
> +	expect_pass(setsockopt(reflect_afd, SOL_IPV6, IPV6_FLOWLABEL_MGR, &reflect_off, sizeof(reflect_off)));
> +	explain("cannot disable reflection twice");
> +	expect_fail(setsockopt(reflect_afd, SOL_IPV6, IPV6_FLOWLABEL_MGR, &reflect_off, sizeof(reflect_off)));
> +	set_flowlabel_consistency(true);
> +	close(reflect_afd);
> +	close(reflect_cfd);
> +	close(reflect_listener);
>  }

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

[Jakub Kicinski]

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.

Re: [PATCH net-next 0/4] net: selftests: adjustments to ipv6_flowlabel_mgr

[Jakub Kicinski]

On Mon, 27 Jul 2026 01:25:06 -0300 Marcelo Mendes Spessoto Junior wrote:
> The ipv6_flowlabel_mgr test file was lacking coverage for the IPV6_FL_A_RENEW
> action, and the IPV6_FL_F_REMOTE and IPV6_FL_F_REFLECT flags. The first
> three patches from this set aim to add a proper test case for each.
> 
> The fourth patch proposes the adoption of "kselftest_harness.h" helpers,
> improving code readability and conforming to the implementation of the most
> recent selftests.

I sent out an AI generated review, please assume that around half of 
the comments are valid and worth addressing...

Re: [PATCH net-next 2/4] selftests: net: test IPV6_FL_F_REMOTE

[Jakub Kicinski]

On Mon, 27 Jul 2026 01:25:08 -0300 Marcelo Mendes Spessoto Junior wrote:
> +#define INIT_SIN6_LOOPBACK(name)					\
> +	struct sockaddr_in6 name = {					\
> +		.sin6_family	= AF_INET6,				\
> +		.sin6_addr	= IN6ADDR_LOOPBACK_INIT,		\
> +		.sin6_port	= htons(8888),				\
> +	}

Please avoid magic macros

> @@ -183,6 +239,21 @@ static void run_tests(int fd)
>  		expect_fail(flowlabel_get(fd, 5, IPV6_FL_S_ANY, IPV6_FL_F_CREATE));
>  	}
>  
> +	explain("Prepare TCP SYN for REMOTE flag validation");
> +	int remote_listener = tcp_listen();
> +	int remote_cfd, remote_afd;

and don't declare variables half way thru the function
C coding style applies