Re: [PATCH mptcp-next v15 05/11] mptcp: defer read_sock cleanup to mptcp_worker

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] mptcp: msk->read_copied overflow in __mptcp_read_sock() skips rbuf cleanup
--

commit dbba5fc9642f463a94cfb4b42a591e715b089eca
Author: Paolo Abeni <pabeni@redhat.com>

mptcp: defer read_sock cleanup to mptcp_worker

This commit defers mptcp_rcv_space_adjust() and mptcp_cleanup_rbuf() from
__mptcp_read_sock() to mptcp_worker() by accumulating consumed bytes in
msk->read_copied and scheduling work with MPTCP_WORK_READ_COMPLETE.

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 01eb7fb314e76..3c3ad8991799a 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -3149,6 +3149,21 @@ static void mptcp_backlog_purge(struct sock *sk)
>  	sk_mem_reclaim(sk);
>  }
>  
> +static void mptcp_read_complete(struct sock *sk)
> +{
> +	struct mptcp_sock *msk = mptcp_sk(sk);
> +	int read_copied;
> +
> +	mptcp_data_lock(sk);
> +	read_copied = msk->read_copied;
> +	msk->read_copied = 0;
> +	mptcp_data_unlock(sk);
> +
> +	mptcp_rcv_space_adjust(msk, read_copied);
> +	if (read_copied > 0)
> +		mptcp_cleanup_rbuf(msk, read_copied);
> +}

[ ... ]

> @@ -4696,11 +4712,14 @@ static int __mptcp_read_sock(struct sock *sk, read_descriptor_t *desc,
>  	if (noack)
>  		goto out;
>  
> -	mptcp_rcv_space_adjust(msk, copied);
> -
> +	/* The backlog flushing is only needed when some data is actually
> +	 * moved and will take place in mptcp_release_cb() via
> +	 * release_sock() in the worker.
> +	 */
>  	if (copied > 0) {
> -		mptcp_recv_skb(sk, &offset);
> -		mptcp_cleanup_rbuf(msk, copied);
> +		msk->read_copied += copied;
> +		set_bit(MPTCP_WORK_READ_COMPLETE, &msk->flags);
> +		mptcp_schedule_work(sk);
>  	}

[Severity: Medium]
Can accumulating copied into msk->read_copied in __mptcp_read_sock() without
saturating cause mptcp_read_complete() to skip receive buffer cleanup and
window updates when msk->read_copied wraps?

In net/mptcp/protocol.h, struct mptcp_sock defines read_copied as a signed
int. If multiple calls to __mptcp_read_sock() (for example via
mptcp_splice_read() or BH-context sk->sk_read_sock()) consume enough data
before mptcp_worker() runs mptcp_read_complete() that msk->read_copied
overflows and wraps to zero or a negative value, mptcp_read_complete() resets
msk->read_copied to 0 and skips mptcp_cleanup_rbuf(msk, read_copied).

At the same time, mptcp_rcv_space_adjust() in net/mptcp/protocol.c also
returns early when copied <= 0, and when given a large positive accumulated
read_copied it adds into another signed int without saturation:

net/mptcp/protocol.c:mptcp_rcv_space_adjust() {
	...
	if (copied <= 0)
		return;

	if (!msk->rcvspace_init)
		mptcp_rcv_space_init(msk, msk->first);

	msk->rcvq_space.copied += copied;
	...
}

Skipping mptcp_cleanup_rbuf() after draining the receive queue prevents
subflows from calling tcp_cleanup_rbuf() to send window-update ACKs, which can
stall the connection if the receive window was exhausted, while also skipping
receive buffer autotuning.

Would it make sense to use saturating addition (or a wider type) when
accumulating msk->read_copied and msk->rcvq_space.copied so thatread_copied
remains positive when mptcp_read_complete() drains it?

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