Re: [PATCH mptcp-next v12 1/5] mptcp: avoid resetting when another subflow available

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 10/10/2023 08:13, Geliang Tang wrote:
> On Mon, Oct 09, 2023 at 06:13:19PM +0200, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> On 09/10/2023 08:02, Geliang Tang wrote:
>>> When closing the msk->first socket in __mptcp_close_ssk(), if there's
>>> another subflow available, it's better to avoid resetting it.
>>>
>>> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
>>> ---
>>>  net/mptcp/protocol.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
>>> index 30e0c29ae0a4..6346a164ed66 100644
>>> --- a/net/mptcp/protocol.c
>>> +++ b/net/mptcp/protocol.c
>>> @@ -2396,7 +2396,7 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
>>>  		goto out_release;
>>>  	}
>>>  
>>> -	dispose_it = msk->free_first || ssk != msk->first;
>>> +	dispose_it = msk->free_first || ssk != msk->first || !list_is_singular(&msk->conn_list);
>>>  	if (dispose_it)
>>>  		list_del(&subflow->node);
>>>  
>>> @@ -2446,7 +2446,7 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
>>>  
>>>  	sock_put(ssk);
>>>  
>>> -	if (ssk == msk->first)
>>> +	if (ssk == msk->first && list_is_singular(&msk->conn_list))
>>>  		WRITE_ONCE(msk->first, NULL);
>>
>> I don't think we should 'dispose it' nor set 'msk->first' to NULL but
> 
> That's what this patch dose. Dispose it, not set msk->first to NULL.

Sorry, I meant: we should not 'dispose it' and we should not set
'msk->first' to NULL if we are not closing the whole MPTCP connection.

>> instead, we should not call tcp_disconnect() directly: if the connection
> 
> But this patch lacks the code to handle:
> 
> 	ssk == msk->first && !list_is_singular(&msk->conn_list)
> 
> Is my understanding correct?
> 
> How about dropping list_is_singular() in the next block, something like:
> 
>          * For established sockets, allow an additional timeout before closing,
>          * as the protocol can still create more subflows.
>          */
> -       if (list_is_singular(&msk->conn_list) && msk->first &&
> -           inet_sk_state_load(msk->first) == TCP_CLOSE) {
> +       if (msk->first && inet_sk_state_load(msk->first) == TCP_CLOSE) {
>                 if (sk->sk_state != TCP_ESTABLISHED ||
>                     msk->in_accept_queue || sock_flag(sk, SOCK_DEAD)) {
>                         inet_sk_state_store(sk, TCP_CLOSE);

I cannot tell like that, but what is important first is not to call
'tcp_disconnect()' on a non already closed socket. So we need something
else before the 'disconnect()'.

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net