[PATCH net v1 0/4] net: fix passive TFO socket having invalid NAPI ID

[PATCH net v1 0/4] net: fix passive TFO socket having invalid NAPI ID

[David Wei]

Found a bug where an accepted passive TFO socket returns an invalid NAPI
ID (i.e. 0) from SO_INCOMING_NAPI_ID. Add a selftest for this using
netdevsim and fix the bug.

Patch 1 is a drive-by fix for the lib.sh include in an existing
drivers/net/netdevsim/peer.sh selftest.

Patch 2 adds a test binary for a simple TFO server/client.

Patch 3 adds a selftest for checking that the NAPI ID of a passive TFO
socket is valid. This will currently fail.

Patch 4 adds a fix for the bug.

David Wei (4):
  selftests: netdevsim: improve lib.sh include in peer.sh
  selftests: net: add passive TFO test binary
  selftests: net: add test for passive TFO socket NAPI ID
  tcp: fix passive TFO socket having invalid NAPI ID

 net/ipv4/tcp_fastopen.c                       |   3 +
 .../selftests/drivers/net/netdevsim/peer.sh   |   3 +-
 tools/testing/selftests/net/.gitignore        |   1 +
 tools/testing/selftests/net/Makefile          |   2 +
 tools/testing/selftests/net/tfo.c             | 171 ++++++++++++++++++
 tools/testing/selftests/net/tfo_passive.sh    | 112 ++++++++++++
 6 files changed, 291 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/net/tfo.c
 create mode 100755 tools/testing/selftests/net/tfo_passive.sh

-- 
2.47.1

[PATCH net v1 1/4] selftests: netdevsim: improve lib.sh include in peer.sh

[David Wei]

Fix the peer.sh test to run from INSTALL_PATH.

Signed-off-by: David Wei <dw@davidwei.uk>
---
 tools/testing/selftests/drivers/net/netdevsim/peer.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/drivers/net/netdevsim/peer.sh b/tools/testing/selftests/drivers/net/netdevsim/peer.sh
index 1bb46ec435d4..7f32b5600925 100755
--- a/tools/testing/selftests/drivers/net/netdevsim/peer.sh
+++ b/tools/testing/selftests/drivers/net/netdevsim/peer.sh
@@ -1,7 +1,8 @@
 #!/bin/bash
 # SPDX-License-Identifier: GPL-2.0-only
 
-source ../../../net/lib.sh
+lib_dir=$(dirname $0)/../../../net
+source $lib_dir/lib.sh
 
 NSIM_DEV_1_ID=$((256 + RANDOM % 256))
 NSIM_DEV_1_SYS=/sys/bus/netdevsim/devices/netdevsim$NSIM_DEV_1_ID
-- 
2.47.1

[PATCH net v1 2/4] selftests: net: add passive TFO test binary

[David Wei]

Add a simple passive TFO server and client test binary. This will be
used to test the SO_INCOMING_NAPI_ID of passive TFO accepted sockets.

Signed-off-by: David Wei <dw@davidwei.uk>
---
 tools/testing/selftests/net/.gitignore |   1 +
 tools/testing/selftests/net/Makefile   |   1 +
 tools/testing/selftests/net/tfo.c      | 171 +++++++++++++++++++++++++
 3 files changed, 173 insertions(+)
 create mode 100644 tools/testing/selftests/net/tfo.c

diff --git a/tools/testing/selftests/net/.gitignore b/tools/testing/selftests/net/.gitignore
index 532bb732bc6d..c6dd2a335cf4 100644
--- a/tools/testing/selftests/net/.gitignore
+++ b/tools/testing/selftests/net/.gitignore
@@ -50,6 +50,7 @@ tap
 tcp_fastopen_backup_key
 tcp_inq
 tcp_mmap
+tfo
 timestamping
 tls
 toeplitz
diff --git a/tools/testing/selftests/net/Makefile b/tools/testing/selftests/net/Makefile
index ab996bd22a5f..ab8da438fd78 100644
--- a/tools/testing/selftests/net/Makefile
+++ b/tools/testing/selftests/net/Makefile
@@ -110,6 +110,7 @@ TEST_GEN_PROGS += proc_net_pktgen
 TEST_PROGS += lwt_dst_cache_ref_loop.sh
 TEST_PROGS += skf_net_off.sh
 TEST_GEN_FILES += skf_net_off
+TEST_GEN_FILES += tfo
 
 # YNL files, must be before "include ..lib.mk"
 YNL_GEN_FILES := busy_poller netlink-dumps
diff --git a/tools/testing/selftests/net/tfo.c b/tools/testing/selftests/net/tfo.c
new file mode 100644
index 000000000000..eb3cac5e583c
--- /dev/null
+++ b/tools/testing/selftests/net/tfo.c
@@ -0,0 +1,171 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <error.h>
+#include <fcntl.h>
+#include <limits.h>
+#include <stdbool.h>
+#include <stdint.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+#include <arpa/inet.h>
+#include <sys/socket.h>
+#include <netinet/tcp.h>
+#include <errno.h>
+
+static int cfg_server;
+static int cfg_client;
+static int cfg_port = 8000;
+static struct sockaddr_in6 cfg_addr;
+static char *cfg_outfile;
+
+static int parse_address(const char *str, int port, struct sockaddr_in6 *sin6)
+{
+	int ret;
+
+	sin6->sin6_family = AF_INET6;
+	sin6->sin6_port = htons(port);
+
+	ret = inet_pton(sin6->sin6_family, str, &sin6->sin6_addr);
+	if (ret != 1) {
+		/* fallback to plain IPv4 */
+		ret = inet_pton(AF_INET, str, &sin6->sin6_addr.s6_addr32[3]);
+		if (ret != 1)
+			return -1;
+
+		/* add ::ffff prefix */
+		sin6->sin6_addr.s6_addr32[0] = 0;
+		sin6->sin6_addr.s6_addr32[1] = 0;
+		sin6->sin6_addr.s6_addr16[4] = 0;
+		sin6->sin6_addr.s6_addr16[5] = 0xffff;
+	}
+
+	return 0;
+}
+
+static void run_server(void)
+{
+	unsigned long qlen = 32;
+	int fd, opt, connfd;
+	socklen_t len;
+	char buf[64];
+	FILE *outfile;
+
+	outfile = fopen(cfg_outfile, "w");
+	if (!outfile)
+		error(1, errno, "fopen() outfile");
+
+	fd = socket(AF_INET6, SOCK_STREAM, 0);
+	if (fd == -1)
+		error(1, errno, "socket()");
+
+	opt = 1;
+	if (setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &opt, sizeof(opt)) < 0)
+		error(1, errno, "setsockopt(SO_REUSEADDR)");
+
+	if (setsockopt(fd, SOL_TCP, TCP_FASTOPEN, &qlen, sizeof(qlen)) < 0)
+		error(1, errno, "setsockopt(TCP_FASTOPEN)");
+
+	if (bind(fd, (struct sockaddr *)&cfg_addr, sizeof(cfg_addr)) < 0)
+		error(1, errno, "bind()");
+
+	if (listen(fd, 5) < 0)
+		error(1, errno, "listen()");
+
+	len = sizeof(cfg_addr);
+	connfd = accept(fd, (struct sockaddr *)&cfg_addr, &len);
+	if (connfd < 0)
+		error(1, errno, "accept()");
+
+	len = sizeof(opt);
+	if (getsockopt(connfd, SOL_SOCKET, SO_INCOMING_NAPI_ID, &opt, &len) < 0)
+		error(1, errno, "getsockopt(SO_INCOMING_NAPI_ID)");
+
+	read(connfd, buf, 64);
+	fprintf(outfile, "%d\n", opt);
+
+	fclose(outfile);
+	close(connfd);
+	close(fd);
+}
+
+static void run_client(void)
+{
+	int fd;
+	char *msg = "Hello, world!";
+
+	fd = socket(AF_INET6, SOCK_STREAM, 0);
+	if (fd == -1)
+		error(1, errno, "socket()");
+
+	sendto(fd, msg, strlen(msg), MSG_FASTOPEN, (struct sockaddr *)&cfg_addr, sizeof(cfg_addr));
+
+	close(fd);
+}
+
+static void usage(const char *filepath)
+{
+	error(1, 0, "Usage: %s (-s|-c) -h<server_ip> -p<port> -o<outfile> ", filepath);
+}
+
+static void parse_opts(int argc, char **argv)
+{
+	struct sockaddr_in6 *addr6 = (void *) &cfg_addr;
+	char *addr = NULL;
+	int ret;
+	int c;
+
+	if (argc <= 1)
+		usage(argv[0]);
+
+	while ((c = getopt(argc, argv, "sch:p:o:")) != -1) {
+		switch (c) {
+		case 's':
+			if (cfg_client)
+				error(1, 0, "Pass one of -s or -c");
+			cfg_server = 1;
+			break;
+		case 'c':
+			if (cfg_server)
+				error(1, 0, "Pass one of -s or -c");
+			cfg_client = 1;
+			break;
+		case 'h':
+			addr = optarg;
+			break;
+		case 'p':
+			cfg_port = strtoul(optarg, NULL, 0);
+			break;
+		case 'o':
+			cfg_outfile = strdup(optarg);
+			if (!cfg_outfile)
+				error(1, 0, "outfile invalid");
+			break;
+		}
+	}
+
+	if (cfg_server && addr)
+		error(1, 0, "Server cannot have -h specified");
+
+	memset(addr6, 0, sizeof(*addr6));
+	addr6->sin6_family = AF_INET6;
+	addr6->sin6_port = htons(cfg_port);
+	addr6->sin6_addr = in6addr_any;
+	if (addr) {
+		ret = parse_address(addr, cfg_port, addr6);
+		if (ret)
+			error(1, 0, "Client address parse error: %s", addr);
+	}
+}
+
+int main(int argc, char **argv)
+{
+	parse_opts(argc, argv);
+
+	if (cfg_server)
+		run_server();
+	else if (cfg_client)
+		run_client();
+
+	return 0;
+}
-- 
2.47.1

[PATCH net v1 3/4] selftests: net: add test for passive TFO socket NAPI ID

[David Wei]

Add a test that checks that the NAPI ID of a passive TFO socket is valid
i.e. not zero.

Signed-off-by: David Wei <dw@davidwei.uk>
---
 tools/testing/selftests/net/Makefile       |   1 +
 tools/testing/selftests/net/tfo_passive.sh | 112 +++++++++++++++++++++
 2 files changed, 113 insertions(+)
 create mode 100755 tools/testing/selftests/net/tfo_passive.sh

diff --git a/tools/testing/selftests/net/Makefile b/tools/testing/selftests/net/Makefile
index ab8da438fd78..332f387615d7 100644
--- a/tools/testing/selftests/net/Makefile
+++ b/tools/testing/selftests/net/Makefile
@@ -111,6 +111,7 @@ TEST_PROGS += lwt_dst_cache_ref_loop.sh
 TEST_PROGS += skf_net_off.sh
 TEST_GEN_FILES += skf_net_off
 TEST_GEN_FILES += tfo
+TEST_PROGS += tfo_passive.sh
 
 # YNL files, must be before "include ..lib.mk"
 YNL_GEN_FILES := busy_poller netlink-dumps
diff --git a/tools/testing/selftests/net/tfo_passive.sh b/tools/testing/selftests/net/tfo_passive.sh
new file mode 100755
index 000000000000..80bf11fdc046
--- /dev/null
+++ b/tools/testing/selftests/net/tfo_passive.sh
@@ -0,0 +1,112 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+source lib.sh
+
+NSIM_SV_ID=$((256 + RANDOM % 256))
+NSIM_SV_SYS=/sys/bus/netdevsim/devices/netdevsim$NSIM_SV_ID
+NSIM_CL_ID=$((512 + RANDOM % 256))
+NSIM_CL_SYS=/sys/bus/netdevsim/devices/netdevsim$NSIM_CL_ID
+
+NSIM_DEV_SYS_NEW=/sys/bus/netdevsim/new_device
+NSIM_DEV_SYS_DEL=/sys/bus/netdevsim/del_device
+NSIM_DEV_SYS_LINK=/sys/bus/netdevsim/link_device
+NSIM_DEV_SYS_UNLINK=/sys/bus/netdevsim/unlink_device
+
+SERVER_IP=192.168.1.1
+CLIENT_IP=192.168.1.2
+SERVER_PORT=48675
+
+setup_ns()
+{
+	set -e
+	ip netns add nssv
+	ip netns add nscl
+
+	NSIM_SV_NAME=$(find $NSIM_SV_SYS/net -maxdepth 1 -type d ! \
+		-path $NSIM_SV_SYS/net -exec basename {} \;)
+	NSIM_CL_NAME=$(find $NSIM_CL_SYS/net -maxdepth 1 -type d ! \
+		-path $NSIM_CL_SYS/net -exec basename {} \;)
+
+	ip link set $NSIM_SV_NAME netns nssv
+	ip link set $NSIM_CL_NAME netns nscl
+
+	ip netns exec nssv ip addr add "${SERVER_IP}/24" dev $NSIM_SV_NAME
+	ip netns exec nscl ip addr add "${CLIENT_IP}/24" dev $NSIM_CL_NAME
+
+	ip netns exec nssv ip link set dev $NSIM_SV_NAME up
+	ip netns exec nscl ip link set dev $NSIM_CL_NAME up
+
+	# Enable passive TFO
+	ip netns exec nssv sysctl -w net.ipv4.tcp_fastopen=519 > /dev/null
+
+	set +e
+}
+
+cleanup_ns()
+{
+	ip netns del nscl
+	ip netns del nssv
+}
+
+###
+### Code start
+###
+
+modprobe netdevsim
+
+# linking
+
+echo $NSIM_SV_ID > $NSIM_DEV_SYS_NEW
+echo $NSIM_CL_ID > $NSIM_DEV_SYS_NEW
+udevadm settle
+
+setup_ns
+
+NSIM_SV_FD=$((256 + RANDOM % 256))
+exec {NSIM_SV_FD}</var/run/netns/nssv
+NSIM_SV_IFIDX=$(ip netns exec nssv cat /sys/class/net/$NSIM_SV_NAME/ifindex)
+
+NSIM_CL_FD=$((256 + RANDOM % 256))
+exec {NSIM_CL_FD}</var/run/netns/nscl
+NSIM_CL_IFIDX=$(ip netns exec nscl cat /sys/class/net/$NSIM_CL_NAME/ifindex)
+
+echo "$NSIM_SV_FD:$NSIM_SV_IFIDX $NSIM_CL_FD:$NSIM_CL_IFIDX" > \
+     $NSIM_DEV_SYS_LINK
+
+if [ $? -ne 0 ]; then
+	echo "linking netdevsim1 with netdevsim2 should succeed"
+	cleanup_ns
+	exit 1
+fi
+
+out_file=$(mktemp)
+
+timeout -k 1s 30s ip netns exec nssv ./tfo        \
+				-s                \
+				-p ${SERVER_PORT} \
+				-o ${out_file}&
+
+wait_local_port_listen nssv ${SERVER_PORT} tcp
+
+ip netns exec nscl ./tfo -c -h ${SERVER_IP} -p ${SERVER_PORT}
+
+wait
+
+res=$(cat $out_file)
+rm $out_file
+
+if [ $res -eq 0 ]; then
+	echo "got invalid NAPI ID from passive TFO socket"
+	cleanup_ns
+	exit 1
+fi
+
+echo "$NSIM_SV_FD:$NSIM_SV_IFIDX" > $NSIM_DEV_SYS_UNLINK
+
+echo $NSIM_CL_ID > $NSIM_DEV_SYS_DEL
+
+cleanup_ns
+
+modprobe -r netdevsim
+
+exit 0
-- 
2.47.1

[PATCH net v1 4/4] tcp: fix passive TFO socket having invalid NAPI ID

[David Wei]

There is a bug with passive TFO sockets returning an invalid NAPI ID 0
from SO_INCOMING_NAPI_ID. Normally this is not an issue, but zero copy
receive relies on a correct NAPI ID to process sockets on the right
queue.

Fix by adding a skb_mark_napi_id().

Signed-off-by: David Wei <dw@davidwei.uk>
---
 net/ipv4/tcp_fastopen.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/ipv4/tcp_fastopen.c b/net/ipv4/tcp_fastopen.c
index 9b83d639b5ac..d0ed1779861b 100644
--- a/net/ipv4/tcp_fastopen.c
+++ b/net/ipv4/tcp_fastopen.c
@@ -3,6 +3,7 @@
 #include <linux/tcp.h>
 #include <linux/rcupdate.h>
 #include <net/tcp.h>
+#include <net/busy_poll.h>
 
 void tcp_fastopen_init_key_once(struct net *net)
 {
@@ -279,6 +280,8 @@ static struct sock *tcp_fastopen_create_child(struct sock *sk,
 
 	refcount_set(&req->rsk_refcnt, 2);
 
+	sk_mark_napi_id(child, skb);
+
 	/* Now finish processing the fastopen child socket. */
 	tcp_init_transfer(child, BPF_SOCK_OPS_PASSIVE_ESTABLISHED_CB, skb);
 
-- 
2.47.1

Re: [PATCH net v1 4/4] tcp: fix passive TFO socket having invalid NAPI ID

[Kuniyuki Iwashima]

From: David Wei <dw@davidwei.uk>
Date: Mon, 16 Jun 2025 11:54:56 -0700
> There is a bug with passive TFO sockets returning an invalid NAPI ID 0
> from SO_INCOMING_NAPI_ID. Normally this is not an issue, but zero copy
> receive relies on a correct NAPI ID to process sockets on the right
> queue.
> 
> Fix by adding a skb_mark_napi_id().
>

Please add Fixes: tag.

> Signed-off-by: David Wei <dw@davidwei.uk>
> ---
>  net/ipv4/tcp_fastopen.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/net/ipv4/tcp_fastopen.c b/net/ipv4/tcp_fastopen.c
> index 9b83d639b5ac..d0ed1779861b 100644
> --- a/net/ipv4/tcp_fastopen.c
> +++ b/net/ipv4/tcp_fastopen.c
> @@ -3,6 +3,7 @@
>  #include <linux/tcp.h>
>  #include <linux/rcupdate.h>
>  #include <net/tcp.h>
> +#include <net/busy_poll.h>
>  
>  void tcp_fastopen_init_key_once(struct net *net)
>  {
> @@ -279,6 +280,8 @@ static struct sock *tcp_fastopen_create_child(struct sock *sk,
>  
>  	refcount_set(&req->rsk_refcnt, 2);
>  
> +	sk_mark_napi_id(child, skb);

I think sk_mark_napi_id_set() is better here.


> +
>  	/* Now finish processing the fastopen child socket. */
>  	tcp_init_transfer(child, BPF_SOCK_OPS_PASSIVE_ESTABLISHED_CB, skb);
>  
> -- 
> 2.47.1
> 

Re: [PATCH net v1 3/4] selftests: net: add test for passive TFO socket NAPI ID

[Jakub Kicinski]

On Mon, 16 Jun 2025 11:54:55 -0700 David Wei wrote:
> Add a test that checks that the NAPI ID of a passive TFO socket is valid
> i.e. not zero.

Could you run shellcheck and make sure none of the warnings are legit?
-- 
pw-bot: cr

Re: [PATCH net v1 4/4] tcp: fix passive TFO socket having invalid NAPI ID

[David Wei]

On 2025-06-16 12:44, Kuniyuki Iwashima wrote:
> From: David Wei <dw@davidwei.uk>
> Date: Mon, 16 Jun 2025 11:54:56 -0700
>> There is a bug with passive TFO sockets returning an invalid NAPI ID 0
>> from SO_INCOMING_NAPI_ID. Normally this is not an issue, but zero copy
>> receive relies on a correct NAPI ID to process sockets on the right
>> queue.
>>
>> Fix by adding a skb_mark_napi_id().
>>
> 
> Please add Fixes: tag.

Not sure which commit to tag as Fixes. 5b7ed089 originally created
tcp_fastopen_create_child() in tcp_fastopen.c by copying from
tcp_v4_conn_req_fastopen(). The bug has been around since either when
TFO was added in 168a8f58 or when SO_INCOMING_NAPI_ID was added in
6d433902. What's your preference?

> 
>> Signed-off-by: David Wei <dw@davidwei.uk>
>> ---
>>   net/ipv4/tcp_fastopen.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/net/ipv4/tcp_fastopen.c b/net/ipv4/tcp_fastopen.c
>> index 9b83d639b5ac..d0ed1779861b 100644
>> --- a/net/ipv4/tcp_fastopen.c
>> +++ b/net/ipv4/tcp_fastopen.c
>> @@ -3,6 +3,7 @@
>>   #include <linux/tcp.h>
>>   #include <linux/rcupdate.h>
>>   #include <net/tcp.h>
>> +#include <net/busy_poll.h>
>>   
>>   void tcp_fastopen_init_key_once(struct net *net)
>>   {
>> @@ -279,6 +280,8 @@ static struct sock *tcp_fastopen_create_child(struct sock *sk,
>>   
>>   	refcount_set(&req->rsk_refcnt, 2);
>>   
>> +	sk_mark_napi_id(child, skb);
> 
> I think sk_mark_napi_id_set() is better here.

Sure, I can switch to sk_mark_napi_id_set().

> 
> 
>> +
>>   	/* Now finish processing the fastopen child socket. */
>>   	tcp_init_transfer(child, BPF_SOCK_OPS_PASSIVE_ESTABLISHED_CB, skb);
>>   
>> -- 
>> 2.47.1
>>

Re: [PATCH net v1 4/4] tcp: fix passive TFO socket having invalid NAPI ID

[Kuniyuki Iwashima]

From: David Wei <dw@davidwei.uk>
Date: Mon, 16 Jun 2025 15:37:40 -0700
> On 2025-06-16 12:44, Kuniyuki Iwashima wrote:
> > From: David Wei <dw@davidwei.uk>
> > Date: Mon, 16 Jun 2025 11:54:56 -0700
> >> There is a bug with passive TFO sockets returning an invalid NAPI ID 0
> >> from SO_INCOMING_NAPI_ID. Normally this is not an issue, but zero copy
> >> receive relies on a correct NAPI ID to process sockets on the right
> >> queue.
> >>
> >> Fix by adding a skb_mark_napi_id().
> >>
> > 
> > Please add Fixes: tag.
> 
> Not sure which commit to tag as Fixes. 5b7ed089 originally created
> tcp_fastopen_create_child() in tcp_fastopen.c by copying from
> tcp_v4_conn_req_fastopen(). The bug has been around since either when
> TFO was added in 168a8f58 or when SO_INCOMING_NAPI_ID was added in
> 6d433902. What's your preference?

6d4339028b35 makes sense to me as SO_INCOMING_NAPI_ID (2017) was
not available as of the TFO commits (2012, 2014).


> 
> > 
> >> Signed-off-by: David Wei <dw@davidwei.uk>
> >> ---
> >>   net/ipv4/tcp_fastopen.c | 3 +++
> >>   1 file changed, 3 insertions(+)
> >>
> >> diff --git a/net/ipv4/tcp_fastopen.c b/net/ipv4/tcp_fastopen.c
> >> index 9b83d639b5ac..d0ed1779861b 100644
> >> --- a/net/ipv4/tcp_fastopen.c
> >> +++ b/net/ipv4/tcp_fastopen.c
> >> @@ -3,6 +3,7 @@
> >>   #include <linux/tcp.h>
> >>   #include <linux/rcupdate.h>
> >>   #include <net/tcp.h>
> >> +#include <net/busy_poll.h>
> >>   
> >>   void tcp_fastopen_init_key_once(struct net *net)
> >>   {
> >> @@ -279,6 +280,8 @@ static struct sock *tcp_fastopen_create_child(struct sock *sk,
> >>   
> >>   	refcount_set(&req->rsk_refcnt, 2);
> >>   
> >> +	sk_mark_napi_id(child, skb);
> > 
> > I think sk_mark_napi_id_set() is better here.
> 
> Sure, I can switch to sk_mark_napi_id_set().
> 
> > 
> > 
> >> +
> >>   	/* Now finish processing the fastopen child socket. */
> >>   	tcp_init_transfer(child, BPF_SOCK_OPS_PASSIVE_ESTABLISHED_CB, skb);
> >>   
> >> -- 
> >> 2.47.1
> >>

Re: [PATCH net v1 3/4] selftests: net: add test for passive TFO socket NAPI ID

[David Wei]

On 2025-06-16 13:13, Jakub Kicinski wrote:
> On Mon, 16 Jun 2025 11:54:55 -0700 David Wei wrote:
>> Add a test that checks that the NAPI ID of a passive TFO socket is valid
>> i.e. not zero.
> 
> Could you run shellcheck and make sure none of the warnings are legit?

The single shellcheck warning I get is this:

In tools/testing/selftests/net/tfo_passive.sh line 76:
if [ $? -ne 0 ]; then
      ^-- SC2320 (warning): This $? refers to echo/printf, not a previous command. Assign to variable to avoid it being overwritten.

I confirmed that if writing to the sysfs file returns an error e.g.
EINVAL then echo returns a non-zero status. The existing peer.sh and
busy_poll_test.sh does the same check.

Re: [PATCH net v1 4/4] tcp: fix passive TFO socket having invalid NAPI ID

[David Wei]

On 2025-06-16 16:05, Kuniyuki Iwashima wrote:
> From: David Wei <dw@davidwei.uk>
> Date: Mon, 16 Jun 2025 15:37:40 -0700
>> On 2025-06-16 12:44, Kuniyuki Iwashima wrote:
>>> From: David Wei <dw@davidwei.uk>
>>> Date: Mon, 16 Jun 2025 11:54:56 -0700
>>>> There is a bug with passive TFO sockets returning an invalid NAPI ID 0
>>>> from SO_INCOMING_NAPI_ID. Normally this is not an issue, but zero copy
>>>> receive relies on a correct NAPI ID to process sockets on the right
>>>> queue.
>>>>
>>>> Fix by adding a skb_mark_napi_id().
>>>>
>>>
>>> Please add Fixes: tag.
>>
>> Not sure which commit to tag as Fixes. 5b7ed089 originally created
>> tcp_fastopen_create_child() in tcp_fastopen.c by copying from
>> tcp_v4_conn_req_fastopen(). The bug has been around since either when
>> TFO was added in 168a8f58 or when SO_INCOMING_NAPI_ID was added in
>> 6d433902. What's your preference?
> 
> 6d4339028b35 makes sense to me as SO_INCOMING_NAPI_ID (2017) was
> not available as of the TFO commits (2012, 2014).

Makes sense, I'll use 6d433902. Thanks.

> 
> 
>>
>>>
>>>> Signed-off-by: David Wei <dw@davidwei.uk>
>>>> ---
>>>>    net/ipv4/tcp_fastopen.c | 3 +++
>>>>    1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/net/ipv4/tcp_fastopen.c b/net/ipv4/tcp_fastopen.c
>>>> index 9b83d639b5ac..d0ed1779861b 100644
>>>> --- a/net/ipv4/tcp_fastopen.c
>>>> +++ b/net/ipv4/tcp_fastopen.c
>>>> @@ -3,6 +3,7 @@
>>>>    #include <linux/tcp.h>
>>>>    #include <linux/rcupdate.h>
>>>>    #include <net/tcp.h>
>>>> +#include <net/busy_poll.h>
>>>>    
>>>>    void tcp_fastopen_init_key_once(struct net *net)
>>>>    {
>>>> @@ -279,6 +280,8 @@ static struct sock *tcp_fastopen_create_child(struct sock *sk,
>>>>    
>>>>    	refcount_set(&req->rsk_refcnt, 2);
>>>>    
>>>> +	sk_mark_napi_id(child, skb);
>>>
>>> I think sk_mark_napi_id_set() is better here.
>>
>> Sure, I can switch to sk_mark_napi_id_set().
>>
>>>
>>>
>>>> +
>>>>    	/* Now finish processing the fastopen child socket. */
>>>>    	tcp_init_transfer(child, BPF_SOCK_OPS_PASSIVE_ESTABLISHED_CB, skb);
>>>>    
>>>> -- 
>>>> 2.47.1
>>>>

Re: [PATCH net v1 4/4] tcp: fix passive TFO socket having invalid NAPI ID

[Eric Dumazet]

On Mon, Jun 16, 2025 at 5:04 PM David Wei <dw@davidwei.uk> wrote:
>
> On 2025-06-16 16:05, Kuniyuki Iwashima wrote:
> > From: David Wei <dw@davidwei.uk>
> > Date: Mon, 16 Jun 2025 15:37:40 -0700
> >> On 2025-06-16 12:44, Kuniyuki Iwashima wrote:
> >>> From: David Wei <dw@davidwei.uk>
> >>> Date: Mon, 16 Jun 2025 11:54:56 -0700
> >>>> There is a bug with passive TFO sockets returning an invalid NAPI ID 0
> >>>> from SO_INCOMING_NAPI_ID. Normally this is not an issue, but zero copy
> >>>> receive relies on a correct NAPI ID to process sockets on the right
> >>>> queue.
> >>>>
> >>>> Fix by adding a skb_mark_napi_id().
> >>>>
> >>>
> >>> Please add Fixes: tag.
> >>
> >> Not sure which commit to tag as Fixes. 5b7ed089 originally created
> >> tcp_fastopen_create_child() in tcp_fastopen.c by copying from
> >> tcp_v4_conn_req_fastopen(). The bug has been around since either when
> >> TFO was added in 168a8f58 or when SO_INCOMING_NAPI_ID was added in
> >> 6d433902. What's your preference?
> >
> > 6d4339028b35 makes sense to me as SO_INCOMING_NAPI_ID (2017) was
> > not available as of the TFO commits (2012, 2014).
>
> Makes sense, I'll use 6d433902. Thanks.

Another good candidate would be

commit e5907459ce7e    tcp: Record Rx hash and NAPI ID in tcp_child_process