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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Inconsistent TCP sequence accounting (copied_seq) for BPF stream_verdict self-redirects depending on ingress queue state.
--

commit 1c84437e57670b82a168813bb40bf5ab64fc2722
Author: Geliang Tang <geliang@kernel.org>

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

This commit attempts to fix a copied_seq double-counting issue by
converting a self-redirect verdict to __SK_PASS in
sk_psock_verdict_apply(). This bypasses the __SK_REDIRECT case entirely
and lets the __SK_PASS path queue the skb to the psock ingress queue.

> 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: High]
Does converting this to __SK_PASS introduce inconsistent TCP sequence
accounting (copied_seq) depending on the ingress queue state?

When the queue is empty, sk_psock_verdict_apply() calls
sk_psock_skb_ingress_self() directly:

net/core/skmsg.c:sk_psock_verdict_apply() {
    ...
    if (skb_queue_empty(&psock->ingress_skb)) {
        ...
        err = sk_psock_skb_ingress_self(psock, skb, off, len, false);
    ...
}

This successfully creates a message with msg->sk = sk, which allows
tcp_bpf_recvmsg_parser() to correctly advance copied_seq.

However, if the ingress queue is not empty, the skb is queued for the backlog
worker. When backlog processing later reaches sk_psock_skb_ingress():

net/core/skmsg.c:sk_psock_skb_ingress() {
    ...
    if (unlikely(skb->sk == sk))
        return sk_psock_skb_ingress_self(psock, skb, off, len, true);
    msg = sk_psock_create_ingress_msg(sk, skb);
    ...
}

Since sk_psock_strp_read() sets skb->sk = NULL before verdict application,
this check fails. The resulting msg has a NULL sk, bypassing copied_seq
advancement entirely.

Could this failure to advance copied_seq lead to TCP window exhaustion and
cause the connection to stall (a Denial of Service for the socket),
essentially replacing the 2x over-counting bug with a 0x under-counting
bug?

>  	switch (verdict) {
>  	case __SK_PASS:
>  		err = -EIO;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/1a8e797a1b26e2f695aaac22ac644c2862f63466.1788858299.git.tanggeliang@kylinos.cn?part=1