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

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 10/09/2025 05:24, Geliang Tang wrote:
> Hi Matt,
> 
> On Sat, 2025-09-06 at 15:42 +0200, Matthieu Baerts wrote:
>> 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..9feb9f437c2bdfff392249e
>>>> 7ad0
>>>> 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.
> 
> I mean not to modify mptcp_check_data_fin and mptcp_check_data_fin_ack,
> but to check TCP_CLOSE in mptcp_worker(), something like:
> 
> @@ -2749,6 +2749,9 @@ static void mptcp_worker(struct work_struct
> *work)
>         mptcp_check_data_fin_ack(sk);
>         mptcp_check_data_fin(sk);
>  
> +       if (sk->sk_state == TCP_CLOSE)
> +               mptcp_shutdown_subflows(msk);
> +
>         if (test_and_clear_bit(MPTCP_WORK_CLOSE_SUBFLOW, &msk->flags))
>                 __mptcp_close_subflow(sk);

No, we should only do that when receiving the DATA_FIN ACK, when
transitioning from FIN_WAIT_2/LAST_ACK to CLOSE.

I do think these data_fin functions are the best place, in the
operations linked to the state machine, not here in the worker.

> Another option is to call mptcp_shutdown_subflows in
> mptcp_close_wake_up, which one do you think is better?

No, that's not linked to 'waking up the userspace'.

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