Re: [PATCH mptcp-next v4 3/5] mptcp: implement read_skb for sockmap stream verdict

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Calling skb_pull() with an unvalidated offset on potentially non-linear SKBs causes a kernel BUG() panic.
- [High] Data consumed by the BPF stream verdict program does not trigger MPTCP receive window updates or buffer auto-tuning.
- [High] Error paths drop SKBs without advancing consumption state, leading to receive window leaks and deadlocks.
--

--- Patch [3]: [PATCH mptcp-next v4 3/5] mptcp: implement read_skb for sockmap stream verdict ---
commit b365e36f46ed4c92c3a7c83b8b32cf1113c12bbf
Author: Geliang Tang <geliang@kernel.org>
mptcp: implement read_skb for sockmap stream verdict

This commit adds mptcp_read_skb() to allow BPF sockmap stream verdict
programs to process MPTCP sockets. It peeks skbs from the receive queue,
adjusts offsets with skb_pull(), unlinks them, and passes them to the BPF
program while updating the bytes_consumed counter.

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 265a07e73a529..c1e26c6e5c17b 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c

[ ... ]

> @@ -4780,6 +4780,36 @@ static ssize_t mptcp_splice_read(struct socket *sock, loff_t *ppos,
>  	return ret;
>  }
>  
> +static int mptcp_read_skb(struct sock *sk, skb_read_actor_t recv_actor)
> +{
> +	struct mptcp_sock *msk = mptcp_sk(sk);
> +	struct sk_buff *skb;
> +	int copied = 0;
> +	u32 offset;
> +
> +	if (sk->sk_state == TCP_LISTEN)
> +		return -ENOTCONN;
> +
> +	while ((skb = mptcp_recv_skb(sk, &offset)) != NULL) {
> +		int used;
> +
> +		if (offset)
> +			skb_pull(skb, offset);

[Severity: Critical]
Does this risk a kernel panic? If the socket buffer in the receive queue is
non-linear (such as from GRO on the TCP subflow or page frags), and the
offset is greater than the linear part of the buffer (skb_headlen), calling
skb_pull() without first linearizing it can cause length underflows.

If skb->len drops below skb->data_len, it will trigger the BUG_ON() in
skb_pull_inline().

Should pskb_may_pull() be used here to ensure the bytes are in the linear
region first?

> +
> +		__skb_unlink(skb, &sk->sk_receive_queue);
> +		WARN_ON_ONCE(!skb_set_owner_sk_safe(skb, sk));
> +		used = recv_actor(sk, skb);
> +		if (used < 0) {
> +			if (!copied)
> +				copied = used;
> +			break;
> +		}

[Severity: High]
If the recv_actor returns an error and drops the buffer, this breaks the
loop without adding the dropped bytes to bytes_consumed.

Since tcp_eat_skb() is a no-op for MPTCP sockets, does this leave
bytes_consumed permanently out of sync with bytes_received? This might lead
to a permanent loss of receive window capacity and eventual connection
stalls.

> +		copied += used;
> +		msk->bytes_consumed += used;

[Severity: High]
Should this update also trigger receive window updates and buffer
auto-tuning?

After advancing bytes_consumed, omitting calls to mptcp_cleanup_rbuf() and
mptcp_rcv_space_adjust() means MPTCP will not proactively send window update
ACKs when the window opens.

The sender might eventually stall and rely on slow zero window probes to
discover the new window.

> +	}
> +	return copied;
> +}

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1787554581.git.tanggeliang@kylinos.cn?part=3