Re: [PATCH mptcp-next v11 1/4] mptcp: set next subflow to msk->first

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

On Sun, Oct 08, 2023 at 04:05:10PM +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 08/10/2023 14:33, Geliang Tang wrote:
> > On Sun, Oct 08, 2023 at 01:11:32PM +0200, Matthieu Baerts wrote:
> >> Hi Geliang,
> >>
> >> +cc Paolo
> >>
> >> On 08/10/2023 12:27, Geliang Tang wrote:
> >>> When closing the msk->first socket in __mptcp_close_ssk(), the original
> >>> behaviour is to reset this socket. But if there's another subflow available
> >>> in this case, it's better to set the first available subflow to msk->first,
> >>> instead of resetting the msk->first.
> >>
> >> I don't think we want to do that. If I remember well, we want to keep
> >> having 'msk->first' pointing to the initial subflow for various reasons
> >> (I don't remember exactly).
> >>
> >> I think what we want to do is not to reset the initial subflow if there
> >> are other subflows still available and we don't want to set it to 'NULL'
> >> in such case.
> > 
> > Is it like this:
> > 
> >        if (ssk == msk->first && !msk->pm.subflows)
> >                WRITE_ONCE(msk->first, NULL);

The code:

        if (ssk == msk->first && list_is_singular(&msk->conn_list))
                WRITE_ONCE(msk->first, NULL);

works, no NULL pointer crash with this now. I update it into v12.

> 
> No, we cannot set it to NULL nor release it but we should avoid the
> "reset" being sent.
> 
> I don't have access to the code for the moment but by looking at your
> patch, I would say you should not modify 'dispose_it' nor 'msk->first'
> but maybe check if you can avoid calling 'mptcp_subflow_ctx_reset()' if
> '!dispose_it', maybe by checking something like
> 'list_is_singular(&msk->conn_list)'? (I think you should avoid looking
> at the PM structure here).

Something like this, right:

                 * disconnect should never fail
                 */
                WARN_ON_ONCE(tcp_disconnect(ssk, 0));
-               mptcp_subflow_ctx_reset(subflow);
+               list_is_singular(&msk->conn_list)
+                       mptcp_subflow_ctx_reset(subflow);
                release_sock(ssk);
 
                goto out;

This doesn't work, if we only avoid calling mptcp_subflow_ctx_reset,
tcp_disconnect will send a RST too.

Thanks,
-Geliang

> 
> Please tell me if it is not clear.
> 
> Cheers,
> Matt
> -- 
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net