Re: [PATCH mptcp-net] mptcp: fix data re-injection from stale subflow

[Mat Martineau <martineau@kernel.org>]

On Tue, 23 Jan 2024, Paolo Abeni wrote:

> When the MPTCP PM detects that a subflow is stale, all the packet
> scheduler must re-inject all the mptcp-level unacked data. To avoid
> acquiring unneeded locks, it first try to check if any unacked data
> is present at all in the RTX queue, but such check is currently
> broken, as it uses TCP-specific helper on an MPTCP socket.
>
> Funnily enough fuzzers and static checkers are happy, as the accessed
> memory still belongs to the mptcp_sock struct, and even from a
> functional perspective the recovery completed successfully, as
> the short-cut test always failed.
>
> A recent unrelated TCP change - commit d5fed5addb2b ("tcp: reorganize
> tcp_sock fast path variables") - exposed the issue, as the tcp field
> reorganization makes the mptcp code always skip the re-inection.
>
> Fix the issue dropping the bogus call: we are on a slow path, the early
> optimization proved once again to be evil.
>

Thanks for tracking this down, Paolo. Fix LGTM:

Reviewed-by: Mat Martineau <martineau@kernel.org>

> Fixes: 1e1d9d6f119c ("mptcp: handle pending data on closed subflow")
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/468
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> net/mptcp/protocol.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 53d6c5544900..a8a94b34a51e 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -2339,9 +2339,6 @@ bool __mptcp_retransmit_pending_data(struct sock *sk)
> 	if (__mptcp_check_fallback(msk))
> 		return false;
>
> -	if (tcp_rtx_and_write_queues_empty(sk))
> -		return false;
> -
> 	/* the closing socket has some data untransmitted and/or unacked:
> 	 * some data in the mptcp rtx queue has not really xmitted yet.
> 	 * keep it simple and re-inject the whole mptcp level rtx queue
> -- 
> 2.43.0
>
>
>