[PATCH net-next v01 0/4] net: af_packet: allow joining a fanout when link is down

[PATCH net-next v01 0/4] net: af_packet: allow joining a fanout when link is down

[Gur Stavi]

PACKET socket can retain its fanout membership through link down and up
and (obviously) leave a fanout while it is not RUNNING (link down).
However, socket was forbidden from joining a fanout while it was not
RUNNING.

This patch allows PACKET socket to join a fanout while not RUNNING.
Selftest psock_fanout is extended to test this scenario.
This is the only test that was performed.

This scenario was identified while studying DPDK pmd_af_packet_drv.
Since sockets are only created during initialization, there is no reason
to fail the initialization if a single link is temporarily down.

I hope it is not considered as breaking user space and that applications
are not designed to expect this failure.

Gur Stavi (4):
  af_packet: allow fanout_add when socket is not RUNNING
  selftests: net/psock_fanout: add loopback up/down toggle facility
  selftests: net/psock_fanout: restore loopback up/down state on exit
  selftests: net/psock_fanout: socket joins fanout when link is down

 net/packet/af_packet.c                     |  10 +-
 tools/testing/selftests/net/psock_fanout.c | 124 ++++++++++++++-------
 2 files changed, 90 insertions(+), 44 deletions(-)


base-commit: c824deb1a89755f70156b5cdaf569fca80698719
-- 
2.45.2

[PATCH net-next v01 1/4] af_packet: allow fanout_add when socket is not RUNNING

[Gur Stavi]

PACKET socket can retain its fanout membership through link down and up
and (obviously) leave a fanout while it is not RUNNING (link down).
However, socket was forbidden from joining a fanout while it was not
RUNNING.

This patch allows PACKET socket to join fanout while not RUNNING.

Signed-off-by: Gur Stavi <gur.stavi@huawei.com>
---
 net/packet/af_packet.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index a705ec214254..c28eee7f6ce0 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -1846,21 +1846,21 @@ static int fanout_add(struct sock *sk, struct fanout_args *args)
 	err = -EINVAL;
 
 	spin_lock(&po->bind_lock);
-	if (packet_sock_flag(po, PACKET_SOCK_RUNNING) &&
-	    match->type == type &&
+	if (match->type == type &&
 	    match->prot_hook.type == po->prot_hook.type &&
 	    match->prot_hook.dev == po->prot_hook.dev) {
 		err = -ENOSPC;
 		if (refcount_read(&match->sk_ref) < match->max_num_members) {
-			__dev_remove_pack(&po->prot_hook);
-
 			/* Paired with packet_setsockopt(PACKET_FANOUT_DATA) */
 			WRITE_ONCE(po->fanout, match);
 
 			po->rollover = rollover;
 			rollover = NULL;
 			refcount_set(&match->sk_ref, refcount_read(&match->sk_ref) + 1);
-			__fanout_link(sk, po);
+			if (packet_sock_flag(po, PACKET_SOCK_RUNNING)) {
+				__dev_remove_pack(&po->prot_hook);
+				__fanout_link(sk, po);
+			}
 			err = 0;
 		}
 	}
-- 
2.45.2

[PATCH net-next v01 2/4] selftests: net/psock_fanout: add loopback up/down toggle facility

[Gur Stavi]

Prepare to test setsockopt with PACKET_FANOUT on a link that is down.

Signed-off-by: Gur Stavi <gur.stavi@huawei.com>
---
 tools/testing/selftests/net/psock_fanout.c | 38 ++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/tools/testing/selftests/net/psock_fanout.c b/tools/testing/selftests/net/psock_fanout.c
index 4f31e92ebd96..a8b22494a635 100644
--- a/tools/testing/selftests/net/psock_fanout.c
+++ b/tools/testing/selftests/net/psock_fanout.c
@@ -48,6 +48,7 @@
 #include <string.h>
 #include <sys/mman.h>
 #include <sys/socket.h>
+#include <sys/ioctl.h>
 #include <sys/stat.h>
 #include <sys/types.h>
 #include <unistd.h>
@@ -58,6 +59,43 @@
 #define RING_NUM_FRAMES			20
 
 static uint32_t cfg_max_num_members;
+static int lo_up_down_toggled;
+
+static void loopback_up_down_toggle(void)
+{
+	struct ifreq ifreq = {};
+	int fd, err;
+
+	fd = socket(AF_PACKET, SOCK_RAW, 0);
+	if (fd == -1)
+		return;
+	strcpy(ifreq.ifr_name, "lo");
+	err = ioctl(fd, SIOCGIFFLAGS, &ifreq);
+	if (err) {
+		perror("SIOCGIFFLAGS");
+		exit(1);
+	}
+	ifreq.ifr_flags ^= IFF_UP;
+	err = ioctl(fd, SIOCSIFFLAGS, &ifreq);
+	if (err) {
+		perror("SIOCSIFFLAGS");
+		exit(1);
+	}
+	lo_up_down_toggled ^= 1;
+	close(fd);
+}
+
+static void loopback_up_down_restore(void)
+{
+	if (lo_up_down_toggled)
+		loopback_up_down_toggle();
+}
+
+static int cleanup_and_exit(int status)
+{
+	loopback_up_down_restore();
+	exit(status);
+}
 
 /* Open a socket in a given fanout mode.
  * @return -1 if mode is bad, a valid socket otherwise */
-- 
2.45.2

[PATCH net-next v01 3/4] selftests: net/psock_fanout: restore loopback up/down state on exit

[Gur Stavi]

Minimize the risk that psock_fanout leaves loopback device in a different
state than the start state.

Restore loopback up/down state when test reaches end of main.
For abort on errors, globally replace all 'exit' with 'cleanup_and_exit'
that restores loopback up/down state.

Signed-off-by: Gur Stavi <gur.stavi@huawei.com>
---
 tools/testing/selftests/net/psock_fanout.c | 73 +++++++++++-----------
 1 file changed, 37 insertions(+), 36 deletions(-)

diff --git a/tools/testing/selftests/net/psock_fanout.c b/tools/testing/selftests/net/psock_fanout.c
index a8b22494a635..64edcd661d43 100644
--- a/tools/testing/selftests/net/psock_fanout.c
+++ b/tools/testing/selftests/net/psock_fanout.c
@@ -108,7 +108,7 @@ static int sock_fanout_open(uint16_t typeflags, uint16_t group_id)
 	fd = socket(PF_PACKET, SOCK_RAW, 0);
 	if (fd < 0) {
 		perror("socket packet");
-		exit(1);
+		cleanup_and_exit(1);
 	}
 
 	pair_udp_setfilter(fd);
@@ -118,11 +118,11 @@ static int sock_fanout_open(uint16_t typeflags, uint16_t group_id)
 	addr.sll_ifindex = if_nametoindex("lo");
 	if (addr.sll_ifindex == 0) {
 		perror("if_nametoindex");
-		exit(1);
+		cleanup_and_exit(1);
 	}
 	if (bind(fd, (void *) &addr, sizeof(addr))) {
 		perror("bind packet");
-		exit(1);
+		cleanup_and_exit(1);
 	}
 
 	if (cfg_max_num_members) {
@@ -139,7 +139,7 @@ static int sock_fanout_open(uint16_t typeflags, uint16_t group_id)
 	if (err) {
 		if (close(fd)) {
 			perror("close packet");
-			exit(1);
+			cleanup_and_exit(1);
 		}
 		return -1;
 	}
@@ -161,7 +161,7 @@ static void sock_fanout_set_cbpf(int fd)
 	if (setsockopt(fd, SOL_PACKET, PACKET_FANOUT_DATA, &bpf_prog,
 		       sizeof(bpf_prog))) {
 		perror("fanout data cbpf");
-		exit(1);
+		cleanup_and_exit(1);
 	}
 }
 
@@ -173,7 +173,7 @@ static void sock_fanout_getopts(int fd, uint16_t *typeflags, uint16_t *group_id)
 	if (getsockopt(fd, SOL_PACKET, PACKET_FANOUT,
 		       &sockopt, &sockopt_len)) {
 		perror("failed to getsockopt");
-		exit(1);
+		cleanup_and_exit(1);
 	}
 	*typeflags = sockopt >> 16;
 	*group_id = sockopt & 0xfffff;
@@ -211,17 +211,17 @@ static void sock_fanout_set_ebpf(int fd)
 	if (pfd < 0) {
 		perror("bpf");
 		fprintf(stderr, "bpf verifier:\n%s\n", log_buf);
-		exit(1);
+		cleanup_and_exit(1);
 	}
 
 	if (setsockopt(fd, SOL_PACKET, PACKET_FANOUT_DATA, &pfd, sizeof(pfd))) {
 		perror("fanout data ebpf");
-		exit(1);
+		cleanup_and_exit(1);
 	}
 
 	if (close(pfd)) {
 		perror("close ebpf");
-		exit(1);
+		cleanup_and_exit(1);
 	}
 }
 
@@ -239,19 +239,19 @@ static char *sock_fanout_open_ring(int fd)
 	if (setsockopt(fd, SOL_PACKET, PACKET_VERSION, (void *) &val,
 		       sizeof(val))) {
 		perror("packetsock ring setsockopt version");
-		exit(1);
+		cleanup_and_exit(1);
 	}
 	if (setsockopt(fd, SOL_PACKET, PACKET_RX_RING, (void *) &req,
 		       sizeof(req))) {
 		perror("packetsock ring setsockopt");
-		exit(1);
+		cleanup_and_exit(1);
 	}
 
 	ring = mmap(0, req.tp_block_size * req.tp_block_nr,
 		    PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
 	if (ring == MAP_FAILED) {
 		perror("packetsock ring mmap");
-		exit(1);
+		cleanup_and_exit(1);
 	}
 
 	return ring;
@@ -297,7 +297,7 @@ static void test_control_single(void)
 	if (sock_fanout_open(PACKET_FANOUT_ROLLOVER |
 			       PACKET_FANOUT_FLAG_ROLLOVER, 0) != -1) {
 		fprintf(stderr, "ERROR: opened socket with dual rollover\n");
-		exit(1);
+		cleanup_and_exit(1);
 	}
 }
 
@@ -311,30 +311,30 @@ static void test_control_group(void)
 	fds[0] = sock_fanout_open(PACKET_FANOUT_HASH, 0);
 	if (fds[0] == -1) {
 		fprintf(stderr, "ERROR: failed to open HASH socket\n");
-		exit(1);
+		cleanup_and_exit(1);
 	}
 	if (sock_fanout_open(PACKET_FANOUT_HASH |
 			       PACKET_FANOUT_FLAG_DEFRAG, 0) != -1) {
 		fprintf(stderr, "ERROR: joined group with wrong flag defrag\n");
-		exit(1);
+		cleanup_and_exit(1);
 	}
 	if (sock_fanout_open(PACKET_FANOUT_HASH |
 			       PACKET_FANOUT_FLAG_ROLLOVER, 0) != -1) {
 		fprintf(stderr, "ERROR: joined group with wrong flag ro\n");
-		exit(1);
+		cleanup_and_exit(1);
 	}
 	if (sock_fanout_open(PACKET_FANOUT_CPU, 0) != -1) {
 		fprintf(stderr, "ERROR: joined group with wrong mode\n");
-		exit(1);
+		cleanup_and_exit(1);
 	}
 	fds[1] = sock_fanout_open(PACKET_FANOUT_HASH, 0);
 	if (fds[1] == -1) {
 		fprintf(stderr, "ERROR: failed to join group\n");
-		exit(1);
+		cleanup_and_exit(1);
 	}
 	if (close(fds[1]) || close(fds[0])) {
 		fprintf(stderr, "ERROR: closing sockets\n");
-		exit(1);
+		cleanup_and_exit(1);
 	}
 }
 
@@ -349,21 +349,21 @@ static void test_control_group_max_num_members(void)
 	cfg_max_num_members = (1 << 16) + 1;
 	if (sock_fanout_open(PACKET_FANOUT_HASH, 0) != -1) {
 		fprintf(stderr, "ERROR: max_num_members > PACKET_FANOUT_MAX\n");
-		exit(1);
+		cleanup_and_exit(1);
 	}
 
 	cfg_max_num_members = 256;
 	fds[0] = sock_fanout_open(PACKET_FANOUT_HASH, 0);
 	if (fds[0] == -1) {
 		fprintf(stderr, "ERROR: failed open\n");
-		exit(1);
+		cleanup_and_exit(1);
 	}
 
 	/* expected failure on joining group with different max_num_members */
 	cfg_max_num_members = 257;
 	if (sock_fanout_open(PACKET_FANOUT_HASH, 0) != -1) {
 		fprintf(stderr, "ERROR: set different max_num_members\n");
-		exit(1);
+		cleanup_and_exit(1);
 	}
 
 	/* success on joining group with same max_num_members */
@@ -371,7 +371,7 @@ static void test_control_group_max_num_members(void)
 	fds[1] = sock_fanout_open(PACKET_FANOUT_HASH, 0);
 	if (fds[1] == -1) {
 		fprintf(stderr, "ERROR: failed to join group\n");
-		exit(1);
+		cleanup_and_exit(1);
 	}
 
 	/* success on joining group with max_num_members unspecified */
@@ -379,12 +379,12 @@ static void test_control_group_max_num_members(void)
 	fds[2] = sock_fanout_open(PACKET_FANOUT_HASH, 0);
 	if (fds[2] == -1) {
 		fprintf(stderr, "ERROR: failed to join group\n");
-		exit(1);
+		cleanup_and_exit(1);
 	}
 
 	if (close(fds[2]) || close(fds[1]) || close(fds[0])) {
 		fprintf(stderr, "ERROR: closing sockets\n");
-		exit(1);
+		cleanup_and_exit(1);
 	}
 }
 
@@ -400,25 +400,25 @@ static void test_unique_fanout_group_ids(void)
 				  PACKET_FANOUT_FLAG_UNIQUEID, 0);
 	if (fds[0] == -1) {
 		fprintf(stderr, "ERROR: failed to create a unique id group.\n");
-		exit(1);
+		cleanup_and_exit(1);
 	}
 
 	sock_fanout_getopts(fds[0], &typeflags, &first_group_id);
 	if (typeflags != PACKET_FANOUT_HASH) {
 		fprintf(stderr, "ERROR: unexpected typeflags %x\n", typeflags);
-		exit(1);
+		cleanup_and_exit(1);
 	}
 
 	if (sock_fanout_open(PACKET_FANOUT_CPU, first_group_id) != -1) {
 		fprintf(stderr, "ERROR: joined group with wrong type.\n");
-		exit(1);
+		cleanup_and_exit(1);
 	}
 
 	fds[1] = sock_fanout_open(PACKET_FANOUT_HASH, first_group_id);
 	if (fds[1] == -1) {
 		fprintf(stderr,
 			"ERROR: failed to join previously created group.\n");
-		exit(1);
+		cleanup_and_exit(1);
 	}
 
 	fds[2] = sock_fanout_open(PACKET_FANOUT_HASH |
@@ -426,7 +426,7 @@ static void test_unique_fanout_group_ids(void)
 	if (fds[2] == -1) {
 		fprintf(stderr,
 			"ERROR: failed to create a second unique id group.\n");
-		exit(1);
+		cleanup_and_exit(1);
 	}
 
 	sock_fanout_getopts(fds[2], &typeflags, &second_group_id);
@@ -434,12 +434,12 @@ static void test_unique_fanout_group_ids(void)
 			     second_group_id) != -1) {
 		fprintf(stderr,
 			"ERROR: specified a group id when requesting unique id\n");
-		exit(1);
+		cleanup_and_exit(1);
 	}
 
 	if (close(fds[0]) || close(fds[1]) || close(fds[2])) {
 		fprintf(stderr, "ERROR: closing sockets\n");
-		exit(1);
+		cleanup_and_exit(1);
 	}
 }
 
@@ -459,7 +459,7 @@ static int test_datapath(uint16_t typeflags, int port_off,
 	fds[1] = sock_fanout_open(typeflags, 0);
 	if (fds[0] == -1 || fds[1] == -1) {
 		fprintf(stderr, "ERROR: failed open\n");
-		exit(1);
+		cleanup_and_exit(1);
 	}
 	if (type == PACKET_FANOUT_CBPF)
 		sock_fanout_set_cbpf(fds[0]);
@@ -485,13 +485,13 @@ static int test_datapath(uint16_t typeflags, int port_off,
 	if (munmap(rings[1], RING_NUM_FRAMES * getpagesize()) ||
 	    munmap(rings[0], RING_NUM_FRAMES * getpagesize())) {
 		fprintf(stderr, "close rings\n");
-		exit(1);
+		cleanup_and_exit(1);
 	}
 	if (close(fds_udp[1][1]) || close(fds_udp[1][0]) ||
 	    close(fds_udp[0][1]) || close(fds_udp[0][0]) ||
 	    close(fds[1]) || close(fds[0])) {
 		fprintf(stderr, "close datapath\n");
-		exit(1);
+		cleanup_and_exit(1);
 	}
 
 	return ret;
@@ -506,7 +506,7 @@ static int set_cpuaffinity(int cpuid)
 	if (sched_setaffinity(0, sizeof(mask), &mask)) {
 		if (errno != EINVAL) {
 			fprintf(stderr, "setaffinity %d\n", cpuid);
-			exit(1);
+			cleanup_and_exit(1);
 		}
 		return 1;
 	}
@@ -569,6 +569,7 @@ int main(int argc, char **argv)
 	ret |= test_datapath(PACKET_FANOUT_FLAG_UNIQUEID, port_off,
 			     expect_uniqueid[0], expect_uniqueid[1]);
 
+	loopback_up_down_restore();
 	if (ret)
 		return 1;
 
-- 
2.45.2

[PATCH net-next v01 4/4] selftests: net/psock_fanout: socket joins fanout when link is down

[Gur Stavi]

Modify test_control_group to have toggle parameter.
When toggle is non-zero, loopback device will be set down for the
initialization of fd[1] which is still expected to successfully join
the fanout.

Signed-off-by: Gur Stavi <gur.stavi@huawei.com>
---
 tools/testing/selftests/net/psock_fanout.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/net/psock_fanout.c b/tools/testing/selftests/net/psock_fanout.c
index 64edcd661d43..49023d9b85b4 100644
--- a/tools/testing/selftests/net/psock_fanout.c
+++ b/tools/testing/selftests/net/psock_fanout.c
@@ -302,17 +302,22 @@ static void test_control_single(void)
 }
 
 /* Test illegal group with different modes or flags */
-static void test_control_group(void)
+static void test_control_group(int toggle)
 {
 	int fds[2];
 
-	fprintf(stderr, "test: control multiple sockets\n");
+	if (toggle)
+		fprintf(stderr, "test: control multiple sockets with link down toggle\n");
+	else
+		fprintf(stderr, "test: control multiple sockets\n");
 
 	fds[0] = sock_fanout_open(PACKET_FANOUT_HASH, 0);
 	if (fds[0] == -1) {
 		fprintf(stderr, "ERROR: failed to open HASH socket\n");
 		cleanup_and_exit(1);
 	}
+	if (toggle)
+		loopback_up_down_toggle();
 	if (sock_fanout_open(PACKET_FANOUT_HASH |
 			       PACKET_FANOUT_FLAG_DEFRAG, 0) != -1) {
 		fprintf(stderr, "ERROR: joined group with wrong flag defrag\n");
@@ -332,6 +337,7 @@ static void test_control_group(void)
 		fprintf(stderr, "ERROR: failed to join group\n");
 		cleanup_and_exit(1);
 	}
+	loopback_up_down_restore();
 	if (close(fds[1]) || close(fds[0])) {
 		fprintf(stderr, "ERROR: closing sockets\n");
 		cleanup_and_exit(1);
@@ -527,7 +533,8 @@ int main(int argc, char **argv)
 	int port_off = 2, tries = 20, ret;
 
 	test_control_single();
-	test_control_group();
+	test_control_group(0);
+	test_control_group(1);
 	test_control_group_max_num_members();
 	test_unique_fanout_group_ids();
 
-- 
2.45.2

Re: [PATCH net-next v01 1/4] af_packet: allow fanout_add when socket is not RUNNING

[Willem de Bruijn]

Gur Stavi wrote:
> PACKET socket can retain its fanout membership through link down and up
> and (obviously) leave a fanout while it is not RUNNING (link down).

Probably just semantics, but a socket cannot leave a fanout group.
__fanout_unlink does remove it from the fanout group on link down
(__unregister_prot_hook). But a subsequent link up will always add it
back.

> However, socket was forbidden from joining a fanout while it was not
> RUNNING.

Your change looks safe to me.

I'm trying to understand where this check came from. Originally it was
this at the start of fanout_add:

+       if (!po->running)
+               return -EINVAL;

Perhaps as indicator of this requirement

    An AF_PACKET socket must be fully bound before it tries to add itself
    to a fanout.  All AF_PACKET sockets trying to join the same fanout
    must all have the same bind settings.

Probably because for the first socket in the fanout group, the group
inherits the relevant bound fields from the socket:

                match->prot_hook.type = po->prot_hook.type;
                match->prot_hook.dev = po->prot_hook.dev;

> This patch allows PACKET socket to join fanout while not RUNNING.
> 
> Signed-off-by: Gur Stavi <gur.stavi@huawei.com>
> ---
>  net/packet/af_packet.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index a705ec214254..c28eee7f6ce0 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -1846,21 +1846,21 @@ static int fanout_add(struct sock *sk, struct fanout_args *args)
>  	err = -EINVAL;
>  
>  	spin_lock(&po->bind_lock);
> -	if (packet_sock_flag(po, PACKET_SOCK_RUNNING) &&
> -	    match->type == type &&
> +	if (match->type == type &&
>  	    match->prot_hook.type == po->prot_hook.type &&
>  	    match->prot_hook.dev == po->prot_hook.dev) {
>  		err = -ENOSPC;
>  		if (refcount_read(&match->sk_ref) < match->max_num_members) {
> -			__dev_remove_pack(&po->prot_hook);
> -
>  			/* Paired with packet_setsockopt(PACKET_FANOUT_DATA) */
>  			WRITE_ONCE(po->fanout, match);
>  
>  			po->rollover = rollover;
>  			rollover = NULL;
>  			refcount_set(&match->sk_ref, refcount_read(&match->sk_ref) + 1);
> -			__fanout_link(sk, po);
> +			if (packet_sock_flag(po, PACKET_SOCK_RUNNING)) {
> +				__dev_remove_pack(&po->prot_hook);
> +				__fanout_link(sk, po);
> +			}
>  			err = 0;
>  		}
>  	}
> -- 
> 2.45.2
> 

Re: [PATCH net-next v01 3/4] selftests: net/psock_fanout: restore loopback up/down state on exit

[Willem de Bruijn]

Gur Stavi wrote:
> Minimize the risk that psock_fanout leaves loopback device in a different
> state than the start state.
> 
> Restore loopback up/down state when test reaches end of main.
> For abort on errors, globally replace all 'exit' with 'cleanup_and_exit'
> that restores loopback up/down state.

Luckily tools/testing/selftests/net/run_afpackettests already runs
this test in a network namespace, so nothing terrible will happen if
it leaves lo down on exit.

I'd like to avoid this many code changes. As long as the test exits
with failure, it's fine if this particular state is left.

And prefer separate set_loopback_up and .._down helpers, rather than
a toggle whose behavior depends on current state.

> Signed-off-by: Gur Stavi <gur.stavi@huawei.com>