BPF List
 help / color / mirror / Atom feed
* [PATCH v2] bpf, sockmap: Fix self-redirect copied_seq double-counting
@ 2026-09-05  7:03 Geliang Tang
  2026-09-07  1:34 ` Jiayuan Chen
  0 siblings, 1 reply; 5+ messages in thread
From: Geliang Tang @ 2026-09-05  7:03 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 converting self-redirect verdict to __SK_PASS at the
beginning of sk_psock_verdict_apply(). This bypasses the
__SK_REDIRECT case entirely (which calls sk_psock_eat_skb), letting
the __SK_PASS path queue the skb to the psock ingress queue. The
data is then read via tcp_bpf_recvmsg_parser(), which advances
copied_seq exactly once through copied_from_self. Cross-socket
redirects continue through __SK_REDIRECT with sk_psock_eat_skb()
unchanged.

Fixes: e5c6de5fa025 ("bpf, sockmap: Incorrectly handling copied_seq")
Suggested-by: Jakub Sitnicki <jakub@cloudflare.com>
Suggested-by: Jiayuan Chen <jiayuan.chen@linux.dev>
Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
v2:
 - fixup the verdict as Jakub and Jiayuan suggested.

v1:
 - https://patchwork.kernel.org/project/netdevbpf/patch/b840c35fdfdf36e9fddedfa645b12699bc51aa34.1787968065.git.tanggeliang@kylinos.cn/
---
 net/core/skmsg.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/core/skmsg.c b/net/core/skmsg.c
index 2521b643fa05..df385a5a961e 100644
--- a/net/core/skmsg.c
+++ b/net/core/skmsg.c
@@ -1000,6 +1000,10 @@ static int sk_psock_verdict_apply(struct sk_psock *psock, struct sk_buff *skb,
 	int err = 0;
 	u32 len, off;
 
+	if (verdict == __SK_REDIRECT && skb_bpf_ingress(skb) &&
+	    skb_bpf_redirect_fetch(skb) == psock->sk)
+		verdict = __SK_PASS;
+
 	switch (verdict) {
 	case __SK_PASS:
 		err = -EIO;
-- 
2.53.0


^ permalink raw reply related	[flat|nested] 5+ messages in thread
* Re: [PATCH v2] bpf, sockmap: Fix self-redirect copied_seq double-counting
@ 2026-09-09 15:06 netdev-bot+sashiko
  0 siblings, 0 replies; 5+ messages in thread
From: netdev-bot+sashiko @ 2026-09-09 15:06 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: 0 · Medium: 1 (1) · Low: 1 (1)

- [Medium] sk_psock_verdict_apply() is the verdict path for every sockmap
  protocol that implements ->read_skb (TCP, AF_UNIX, vsock, UDP), not…
- [Low] The changelog names a function that does not exist.

Pre-existing issues:
- [Medium] psock->ingress_bytes is a plain non-atomic u32 that is
  incremented in sk_psock_skb_ingress_enqueue() (net/core/skmsg.c:575)…
- [Low] Reported concern: routing an ingress self-redirect through `case
  __SK_PASS` only balances copied_seq when the inline…

--

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

The patch rewrites an ingress self-redirect verdict into __SK_PASS at the
top of sk_psock_verdict_apply() so the skb goes to the psock ingress queue
instead of through the __SK_REDIRECT arm. The intent is that copied_seq is
advanced only once, by the copied_from_self accounting in
tcp_bpf_recvmsg_parser().

A few questions on the changelog and on the scope of the new check.

The changelog says:

  "This bypasses the __SK_REDIRECT case entirely (which calls
   sk_psock_eat_skb), letting the __SK_PASS path queue the skb to the
   psock ingress queue."

and:

  "Cross-socket redirects continue through __SK_REDIRECT with
   sk_psock_eat_skb() unchanged."

[Severity: Low]
Is sk_psock_eat_skb() the intended name here? A grep of the tree finds no
such symbol. The helper actually invoked in the __SK_REDIRECT arm of
sk_psock_verdict_apply(), and in the drop arm, is tcp_eat_skb() from
net/ipv4/tcp_bpf.c, which the first paragraph of the same changelog names
correctly. Since this carries a Fixes: tag and will likely be backported,
could the name be corrected to tcp_eat_skb()?

> diff --git a/net/core/skmsg.c b/net/core/skmsg.c
> index 2521b643fa05d..df385a5a961e8 100644
> --- a/net/core/skmsg.c
> +++ b/net/core/skmsg.c
> @@ -1000,6 +1000,10 @@ static int sk_psock_verdict_apply(struct sk_psock *psock, struct sk_buff *skb,
>  	int err = 0;
>  	u32 len, off;
>  
> +	if (verdict == __SK_REDIRECT && skb_bpf_ingress(skb) &&
> +	    skb_bpf_redirect_fetch(skb) == psock->sk)
> +		verdict = __SK_PASS;
> +

[Severity: Medium]
Should this remap be gated on sk_is_tcp(psock->sk)?

sk_psock_verdict_apply() is reached from every protocol that implements
->read_skb, not only TCP: unix_read_skb() and the vsock/UDP equivalents all
funnel into sk_psock_verdict_recv() -> sk_psock_verdict_apply(). For those
protocols there is no copied_seq to fix, since tcp_eat_skb() returns
immediately for !sk_is_tcp(sk).

For AF_UNIX the skb owner is the sender socket (set by
sock_alloc_send_pskb()), so before this patch a self-redirect took:

  __SK_REDIRECT
    sk_psock_skb_redirect()      -> psock->ingress_skb
      sk_psock_backlog()
        sk_psock_skb_ingress()

and there the "if (unlikely(skb->sk == sk))" shortcut is not taken, so
sk_psock_create_ingress_msg() ran its admission test:

net/core/skmsg.c:sk_psock_create_ingress_msg() {
	if (atomic_read(&sk->sk_rmem_alloc) > sk->sk_rcvbuf)
		return NULL;

	if (!sk_rmem_schedule(sk, skb, skb->truesize))
		return NULL;
	...
}

With the remap, the same traffic enters case __SK_PASS and, whenever
psock->ingress_skb is empty (now the steady state, since nothing is queued
there any more), goes straight to sk_psock_skb_ingress_self(), which only
does skb_set_owner_r() and performs no sk_rcvbuf or sk_rmem_schedule()
check.

Can psock->ingress_msg then grow past SO_RCVBUF for AF_UNIX/vsock
self-redirect traffic, with the sender un-throttled once skb_orphan() inside
skb_set_owner_r() drops its wmem charge?

The changelog describes the change as TCP copied_seq accounting only, so the
non-TCP behaviour change is not mentioned anywhere. Could the condition be
scoped to the protocol that actually has the defect?

[Severity: Low]
This is a pre-existing issue rather than something introduced here, but the
remap newly routes self-redirected data into it, so it may be worth a note.

Routing the self-redirect through case __SK_PASS only balances copied_seq
when the inline sk_psock_skb_ingress_self() enqueue succeeds. When
psock->ingress_skb is non-empty the skb is merely queued and
psock->ingress_bytes is not incremented during the read pass, while
tcp_bpf_strp_read_sock() still commits:

net/ipv4/tcp_bpf.c:tcp_bpf_strp_read_sock() {
	tp->copied_seq = psock->copied_seq - psock->ingress_bytes;
	...
	__tcp_cleanup_rbuf(sk, copied - psock->ingress_bytes);
}

That common deferral looks balanced, because sk_psock_strp_read() clears the
owner before applying the verdict:

net/core/skmsg.c:sk_psock_strp_read() {
		ret = sk_psock_map_verd(ret, skb_bpf_redirect_fetch(skb));
		skb->sk = NULL;
	}
	sk_psock_verdict_apply(psock, skb, ret);

so the worker later misses the "skb->sk == sk" shortcut in
sk_psock_skb_ingress(), msg->sk stays NULL, and "from_self = msg_rx->sk ==
sk" in __sk_msg_recvmsg() is false.

The one case that does not balance is when sk_psock_skb_ingress_self() has
already run skb_set_owner_r() (making skb->sk == psock->sk) and then fails
in sk_psock_skb_ingress_enqueue() because skb_to_sgvec() exceeds
MAX_MSG_FRAGS and skb_linearize() returns -ENOMEM. The deferred skb then
does take the self branch, msg->sk is set, and copied_from_self advances
bytes that tp->copied_seq already covers, giving the same warnings quoted in
the changelog. The identical window exists for ordinary __SK_PASS strparser
traffic today.

[Severity: Medium]
This isn't a bug introduced by this patch either, but since the remap makes
the socket's own data depend on this counter in strparser mode, is the
accounting of psock->ingress_bytes safe?

It is a plain u32 with a non-atomic increment in
sk_psock_skb_ingress_enqueue():

net/core/skmsg.c:sk_psock_skb_ingress_enqueue() {
#if IS_ENABLED(CONFIG_BPF_STREAM_PARSER)
	psock->ingress_bytes += len;
#endif
}

That site is reached both with the socket lock held (strp_read_sock() ->
tcp_bpf_strp_read_sock() -> sk_psock_strp_read() ->
sk_psock_verdict_apply()) and from sk_psock_backlog(), which runs on the
workqueue holding only psock->work_mutex and never takes lock_sock().

CPU1 (socket lock held)
tcp_bpf_strp_read_sock()
    psock->ingress_bytes = 0;
    tcp_read_sock_noack(...);
    tp->copied_seq = psock->copied_seq - psock->ingress_bytes;
    __tcp_cleanup_rbuf(sk, copied - psock->ingress_bytes);

CPU0 (work_mutex only), draining an skb another socket redirected in via
sk_psock_skb_redirect(), whose bytes were never part of psock->copied_seq:

sk_psock_backlog()
    sk_psock_handle_skb()
        sk_psock_skb_ingress_enqueue()
            psock->ingress_bytes += len;

If the worker increment lands inside that window, does tp->copied_seq end up
subtracting foreign bytes, and can "copied - psock->ingress_bytes" go
negative? There is no lock, atomic, or READ_ONCE/WRITE_ONCE pairing on this
field at any of its three access sites.

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

^ permalink raw reply	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2026-09-09 15:06 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-09-05  7:03 [PATCH v2] bpf, sockmap: Fix self-redirect copied_seq double-counting Geliang Tang
2026-09-07  1:34 ` Jiayuan Chen
2026-09-07 10:19   ` Geliang Tang
2026-09-07 10:52     ` Jiayuan Chen
  -- strict thread matches above, loose matches on Subject: below --
2026-09-09 15:06 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