Re: [PATCH bpf] bpf, sockmap: fix BUG_ON in skb_to_sgvec() on a resized ingress skb

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Returning -EINVAL when an SKB is shrunk by a BPF program causes the `sk_psock_backlog` worker to incorrectly treat the valid shrink as a fatal error, leading to an unintended broken pipe (EPIPE).
--

commit 54e4eaf90ac6a727b56c416746a6a961673b62f5
Author: Sechang Lim <rhkrqnwk98@gmail.com>

bpf, sockmap: fix BUG_ON in skb_to_sgvec() on a resized ingress skb

This commit prevents a BUG_ON when a BPF program shrinks an ingress SKB.
It accomplishes this by clamping the length to skb->len - off and
returning -EINVAL if off is already past the SKB length.

> diff --git a/net/core/skmsg.c b/net/core/skmsg.c
> index e1850caf1a71a..2961178ebd1e1 100644
> --- a/net/core/skmsg.c
> +++ b/net/core/skmsg.c
> @@ -550,6 +550,10 @@ static int sk_psock_skb_ingress_enqueue(struct sk_buff *skb,
>  {
>  	int num_sge, copied;
>  
> +	if (off >= skb->len)
> +		return -EINVAL;
> +	len = min_t(u32, len, skb->len - off);
> +

[Severity: High]
Could returning -EINVAL here cause the sk_psock_backlog() worker to
incorrectly treat a valid SKB shrink as a permanent error?

If an SKB is queued to the backlog worker, sk_psock_backlog() uses a
do { ... } while (len) loop to process stm->full_len:

net/core/skmsg.c:sk_psock_backlog()
    ...
    do {
        ret = -EIO;
        if (!sock_flag(psock->sk, SOCK_DEAD))
            ret = sk_psock_handle_skb(psock, skb, off, len, ingress);
        if (ret <= 0) {
            if (ret == -EAGAIN) {
                ...
            }
            /* Hard errors break pipe and stop xmit. */
            sk_psock_report_error(psock, ret ? -ret : EPIPE);
            sk_psock_clear_state(psock, SK_PSOCK_TX_ENABLED);
            goto end;
        }
        off += ret;
        len -= ret;
    } while (len);

If a BPF program shrinks an SKB so skb->len is 50, but stm->full_len
(len) is 100 and off is 0, the first loop iteration will process the
clamped 50 bytes.

On the second iteration, the loop passes off=50 and len=50 to
sk_psock_handle_skb(). This evaluates if (off >= skb->len) and returns
-EINVAL. The backlog worker then treats this as a hard error, calling
sk_psock_report_error() and permanently destroying the socket's TX state.

Additionally, is there an inconsistency in how sk_psock_verdict_apply()
handles this clamped length?

net/core/skmsg.c:sk_psock_verdict_apply()
    ...
    err = sk_psock_skb_ingress_self(psock, skb, off, len, false);
    if (err < 0) {
    ...

If the same shrunk SKB is processed directly by sk_psock_verdict_apply(),
the clamped positive length is treated as a success. The remaining bytes
of stm->full_len are ignored without error, rather than dropping the SKB.

This seems to create a situation where the behavior (silent truncation vs
broken pipe) depends entirely on backlog queue timing.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260613082442.3252576-1-rhkrqnwk98@gmail.com?part=1