BPF List
 help / color / mirror / Atom feed
* [PATCH] bpf, sockmap: Fix self-redirect copied_seq double-counting
@ 2026-08-29  2:00 Geliang Tang
  2026-08-29  2:15 ` sashiko-bot
  2026-09-01 11:38 ` Jakub Sitnicki
  0 siblings, 2 replies; 6+ messages in thread
From: Geliang Tang @ 2026-08-29  2:00 UTC (permalink / raw)
  To: John Fastabend, Jakub Sitnicki, Jiayuan Chen, David S. Miller,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni, Simon Horman,
	Daniel Borkmann
  Cc: Geliang Tang, netdev, bpf

From: Geliang Tang <tanggeliang@kylinos.cn>

When a BPF stream_verdict program redirects an skb back to the same
socket (self-redirect with BPF_F_INGRESS), sk_psock_verdict_apply()
calls tcp_eat_skb() which advances tcp_sk->copied_seq. However, the
skb is then delivered to the socket's psock ingress queue and later
read by tcp_bpf_recvmsg_parser(), which also advances copied_seq via
the copied_from_self accounting path. This double-counting causes
copied_seq to advance by 2x the actual data length, triggering:

  TCP recvmsg seq # bug 2: copied BF2E806, seq BF2E7FD, \
			   rcvnxt BF2E806, fl 0
  WARNING: net/ipv4/tcp.c:2745 at tcp_recvmsg_locked+0x72b/0x2640
  Call Trace:
   tcp_recvmsg+0x10a/0x500
   sock_recvmsg+0x168/0x1d0
   __sys_recvfrom+0x19a/0x2a0
   __x64_sys_recvfrom+0xe4/0x1f0
   do_syscall_64+0xf7/0x530
   entry_SYSCALL_64_after_hwframe+0x77/0x7f

  cleanup rbuf bug: copied BF2E806 seq BF2E806 rcvnxt BF2E806
  WARNING: net/ipv4/tcp.c:1609 at tcp_cleanup_rbuf+0xf2/0x1c0
  Call Trace:
   tcp_recvmsg_locked+0x8d1/0x2640
   tcp_recvmsg+0x10a/0x500
   sock_recvmsg+0x168/0x1d0
   __sys_recvfrom+0x19a/0x2a0
   __x64_sys_recvfrom+0xe4/0x1f0
   do_syscall_64+0xf7/0x530
   entry_SYSCALL_64_after_hwframe+0x77/0x7f

Fix this by checking if the redirect destination is the same socket.
For self-redirect (dst == psock->sk), skip tcp_eat_skb() since the
copied_seq will be advanced when the data is actually read from the
ingress queue. For cross-socket redirects, tcp_eat_skb() is still
needed to account for data leaving the source socket.

Fixes: e5c6de5fa025 ("bpf, sockmap: Incorrectly handling copied_seq")
Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
Hi,

I encountered this while adding MPTCP BPF sockmap support. The existing
TCP sockmap selftests don't cover self-redirect, but the MPTCP tests do,
exposing this latent issue.

With this fix, both TCP and MPTCP tests pass, validating self-redirect
functionality.
---
 net/core/skmsg.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/net/core/skmsg.c b/net/core/skmsg.c
index 2521b643fa05..5fa7b9639eef 100644
--- a/net/core/skmsg.c
+++ b/net/core/skmsg.c
@@ -1039,10 +1039,14 @@ static int sk_psock_verdict_apply(struct sk_psock *psock, struct sk_buff *skb,
 				goto out_free;
 		}
 		break;
-	case __SK_REDIRECT:
-		tcp_eat_skb(psock->sk, skb);
+	case __SK_REDIRECT: {
+		struct sock *dst = skb_bpf_redirect_fetch(skb);
+
+		if (dst != psock->sk)
+			tcp_eat_skb(psock->sk, skb);
 		err = sk_psock_skb_redirect(psock, skb);
 		break;
+	}
 	case __SK_DROP:
 	default:
 out_free:
-- 
2.53.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] bpf, sockmap: Fix self-redirect copied_seq double-counting
@ 2026-09-04  3:03 netdev-bot+sashiko
  0 siblings, 0 replies; 6+ messages in thread
From: netdev-bot+sashiko @ 2026-09-04  3:03 UTC (permalink / raw)
  To: geliang
  Cc: john.fastabend, jakub, jiayuan.chen, davem, edumazet, kuba,
	pabeni, horms, daniel, tanggeliang, netdev, bpf

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:

Critical: 0 · High: 2 · Medium: 0 · Low: 0

- [High] net/core/skmsg.c:1042-1047 (sk_psock_verdict_apply(),
  __SK_REDIRECT arm): the new self-redirect test compares only the…
- [High] net/core/skmsg.c:1042-1047 (sk_psock_verdict_apply()): for a
  self-redirect the patch drops the up-front tcp_eat_skb() and relies on…

--

bpf, sockmap: Fix self-redirect copied_seq double-counting

This changes the __SK_REDIRECT arm of sk_psock_verdict_apply() so that
tcp_eat_skb() is only called when the redirect destination differs from
psock->sk.  The intent is to avoid advancing tcp_sk->copied_seq twice when
a stream_verdict program redirects an skb back into the same socket's
ingress queue.

> diff --git a/net/core/skmsg.c b/net/core/skmsg.c
> index 2521b643fa05..5fa7b9639eef 100644
> --- a/net/core/skmsg.c
> +++ b/net/core/skmsg.c
> @@ -1039,10 +1039,14 @@ static int sk_psock_verdict_apply(struct sk_psock *psock, struct sk_buff *skb,
>  				goto out_free;
>  		}
>  		break;
> -	case __SK_REDIRECT:
> -		tcp_eat_skb(psock->sk, skb);
> +	case __SK_REDIRECT: {
> +		struct sock *dst = skb_bpf_redirect_fetch(skb);
> +
> +		if (dst != psock->sk)
> +			tcp_eat_skb(psock->sk, skb);

[Severity: High]
The commit message describes the problem as "self-redirect with
BPF_F_INGRESS", but this test only compares the destination socket and
never looks at the direction.  Does the same-socket *egress* case end up
here too?

skb_bpf_redirect_fetch() in include/linux/skmsg.h masks the flag bits off:

  static inline struct sock *skb_bpf_redirect_fetch(struct sk_buff *skb)
  {
	unsigned long sk_redir = skb->_sk_redir;

	return (struct sock *)(sk_redir & BPF_F_PTR_MASK);
  }

and bpf_sk_redirect_map() in net/core/sock_map.c stores the direction
separately, allowing flags == 0:

	if (unlikely(flags & ~(BPF_F_INGRESS)))
		return SK_DROP;
	...
	skb_bpf_set_redir(skb, sk, flags & BPF_F_INGRESS);

So a program doing bpf_sk_redirect_map(skb, &sock_map, idx, 0) where
sock_map[idx] resolves to the source socket satisfies dst == psock->sk and
skips tcp_eat_skb().

For that egress case nothing later advances copied_seq.  sk_psock_backlog()
computes:

		ingress = skb_bpf_ingress(skb);

which is false, so sk_psock_handle_skb() takes the transmit path:

		return skb_send_sock(psock->sk, skb, off, len);

and the skb is freed.  It never becomes an sk_msg with msg->sk == sk, so
the from_self accounting in __sk_msg_recvmsg():

		from_self = msg_rx->sk == sk;

never runs, and neither does the copied_from_self path in
tcp_bpf_recvmsg_parser() that the commit message relies on.

Meanwhile tcp_read_skb() already removed the bytes from the receive queue
without touching tp->copied_seq:

		__skb_unlink(skb, &sk->sk_receive_queue);

Can this leave tp->copied_seq permanently behind tp->rcv_nxt, so that
SIOCINQ reports data that no longer exists, __tcp_cleanup_rbuf() and
tcp_rcv_space_adjust() never run for those bytes, tcp_close() sees
data_was_unread != 0 and sends an RST instead of a FIN, and a later plain
tcp_recvmsg_locked() (after the psock is dropped) trips

	WARN(before(*seq, TCP_SKB_CB(skb)->seq), "TCP recvmsg seq # bug...

that is, the same copied_seq desync in the opposite direction?

Worth noting that the program cited as validation,
tools/testing/selftests/bpf/progs/mptcp_sockmap.c, uses exactly the egress
form:

	return bpf_sk_redirect_map(skb, &sock_map, redirect_idx, 0);

Would adding the direction to the condition cover this, e.g.

	if (dst != psock->sk || !skb_bpf_ingress(skb))
		tcp_eat_skb(psock->sk, skb);

[Severity: High]
A second question about the same condition: the accounting is now skipped
before the outcome of the redirect is known.  Is it guaranteed that the skb
really reaches ingress_msg and is read by tcp_bpf_recvmsg_parser()?

sk_psock_skb_redirect() has drop paths that free the skb and return -EIO
without any copied_seq update, and for a self-redirect psock_other is this
very same psock:

  net/core/skmsg.c:sk_psock_skb_redirect() {
	...
	if (!psock_other || sock_flag(sk_other, SOCK_DEAD)) {
		skb_bpf_redirect_clear(skb);
		sock_drop(from->sk, skb);
		return -EIO;
	}
	spin_lock_bh(&psock_other->ingress_lock);
	if (!sk_psock_test_state(psock_other, SK_PSOCK_TX_ENABLED)) {
		spin_unlock_bh(&psock_other->ingress_lock);
		skb_bpf_redirect_clear(skb);
		sock_drop(from->sk, skb);
		return -EIO;
	}
	...
  }

SK_PSOCK_TX_ENABLED can already be clear while sk->sk_data_ready is still
sk_psock_verdict_data_ready, because sk_psock_backlog() clears it on a hard
transmit/ingress error:

			if (ret == -EAGAIN) {
				...
			} else {
				sk_psock_clear_state(psock, SK_PSOCK_TX_ENABLED);
				...
			}

and sk_psock_stop() clears it during close/map-delete before the socket
backlog is drained.  The __SK_PASS arm just above tests the same two states
(SOCK_DEAD and !SK_PSOCK_TX_ENABLED), so they appear reachable at this
point.

Even after a successful skb_queue_tail(), skbs still on ingress_skb are
purged without accounting:

  net/core/skmsg.c:__sk_psock_zap_ingress() {
	while ((skb = skb_dequeue(&psock->ingress_skb)) != NULL) {
		skb_bpf_redirect_clear(skb);
		sock_drop(psock->sk, skb);
	}
	...
  }

and __sk_psock_purge_ingress_msg() frees queued sk_msgs the same way.

In all of these cases tcp_read_skb() has already unlinked the bytes from
sk->sk_receive_queue, and tcp_eat_skb() is the only place that catches up:

  net/ipv4/tcp_bpf.c:tcp_eat_skb() {
	copied = tcp->copied_seq + skb->len;
	WRITE_ONCE(tcp->copied_seq, copied);
	tcp_rcv_space_adjust(sk);
	__tcp_cleanup_rbuf(sk, skb->len);
  }

Note the sibling drop arm in this same switch keeps that contract:

  out_free:
		skb_bpf_redirect_clear(skb);
		tcp_eat_skb(psock->sk, skb);
		sock_drop(psock->sk, skb);

Should the skipped accounting be performed on the redirect-failure and
purge paths as well, or should tcp_eat_skb() only be skipped once the skb
is known to have been retained for same-socket ingress consumption?

>  		err = sk_psock_skb_redirect(psock, skb);
>  		break;
> +	}
>  	case __SK_DROP:
>  	default:
>  out_free:

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/b840c35fdfdf36e9fddedfa645b12699bc51aa34.1787968065.git.tanggeliang%40kylinos.cn

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2026-09-04 14:11 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-08-29  2:00 [PATCH] bpf, sockmap: Fix self-redirect copied_seq double-counting Geliang Tang
2026-08-29  2:15 ` sashiko-bot
2026-09-01 11:38 ` Jakub Sitnicki
2026-09-01 13:00   ` Jiayuan Chen
2026-09-04 14:11     ` Geliang Tang
  -- strict thread matches above, loose matches on Subject: below --
2026-09-04  3:03 netdev-bot+sashiko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox