[net-next 0/3] Extend packet socket selftests

[net-next 0/3] Extend packet socket selftests

[Joe Damato]

Greetings:

I was looking around the packet socket code and noticed there were a few
features that could be tested by extending the existing packet socket
tests.

I extended the test to test stats, drops, and auxdata and re-ran the tests.
The existing and new tests passed.

Thanks,
Joe

v2:
  - patch 2: remove argument from do_tx and use global instead
  - patch 3: add argument to do_rx to specify if the socket is a packet
    socket. factored out the aux data check into its own function for
    readability.

Joe Damato (3):
  selftests/net: Test PACKET_STATISTICS
  selftests/net: Test PACKET_STATISTICS drops
  selftests/net: Test PACKET_AUXDATA

 tools/testing/selftests/net/psock_snd.c  | 137 +++++++++++++++++++++--
 tools/testing/selftests/net/psock_snd.sh |  10 ++
 2 files changed, 139 insertions(+), 8 deletions(-)


base-commit: 8b0e64d6c9e7feec5ba5643b4fa8b7fd54464778
-- 
2.52.0

[net-next v2 1/3] selftests/net: Test PACKET_STATISTICS

[Joe Damato]

Update the existing packet socket test to include a test for the sockopt
PACKET_STATISTICS.

Signed-off-by: Joe Damato <joe@dama.to>
---
 tools/testing/selftests/net/psock_snd.c | 32 ++++++++++++++++++++++++-
 1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/net/psock_snd.c b/tools/testing/selftests/net/psock_snd.c
index edf1e6f80d41..5be481a3d2bd 100644
--- a/tools/testing/selftests/net/psock_snd.c
+++ b/tools/testing/selftests/net/psock_snd.c
@@ -359,6 +359,34 @@ static void parse_opts(int argc, char **argv)
 		error(1, 0, "option gso (-g) requires csum offload (-c)");
 }
 
+static void check_packet_stats(int fd)
+{
+	struct tpacket_stats st = {};
+	socklen_t len = sizeof(st);
+
+	if (getsockopt(fd, SOL_PACKET, PACKET_STATISTICS, &st, &len))
+		error(1, errno, "getsockopt packet statistics");
+
+	if (st.tp_packets != 1)
+		error(1, 0, "stats: tp_packets %u != 1", st.tp_packets);
+
+	if (st.tp_drops != 0)
+		error(1, 0, "stats: tp_drops %u != 0", st.tp_drops);
+
+	/* verify clear on read */
+	memset(&st, 0xff, sizeof(st));
+	len = sizeof(st);
+
+	if (getsockopt(fd, SOL_PACKET, PACKET_STATISTICS, &st, &len))
+		error(1, errno, "getsockopt packet statistics");
+
+	if (st.tp_packets != 0)
+		error(1, 0, "stats: tp_packets %u != 0 after clear", st.tp_packets);
+
+	if (st.tp_drops != 0)
+		error(1, 0, "stats: tp_drops %u != 0 after clear", st.tp_drops);
+}
+
 static void run_test(void)
 {
 	int fdr, fds, total_len;
@@ -369,9 +397,11 @@ static void run_test(void)
 	total_len = do_tx();
 
 	/* BPF filter accepts only this length, vlan changes MAC */
-	if (cfg_payload_len == DATA_LEN && !cfg_use_vlan)
+	if (cfg_payload_len == DATA_LEN && !cfg_use_vlan) {
 		do_rx(fds, total_len - sizeof(struct virtio_net_hdr),
 		      tbuf + sizeof(struct virtio_net_hdr));
+		check_packet_stats(fds);
+	}
 
 	do_rx(fdr, cfg_payload_len, tbuf + total_len - cfg_payload_len);
 
-- 
2.52.0

[net-next v2 2/3] selftests/net: Test PACKET_STATISTICS drops

[Joe Damato]

Extend psock_snd to test drops by setting a tiny receive buffer and
sending a large burst of packets.

Signed-off-by: Joe Damato <joe@dama.to>
---
 tools/testing/selftests/net/psock_snd.c  | 48 ++++++++++++++++++++----
 tools/testing/selftests/net/psock_snd.sh |  5 +++
 2 files changed, 46 insertions(+), 7 deletions(-)

v2:
  - Remove do_tx argument and use global cfg_drop instead

diff --git a/tools/testing/selftests/net/psock_snd.c b/tools/testing/selftests/net/psock_snd.c
index 5be481a3d2bd..81096df5cffc 100644
--- a/tools/testing/selftests/net/psock_snd.c
+++ b/tools/testing/selftests/net/psock_snd.c
@@ -39,6 +39,7 @@ static bool	cfg_use_gso;
 static bool	cfg_use_qdisc_bypass;
 static bool	cfg_use_vlan;
 static bool	cfg_use_vnet;
+static bool	cfg_drop;
 
 static char	*cfg_ifname = "lo";
 static int	cfg_mtu	= 1500;
@@ -49,6 +50,8 @@ static uint16_t	cfg_port = 8000;
 /* test sending up to max mtu + 1 */
 #define TEST_SZ	(sizeof(struct virtio_net_hdr) + ETH_HLEN + ETH_MAX_MTU + 1)
 
+#define BURST_CNT (1000)
+
 static char tbuf[TEST_SZ], rbuf[TEST_SZ];
 
 static unsigned long add_csum_hword(const uint16_t *start, int num_u16)
@@ -212,13 +215,14 @@ static void do_send(int fd, char *buf, int len)
 	if (ret != len)
 		error(1, 0, "write: %u %u", ret, len);
 
-	fprintf(stderr, "tx: %u\n", ret);
+	if (!cfg_drop)
+		fprintf(stderr, "tx: %u\n", ret);
 }
 
 static int do_tx(void)
 {
 	const int one = 1;
-	int fd, len;
+	int i, fd, len;
 
 	fd = socket(PF_PACKET, cfg_use_dgram ? SOCK_DGRAM : SOCK_RAW, 0);
 	if (fd == -1)
@@ -242,6 +246,10 @@ static int do_tx(void)
 
 	do_send(fd, tbuf, len);
 
+	if (cfg_drop)
+		for (i = 0; i < BURST_CNT; i++)
+			do_send(fd, tbuf, len);
+
 	if (close(fd))
 		error(1, errno, "close t");
 
@@ -290,6 +298,7 @@ static void do_rx(int fd, int expected_len, char *expected)
 static int setup_sniffer(void)
 {
 	struct timeval tv = { .tv_usec = 100 * 1000 };
+	const int one = 1;
 	int fd;
 
 	fd = socket(PF_PACKET, SOCK_RAW, 0);
@@ -299,6 +308,10 @@ static int setup_sniffer(void)
 	if (setsockopt(fd, SOL_SOCKET, SO_RCVTIMEO, &tv, sizeof(tv)))
 		error(1, errno, "setsockopt rcv timeout");
 
+	if (cfg_drop)
+		if (setsockopt(fd, SOL_SOCKET, SO_RCVBUF, &one, sizeof(one)))
+			error(1, errno, "setsockopt SO_RCVBUF");
+
 	pair_udp_setfilter(fd);
 	do_bind(fd);
 
@@ -309,7 +322,7 @@ static void parse_opts(int argc, char **argv)
 {
 	int c;
 
-	while ((c = getopt(argc, argv, "bcCdgl:qt:vV")) != -1) {
+	while ((c = getopt(argc, argv, "bcCdDgl:qt:vV")) != -1) {
 		switch (c) {
 		case 'b':
 			cfg_use_bind = true;
@@ -323,6 +336,9 @@ static void parse_opts(int argc, char **argv)
 		case 'd':
 			cfg_use_dgram = true;
 			break;
+		case 'D':
+			cfg_drop = true;
+			break;
 		case 'g':
 			cfg_use_gso = true;
 			break;
@@ -367,11 +383,23 @@ static void check_packet_stats(int fd)
 	if (getsockopt(fd, SOL_PACKET, PACKET_STATISTICS, &st, &len))
 		error(1, errno, "getsockopt packet statistics");
 
-	if (st.tp_packets != 1)
-		error(1, 0, "stats: tp_packets %u != 1", st.tp_packets);
+	if (cfg_drop) {
+		/* PACKET_STATISTICS reports all packets seen (including
+		 * drops) in tp_packets
+		 */
+		if (st.tp_packets < st.tp_drops)
+			error(1, 0, "stats: tp_packets %u < tp_drops %u",
+			      st.tp_packets, st.tp_drops);
 
-	if (st.tp_drops != 0)
-		error(1, 0, "stats: tp_drops %u != 0", st.tp_drops);
+		if (st.tp_drops == 0)
+			error(1, 0, "stats: expected drops but tp_drops == 0");
+	} else {
+		if (st.tp_packets != 1)
+			error(1, 0, "stats: tp_packets %u != 1", st.tp_packets);
+
+		if (st.tp_drops != 0)
+			error(1, 0, "stats: tp_drops %u != 0", st.tp_drops);
+	}
 
 	/* verify clear on read */
 	memset(&st, 0xff, sizeof(st));
@@ -396,6 +424,11 @@ static void run_test(void)
 
 	total_len = do_tx();
 
+	if (cfg_drop) {
+		check_packet_stats(fds);
+		goto out;
+	}
+
 	/* BPF filter accepts only this length, vlan changes MAC */
 	if (cfg_payload_len == DATA_LEN && !cfg_use_vlan) {
 		do_rx(fds, total_len - sizeof(struct virtio_net_hdr),
@@ -405,6 +438,7 @@ static void run_test(void)
 
 	do_rx(fdr, cfg_payload_len, tbuf + total_len - cfg_payload_len);
 
+out:
 	if (close(fds))
 		error(1, errno, "close s");
 	if (close(fdr))
diff --git a/tools/testing/selftests/net/psock_snd.sh b/tools/testing/selftests/net/psock_snd.sh
index 1cbfeb5052ec..b6ef12fad5d5 100755
--- a/tools/testing/selftests/net/psock_snd.sh
+++ b/tools/testing/selftests/net/psock_snd.sh
@@ -92,4 +92,9 @@ echo "raw gso max size"
 echo "raw gso max size + 1 (expected to fail)"
 (! ./in_netns.sh ./psock_snd -v -c -g -l "${max_mss_exceeds}")
 
+# test drops statistics
+
+echo "test drops statistics"
+./in_netns.sh ./psock_snd -D
+
 echo "OK. All tests passed"
-- 
2.52.0

[net-next v2 3/3] selftests/net: Test PACKET_AUXDATA

[Joe Damato]

Extend the packet socket selftest, adding a recvmsg path, to test
PACKET_AUXDATA. Check basic attributes of tpacket_auxdata.

Signed-off-by: Joe Damato <joe@dama.to>
---
 tools/testing/selftests/net/psock_snd.c  | 67 ++++++++++++++++++++++--
 tools/testing/selftests/net/psock_snd.sh |  5 ++
 2 files changed, 67 insertions(+), 5 deletions(-)

v2:
  - Add is_psock bool argument to do_rx.
  - Factor out aux data check into its own function for readability.

diff --git a/tools/testing/selftests/net/psock_snd.c b/tools/testing/selftests/net/psock_snd.c
index 81096df5cffc..5464317c1764 100644
--- a/tools/testing/selftests/net/psock_snd.c
+++ b/tools/testing/selftests/net/psock_snd.c
@@ -40,6 +40,7 @@ static bool	cfg_use_qdisc_bypass;
 static bool	cfg_use_vlan;
 static bool	cfg_use_vnet;
 static bool	cfg_drop;
+static bool	cfg_aux_data;
 
 static char	*cfg_ifname = "lo";
 static int	cfg_mtu	= 1500;
@@ -279,11 +280,54 @@ static int setup_rx(void)
 	return fd;
 }
 
-static void do_rx(int fd, int expected_len, char *expected)
+static void check_aux_data(struct cmsghdr *cmsg, int expected_len)
 {
+	struct tpacket_auxdata *adata;
+
+	if (!cmsg)
+		error(1, 0, "auxdata null");
+
+	if (cmsg->cmsg_level != SOL_PACKET)
+		error(1, 0, "cmsg_level != SOL_PACKET");
+
+	if (cmsg->cmsg_type != PACKET_AUXDATA)
+		error(1, 0, "cmsg_type != PACKET_AUXDATA");
+
+	adata = (struct tpacket_auxdata *)CMSG_DATA(cmsg);
+
+	if (adata->tp_net != ETH_HLEN)
+		error(1, 0, "cmsg tp_net != ETH_HLEN");
+
+	if (adata->tp_len != expected_len)
+		error(1, 0, "cmsg tp_len != %u", expected_len);
+
+	if (adata->tp_snaplen != expected_len)
+		error(1, 0, "cmsg tp_snaplen != %u", expected_len);
+}
+
+static void do_rx(int fd, int expected_len, char *expected, bool is_psock)
+{
+	bool aux = is_psock && cfg_aux_data;
+	char cmsg_buf[1024] = {};
+	struct msghdr msg = {};
+	struct iovec iov[1];
 	int ret;
 
-	ret = recv(fd, rbuf, sizeof(rbuf), 0);
+	if (aux) {
+		iov[0].iov_base = rbuf;
+		iov[0].iov_len = sizeof(rbuf);
+
+		msg.msg_iov = iov;
+		msg.msg_iovlen = 1;
+
+		msg.msg_control = cmsg_buf;
+		msg.msg_controllen = sizeof(cmsg_buf);
+
+		ret = recvmsg(fd, &msg, 0);
+	} else {
+		ret = recv(fd, rbuf, sizeof(rbuf), 0);
+	}
+
 	if (ret == -1)
 		error(1, errno, "recv");
 	if (ret != expected_len)
@@ -292,6 +336,12 @@ static void do_rx(int fd, int expected_len, char *expected)
 	if (memcmp(rbuf, expected, ret))
 		error(1, 0, "recv: data mismatch");
 
+	if (aux) {
+		struct cmsghdr *cmsg = CMSG_FIRSTHDR(&msg);
+
+		check_aux_data(cmsg, expected_len);
+	}
+
 	fprintf(stderr, "rx: %u\n", ret);
 }
 
@@ -312,6 +362,10 @@ static int setup_sniffer(void)
 		if (setsockopt(fd, SOL_SOCKET, SO_RCVBUF, &one, sizeof(one)))
 			error(1, errno, "setsockopt SO_RCVBUF");
 
+	if (cfg_aux_data)
+		if (setsockopt(fd, SOL_PACKET, PACKET_AUXDATA, &one, sizeof(one)))
+			error(1, errno, "setsockopt PACKET_AUXDATA");
+
 	pair_udp_setfilter(fd);
 	do_bind(fd);
 
@@ -322,8 +376,11 @@ static void parse_opts(int argc, char **argv)
 {
 	int c;
 
-	while ((c = getopt(argc, argv, "bcCdDgl:qt:vV")) != -1) {
+	while ((c = getopt(argc, argv, "abcCdDgl:qt:vV")) != -1) {
 		switch (c) {
+		case 'a':
+			cfg_aux_data = true;
+			break;
 		case 'b':
 			cfg_use_bind = true;
 			break;
@@ -432,11 +489,11 @@ static void run_test(void)
 	/* BPF filter accepts only this length, vlan changes MAC */
 	if (cfg_payload_len == DATA_LEN && !cfg_use_vlan) {
 		do_rx(fds, total_len - sizeof(struct virtio_net_hdr),
-		      tbuf + sizeof(struct virtio_net_hdr));
+		      tbuf + sizeof(struct virtio_net_hdr), true);
 		check_packet_stats(fds);
 	}
 
-	do_rx(fdr, cfg_payload_len, tbuf + total_len - cfg_payload_len);
+	do_rx(fdr, cfg_payload_len, tbuf + total_len - cfg_payload_len, false);
 
 out:
 	if (close(fds))
diff --git a/tools/testing/selftests/net/psock_snd.sh b/tools/testing/selftests/net/psock_snd.sh
index b6ef12fad5d5..111c9e2f0d21 100755
--- a/tools/testing/selftests/net/psock_snd.sh
+++ b/tools/testing/selftests/net/psock_snd.sh
@@ -97,4 +97,9 @@ echo "raw gso max size + 1 (expected to fail)"
 echo "test drops statistics"
 ./in_netns.sh ./psock_snd -D
 
+# test aux data
+
+echo "test aux data"
+./in_netns.sh ./psock_snd -a
+
 echo "OK. All tests passed"
-- 
2.52.0

Re: [net-next v2 2/3] selftests/net: Test PACKET_STATISTICS drops

[Willem de Bruijn]

Joe Damato wrote:
> Extend psock_snd to test drops by setting a tiny receive buffer and
> sending a large burst of packets.
> 
> Signed-off-by: Joe Damato <joe@dama.to>
> ---
>  tools/testing/selftests/net/psock_snd.c  | 48 ++++++++++++++++++++----
>  tools/testing/selftests/net/psock_snd.sh |  5 +++
>  2 files changed, 46 insertions(+), 7 deletions(-)
> 
> v2:
>   - Remove do_tx argument and use global cfg_drop instead
> 
> diff --git a/tools/testing/selftests/net/psock_snd.c b/tools/testing/selftests/net/psock_snd.c
> index 5be481a3d2bd..81096df5cffc 100644
> --- a/tools/testing/selftests/net/psock_snd.c
> +++ b/tools/testing/selftests/net/psock_snd.c
> @@ -39,6 +39,7 @@ static bool	cfg_use_gso;
>  static bool	cfg_use_qdisc_bypass;
>  static bool	cfg_use_vlan;
>  static bool	cfg_use_vnet;
> +static bool	cfg_drop;
>  
>  static char	*cfg_ifname = "lo";
>  static int	cfg_mtu	= 1500;
> @@ -49,6 +50,8 @@ static uint16_t	cfg_port = 8000;
>  /* test sending up to max mtu + 1 */
>  #define TEST_SZ	(sizeof(struct virtio_net_hdr) + ETH_HLEN + ETH_MAX_MTU + 1)
>  
> +#define BURST_CNT (1000)
> +
>  static char tbuf[TEST_SZ], rbuf[TEST_SZ];
>  
>  static unsigned long add_csum_hword(const uint16_t *start, int num_u16)
> @@ -212,13 +215,14 @@ static void do_send(int fd, char *buf, int len)
>  	if (ret != len)
>  		error(1, 0, "write: %u %u", ret, len);
>  
> -	fprintf(stderr, "tx: %u\n", ret);
> +	if (!cfg_drop)
> +		fprintf(stderr, "tx: %u\n", ret);
>  }
>  
>  static int do_tx(void)
>  {
>  	const int one = 1;
> -	int fd, len;
> +	int i, fd, len;
>  
>  	fd = socket(PF_PACKET, cfg_use_dgram ? SOCK_DGRAM : SOCK_RAW, 0);
>  	if (fd == -1)
> @@ -242,6 +246,10 @@ static int do_tx(void)
>  
>  	do_send(fd, tbuf, len);
>  
> +	if (cfg_drop)
> +		for (i = 0; i < BURST_CNT; i++)
> +			do_send(fd, tbuf, len);
> +
>  	if (close(fd))
>  		error(1, errno, "close t");
>  
> @@ -290,6 +298,7 @@ static void do_rx(int fd, int expected_len, char *expected)
>  static int setup_sniffer(void)
>  {
>  	struct timeval tv = { .tv_usec = 100 * 1000 };
> +	const int one = 1;
>  	int fd;
>  
>  	fd = socket(PF_PACKET, SOCK_RAW, 0);
> @@ -299,6 +308,10 @@ static int setup_sniffer(void)
>  	if (setsockopt(fd, SOL_SOCKET, SO_RCVTIMEO, &tv, sizeof(tv)))
>  		error(1, errno, "setsockopt rcv timeout");
>  
> +	if (cfg_drop)
> +		if (setsockopt(fd, SOL_SOCKET, SO_RCVBUF, &one, sizeof(one)))
> +			error(1, errno, "setsockopt SO_RCVBUF");
> +
>  	pair_udp_setfilter(fd);
>  	do_bind(fd);
>  
> @@ -309,7 +322,7 @@ static void parse_opts(int argc, char **argv)
>  {
>  	int c;
>  
> -	while ((c = getopt(argc, argv, "bcCdgl:qt:vV")) != -1) {
> +	while ((c = getopt(argc, argv, "bcCdDgl:qt:vV")) != -1) {
>  		switch (c) {
>  		case 'b':
>  			cfg_use_bind = true;
> @@ -323,6 +336,9 @@ static void parse_opts(int argc, char **argv)
>  		case 'd':
>  			cfg_use_dgram = true;
>  			break;
> +		case 'D':
> +			cfg_drop = true;
> +			break;
>  		case 'g':
>  			cfg_use_gso = true;
>  			break;
> @@ -367,11 +383,23 @@ static void check_packet_stats(int fd)
>  	if (getsockopt(fd, SOL_PACKET, PACKET_STATISTICS, &st, &len))
>  		error(1, errno, "getsockopt packet statistics");
>  
> -	if (st.tp_packets != 1)
> -		error(1, 0, "stats: tp_packets %u != 1", st.tp_packets);
> +	if (cfg_drop) {
> +		/* PACKET_STATISTICS reports all packets seen (including
> +		 * drops) in tp_packets
> +		 */
> +		if (st.tp_packets < st.tp_drops)
> +			error(1, 0, "stats: tp_packets %u < tp_drops %u",
> +			      st.tp_packets, st.tp_drops);
>  
> -	if (st.tp_drops != 0)
> -		error(1, 0, "stats: tp_drops %u != 0", st.tp_drops);
> +		if (st.tp_drops == 0)
> +			error(1, 0, "stats: expected drops but tp_drops == 0");
> +	} else {
> +		if (st.tp_packets != 1)
> +			error(1, 0, "stats: tp_packets %u != 1", st.tp_packets);
> +
> +		if (st.tp_drops != 0)
> +			error(1, 0, "stats: tp_drops %u != 0", st.tp_drops);
> +	}
>  
>  	/* verify clear on read */
>  	memset(&st, 0xff, sizeof(st));
> @@ -396,6 +424,11 @@ static void run_test(void)
>  
>  	total_len = do_tx();
>  
> +	if (cfg_drop) {
> +		check_packet_stats(fds);
> +		goto out;
> +	}

Sashiko pointed out that there may be a race here between rx
processing in the softirq and check_packet_stats. Seems plausible.

>  	/* BPF filter accepts only this length, vlan changes MAC */
>  	if (cfg_payload_len == DATA_LEN && !cfg_use_vlan) {
>  		do_rx(fds, total_len - sizeof(struct virtio_net_hdr),
> @@ -405,6 +438,7 @@ static void run_test(void)
>  
>  	do_rx(fdr, cfg_payload_len, tbuf + total_len - cfg_payload_len);
>  
> +out:
>  	if (close(fds))
>  		error(1, errno, "close s");
>  	if (close(fdr))
> diff --git a/tools/testing/selftests/net/psock_snd.sh b/tools/testing/selftests/net/psock_snd.sh
> index 1cbfeb5052ec..b6ef12fad5d5 100755
> --- a/tools/testing/selftests/net/psock_snd.sh
> +++ b/tools/testing/selftests/net/psock_snd.sh
> @@ -92,4 +92,9 @@ echo "raw gso max size"
>  echo "raw gso max size + 1 (expected to fail)"
>  (! ./in_netns.sh ./psock_snd -v -c -g -l "${max_mss_exceeds}")
>  
> +# test drops statistics
> +
> +echo "test drops statistics"
> +./in_netns.sh ./psock_snd -D
> +
>  echo "OK. All tests passed"
> -- 
> 2.52.0
> 

Re: [net-next v2 3/3] selftests/net: Test PACKET_AUXDATA

[Willem de Bruijn]

Joe Damato wrote:
> Extend the packet socket selftest, adding a recvmsg path, to test
> PACKET_AUXDATA. Check basic attributes of tpacket_auxdata.
> 
> Signed-off-by: Joe Damato <joe@dama.to>

> @@ -322,8 +376,11 @@ static void parse_opts(int argc, char **argv)
>  {
>  	int c;
>  
> -	while ((c = getopt(argc, argv, "bcCdDgl:qt:vV")) != -1) {
> +	while ((c = getopt(argc, argv, "abcCdDgl:qt:vV")) != -1) {
>  		switch (c) {
> +		case 'a':
> +			cfg_aux_data = true;
> +			break;

These are intended as independent test cases: test_auxdata,
test_drops. By using flags the flags can be combined. Are all
combinations of flags expected to work? Else maybe we should instead
have a -T "test_name", or so, to keep them mutually exclusive and
easy to reason about.

Or just a test that two flags are not set at the same time.

Re: [net-next v2 3/3] selftests/net: Test PACKET_AUXDATA

[Willem de Bruijn]

Joe Damato wrote:
> Extend the packet socket selftest, adding a recvmsg path, to test
> PACKET_AUXDATA. Check basic attributes of tpacket_auxdata.
> 
> Signed-off-by: Joe Damato <joe@dama.to>
> ---
>  tools/testing/selftests/net/psock_snd.c  | 67 ++++++++++++++++++++++--
>  tools/testing/selftests/net/psock_snd.sh |  5 ++
>  2 files changed, 67 insertions(+), 5 deletions(-)
> 
> v2:
>   - Add is_psock bool argument to do_rx.
>   - Factor out aux data check into its own function for readability.
> 
> diff --git a/tools/testing/selftests/net/psock_snd.c b/tools/testing/selftests/net/psock_snd.c
> index 81096df5cffc..5464317c1764 100644
> --- a/tools/testing/selftests/net/psock_snd.c
> +++ b/tools/testing/selftests/net/psock_snd.c
> @@ -40,6 +40,7 @@ static bool	cfg_use_qdisc_bypass;
>  static bool	cfg_use_vlan;
>  static bool	cfg_use_vnet;
>  static bool	cfg_drop;
> +static bool	cfg_aux_data;
>  
>  static char	*cfg_ifname = "lo";
>  static int	cfg_mtu	= 1500;
> @@ -279,11 +280,54 @@ static int setup_rx(void)
>  	return fd;
>  }
>  
> -static void do_rx(int fd, int expected_len, char *expected)
> +static void check_aux_data(struct cmsghdr *cmsg, int expected_len)
>  {
> +	struct tpacket_auxdata *adata;
> +
> +	if (!cmsg)
> +		error(1, 0, "auxdata null");
> +
> +	if (cmsg->cmsg_level != SOL_PACKET)
> +		error(1, 0, "cmsg_level != SOL_PACKET");
> +
> +	if (cmsg->cmsg_type != PACKET_AUXDATA)
> +		error(1, 0, "cmsg_type != PACKET_AUXDATA");
> +
> +	adata = (struct tpacket_auxdata *)CMSG_DATA(cmsg);

Sashiko had another interesting observation that this access may be
unaligned, as cmsg_buf[1024] has 1-byte alignment.

That is not new in this patch. Indeed most tests in this dir just
deference msg_control as struct cmsghdr * and CMSG_DATA as whatever
domain specific type.

The man page also warns about this and suggests using memcpy to
access CMSG_DATA. Not sure why it does not warn about the other
cmsg_.. fields.

Indeed I can trigger this, e.g., with ipv6_flowlabel.c with

-       char control[CMSG_SPACE(sizeof(flowlabel))] = {0};
+       char control[1 + CMSG_SPACE(sizeof(flowlabel))] = {0};

-               cm = (void *)control;
+               cm = (void *)control + 1;

and compiling with -fsanitize=alignment. That triggers warnings for
all fields, starting from cmsg_len on line 78.

In practice this does not cause issues, because the compiler appears
to align char[] to 16B, even though __alignof__(control) shows 1. This
seems true for x86_64, but I am not aware that it is true across all
archs, especially those that cannot handle unaligned access.

I think the x86_64 source is the AMD64 ABI Draft, e.g., v 0.99.6

   An array uses the same alignment as its elements, except that
   a local or global array variable of length at least 16 bytes
   or a C99 variable-length array variable always has alignment
   of at least 16 bytes(4)

   (4) The alignment requirement allows the use of SSE instructions
   when operating on the array. [..]

Makes sense as sizeof struct cmsghdr == 16.

cmsg_len has length 8 (size_t). We'll be hardpressed to find a
CMSG_DATA example with a larger alignment requirement. Indeed I did
not in this directory. So satisfying 8-byte alignment for msg_control
will suffice for all tests in this directory.

Unless we're certain that 8B alignment for stack aligned char[] is
guaranteed across platforms, one safe approach it to add explicit
alignment:

-       char control[CMSG_SPACE(sizeof(flowlabel))] = {0};
+       char control[CMSG_SPACE(sizeof(flowlabel))] __attribute__((aligned(8))) = {0};

I can update the (other) tests. Unless someone knows that this is
indeed not needed in practice on any platform.

> +
> +	if (adata->tp_net != ETH_HLEN)
> +		error(1, 0, "cmsg tp_net != ETH_HLEN");
> +
> +	if (adata->tp_len != expected_len)
> +		error(1, 0, "cmsg tp_len != %u", expected_len);
> +
> +	if (adata->tp_snaplen != expected_len)
> +		error(1, 0, "cmsg tp_snaplen != %u", expected_len);
> +}
> +
> +static void do_rx(int fd, int expected_len, char *expected, bool is_psock)
> +{
> +	bool aux = is_psock && cfg_aux_data;
> +	char cmsg_buf[1024] = {};
> +	struct msghdr msg = {};
> +	struct iovec iov[1];
>  	int ret;
>  
> -	ret = recv(fd, rbuf, sizeof(rbuf), 0);
> +	if (aux) {
> +		iov[0].iov_base = rbuf;
> +		iov[0].iov_len = sizeof(rbuf);
> +
> +		msg.msg_iov = iov;
> +		msg.msg_iovlen = 1;
> +
> +		msg.msg_control = cmsg_buf;
> +		msg.msg_controllen = sizeof(cmsg_buf);
> +
> +		ret = recvmsg(fd, &msg, 0);
> +	} else {
> +		ret = recv(fd, rbuf, sizeof(rbuf), 0);
> +	}
> +

Re: [net-next v2 3/3] selftests/net: Test PACKET_AUXDATA

[Willem de Bruijn]

Willem de Bruijn wrote:
> Joe Damato wrote:
> > Extend the packet socket selftest, adding a recvmsg path, to test
> > PACKET_AUXDATA. Check basic attributes of tpacket_auxdata.
> > 
> > Signed-off-by: Joe Damato <joe@dama.to>
> > ---
> >  tools/testing/selftests/net/psock_snd.c  | 67 ++++++++++++++++++++++--
> >  tools/testing/selftests/net/psock_snd.sh |  5 ++
> >  2 files changed, 67 insertions(+), 5 deletions(-)
> > 
> > v2:
> >   - Add is_psock bool argument to do_rx.
> >   - Factor out aux data check into its own function for readability.
> > 
> > diff --git a/tools/testing/selftests/net/psock_snd.c b/tools/testing/selftests/net/psock_snd.c
> > index 81096df5cffc..5464317c1764 100644
> > --- a/tools/testing/selftests/net/psock_snd.c
> > +++ b/tools/testing/selftests/net/psock_snd.c
> > @@ -40,6 +40,7 @@ static bool	cfg_use_qdisc_bypass;
> >  static bool	cfg_use_vlan;
> >  static bool	cfg_use_vnet;
> >  static bool	cfg_drop;
> > +static bool	cfg_aux_data;
> >  
> >  static char	*cfg_ifname = "lo";
> >  static int	cfg_mtu	= 1500;
> > @@ -279,11 +280,54 @@ static int setup_rx(void)
> >  	return fd;
> >  }
> >  
> > -static void do_rx(int fd, int expected_len, char *expected)
> > +static void check_aux_data(struct cmsghdr *cmsg, int expected_len)
> >  {
> > +	struct tpacket_auxdata *adata;
> > +
> > +	if (!cmsg)
> > +		error(1, 0, "auxdata null");
> > +
> > +	if (cmsg->cmsg_level != SOL_PACKET)
> > +		error(1, 0, "cmsg_level != SOL_PACKET");
> > +
> > +	if (cmsg->cmsg_type != PACKET_AUXDATA)
> > +		error(1, 0, "cmsg_type != PACKET_AUXDATA");
> > +
> > +	adata = (struct tpacket_auxdata *)CMSG_DATA(cmsg);
> 
> Sashiko had another interesting observation that this access may be
> unaligned, as cmsg_buf[1024] has 1-byte alignment.
> 
> That is not new in this patch. Indeed most tests in this dir just
> deference msg_control as struct cmsghdr * and CMSG_DATA as whatever
> domain specific type.
> 
> The man page also warns about this and suggests using memcpy to
> access CMSG_DATA. Not sure why it does not warn about the other
> cmsg_.. fields.

The commit that introduced that has more context

https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/commit/man/man3/cmsg.3?id=36d25246b4333513fefdbec7f78f29d193cf5d9a

It points out 32-bit platforms where cmsghdr is 12 bytes.

At least one example given, ptpd, uses a union to ensure alignment

	union {
		struct cmsghdr cm;
		char	control[256];
	}     cmsg_un;

But at least one other example, ssmping, does not. So I think not even
the 4B (on 32-bit archs) of cmsghdr fields can be depended on.

> Indeed I can trigger this, e.g., with ipv6_flowlabel.c with
> 
> -       char control[CMSG_SPACE(sizeof(flowlabel))] = {0};
> +       char control[1 + CMSG_SPACE(sizeof(flowlabel))] = {0};
> 
> -               cm = (void *)control;
> +               cm = (void *)control + 1;
> 
> and compiling with -fsanitize=alignment. That triggers warnings for
> all fields, starting from cmsg_len on line 78.
> 
> In practice this does not cause issues, because the compiler appears
> to align char[] to 16B, even though __alignof__(control) shows 1. This
> seems true for x86_64, but I am not aware that it is true across all
> archs, especially those that cannot handle unaligned access.
> 
> I think the x86_64 source is the AMD64 ABI Draft, e.g., v 0.99.6
> 
>    An array uses the same alignment as its elements, except that
>    a local or global array variable of length at least 16 bytes
>    or a C99 variable-length array variable always has alignment
>    of at least 16 bytes(4)
> 
>    (4) The alignment requirement allows the use of SSE instructions
>    when operating on the array. [..]
> 
> Makes sense as sizeof struct cmsghdr == 16.
> 
> cmsg_len has length 8 (size_t). We'll be hardpressed to find a
> CMSG_DATA example with a larger alignment requirement. Indeed I did
> not in this directory. So satisfying 8-byte alignment for msg_control
> will suffice for all tests in this directory.
> 
> Unless we're certain that 8B alignment for stack aligned char[] is
> guaranteed across platforms, one safe approach it to add explicit
> alignment:
> 
> -       char control[CMSG_SPACE(sizeof(flowlabel))] = {0};
> +       char control[CMSG_SPACE(sizeof(flowlabel))] __attribute__((aligned(8))) = {0};
> 
> I can update the (other) tests. Unless someone knows that this is
> indeed not needed in practice on any platform.
> 
> > +
> > +	if (adata->tp_net != ETH_HLEN)
> > +		error(1, 0, "cmsg tp_net != ETH_HLEN");
> > +
> > +	if (adata->tp_len != expected_len)
> > +		error(1, 0, "cmsg tp_len != %u", expected_len);
> > +
> > +	if (adata->tp_snaplen != expected_len)
> > +		error(1, 0, "cmsg tp_snaplen != %u", expected_len);
> > +}
> > +
> > +static void do_rx(int fd, int expected_len, char *expected, bool is_psock)
> > +{
> > +	bool aux = is_psock && cfg_aux_data;
> > +	char cmsg_buf[1024] = {};
> > +	struct msghdr msg = {};
> > +	struct iovec iov[1];
> >  	int ret;
> >  
> > -	ret = recv(fd, rbuf, sizeof(rbuf), 0);
> > +	if (aux) {
> > +		iov[0].iov_base = rbuf;
> > +		iov[0].iov_len = sizeof(rbuf);
> > +
> > +		msg.msg_iov = iov;
> > +		msg.msg_iovlen = 1;
> > +
> > +		msg.msg_control = cmsg_buf;
> > +		msg.msg_controllen = sizeof(cmsg_buf);
> > +
> > +		ret = recvmsg(fd, &msg, 0);
> > +	} else {
> > +		ret = recv(fd, rbuf, sizeof(rbuf), 0);
> > +	}
> > +

Re: [net-next v2 2/3] selftests/net: Test PACKET_STATISTICS drops

[Joe Damato]

On Sat, Apr 04, 2026 at 11:08:15AM -0400, Willem de Bruijn wrote:
> Joe Damato wrote:

[...]

> > @@ -396,6 +424,11 @@ static void run_test(void)
> >  
> >  	total_len = do_tx();
> >  
> > +	if (cfg_drop) {
> > +		check_packet_stats(fds);
> > +		goto out;
> > +	}
> 
> Sashiko pointed out

In the future, please link to the AI report.

> that there may be a race here between rx
> processing in the softirq and check_packet_stats. Seems plausible.

Can you elaborate why this is true?

I could be wrong, but I scanned the code and __local_bh_enable_ip seems to run
softirq synchronously in this case, so I think the code is correct.

Re: [net-next v2 3/3] selftests/net: Test PACKET_AUXDATA

[Joe Damato]

On Sat, Apr 04, 2026 at 11:10:13AM -0400, Willem de Bruijn wrote:
> Joe Damato wrote:
> > Extend the packet socket selftest, adding a recvmsg path, to test
> > PACKET_AUXDATA. Check basic attributes of tpacket_auxdata.
> > 
> > Signed-off-by: Joe Damato <joe@dama.to>
> 
> > @@ -322,8 +376,11 @@ static void parse_opts(int argc, char **argv)
> >  {
> >  	int c;
> >  
> > -	while ((c = getopt(argc, argv, "bcCdDgl:qt:vV")) != -1) {
> > +	while ((c = getopt(argc, argv, "abcCdDgl:qt:vV")) != -1) {
> >  		switch (c) {
> > +		case 'a':
> > +			cfg_aux_data = true;
> > +			break;
> 
> These are intended as independent test cases: test_auxdata,
> test_drops. By using flags the flags can be combined. Are all
> combinations of flags expected to work? Else maybe we should instead
> have a -T "test_name", or so, to keep them mutually exclusive and
> easy to reason about.
> 
> Or just a test that two flags are not set at the same time.

Sure, I can add a two line at the bottom with the existing flags checks. I
don't think adding a -T "test_name" is worth the effort, though.

Re: [net-next v2 3/3] selftests/net: Test PACKET_AUXDATA

[Joe Damato]

On Sat, Apr 04, 2026 at 11:30:09PM -0400, Willem de Bruijn wrote:
> Willem de Bruijn wrote:
> > Joe Damato wrote:

[...]

> > > -static void do_rx(int fd, int expected_len, char *expected)
> > > +static void check_aux_data(struct cmsghdr *cmsg, int expected_len)
> > >  {
> > > +	struct tpacket_auxdata *adata;
> > > +
> > > +	if (!cmsg)
> > > +		error(1, 0, "auxdata null");
> > > +
> > > +	if (cmsg->cmsg_level != SOL_PACKET)
> > > +		error(1, 0, "cmsg_level != SOL_PACKET");
> > > +
> > > +	if (cmsg->cmsg_type != PACKET_AUXDATA)
> > > +		error(1, 0, "cmsg_type != PACKET_AUXDATA");
> > > +
> > > +	adata = (struct tpacket_auxdata *)CMSG_DATA(cmsg);
> > 
> > Sashiko had another interesting observation that this access may be
> > unaligned, as cmsg_buf[1024] has 1-byte alignment.
> > 
> > That is not new in this patch. Indeed most tests in this dir just
> > deference msg_control as struct cmsghdr * and CMSG_DATA as whatever
> > domain specific type.
> > 
> > The man page also warns about this and suggests using memcpy to
> > access CMSG_DATA. Not sure why it does not warn about the other
> > cmsg_.. fields.
> 
> The commit that introduced that has more context
> 
> https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/commit/man/man3/cmsg.3?id=36d25246b4333513fefdbec7f78f29d193cf5d9a
> 
> It points out 32-bit platforms where cmsghdr is 12 bytes.
> 
> At least one example given, ptpd, uses a union to ensure alignment
> 
> 	union {
> 		struct cmsghdr cm;
> 		char	control[256];
> 	}     cmsg_un;
> 
> But at least one other example, ssmping, does not. So I think not even
> the 4B (on 32-bit archs) of cmsghdr fields can be depended on.

I'm fine with adding an "__attribute__((aligned(8)))" to be safe, but FWIW, I
think the key point from the commit message linked above is:
 
  shows access to int payload via *(int *)CMSG_DATA(cmsg) (of course
  int is safe because its alignment is <= header alignment, but this
  is not mentioned).

struct tpacket_auxdata only has u32 and u16, so 4 byte alignment seems fine
and I don't think the issue applies in this particular case. But, as I said
above, I am fine with adding the attribute to be defensive.

Re: [net-next v2 2/3] selftests/net: Test PACKET_STATISTICS drops

[Jakub Kicinski]

On Mon, 6 Apr 2026 09:29:03 -0700 Joe Damato wrote:
> > > +	if (cfg_drop) {
> > > +		check_packet_stats(fds);
> > > +		goto out;
> > > +	}  
> > 
> > Sashiko pointed out  
> 
> In the future, please link to the AI report.

No. We don't link to CI reports as a general rule to avoid people
posting code just to have it tested.

Re: [net-next v2 3/3] selftests/net: Test PACKET_AUXDATA

[Willem de Bruijn]

Joe Damato wrote:
> On Sat, Apr 04, 2026 at 11:30:09PM -0400, Willem de Bruijn wrote:
> > Willem de Bruijn wrote:
> > > Joe Damato wrote:
> 
> [...]
> 
> > > > -static void do_rx(int fd, int expected_len, char *expected)
> > > > +static void check_aux_data(struct cmsghdr *cmsg, int expected_len)
> > > >  {
> > > > +	struct tpacket_auxdata *adata;
> > > > +
> > > > +	if (!cmsg)
> > > > +		error(1, 0, "auxdata null");
> > > > +
> > > > +	if (cmsg->cmsg_level != SOL_PACKET)
> > > > +		error(1, 0, "cmsg_level != SOL_PACKET");
> > > > +
> > > > +	if (cmsg->cmsg_type != PACKET_AUXDATA)
> > > > +		error(1, 0, "cmsg_type != PACKET_AUXDATA");
> > > > +
> > > > +	adata = (struct tpacket_auxdata *)CMSG_DATA(cmsg);
> > > 
> > > Sashiko had another interesting observation that this access may be
> > > unaligned, as cmsg_buf[1024] has 1-byte alignment.
> > > 
> > > That is not new in this patch. Indeed most tests in this dir just
> > > deference msg_control as struct cmsghdr * and CMSG_DATA as whatever
> > > domain specific type.
> > > 
> > > The man page also warns about this and suggests using memcpy to
> > > access CMSG_DATA. Not sure why it does not warn about the other
> > > cmsg_.. fields.
> > 
> > The commit that introduced that has more context
> > 
> > https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/commit/man/man3/cmsg.3?id=36d25246b4333513fefdbec7f78f29d193cf5d9a
> > 
> > It points out 32-bit platforms where cmsghdr is 12 bytes.
> > 
> > At least one example given, ptpd, uses a union to ensure alignment
> > 
> > 	union {
> > 		struct cmsghdr cm;
> > 		char	control[256];
> > 	}     cmsg_un;
> > 
> > But at least one other example, ssmping, does not. So I think not even
> > the 4B (on 32-bit archs) of cmsghdr fields can be depended on.
> 
> I'm fine with adding an "__attribute__((aligned(8)))" to be safe, but FWIW, I
> think the key point from the commit message linked above is:
>  
>   shows access to int payload via *(int *)CMSG_DATA(cmsg) (of course
>   int is safe because its alignment is <= header alignment, but this
>   is not mentioned).

But that conditional on having a union as showed to make sure that
the structure has the struct cmsghdr alignment.

A bare char[] won't have that.

Anyway, in practice on the platforms we test this hasn't been an issue.
Fine to copy the example from other tests, leaving out the alignment.

I will follow-up with one patch to update all of them at once, once
I'm certain that I did not overlook some other mechanism that
implicitly does guarantee this alignment on all platforms.
 
> struct tpacket_auxdata only has u32 and u16, so 4 byte alignment seems fine
> and I don't think the issue applies in this particular case. But, as I said
> above, I am fine with adding the attribute to be defensive.

Re: [net-next v2 2/3] selftests/net: Test PACKET_STATISTICS drops

[Willem de Bruijn]

Joe Damato wrote:
> On Sat, Apr 04, 2026 at 11:08:15AM -0400, Willem de Bruijn wrote:
> > Joe Damato wrote:
> 
> [...]
> 
> > > @@ -396,6 +424,11 @@ static void run_test(void)
> > >  
> > >  	total_len = do_tx();
> > >  
> > > +	if (cfg_drop) {
> > > +		check_packet_stats(fds);
> > > +		goto out;
> > > +	}
> > 
> > Sashiko pointed out
> 
> In the future, please link to the AI report.
> 
> > that there may be a race here between rx
> > processing in the softirq and check_packet_stats. Seems plausible.
> 
> Can you elaborate why this is true?
> 
> I could be wrong, but I scanned the code and __local_bh_enable_ip seems to run
> softirq synchronously in this case, so I think the code is correct.

You mean that after the return to user in sendmsg, the packets are
guaranteed to have been queued to the sd->input_pkt_queue, the napi
schedule was run by enqueue_to_backlog and napi was run on this
cpu as a result of reenabling BH. At the end of dev_queue_xmit? I
recall that there may be a common one on syscall return too, but the
details elude me right now.

Either way, makes sense. I had not fully appreciated that the 
softirq schedule is this deterministic.