[PATCH v6 0/2] bpf, sockmap: handle spurious tcp_msg_wait_data() wakeup

[PATCH v6 0/2] bpf, sockmap: handle spurious tcp_msg_wait_data() wakeup

[Nnamdi Onyeyiri]

Spurious wakeups in tcp_msg_wait_data() aren't being handled by
tcp_bpf_recvmsg() and tcp_bpf_recvmsg_parser(), leading to unexpected
EAGAIN errors returned by recvfrom()/recv().  Adding handling for the
wakeup and a selftest.

This issue was first discovered in an application that adds sockets to
a sockmap as a way to view the received data.  No redirects or any
other operations are performed.

Sashiko has noted a few other pre-existing issues in the same area
(https://patch.msgid.link/20260714205118.17DB11F000E9@smtp.kernel.org)
that can lead to EAGAIN in tcp_bpf_recvmsg() and
tcp_bpf_recvmsg_parser().  A local run of Sashiko also identified a
potential issue in tcp_bpf_recvmsg() handling zero-data FIN packets when
the MSG_PEEK flag is set.  To prevent this patchset from growing too
large, I intend to submit follow up patches to address these once this
one has been accepted.

Changes in v6:
- Closing the file descriptor in the selftest worker thread.
- Ensuring the selftest loop breaks early for an error in the worker
  thread.
- Added comments to selftest regarding focus on EAGAIN error.
- Updated selftest commit message to imperative mood.
- Link to v5: https://patch.msgid.link/20260717155348.54975-1-nnamdio@gmail.com

Changes in v5:
- Move selftest from net into bpf/prog_tests/sockmap_basic.c
- Link to v4: https://patch.msgid.link/20260715213538.37229-1-nnamdio@gmail.com

Changes in v4:
- Fix potential data loss in tcp_bpf_recvmsg() when a FIN or RST has
  been received.
- Check the return code of pthread_create() in the selftest.
- Fix race caused by using EXPECT macros in a thread in selftest.
- Link to v3: https://patch.msgid.link/20260714203927.32289-1-nnamdio@gmail.com

Changes in v3:
- Added the sockmap_recvfrom selftest.
- Link to v2: https://patch.msgid.link/alFRK66z45eDNZA7@localhost.localdomain

Changes in v2:
- In tcp_bpf_recvmsg, handle signals and the socket closing in the loop.
- Fix spurious wakeups when SO_RCVTIMEO has been set on the socket.
- Link to v1: https://patch.msgid.link/ak_rR-Skd8Mvn4mH@localhost.localdomain

Signed-off-by: Nnamdi Onyeyiri <nnamdio@gmail.com>
---
Nnamdi Onyeyiri (2):
  bpf, sockmap: handle spurious tcp_msg_wait_data() wakeup
  selftests/bpf: add sockmap recvfrom EAGAIN selftest

 net/ipv4/tcp_bpf.c                            |  69 ++++++++--
 .../selftests/bpf/prog_tests/sockmap_basic.c  | 124 ++++++++++++++++++
 2 files changed, 184 insertions(+), 9 deletions(-)

-- 
2.52.0

[PATCH v6 1/2] bpf, sockmap: handle spurious tcp_msg_wait_data() wakeup

[Nnamdi Onyeyiri]

recvfrom()/recv() are documented as only returning EAGAIN for blocking sockets
when they have a receive timeout configured.  However, adding a blocking
ipv4 tcp socket without a receive timeout to a sockmap will cause EAGAIN errors
sporadically.  A socket with a receive timeout may return EAGAIN before the
timeout expires.

There are 2 code paths affected by this:

  1. tcp_bpf_recvmsg() - Used when the socket has been added to a sockmap
     that has no verdict program attached.

  2. tcp_bpf_recvmsg_parser() - Used when the socket has been added to a
     sockmap that has a verdict program.  To reproduce this issue, it is
     enough for the verdict program to do nothing but return SK_PASS.

In both cases this happens when tcp_msg_wait_data() wakes spuriously
(returning 0).  To fix it, we now loop back to msg_bytes_ready instead
of returning -EAGAIN on spurious wakeup.

To ensure the looping does not cause sockets with a SO_RCVTIMEO set to
wait excessively long, tcp_msg_wait_data() now takes a pointer to timeo,
allowing sk_wait_event() to update it as appropriate.

The logic in tcp_bpf_recvmsg_parser() that allow it to handle signals,
socket errors and closuers in its loop was also added to tcp_bpf_recvmsg().

Signed-off-by: Nnamdi Onyeyiri <nnamdio@gmail.com>
---
 net/ipv4/tcp_bpf.c | 69 ++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 60 insertions(+), 9 deletions(-)

diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
index cc0bd73f36b6..aa5c5d741599 100644
--- a/net/ipv4/tcp_bpf.c
+++ b/net/ipv4/tcp_bpf.c
@@ -179,7 +179,7 @@ EXPORT_SYMBOL_GPL(tcp_bpf_sendmsg_redir);
 
 #ifdef CONFIG_BPF_SYSCALL
 static int tcp_msg_wait_data(struct sock *sk, struct sk_psock *psock,
-			     long timeo)
+			     long *timeo)
 {
 	DEFINE_WAIT_FUNC(wait, woken_wake_function);
 	int ret = 0;
@@ -187,12 +187,12 @@ static int tcp_msg_wait_data(struct sock *sk, struct sk_psock *psock,
 	if (sk->sk_shutdown & RCV_SHUTDOWN)
 		return 1;
 
-	if (!timeo)
+	if (!*timeo)
 		return ret;
 
 	add_wait_queue(sk_sleep(sk), &wait);
 	sk_set_bit(SOCKWQ_ASYNC_WAITDATA, sk);
-	ret = sk_wait_event(sk, &timeo,
+	ret = sk_wait_event(sk, timeo,
 			    !list_empty(&psock->ingress_msg) ||
 			    !skb_queue_empty_lockless(&sk->sk_receive_queue), &wait);
 	sk_clear_bit(SOCKWQ_ASYNC_WAITDATA, sk);
@@ -229,6 +229,7 @@ static int tcp_bpf_recvmsg_parser(struct sock *sk,
 	int copied_from_self = 0;
 	int copied = 0;
 	u32 seq;
+	long timeo;
 
 	if (unlikely(flags & MSG_ERRQUEUE))
 		return inet_recv_error(sk, msg, len);
@@ -262,6 +263,8 @@ static int tcp_bpf_recvmsg_parser(struct sock *sk,
 		}
 	}
 
+	timeo = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);
+
 msg_bytes_ready:
 	copied = __sk_msg_recvmsg(sk, psock, msg, len, flags, &copied_from_self);
 	/* The typical case for EFAULT is the socket was gracefully
@@ -280,7 +283,6 @@ static int tcp_bpf_recvmsg_parser(struct sock *sk,
 	}
 	seq += copied_from_self;
 	if (!copied) {
-		long timeo;
 		int data;
 
 		if (sock_flag(sk, SOCK_DONE))
@@ -299,7 +301,6 @@ static int tcp_bpf_recvmsg_parser(struct sock *sk,
 			goto out;
 		}
 
-		timeo = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);
 		if (!timeo) {
 			copied = -EAGAIN;
 			goto out;
@@ -310,13 +311,15 @@ static int tcp_bpf_recvmsg_parser(struct sock *sk,
 			goto out;
 		}
 
-		data = tcp_msg_wait_data(sk, psock, timeo);
+		data = tcp_msg_wait_data(sk, psock, &timeo);
 		if (data < 0) {
 			copied = data;
 			goto unlock;
 		}
 		if (data && !sk_psock_queue_empty(psock))
 			goto msg_bytes_ready;
+		if (!data && timeo > 0)
+			goto msg_bytes_ready;
 		copied = -EAGAIN;
 	}
 out:
@@ -355,6 +358,7 @@ static int tcp_bpf_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
 {
 	struct sk_psock *psock;
 	int copied, ret;
+	long timeo;
 
 	if (unlikely(flags & MSG_ERRQUEUE))
 		return inet_recv_error(sk, msg, len);
@@ -371,14 +375,59 @@ static int tcp_bpf_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
 		return tcp_recvmsg(sk, msg, len, flags);
 	}
 	lock_sock(sk);
+
+	timeo = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);
+
 msg_bytes_ready:
 	copied = sk_msg_recvmsg(sk, psock, msg, len, flags);
 	if (!copied) {
-		long timeo;
 		int data;
 
-		timeo = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);
-		data = tcp_msg_wait_data(sk, psock, timeo);
+		if (sock_flag(sk, SOCK_DONE)) {
+			ret = 0;
+			goto unlock;
+		}
+
+		if (sk->sk_err) {
+			if (!sk_psock_queue_empty(psock))
+				goto msg_bytes_ready;
+			if (!skb_queue_empty(&sk->sk_receive_queue)) {
+				release_sock(sk);
+				sk_psock_put(sk, psock);
+				return tcp_recvmsg(sk, msg, len, flags);
+			}
+			ret = sock_error(sk);
+			goto unlock;
+		}
+
+		if (sk->sk_shutdown & RCV_SHUTDOWN) {
+			if (!sk_psock_queue_empty(psock))
+				goto msg_bytes_ready;
+			if (!skb_queue_empty(&sk->sk_receive_queue)) {
+				release_sock(sk);
+				sk_psock_put(sk, psock);
+				return tcp_recvmsg(sk, msg, len, flags);
+			}
+			ret = 0;
+			goto unlock;
+		}
+
+		if (sk->sk_state == TCP_CLOSE) {
+			ret = -ENOTCONN;
+			goto unlock;
+		}
+
+		if (!timeo) {
+			ret = -EAGAIN;
+			goto unlock;
+		}
+
+		if (signal_pending(current)) {
+			ret = sock_intr_errno(timeo);
+			goto unlock;
+		}
+
+		data = tcp_msg_wait_data(sk, psock, &timeo);
 		if (data < 0) {
 			ret = data;
 			goto unlock;
@@ -390,6 +439,8 @@ static int tcp_bpf_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
 			sk_psock_put(sk, psock);
 			return tcp_recvmsg(sk, msg, len, flags);
 		}
+		if (!data && timeo > 0)
+			goto msg_bytes_ready;
 		copied = -EAGAIN;
 	}
 	ret = copied;
-- 
2.52.0

[PATCH v6 2/2] selftests/bpf: add sockmap recvfrom EAGAIN selftest

[Nnamdi Onyeyiri]

These selftests exercise the tcp_bpf_recvmsg() and tcp_bpf_recvmsg_parser()
functions, to ensure that they are properly handling spurious wakeups in
tcp_msg_wait_data().

The expected behaviour is that recvfrom() does not return an EAGAIN
error.  If the spurious wakeups are incorrectly handled, this assertion
will fail.

Signed-off-by: Nnamdi Onyeyiri <nnamdio@gmail.com>
---
 .../selftests/bpf/prog_tests/sockmap_basic.c  | 124 ++++++++++++++++++
 1 file changed, 124 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
index cb3229711f93..d18faf46fac0 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
@@ -1373,6 +1373,126 @@ static void test_sockmap_multi_channels(int sotype)
 	test_sockmap_pass_prog__destroy(skel);
 }
 
+static void *test_sockmap_recvfrom_eagain_thread(void *arg)
+{
+	int fd = *(int *)arg;
+	char buf[1024];
+	void *result = NULL;
+
+	while (true) {
+		ssize_t len = recvfrom(fd, buf, sizeof(buf), 0, NULL, NULL);
+
+		if (len == -1) {
+			if (errno == EINTR)
+				continue;
+			result = (void *)1;
+			break;
+		}
+
+		if (!len || buf[len - 1] == 'e')
+			break;
+	}
+
+	send(fd, "test", 4, MSG_NOSIGNAL);
+
+	close(fd);
+
+	return result;
+}
+
+static void test_sockmap_recvfrom_eagain(bool with_verdict)
+{
+	struct test_sockmap_pass_prog *skel = NULL;
+	struct bpf_program *prog = NULL;
+	size_t buflen = 1024 * 1024 * 25;
+	char *buf = NULL;
+	int map, err;
+
+	skel = test_sockmap_pass_prog__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "open_and_load"))
+		return;
+
+	map = bpf_map__fd(skel->maps.sock_map_msg);
+
+	if (with_verdict) {
+		prog = skel->progs.prog_skb_verdict;
+		err = bpf_prog_attach(bpf_program__fd(prog), map, BPF_SK_SKB_STREAM_VERDICT, 0);
+		if (!ASSERT_OK(err, "bpf_prog_attach verdict"))
+			goto cleanup;
+	}
+
+	buf = malloc(buflen);
+	if (!ASSERT_OK_PTR(buf, "malloc buf"))
+		goto cleanup;
+	memset(buf, 0, buflen);
+	buf[buflen - 1] = 'e';
+
+	for (int i = 0; i < 200; ++i) {
+		ssize_t sent;
+		char ignored[128];
+		pthread_t thread;
+		bool thread_created = false;
+		size_t rem = buflen;
+		int c = -1, p = -1, zero = 0;
+		bool success = false;
+
+		err = create_pair(AF_INET, SOCK_STREAM, &c, &p);
+		if (!ASSERT_OK(err, "create_pair"))
+			goto end_attempt;
+
+		err = pthread_create(&thread, NULL, &test_sockmap_recvfrom_eagain_thread, &p);
+		if (!ASSERT_OK(err, "pthread_create"))
+			goto end_attempt;
+		thread_created = true;
+
+		err = bpf_map_update_elem(map, &zero, &c, BPF_ANY);
+		if (!ASSERT_OK(err, "bpf_map_update_elem"))
+			goto end_attempt;
+
+		while (rem) {
+			sent = xsend(c, buf + (buflen - rem), rem, 0);
+			if (sent == -1)
+				goto end_attempt;
+			rem -= sent;
+		}
+
+		/* we cannot use recv_timeout(), otherwise EAGAIN would be an expected errno. */
+		err = recvfrom(c, ignored, sizeof(ignored), 0, NULL, NULL);
+
+		/*
+		 * we are checking for the invalid return of EAGAIN, any other return is considered
+		 * successful for the purposes of this test.
+		 */
+		if (err < 0 && !ASSERT_NEQ(errno, EAGAIN, "recvfrom eagain"))
+			goto end_attempt;
+
+		success = true;
+
+end_attempt:
+		if (c >= 0)
+			close(c);
+
+		if (thread_created) {
+			void *retval = NULL;
+
+			pthread_join(thread, &retval);
+			if (!ASSERT_NULL(retval, "retval"))
+				success = false;
+		}
+
+		if (!thread_created && p >= 0)
+			close(p);
+		if (!success)
+			break;
+	}
+
+cleanup:
+	if (buf)
+		free(buf);
+
+	test_sockmap_pass_prog__destroy(skel);
+}
+
 void test_sockmap_basic(void)
 {
 	if (test__start_subtest("sockmap create_update_free"))
@@ -1451,4 +1571,8 @@ void test_sockmap_basic(void)
 		test_sockmap_multi_channels(SOCK_STREAM);
 	if (test__start_subtest("sockmap udp multi channels"))
 		test_sockmap_multi_channels(SOCK_DGRAM);
+	if (test__start_subtest("sockmap recvfrom eagain"))
+		test_sockmap_recvfrom_eagain(false);
+	if (test__start_subtest("sockmap recvfrom eagain with verdict"))
+		test_sockmap_recvfrom_eagain(true);
 }
-- 
2.52.0

Re: [PATCH v6 1/2] bpf, sockmap: handle spurious tcp_msg_wait_data() wakeup

[Emil Tsalapatis]

On Mon Jul 20, 2026 at 1:15 PM EDT, Nnamdi Onyeyiri wrote:
> recvfrom()/recv() are documented as only returning EAGAIN for blocking sockets
> when they have a receive timeout configured.  However, adding a blocking
> ipv4 tcp socket without a receive timeout to a sockmap will cause EAGAIN errors
> sporadically.  A socket with a receive timeout may return EAGAIN before the
> timeout expires.
>
> There are 2 code paths affected by this:
>
>   1. tcp_bpf_recvmsg() - Used when the socket has been added to a sockmap
>      that has no verdict program attached.
>
>   2. tcp_bpf_recvmsg_parser() - Used when the socket has been added to a
>      sockmap that has a verdict program.  To reproduce this issue, it is
>      enough for the verdict program to do nothing but return SK_PASS.
>
> In both cases this happens when tcp_msg_wait_data() wakes spuriously
> (returning 0).  To fix it, we now loop back to msg_bytes_ready instead
> of returning -EAGAIN on spurious wakeup.
>
> To ensure the looping does not cause sockets with a SO_RCVTIMEO set to
> wait excessively long, tcp_msg_wait_data() now takes a pointer to timeo,
> allowing sk_wait_event() to update it as appropriate.
>
> The logic in tcp_bpf_recvmsg_parser() that allow it to handle signals,
> socket errors and closuers in its loop was also added to tcp_bpf_recvmsg().
>
> Signed-off-by: Nnamdi Onyeyiri <nnamdio@gmail.com>
> ---
>  net/ipv4/tcp_bpf.c | 69 ++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 60 insertions(+), 9 deletions(-)
>
> diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
> index cc0bd73f36b6..aa5c5d741599 100644
> --- a/net/ipv4/tcp_bpf.c
> +++ b/net/ipv4/tcp_bpf.c
> @@ -179,7 +179,7 @@ EXPORT_SYMBOL_GPL(tcp_bpf_sendmsg_redir);
>  
>  #ifdef CONFIG_BPF_SYSCALL
>  static int tcp_msg_wait_data(struct sock *sk, struct sk_psock *psock,
> -			     long timeo)
> +			     long *timeo)
>  {
>  	DEFINE_WAIT_FUNC(wait, woken_wake_function);
>  	int ret = 0;
> @@ -187,12 +187,12 @@ static int tcp_msg_wait_data(struct sock *sk, struct sk_psock *psock,
>  	if (sk->sk_shutdown & RCV_SHUTDOWN)
>  		return 1;
>  
> -	if (!timeo)
> +	if (!*timeo)
>  		return ret;
>  
>  	add_wait_queue(sk_sleep(sk), &wait);
>  	sk_set_bit(SOCKWQ_ASYNC_WAITDATA, sk);
> -	ret = sk_wait_event(sk, &timeo,
> +	ret = sk_wait_event(sk, timeo,
>  			    !list_empty(&psock->ingress_msg) ||
>  			    !skb_queue_empty_lockless(&sk->sk_receive_queue), &wait);
>  	sk_clear_bit(SOCKWQ_ASYNC_WAITDATA, sk);
> @@ -229,6 +229,7 @@ static int tcp_bpf_recvmsg_parser(struct sock *sk,
>  	int copied_from_self = 0;
>  	int copied = 0;
>  	u32 seq;
> +	long timeo;
>  
>  	if (unlikely(flags & MSG_ERRQUEUE))
>  		return inet_recv_error(sk, msg, len);
> @@ -262,6 +263,8 @@ static int tcp_bpf_recvmsg_parser(struct sock *sk,
>  		}
>  	}
>  
> +	timeo = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);
> +
>  msg_bytes_ready:
>  	copied = __sk_msg_recvmsg(sk, psock, msg, len, flags, &copied_from_self);
>  	/* The typical case for EFAULT is the socket was gracefully
> @@ -280,7 +283,6 @@ static int tcp_bpf_recvmsg_parser(struct sock *sk,
>  	}
>  	seq += copied_from_self;
>  	if (!copied) {
> -		long timeo;
>  		int data;
>  
>  		if (sock_flag(sk, SOCK_DONE))
> @@ -299,7 +301,6 @@ static int tcp_bpf_recvmsg_parser(struct sock *sk,
>  			goto out;
>  		}
>  
> -		timeo = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);
>  		if (!timeo) {
>  			copied = -EAGAIN;
>  			goto out;
> @@ -310,13 +311,15 @@ static int tcp_bpf_recvmsg_parser(struct sock *sk,
>  			goto out;
>  		}
>  
> -		data = tcp_msg_wait_data(sk, psock, timeo);
> +		data = tcp_msg_wait_data(sk, psock, &timeo);
>  		if (data < 0) {
>  			copied = data;
>  			goto unlock;
>  		}
>  		if (data && !sk_psock_queue_empty(psock))
>  			goto msg_bytes_ready;
> +		if (!data && timeo > 0)
> +			goto msg_bytes_ready;
>  		copied = -EAGAIN;
>  	}
>  out:
> @@ -355,6 +358,7 @@ static int tcp_bpf_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
>  {
>  	struct sk_psock *psock;
>  	int copied, ret;
> +	long timeo;
>  
>  	if (unlikely(flags & MSG_ERRQUEUE))
>  		return inet_recv_error(sk, msg, len);
> @@ -371,14 +375,59 @@ static int tcp_bpf_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
>  		return tcp_recvmsg(sk, msg, len, flags);
>  	}
>  	lock_sock(sk);
> +
> +	timeo = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);
> +
>  msg_bytes_ready:
>  	copied = sk_msg_recvmsg(sk, psock, msg, len, flags);
>  	if (!copied) {
> -		long timeo;
>  		int data;
>  
> -		timeo = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);
> -		data = tcp_msg_wait_data(sk, psock, timeo);
> +		if (sock_flag(sk, SOCK_DONE)) {
> +			ret = 0;
> +			goto unlock;
> +		}
> +
> +		if (sk->sk_err) {
> +			if (!sk_psock_queue_empty(psock))
> +				goto msg_bytes_ready;
> +			if (!skb_queue_empty(&sk->sk_receive_queue)) {
> +				release_sock(sk);
> +				sk_psock_put(sk, psock);
> +				return tcp_recvmsg(sk, msg, len, flags);
> +			}
> +			ret = sock_error(sk);
> +			goto unlock;
> +		}
> +
> +		if (sk->sk_shutdown & RCV_SHUTDOWN) {
> +			if (!sk_psock_queue_empty(psock))
> +				goto msg_bytes_ready;
> +			if (!skb_queue_empty(&sk->sk_receive_queue)) {
> +				release_sock(sk);
> +				sk_psock_put(sk, psock);
> +				return tcp_recvmsg(sk, msg, len, flags);
> +			}
> +			ret = 0;
> +			goto unlock;

These two error handling routines above look identical. Can you refactor
them?

> +		}
> +
> +		if (sk->sk_state == TCP_CLOSE) {
> +			ret = -ENOTCONN;
> +			goto unlock;
> +		}
> +
> +		if (!timeo) {
> +			ret = -EAGAIN;
> +			goto unlock;
> +		}
> +

Since this handling (which Sashiko flags by the way, correctly AFAICT) 
are taken from tcp_bpf_recvmsg, there is obvious overlap between the two
functions. Please factor those out so that they share the logic between
them.

pw-bot: cr

> +		if (signal_pending(current)) {
> +			ret = sock_intr_errno(timeo);
> +			goto unlock;
> +		}
> +
> +		data = tcp_msg_wait_data(sk, psock, &timeo);
>  		if (data < 0) {
>  			ret = data;
>  			goto unlock;
> @@ -390,6 +439,8 @@ static int tcp_bpf_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
>  			sk_psock_put(sk, psock);
>  			return tcp_recvmsg(sk, msg, len, flags);
>  		}
> +		if (!data && timeo > 0)
> +			goto msg_bytes_ready;
>  		copied = -EAGAIN;
>  	}
>  	ret = copied;

Re: [PATCH v6 2/2] selftests/bpf: add sockmap recvfrom EAGAIN selftest

[Emil Tsalapatis]

On Mon Jul 20, 2026 at 1:15 PM EDT, Nnamdi Onyeyiri wrote:
> These selftests exercise the tcp_bpf_recvmsg() and tcp_bpf_recvmsg_parser()
> functions, to ensure that they are properly handling spurious wakeups in
> tcp_msg_wait_data().
>
> The expected behaviour is that recvfrom() does not return an EAGAIN
> error.  If the spurious wakeups are incorrectly handled, this assertion
> will fail.
>
> Signed-off-by: Nnamdi Onyeyiri <nnamdio@gmail.com>

The test looks fine, even if slightly flaky. Running with only patch 2/2
still passes sometimes on my box. Can we handle this somehow, e.g., do
more attempts?

There's also a couple magic numbers in the tests that may need some
explanation (noted below).

> ---
>  .../selftests/bpf/prog_tests/sockmap_basic.c  | 124 ++++++++++++++++++
>  1 file changed, 124 insertions(+)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> index cb3229711f93..d18faf46fac0 100644
> --- a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> @@ -1373,6 +1373,126 @@ static void test_sockmap_multi_channels(int sotype)
>  	test_sockmap_pass_prog__destroy(skel);
>  }
>  
> +static void *test_sockmap_recvfrom_eagain_thread(void *arg)
> +{
> +	int fd = *(int *)arg;
> +	char buf[1024];
> +	void *result = NULL;
> +
> +	while (true) {
> +		ssize_t len = recvfrom(fd, buf, sizeof(buf), 0, NULL, NULL);
> +
> +		if (len == -1) {
> +			if (errno == EINTR)
> +				continue;
> +			result = (void *)1;
> +			break;
> +		}
> +
> +		if (!len || buf[len - 1] == 'e')
> +			break;
> +	}
> +
> +	send(fd, "test", 4, MSG_NOSIGNAL);
> +
> +	close(fd);
> +
> +	return result;
> +}
> +
> +static void test_sockmap_recvfrom_eagain(bool with_verdict)
> +{
> +	struct test_sockmap_pass_prog *skel = NULL;
> +	struct bpf_program *prog = NULL;
> +	size_t buflen = 1024 * 1024 * 25;

Here

> +	char *buf = NULL;
> +	int map, err;
> +
> +	skel = test_sockmap_pass_prog__open_and_load();
> +	if (!ASSERT_OK_PTR(skel, "open_and_load"))
> +		return;
> +
> +	map = bpf_map__fd(skel->maps.sock_map_msg);
> +
> +	if (with_verdict) {
> +		prog = skel->progs.prog_skb_verdict;
> +		err = bpf_prog_attach(bpf_program__fd(prog), map, BPF_SK_SKB_STREAM_VERDICT, 0);
> +		if (!ASSERT_OK(err, "bpf_prog_attach verdict"))
> +			goto cleanup;
> +	}
> +
> +	buf = malloc(buflen);
> +	if (!ASSERT_OK_PTR(buf, "malloc buf"))
> +		goto cleanup;
> +	memset(buf, 0, buflen);
> +	buf[buflen - 1] = 'e';
> +
> +	for (int i = 0; i < 200; ++i) {

Also here. Why 200 iterations specifically? Can we at least name the
defaults to make it clearer that we've chosen those numbers because
that's how we trigger the bug?

> +		ssize_t sent;
> +		char ignored[128];
> +		pthread_t thread;
> +		bool thread_created = false;
> +		size_t rem = buflen;
> +		int c = -1, p = -1, zero = 0;
> +		bool success = false;
> +
> +		err = create_pair(AF_INET, SOCK_STREAM, &c, &p);
> +		if (!ASSERT_OK(err, "create_pair"))
> +			goto end_attempt;
> +
> +		err = pthread_create(&thread, NULL, &test_sockmap_recvfrom_eagain_thread, &p);
> +		if (!ASSERT_OK(err, "pthread_create"))
> +			goto end_attempt;
> +		thread_created = true;
> +
> +		err = bpf_map_update_elem(map, &zero, &c, BPF_ANY);
> +		if (!ASSERT_OK(err, "bpf_map_update_elem"))
> +			goto end_attempt;
> +
> +		while (rem) {
> +			sent = xsend(c, buf + (buflen - rem), rem, 0);
> +			if (sent == -1)
> +				goto end_attempt;
> +			rem -= sent;
> +		}
> +
> +		/* we cannot use recv_timeout(), otherwise EAGAIN would be an expected errno. */
> +		err = recvfrom(c, ignored, sizeof(ignored), 0, NULL, NULL);
> +
> +		/*
> +		 * we are checking for the invalid return of EAGAIN, any other return is considered
> +		 * successful for the purposes of this test.
> +		 */
> +		if (err < 0 && !ASSERT_NEQ(errno, EAGAIN, "recvfrom eagain"))
> +			goto end_attempt;
> +
> +		success = true;
> +
> +end_attempt:
> +		if (c >= 0)
> +			close(c);
> +
> +		if (thread_created) {
> +			void *retval = NULL;
> +
> +			pthread_join(thread, &retval);
> +			if (!ASSERT_NULL(retval, "retval"))
> +				success = false;
> +		}
> +
> +		if (!thread_created && p >= 0)
> +			close(p);
> +		if (!success)
> +			break;
> +	}
> +
> +cleanup:
> +	if (buf)
> +		free(buf);
> +
> +	test_sockmap_pass_prog__destroy(skel);
> +}
> +
>  void test_sockmap_basic(void)
>  {
>  	if (test__start_subtest("sockmap create_update_free"))
> @@ -1451,4 +1571,8 @@ void test_sockmap_basic(void)
>  		test_sockmap_multi_channels(SOCK_STREAM);
>  	if (test__start_subtest("sockmap udp multi channels"))
>  		test_sockmap_multi_channels(SOCK_DGRAM);
> +	if (test__start_subtest("sockmap recvfrom eagain"))
> +		test_sockmap_recvfrom_eagain(false);
> +	if (test__start_subtest("sockmap recvfrom eagain with verdict"))
> +		test_sockmap_recvfrom_eagain(true);
>  }

Re: [PATCH v6 2/2] selftests/bpf: add sockmap recvfrom EAGAIN selftest

[Nnamdi Onyeyiri]

On Mon, Jul 20, 2026 at 05:47:09PM -0400, Emil Tsalapatis wrote:
> On Mon Jul 20, 2026 at 1:15 PM EDT, Nnamdi Onyeyiri wrote:
> > These selftests exercise the tcp_bpf_recvmsg() and tcp_bpf_recvmsg_parser()
> > functions, to ensure that they are properly handling spurious wakeups in
> > tcp_msg_wait_data().
> >
> > The expected behaviour is that recvfrom() does not return an EAGAIN
> > error.  If the spurious wakeups are incorrectly handled, this assertion
> > will fail.
> >
> > Signed-off-by: Nnamdi Onyeyiri <nnamdio@gmail.com>
> 
> The test looks fine, even if slightly flaky. Running with only patch 2/2
> still passes sometimes on my box. Can we handle this somehow, e.g., do
> more attempts?
> 
> There's also a couple magic numbers in the tests that may need some
> explanation (noted below).
>

Thanks for the review!  I'll attach an explaination to the numbers.
Essentially the larger the payload the fewer iterations seemed to be
required to reproduce (on my machine).

With the issue fixed though, larger payloads and more iterations make
the test run longer.  Is there is a rule of thumb I should follow for
tuning the runtime?

> > ---
> >  .../selftests/bpf/prog_tests/sockmap_basic.c  | 124 ++++++++++++++++++
> >  1 file changed, 124 insertions(+)
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> > index cb3229711f93..d18faf46fac0 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> > @@ -1373,6 +1373,126 @@ static void test_sockmap_multi_channels(int sotype)
> >  	test_sockmap_pass_prog__destroy(skel);
> >  }
> >  
> > +static void *test_sockmap_recvfrom_eagain_thread(void *arg)
> > +{
> > +	int fd = *(int *)arg;
> > +	char buf[1024];
> > +	void *result = NULL;
> > +
> > +	while (true) {
> > +		ssize_t len = recvfrom(fd, buf, sizeof(buf), 0, NULL, NULL);
> > +
> > +		if (len == -1) {
> > +			if (errno == EINTR)
> > +				continue;
> > +			result = (void *)1;
> > +			break;
> > +		}
> > +
> > +		if (!len || buf[len - 1] == 'e')
> > +			break;
> > +	}
> > +
> > +	send(fd, "test", 4, MSG_NOSIGNAL);
> > +
> > +	close(fd);
> > +
> > +	return result;
> > +}
> > +
> > +static void test_sockmap_recvfrom_eagain(bool with_verdict)
> > +{
> > +	struct test_sockmap_pass_prog *skel = NULL;
> > +	struct bpf_program *prog = NULL;
> > +	size_t buflen = 1024 * 1024 * 25;
> 
> Here
> 
> > +	char *buf = NULL;
> > +	int map, err;
> > +
> > +	skel = test_sockmap_pass_prog__open_and_load();
> > +	if (!ASSERT_OK_PTR(skel, "open_and_load"))
> > +		return;
> > +
> > +	map = bpf_map__fd(skel->maps.sock_map_msg);
> > +
> > +	if (with_verdict) {
> > +		prog = skel->progs.prog_skb_verdict;
> > +		err = bpf_prog_attach(bpf_program__fd(prog), map, BPF_SK_SKB_STREAM_VERDICT, 0);
> > +		if (!ASSERT_OK(err, "bpf_prog_attach verdict"))
> > +			goto cleanup;
> > +	}
> > +
> > +	buf = malloc(buflen);
> > +	if (!ASSERT_OK_PTR(buf, "malloc buf"))
> > +		goto cleanup;
> > +	memset(buf, 0, buflen);
> > +	buf[buflen - 1] = 'e';
> > +
> > +	for (int i = 0; i < 200; ++i) {
> 
> Also here. Why 200 iterations specifically? Can we at least name the
> defaults to make it clearer that we've chosen those numbers because
> that's how we trigger the bug?
> 
> > +		ssize_t sent;
> > +		char ignored[128];
> > +		pthread_t thread;
> > +		bool thread_created = false;
> > +		size_t rem = buflen;
> > +		int c = -1, p = -1, zero = 0;
> > +		bool success = false;
> > +
> > +		err = create_pair(AF_INET, SOCK_STREAM, &c, &p);
> > +		if (!ASSERT_OK(err, "create_pair"))
> > +			goto end_attempt;
> > +
> > +		err = pthread_create(&thread, NULL, &test_sockmap_recvfrom_eagain_thread, &p);
> > +		if (!ASSERT_OK(err, "pthread_create"))
> > +			goto end_attempt;
> > +		thread_created = true;
> > +
> > +		err = bpf_map_update_elem(map, &zero, &c, BPF_ANY);
> > +		if (!ASSERT_OK(err, "bpf_map_update_elem"))
> > +			goto end_attempt;
> > +
> > +		while (rem) {
> > +			sent = xsend(c, buf + (buflen - rem), rem, 0);
> > +			if (sent == -1)
> > +				goto end_attempt;
> > +			rem -= sent;
> > +		}
> > +
> > +		/* we cannot use recv_timeout(), otherwise EAGAIN would be an expected errno. */
> > +		err = recvfrom(c, ignored, sizeof(ignored), 0, NULL, NULL);
> > +
> > +		/*
> > +		 * we are checking for the invalid return of EAGAIN, any other return is considered
> > +		 * successful for the purposes of this test.
> > +		 */
> > +		if (err < 0 && !ASSERT_NEQ(errno, EAGAIN, "recvfrom eagain"))
> > +			goto end_attempt;
> > +
> > +		success = true;
> > +
> > +end_attempt:
> > +		if (c >= 0)
> > +			close(c);
> > +
> > +		if (thread_created) {
> > +			void *retval = NULL;
> > +
> > +			pthread_join(thread, &retval);
> > +			if (!ASSERT_NULL(retval, "retval"))
> > +				success = false;
> > +		}
> > +
> > +		if (!thread_created && p >= 0)
> > +			close(p);
> > +		if (!success)
> > +			break;
> > +	}
> > +
> > +cleanup:
> > +	if (buf)
> > +		free(buf);
> > +
> > +	test_sockmap_pass_prog__destroy(skel);
> > +}
> > +
> >  void test_sockmap_basic(void)
> >  {
> >  	if (test__start_subtest("sockmap create_update_free"))
> > @@ -1451,4 +1571,8 @@ void test_sockmap_basic(void)
> >  		test_sockmap_multi_channels(SOCK_STREAM);
> >  	if (test__start_subtest("sockmap udp multi channels"))
> >  		test_sockmap_multi_channels(SOCK_DGRAM);
> > +	if (test__start_subtest("sockmap recvfrom eagain"))
> > +		test_sockmap_recvfrom_eagain(false);
> > +	if (test__start_subtest("sockmap recvfrom eagain with verdict"))
> > +		test_sockmap_recvfrom_eagain(true);
> >  }
> 

Re: [PATCH v6 2/2] selftests/bpf: add sockmap recvfrom EAGAIN selftest

[Emil Tsalapatis]

On Mon Jul 20, 2026 at 6:07 PM EDT, Nnamdi Onyeyiri wrote:
> On Mon, Jul 20, 2026 at 05:47:09PM -0400, Emil Tsalapatis wrote:
>> On Mon Jul 20, 2026 at 1:15 PM EDT, Nnamdi Onyeyiri wrote:
>> > These selftests exercise the tcp_bpf_recvmsg() and tcp_bpf_recvmsg_parser()
>> > functions, to ensure that they are properly handling spurious wakeups in
>> > tcp_msg_wait_data().
>> >
>> > The expected behaviour is that recvfrom() does not return an EAGAIN
>> > error.  If the spurious wakeups are incorrectly handled, this assertion
>> > will fail.
>> >
>> > Signed-off-by: Nnamdi Onyeyiri <nnamdio@gmail.com>
>> 
>> The test looks fine, even if slightly flaky. Running with only patch 2/2
>> still passes sometimes on my box. Can we handle this somehow, e.g., do
>> more attempts?
>> 
>> There's also a couple magic numbers in the tests that may need some
>> explanation (noted below).
>>
>
> Thanks for the review!  I'll attach an explaination to the numbers.
> Essentially the larger the payload the fewer iterations seemed to be
> required to reproduce (on my machine).
>
> With the issue fixed though, larger payloads and more iterations make
> the test run longer.  Is there is a rule of thumb I should follow for
> tuning the runtime?

Unfortunately there's no fixed rule, I'd say the two requirements are:
a) The test should not be flakey, esp. false positives are a no-go.
b) The test shouldn't add noticeable latency to the testbench.

As it stands the test is fine, since the main issue is a false negative
(the test spuriously passes without the fix). That's still an issue, but
as long as the test without the fix properly fails the vast majority of
the time I think think it's fine.

>
>> > ---
>> >  .../selftests/bpf/prog_tests/sockmap_basic.c  | 124 ++++++++++++++++++
>> >  1 file changed, 124 insertions(+)
>> >
>> > diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
>> > index cb3229711f93..d18faf46fac0 100644
>> > --- a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
>> > +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
>> > @@ -1373,6 +1373,126 @@ static void test_sockmap_multi_channels(int sotype)
>> >  	test_sockmap_pass_prog__destroy(skel);
>> >  }
>> >  
>> > +static void *test_sockmap_recvfrom_eagain_thread(void *arg)
>> > +{
>> > +	int fd = *(int *)arg;
>> > +	char buf[1024];
>> > +	void *result = NULL;
>> > +
>> > +	while (true) {
>> > +		ssize_t len = recvfrom(fd, buf, sizeof(buf), 0, NULL, NULL);
>> > +
>> > +		if (len == -1) {
>> > +			if (errno == EINTR)
>> > +				continue;
>> > +			result = (void *)1;
>> > +			break;
>> > +		}
>> > +
>> > +		if (!len || buf[len - 1] == 'e')
>> > +			break;
>> > +	}
>> > +
>> > +	send(fd, "test", 4, MSG_NOSIGNAL);
>> > +
>> > +	close(fd);
>> > +
>> > +	return result;
>> > +}
>> > +
>> > +static void test_sockmap_recvfrom_eagain(bool with_verdict)
>> > +{
>> > +	struct test_sockmap_pass_prog *skel = NULL;
>> > +	struct bpf_program *prog = NULL;
>> > +	size_t buflen = 1024 * 1024 * 25;
>> 
>> Here
>> 
>> > +	char *buf = NULL;
>> > +	int map, err;
>> > +
>> > +	skel = test_sockmap_pass_prog__open_and_load();
>> > +	if (!ASSERT_OK_PTR(skel, "open_and_load"))
>> > +		return;
>> > +
>> > +	map = bpf_map__fd(skel->maps.sock_map_msg);
>> > +
>> > +	if (with_verdict) {
>> > +		prog = skel->progs.prog_skb_verdict;
>> > +		err = bpf_prog_attach(bpf_program__fd(prog), map, BPF_SK_SKB_STREAM_VERDICT, 0);
>> > +		if (!ASSERT_OK(err, "bpf_prog_attach verdict"))
>> > +			goto cleanup;
>> > +	}
>> > +
>> > +	buf = malloc(buflen);
>> > +	if (!ASSERT_OK_PTR(buf, "malloc buf"))
>> > +		goto cleanup;
>> > +	memset(buf, 0, buflen);
>> > +	buf[buflen - 1] = 'e';
>> > +
>> > +	for (int i = 0; i < 200; ++i) {
>> 
>> Also here. Why 200 iterations specifically? Can we at least name the
>> defaults to make it clearer that we've chosen those numbers because
>> that's how we trigger the bug?
>> 
>> > +		ssize_t sent;
>> > +		char ignored[128];
>> > +		pthread_t thread;
>> > +		bool thread_created = false;
>> > +		size_t rem = buflen;
>> > +		int c = -1, p = -1, zero = 0;
>> > +		bool success = false;
>> > +
>> > +		err = create_pair(AF_INET, SOCK_STREAM, &c, &p);
>> > +		if (!ASSERT_OK(err, "create_pair"))
>> > +			goto end_attempt;
>> > +
>> > +		err = pthread_create(&thread, NULL, &test_sockmap_recvfrom_eagain_thread, &p);
>> > +		if (!ASSERT_OK(err, "pthread_create"))
>> > +			goto end_attempt;
>> > +		thread_created = true;
>> > +
>> > +		err = bpf_map_update_elem(map, &zero, &c, BPF_ANY);
>> > +		if (!ASSERT_OK(err, "bpf_map_update_elem"))
>> > +			goto end_attempt;
>> > +
>> > +		while (rem) {
>> > +			sent = xsend(c, buf + (buflen - rem), rem, 0);
>> > +			if (sent == -1)
>> > +				goto end_attempt;
>> > +			rem -= sent;
>> > +		}
>> > +
>> > +		/* we cannot use recv_timeout(), otherwise EAGAIN would be an expected errno. */
>> > +		err = recvfrom(c, ignored, sizeof(ignored), 0, NULL, NULL);
>> > +
>> > +		/*
>> > +		 * we are checking for the invalid return of EAGAIN, any other return is considered
>> > +		 * successful for the purposes of this test.
>> > +		 */
>> > +		if (err < 0 && !ASSERT_NEQ(errno, EAGAIN, "recvfrom eagain"))
>> > +			goto end_attempt;
>> > +
>> > +		success = true;
>> > +
>> > +end_attempt:
>> > +		if (c >= 0)
>> > +			close(c);
>> > +
>> > +		if (thread_created) {
>> > +			void *retval = NULL;
>> > +
>> > +			pthread_join(thread, &retval);
>> > +			if (!ASSERT_NULL(retval, "retval"))
>> > +				success = false;
>> > +		}
>> > +
>> > +		if (!thread_created && p >= 0)
>> > +			close(p);
>> > +		if (!success)
>> > +			break;
>> > +	}
>> > +
>> > +cleanup:
>> > +	if (buf)
>> > +		free(buf);
>> > +
>> > +	test_sockmap_pass_prog__destroy(skel);
>> > +}
>> > +
>> >  void test_sockmap_basic(void)
>> >  {
>> >  	if (test__start_subtest("sockmap create_update_free"))
>> > @@ -1451,4 +1571,8 @@ void test_sockmap_basic(void)
>> >  		test_sockmap_multi_channels(SOCK_STREAM);
>> >  	if (test__start_subtest("sockmap udp multi channels"))
>> >  		test_sockmap_multi_channels(SOCK_DGRAM);
>> > +	if (test__start_subtest("sockmap recvfrom eagain"))
>> > +		test_sockmap_recvfrom_eagain(false);
>> > +	if (test__start_subtest("sockmap recvfrom eagain with verdict"))
>> > +		test_sockmap_recvfrom_eagain(true);
>> >  }
>> 

Re: [PATCH v6 1/2] bpf, sockmap: handle spurious tcp_msg_wait_data() wakeup

[Nnamdi Onyeyiri]

On Mon, Jul 20, 2026 at 05:16:08PM -0400, Emil Tsalapatis wrote:
> On Mon Jul 20, 2026 at 1:15 PM EDT, Nnamdi Onyeyiri wrote:
> > recvfrom()/recv() are documented as only returning EAGAIN for blocking sockets
> > when they have a receive timeout configured.  However, adding a blocking
> > ipv4 tcp socket without a receive timeout to a sockmap will cause EAGAIN errors
> > sporadically.  A socket with a receive timeout may return EAGAIN before the
> > timeout expires.
> >
> > There are 2 code paths affected by this:
> >
> >   1. tcp_bpf_recvmsg() - Used when the socket has been added to a sockmap
> >      that has no verdict program attached.
> >
> >   2. tcp_bpf_recvmsg_parser() - Used when the socket has been added to a
> >      sockmap that has a verdict program.  To reproduce this issue, it is
> >      enough for the verdict program to do nothing but return SK_PASS.
> >
> > In both cases this happens when tcp_msg_wait_data() wakes spuriously
> > (returning 0).  To fix it, we now loop back to msg_bytes_ready instead
> > of returning -EAGAIN on spurious wakeup.
> >
> > To ensure the looping does not cause sockets with a SO_RCVTIMEO set to
> > wait excessively long, tcp_msg_wait_data() now takes a pointer to timeo,
> > allowing sk_wait_event() to update it as appropriate.
> >
> > The logic in tcp_bpf_recvmsg_parser() that allow it to handle signals,
> > socket errors and closuers in its loop was also added to tcp_bpf_recvmsg().
> >
> > Signed-off-by: Nnamdi Onyeyiri <nnamdio@gmail.com>
> > ---
> >  net/ipv4/tcp_bpf.c | 69 ++++++++++++++++++++++++++++++++++++++++------
> >  1 file changed, 60 insertions(+), 9 deletions(-)
> >
> > diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
> > index cc0bd73f36b6..aa5c5d741599 100644
> > --- a/net/ipv4/tcp_bpf.c
> > +++ b/net/ipv4/tcp_bpf.c
> > @@ -179,7 +179,7 @@ EXPORT_SYMBOL_GPL(tcp_bpf_sendmsg_redir);
> >  
> >  #ifdef CONFIG_BPF_SYSCALL
> >  static int tcp_msg_wait_data(struct sock *sk, struct sk_psock *psock,
> > -			     long timeo)
> > +			     long *timeo)
> >  {
> >  	DEFINE_WAIT_FUNC(wait, woken_wake_function);
> >  	int ret = 0;
> > @@ -187,12 +187,12 @@ static int tcp_msg_wait_data(struct sock *sk, struct sk_psock *psock,
> >  	if (sk->sk_shutdown & RCV_SHUTDOWN)
> >  		return 1;
> >  
> > -	if (!timeo)
> > +	if (!*timeo)
> >  		return ret;
> >  
> >  	add_wait_queue(sk_sleep(sk), &wait);
> >  	sk_set_bit(SOCKWQ_ASYNC_WAITDATA, sk);
> > -	ret = sk_wait_event(sk, &timeo,
> > +	ret = sk_wait_event(sk, timeo,
> >  			    !list_empty(&psock->ingress_msg) ||
> >  			    !skb_queue_empty_lockless(&sk->sk_receive_queue), &wait);
> >  	sk_clear_bit(SOCKWQ_ASYNC_WAITDATA, sk);
> > @@ -229,6 +229,7 @@ static int tcp_bpf_recvmsg_parser(struct sock *sk,
> >  	int copied_from_self = 0;
> >  	int copied = 0;
> >  	u32 seq;
> > +	long timeo;
> >  
> >  	if (unlikely(flags & MSG_ERRQUEUE))
> >  		return inet_recv_error(sk, msg, len);
> > @@ -262,6 +263,8 @@ static int tcp_bpf_recvmsg_parser(struct sock *sk,
> >  		}
> >  	}
> >  
> > +	timeo = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);
> > +
> >  msg_bytes_ready:
> >  	copied = __sk_msg_recvmsg(sk, psock, msg, len, flags, &copied_from_self);
> >  	/* The typical case for EFAULT is the socket was gracefully
> > @@ -280,7 +283,6 @@ static int tcp_bpf_recvmsg_parser(struct sock *sk,
> >  	}
> >  	seq += copied_from_self;
> >  	if (!copied) {
> > -		long timeo;
> >  		int data;
> >  
> >  		if (sock_flag(sk, SOCK_DONE))
> > @@ -299,7 +301,6 @@ static int tcp_bpf_recvmsg_parser(struct sock *sk,
> >  			goto out;
> >  		}
> >  
> > -		timeo = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);
> >  		if (!timeo) {
> >  			copied = -EAGAIN;
> >  			goto out;
> > @@ -310,13 +311,15 @@ static int tcp_bpf_recvmsg_parser(struct sock *sk,
> >  			goto out;
> >  		}
> >  
> > -		data = tcp_msg_wait_data(sk, psock, timeo);
> > +		data = tcp_msg_wait_data(sk, psock, &timeo);
> >  		if (data < 0) {
> >  			copied = data;
> >  			goto unlock;
> >  		}
> >  		if (data && !sk_psock_queue_empty(psock))
> >  			goto msg_bytes_ready;
> > +		if (!data && timeo > 0)
> > +			goto msg_bytes_ready;
> >  		copied = -EAGAIN;
> >  	}
> >  out:
> > @@ -355,6 +358,7 @@ static int tcp_bpf_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
> >  {
> >  	struct sk_psock *psock;
> >  	int copied, ret;
> > +	long timeo;
> >  
> >  	if (unlikely(flags & MSG_ERRQUEUE))
> >  		return inet_recv_error(sk, msg, len);
> > @@ -371,14 +375,59 @@ static int tcp_bpf_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
> >  		return tcp_recvmsg(sk, msg, len, flags);
> >  	}
> >  	lock_sock(sk);
> > +
> > +	timeo = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);
> > +
> >  msg_bytes_ready:
> >  	copied = sk_msg_recvmsg(sk, psock, msg, len, flags);
> >  	if (!copied) {
> > -		long timeo;
> >  		int data;
> >  
> > -		timeo = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);
> > -		data = tcp_msg_wait_data(sk, psock, timeo);
> > +		if (sock_flag(sk, SOCK_DONE)) {
> > +			ret = 0;
> > +			goto unlock;
> > +		}
> > +
> > +		if (sk->sk_err) {
> > +			if (!sk_psock_queue_empty(psock))
> > +				goto msg_bytes_ready;
> > +			if (!skb_queue_empty(&sk->sk_receive_queue)) {
> > +				release_sock(sk);
> > +				sk_psock_put(sk, psock);
> > +				return tcp_recvmsg(sk, msg, len, flags);
> > +			}
> > +			ret = sock_error(sk);
> > +			goto unlock;
> > +		}
> > +
> > +		if (sk->sk_shutdown & RCV_SHUTDOWN) {
> > +			if (!sk_psock_queue_empty(psock))
> > +				goto msg_bytes_ready;
> > +			if (!skb_queue_empty(&sk->sk_receive_queue)) {
> > +				release_sock(sk);
> > +				sk_psock_put(sk, psock);
> > +				return tcp_recvmsg(sk, msg, len, flags);
> > +			}
> > +			ret = 0;
> > +			goto unlock;
> 
> These two error handling routines above look identical. Can you refactor
> them?
>

Will do.  My understanding is the same logic is needed to address the
issue Sashiko raised with the SOCK_DONE check as well.

> > +		}
> > +
> > +		if (sk->sk_state == TCP_CLOSE) {
> > +			ret = -ENOTCONN;
> > +			goto unlock;
> > +		}
> > +
> > +		if (!timeo) {
> > +			ret = -EAGAIN;
> > +			goto unlock;
> > +		}
> > +
> 
> Since this handling (which Sashiko flags by the way, correctly AFAICT) 
> are taken from tcp_bpf_recvmsg, there is obvious overlap between the two
> functions. Please factor those out so that they share the logic between
> them.
> 
> pw-bot: cr
> 

Sashiko highlighted the "if (!timeo)" and signal_pending early returns
when MSG_DONTWAIT is set, but I think I'm missing part of the picture.
By the time we reach these branches, haven't we already checked for data
in sk_receive_queue (line 372, after the patch is applied to 7.2-rc2)
[copied below for ease of viewing]:

    if (!skb_queue_empty(&sk->sk_receive_queue) &&
        sk_psock_queue_empty(psock)) {
        sk_psock_put(sk, psock);
        return tcp_recvmsg(sk, msg, len, flags);
    }

and in the psock (line 382) [again copied below for viewing]:

    copied = sk_msg_recvmsg(sk, psock, msg, len, flags);

> > +		if (signal_pending(current)) {
> > +			ret = sock_intr_errno(timeo);
> > +			goto unlock;
> > +		}
> > +
> > +		data = tcp_msg_wait_data(sk, psock, &timeo);
> >  		if (data < 0) {
> >  			ret = data;
> >  			goto unlock;
> > @@ -390,6 +439,8 @@ static int tcp_bpf_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
> >  			sk_psock_put(sk, psock);
> >  			return tcp_recvmsg(sk, msg, len, flags);
> >  		}
> > +		if (!data && timeo > 0)
> > +			goto msg_bytes_ready;
> >  		copied = -EAGAIN;
> >  	}
> >  	ret = copied;
> 

Re: [PATCH v6 1/2] bpf, sockmap: handle spurious tcp_msg_wait_data() wakeup

[Emil Tsalapatis]

On Mon Jul 20, 2026 at 6:53 PM EDT, Nnamdi Onyeyiri wrote:
> On Mon, Jul 20, 2026 at 05:16:08PM -0400, Emil Tsalapatis wrote:
>> On Mon Jul 20, 2026 at 1:15 PM EDT, Nnamdi Onyeyiri wrote:
>> > recvfrom()/recv() are documented as only returning EAGAIN for blocking sockets
>> > when they have a receive timeout configured.  However, adding a blocking
>> > ipv4 tcp socket without a receive timeout to a sockmap will cause EAGAIN errors
>> > sporadically.  A socket with a receive timeout may return EAGAIN before the
>> > timeout expires.
>> >
>> > There are 2 code paths affected by this:
>> >
>> >   1. tcp_bpf_recvmsg() - Used when the socket has been added to a sockmap
>> >      that has no verdict program attached.
>> >
>> >   2. tcp_bpf_recvmsg_parser() - Used when the socket has been added to a
>> >      sockmap that has a verdict program.  To reproduce this issue, it is
>> >      enough for the verdict program to do nothing but return SK_PASS.
>> >
>> > In both cases this happens when tcp_msg_wait_data() wakes spuriously
>> > (returning 0).  To fix it, we now loop back to msg_bytes_ready instead
>> > of returning -EAGAIN on spurious wakeup.
>> >
>> > To ensure the looping does not cause sockets with a SO_RCVTIMEO set to
>> > wait excessively long, tcp_msg_wait_data() now takes a pointer to timeo,
>> > allowing sk_wait_event() to update it as appropriate.
>> >
>> > The logic in tcp_bpf_recvmsg_parser() that allow it to handle signals,
>> > socket errors and closuers in its loop was also added to tcp_bpf_recvmsg().
>> >
>> > Signed-off-by: Nnamdi Onyeyiri <nnamdio@gmail.com>
>> > ---
>> >  net/ipv4/tcp_bpf.c | 69 ++++++++++++++++++++++++++++++++++++++++------
>> >  1 file changed, 60 insertions(+), 9 deletions(-)
>> >
>> > diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
>> > index cc0bd73f36b6..aa5c5d741599 100644
>> > --- a/net/ipv4/tcp_bpf.c
>> > +++ b/net/ipv4/tcp_bpf.c
>> > @@ -179,7 +179,7 @@ EXPORT_SYMBOL_GPL(tcp_bpf_sendmsg_redir);
>> >  
>> >  #ifdef CONFIG_BPF_SYSCALL
>> >  static int tcp_msg_wait_data(struct sock *sk, struct sk_psock *psock,
>> > -			     long timeo)
>> > +			     long *timeo)
>> >  {
>> >  	DEFINE_WAIT_FUNC(wait, woken_wake_function);
>> >  	int ret = 0;
>> > @@ -187,12 +187,12 @@ static int tcp_msg_wait_data(struct sock *sk, struct sk_psock *psock,
>> >  	if (sk->sk_shutdown & RCV_SHUTDOWN)
>> >  		return 1;
>> >  
>> > -	if (!timeo)
>> > +	if (!*timeo)
>> >  		return ret;
>> >  
>> >  	add_wait_queue(sk_sleep(sk), &wait);
>> >  	sk_set_bit(SOCKWQ_ASYNC_WAITDATA, sk);
>> > -	ret = sk_wait_event(sk, &timeo,
>> > +	ret = sk_wait_event(sk, timeo,
>> >  			    !list_empty(&psock->ingress_msg) ||
>> >  			    !skb_queue_empty_lockless(&sk->sk_receive_queue), &wait);
>> >  	sk_clear_bit(SOCKWQ_ASYNC_WAITDATA, sk);
>> > @@ -229,6 +229,7 @@ static int tcp_bpf_recvmsg_parser(struct sock *sk,
>> >  	int copied_from_self = 0;
>> >  	int copied = 0;
>> >  	u32 seq;
>> > +	long timeo;
>> >  
>> >  	if (unlikely(flags & MSG_ERRQUEUE))
>> >  		return inet_recv_error(sk, msg, len);
>> > @@ -262,6 +263,8 @@ static int tcp_bpf_recvmsg_parser(struct sock *sk,
>> >  		}
>> >  	}
>> >  
>> > +	timeo = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);
>> > +
>> >  msg_bytes_ready:
>> >  	copied = __sk_msg_recvmsg(sk, psock, msg, len, flags, &copied_from_self);
>> >  	/* The typical case for EFAULT is the socket was gracefully
>> > @@ -280,7 +283,6 @@ static int tcp_bpf_recvmsg_parser(struct sock *sk,
>> >  	}
>> >  	seq += copied_from_self;
>> >  	if (!copied) {
>> > -		long timeo;
>> >  		int data;
>> >  
>> >  		if (sock_flag(sk, SOCK_DONE))
>> > @@ -299,7 +301,6 @@ static int tcp_bpf_recvmsg_parser(struct sock *sk,
>> >  			goto out;
>> >  		}
>> >  
>> > -		timeo = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);
>> >  		if (!timeo) {
>> >  			copied = -EAGAIN;
>> >  			goto out;
>> > @@ -310,13 +311,15 @@ static int tcp_bpf_recvmsg_parser(struct sock *sk,
>> >  			goto out;
>> >  		}
>> >  
>> > -		data = tcp_msg_wait_data(sk, psock, timeo);
>> > +		data = tcp_msg_wait_data(sk, psock, &timeo);
>> >  		if (data < 0) {
>> >  			copied = data;
>> >  			goto unlock;
>> >  		}
>> >  		if (data && !sk_psock_queue_empty(psock))
>> >  			goto msg_bytes_ready;
>> > +		if (!data && timeo > 0)
>> > +			goto msg_bytes_ready;
>> >  		copied = -EAGAIN;
>> >  	}
>> >  out:
>> > @@ -355,6 +358,7 @@ static int tcp_bpf_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
>> >  {
>> >  	struct sk_psock *psock;
>> >  	int copied, ret;
>> > +	long timeo;
>> >  
>> >  	if (unlikely(flags & MSG_ERRQUEUE))
>> >  		return inet_recv_error(sk, msg, len);
>> > @@ -371,14 +375,59 @@ static int tcp_bpf_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
>> >  		return tcp_recvmsg(sk, msg, len, flags);
>> >  	}
>> >  	lock_sock(sk);
>> > +
>> > +	timeo = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);
>> > +
>> >  msg_bytes_ready:
>> >  	copied = sk_msg_recvmsg(sk, psock, msg, len, flags);
>> >  	if (!copied) {
>> > -		long timeo;
>> >  		int data;
>> >  
>> > -		timeo = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);
>> > -		data = tcp_msg_wait_data(sk, psock, timeo);
>> > +		if (sock_flag(sk, SOCK_DONE)) {
>> > +			ret = 0;
>> > +			goto unlock;
>> > +		}
>> > +
>> > +		if (sk->sk_err) {
>> > +			if (!sk_psock_queue_empty(psock))
>> > +				goto msg_bytes_ready;
>> > +			if (!skb_queue_empty(&sk->sk_receive_queue)) {
>> > +				release_sock(sk);
>> > +				sk_psock_put(sk, psock);
>> > +				return tcp_recvmsg(sk, msg, len, flags);
>> > +			}
>> > +			ret = sock_error(sk);
>> > +			goto unlock;
>> > +		}
>> > +
>> > +		if (sk->sk_shutdown & RCV_SHUTDOWN) {
>> > +			if (!sk_psock_queue_empty(psock))
>> > +				goto msg_bytes_ready;
>> > +			if (!skb_queue_empty(&sk->sk_receive_queue)) {
>> > +				release_sock(sk);
>> > +				sk_psock_put(sk, psock);
>> > +				return tcp_recvmsg(sk, msg, len, flags);
>> > +			}
>> > +			ret = 0;
>> > +			goto unlock;
>> 
>> These two error handling routines above look identical. Can you refactor
>> them?
>>
>
> Will do.  My understanding is the same logic is needed to address the
> issue Sashiko raised with the SOCK_DONE check as well.
>
>> > +		}
>> > +
>> > +		if (sk->sk_state == TCP_CLOSE) {
>> > +			ret = -ENOTCONN;
>> > +			goto unlock;
>> > +		}
>> > +
>> > +		if (!timeo) {
>> > +			ret = -EAGAIN;
>> > +			goto unlock;
>> > +		}
>> > +
>> 
>> Since this handling (which Sashiko flags by the way, correctly AFAICT) 
>> are taken from tcp_bpf_recvmsg, there is obvious overlap between the two
>> functions. Please factor those out so that they share the logic between
>> them.
>> 
>> pw-bot: cr
>> 
>
> Sashiko highlighted the "if (!timeo)" and signal_pending early returns
> when MSG_DONTWAIT is set, but I think I'm missing part of the picture.
> By the time we reach these branches, haven't we already checked for data
> in sk_receive_queue (line 372, after the patch is applied to 7.2-rc2)
> [copied below for ease of viewing]:
>
>     if (!skb_queue_empty(&sk->sk_receive_queue) &&
>         sk_psock_queue_empty(psock)) {
>         sk_psock_put(sk, psock);
>         return tcp_recvmsg(sk, msg, len, flags);
>     }
>
> and in the psock (line 382) [again copied below for viewing]:
>
>     copied = sk_msg_recvmsg(sk, psock, msg, len, flags);

If I'm understanding your question correctly, and AFAICT:
sk_msg_recvmsg only drains the psock out of sk_msg data. If any data is
still in the backing struct sock but has _not_ been drained into the psock
it gets missed under the new code.

>
>> > +		if (signal_pending(current)) {
>> > +			ret = sock_intr_errno(timeo);
>> > +			goto unlock;
>> > +		}
>> > +
>> > +		data = tcp_msg_wait_data(sk, psock, &timeo);
>> >  		if (data < 0) {
>> >  			ret = data;
>> >  			goto unlock;
>> > @@ -390,6 +439,8 @@ static int tcp_bpf_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
>> >  			sk_psock_put(sk, psock);
>> >  			return tcp_recvmsg(sk, msg, len, flags);
>> >  		}
>> > +		if (!data && timeo > 0)
>> > +			goto msg_bytes_ready;
>> >  		copied = -EAGAIN;
>> >  	}
>> >  	ret = copied;
>>