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

[Geliang Tang <geliang.tang@suse.com>]

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.

> 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);

> is still in ESTABLISHED, tcp_disconnect() will send a RST if I'm not
> mistaken. That's what we want to avoid, no?

Yes, tcp_disconnect will send a RST.

Thanks,
-Geliang

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