Re: Patch "mptcp: drop bogus optimization in __mptcp_check_push()" has been added to the 5.15-stable tree

[Matthieu Baerts <matttbe@kernel.org>]

Hi Greg, Sasha,

On 03/11/2025 02:38, gregkh@linuxfoundation.org wrote:
> 
> This is a note to let you know that I've just added the patch titled
> 
>     mptcp: drop bogus optimization in __mptcp_check_push()
> 
> to the 5.15-stable tree which can be found at:
>     http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
> 
> The filename of the patch is:
>      mptcp-drop-bogus-optimization-in-__mptcp_check_push.patch
> and it can be found in the queue-5.15 subdirectory.
> 
> If you, or anyone else, feels it should not be added to the stable tree,
> please let <stable@vger.kernel.org> know about it.

Can you please drop this patch from v5.15? It looks like it is causing
some issues with MP_PRIO tests. I think that's because back then, the
path is selected differently, with the use of 'msk->last_snd' which will
bypass some decisions to where to send the next data.

I will try to check if another version of this patch is needed for v5.15.

Cheers,
Matt


(I kept the patch below just in case some people from the MPTCP ML want
to react.)

> From stable+bounces-192087-greg=kroah.com@vger.kernel.org Mon Nov  3 05:15:58 2025
> From: Sasha Levin <sashal@kernel.org>
> Date: Sun,  2 Nov 2025 15:15:50 -0500
> Subject: mptcp: drop bogus optimization in __mptcp_check_push()
> To: stable@vger.kernel.org
> Cc: Paolo Abeni <pabeni@redhat.com>, Geliang Tang <geliang@kernel.org>, Mat Martineau <martineau@kernel.org>, "Matthieu Baerts (NGI0)" <matttbe@kernel.org>, Jakub Kicinski <kuba@kernel.org>, Sasha Levin <sashal@kernel.org>
> Message-ID: <20251102201550.3588174-1-sashal@kernel.org>
> 
> From: Paolo Abeni <pabeni@redhat.com>
> 
> [ Upstream commit 27b0e701d3872ba59c5b579a9e8a02ea49ad3d3b ]
> 
> Accessing the transmit queue without owning the msk socket lock is
> inherently racy, hence __mptcp_check_push() could actually quit early
> even when there is pending data.
> 
> That in turn could cause unexpected tx lock and timeout.
> 
> Dropping the early check avoids the race, implicitly relaying on later
> tests under the relevant lock. With such change, all the other
> mptcp_send_head() call sites are now under the msk socket lock and we
> can additionally drop the now unneeded annotation on the transmit head
> pointer accesses.
> 
> Fixes: 6e628cd3a8f7 ("mptcp: use mptcp release_cb for delayed tasks")
> Cc: stable@vger.kernel.org
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> Reviewed-by: Geliang Tang <geliang@kernel.org>
> Tested-by: Geliang Tang <geliang@kernel.org>
> Reviewed-by: Mat Martineau <martineau@kernel.org>
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> Link: https://patch.msgid.link/20251028-net-mptcp-send-timeout-v1-1-38ffff5a9ec8@kernel.org
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> [ split upstream __subflow_push_pending modification across __mptcp_push_pending and __mptcp_subflow_push_pending ]
> Signed-off-by: Sasha Levin <sashal@kernel.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  net/mptcp/protocol.c |   13 +++++--------
>  net/mptcp/protocol.h |    2 +-
>  2 files changed, 6 insertions(+), 9 deletions(-)
> 
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -1137,7 +1137,7 @@ static void __mptcp_clean_una(struct soc
>  			if (WARN_ON_ONCE(!msk->recovery))
>  				break;
>  
> -			WRITE_ONCE(msk->first_pending, mptcp_send_next(sk));
> +			msk->first_pending = mptcp_send_next(sk);
>  		}
>  
>  		dfrag_clear(sk, dfrag);
> @@ -1674,7 +1674,7 @@ void __mptcp_push_pending(struct sock *s
>  
>  			mptcp_update_post_push(msk, dfrag, ret);
>  		}
> -		WRITE_ONCE(msk->first_pending, mptcp_send_next(sk));
> +		msk->first_pending = mptcp_send_next(sk);
>  	}
>  
>  	/* at this point we held the socket lock for the last subflow we used */
> @@ -1732,7 +1732,7 @@ static void __mptcp_subflow_push_pending
>  
>  			mptcp_update_post_push(msk, dfrag, ret);
>  		}
> -		WRITE_ONCE(msk->first_pending, mptcp_send_next(sk));
> +		msk->first_pending = mptcp_send_next(sk);
>  	}
>  
>  out:
> @@ -1850,7 +1850,7 @@ static int mptcp_sendmsg(struct sock *sk
>  			get_page(dfrag->page);
>  			list_add_tail(&dfrag->list, &msk->rtx_queue);
>  			if (!msk->first_pending)
> -				WRITE_ONCE(msk->first_pending, dfrag);
> +				msk->first_pending = dfrag;
>  		}
>  		pr_debug("msk=%p dfrag at seq=%llu len=%u sent=%u new=%d\n", msk,
>  			 dfrag->data_seq, dfrag->data_len, dfrag->already_sent,
> @@ -2645,7 +2645,7 @@ static void __mptcp_clear_xmit(struct so
>  	struct mptcp_sock *msk = mptcp_sk(sk);
>  	struct mptcp_data_frag *dtmp, *dfrag;
>  
> -	WRITE_ONCE(msk->first_pending, NULL);
> +	msk->first_pending = NULL;
>  	list_for_each_entry_safe(dfrag, dtmp, &msk->rtx_queue, list)
>  		dfrag_clear(sk, dfrag);
>  }
> @@ -3114,9 +3114,6 @@ void __mptcp_data_acked(struct sock *sk)
>  
>  void __mptcp_check_push(struct sock *sk, struct sock *ssk)
>  {
> -	if (!mptcp_send_head(sk))
> -		return;
> -
>  	if (!sock_owned_by_user(sk)) {
>  		struct sock *xmit_ssk = mptcp_subflow_get_send(mptcp_sk(sk));
>  
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -325,7 +325,7 @@ static inline struct mptcp_data_frag *mp
>  {
>  	const struct mptcp_sock *msk = mptcp_sk(sk);
>  
> -	return READ_ONCE(msk->first_pending);
> +	return msk->first_pending;
>  }
>  
>  static inline struct mptcp_data_frag *mptcp_send_next(struct sock *sk)

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.