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

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 06/09/2025 02:45, Geliang Tang wrote:
> 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.

Thank you for the review. I'm not sure to understand what you mean about
the "original purpose": they both set the appropriated actions when
DATA_FIN have been received. Here, I do want to shut the subflows down
when the DATA_FIN ACK are received, so that's the good spot, no?

Note that commit 3721b9b64676 ("mptcp: Track received DATA_FIN sequence
number and add related helpers") already had a hint of what should have
been done: see the "Close subflows now?" comment, later removed in
commit e16163b6e2b7 ("mptcp: refactor shutdown and close").

Or maybe do you mean their original name "check_data_fin(_ack)" is
confusing? The "check" here is not just to check if the DATA_FIN (ACK)
was set, but to check actions related to them.

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