Re: [PATCH mptcp-net v2 1/5] mptcp: propagate shutdown to subflows when possible

[Geliang Tang <geliang@kernel.org>]

On Fri, 2025-09-05 at 20:18 +0200, Matthieu Baerts (NGI0) wrote:
> When the MPTCP DATA FIN have been ACKed, there is no more MPTCP
> related
> metadata to exchange, and all subflows can be safely shutdown.
> 
> Before this patch, the subflows were actually terminated at 'close()'
> time. That's certainly fine most of the time, but not when the
> userspace
> 'shutdown()' a connection, without close()ing it. When doing so, the
> subflows were staying in LAST_ACK state on one side -- and
> consequently
> in FIN_WAIT2 on the other side -- until the 'close()' of the MPTCP
> socket.
> 
> Now, when the DATA FIN have been ACKed, all subflows are shutdown. A
> consequence of this is that the TCP 'FIN' flag can be set earlier
> now,
> but the end result is the same. This affects the packetdrill tests
> looking at the end of the MPTCP connections, but for a good reason.
> 
> Fixes: 3721b9b64676 ("mptcp: Track received DATA_FIN sequence number
> and add related helpers")
> Fixes: 16a9a9da1723 ("mptcp: Add helper to process acks of DATA_FIN")
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
>  net/mptcp/protocol.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index
> 1c26acf6c4145896ecbb5c7f7004121c66a20649..9feb9f437c2bdfff392249e7ad0
> 0edd09ba67ac3 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -372,6 +372,20 @@ static void mptcp_close_wake_up(struct sock *sk)
>  		sk_wake_async(sk, SOCK_WAKE_WAITD, POLL_IN);
>  }
>  
> +static void mptcp_shutdown_subflows(struct mptcp_sock *msk)
> +{
> +	struct mptcp_subflow_context *subflow;
> +
> +	mptcp_for_each_subflow(msk, subflow) {
> +		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
> +		bool slow;
> +
> +		slow = lock_sock_fast(ssk);
> +		tcp_shutdown(ssk, SEND_SHUTDOWN);
> +		unlock_sock_fast(ssk, slow);
> +	}
> +}
> +
>  /* called under the msk socket lock */
>  static bool mptcp_pending_data_fin_ack(struct sock *sk)
>  {
> @@ -396,6 +410,7 @@ static void mptcp_check_data_fin_ack(struct sock
> *sk)
>  			break;
>  		case TCP_CLOSING:
>  		case TCP_LAST_ACK:
> +			mptcp_shutdown_subflows(msk);
>  			mptcp_set_state(sk, TCP_CLOSE);
>  			break;
>  		}
> @@ -564,6 +579,7 @@ static bool mptcp_check_data_fin(struct sock *sk)
>  			mptcp_set_state(sk, TCP_CLOSING);
>  			break;
>  		case TCP_FIN_WAIT2:
> +			mptcp_shutdown_subflows(msk);

I think we should not directly call mptcp_shutdown_subflows() within
mptcp_check_data_fin() and mptcp_check_data_fin_ack(), but should
instead call it after these functions return and the sk state is
TCP_CLOSE. This ensures that the original purpose of these two
functions remains unchanged.

WDYT?

Thanks,
-Geliang

>  			mptcp_set_state(sk, TCP_CLOSE);
>  			break;
>  		default: