Re: [PATCH v6 1/2] bpf, sockmap: handle spurious tcp_msg_wait_data() wakeup
From: Emil Tsalapatis
Date: Mon Jul 20 2026 - 17:18:02 EST
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@xxxxxxxxx>
> ---
> 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;