Re: [PATCH v2] bpf, sockmap: Fix self-redirect copied_seq double-counting

[netdev-bot+sashiko@kernel.org]

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