Re: [PATCH v3 mptcp-net 1/3] mptcp: fix grafting corner case

[Matthieu Baerts <matttbe@kernel.org>]

Hi Paolo,

On 13/11/2025 01:10, Paolo Abeni wrote:
> If a passive MPTCP socket creates active subflows while still unaccepted,
> __mptcp_subflow_connect() will try to graft such subflows to the msk,
> but the msk struct socket is not yet initialized at that point:
> the subflows will misbehave.

What kind of errors were visible?

> Address the issue always trying to graft the subflow in
> mptcp_finish_join(), regardless of the subflow itself being active or
> passive. To avoid races with accept(), access the msk->sk_socket under
> the callback lock.

Thank you for addressing this issue!

> 

By chance, do you have a Fixes tag to add here? (if it is for -net)

> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
>  net/mptcp/protocol.c | 23 +++++++++++++++++------
>  1 file changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 8965abb94b81..1b3c5fd01600 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -913,12 +913,6 @@ static bool __mptcp_finish_join(struct mptcp_sock *msk, struct sock *ssk)
>  	mptcp_subflow_joined(msk, ssk);
>  	spin_unlock_bh(&msk->fallback_lock);
>  
> -	/* attach to msk socket only after we are sure we will deal with it
> -	 * at close time
> -	 */
> -	if (sk->sk_socket && !ssk->sk_socket)
> -		mptcp_sock_graft(ssk, sk->sk_socket);
> -
>  	mptcp_subflow_ctx(ssk)->subflow_id = msk->subflow_id++;
>  	mptcp_sockopt_sync_locked(msk, ssk);
>  	mptcp_stop_tout_timer(sk);
> @@ -3734,6 +3728,20 @@ void mptcp_sock_graft(struct sock *sk, struct socket *parent)
>  	write_unlock_bh(&sk->sk_callback_lock);
>  }
>  
> +static void mptcp_check_graft(struct sock *sk, struct sock *ssk)
> +{
> +	struct socket *sock;
> +
> +	if (ssk->sk_socket)
> +		return;
> +
> +	write_lock_bh(&sk->sk_callback_lock);
> +	sock = sk->sk_socket;
> +	write_lock_bh(&sk->sk_callback_lock);

The build job failed because here it should be the unlock version
(write_unlock_bh()).
(and probably an empty line just after).

If there is only that, I can fix that when applying the patches.

> +	if (sock)
> +		mptcp_sock_graft(ssk, sock);
> +}
> +
>  bool mptcp_finish_join(struct sock *ssk)
>  {
>  	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
> @@ -3758,6 +3766,7 @@ bool mptcp_finish_join(struct sock *ssk)
>  		}
>  		mptcp_subflow_joined(msk, ssk);
>  		spin_unlock_bh(&msk->fallback_lock);
> +		mptcp_check_graft(parent, ssk);
>  		mptcp_propagate_sndbuf(parent, ssk);
>  		return true;
>  	}
> @@ -3767,6 +3776,8 @@ bool mptcp_finish_join(struct sock *ssk)
>  		goto err_prohibited;
>  	}
>  
> +	mptcp_check_graft(parent, ssk);

Is it OK to graft it even in case of errors in __mptcp_finish_join()?
i.e. not in an established state or !msk->allow_subflows.

Should it not be done after the block below, if there was no error?

> +
>  	/* If we can't acquire msk socket lock here, let the release callback
>  	 * handle it
>  	 */

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