Re: [PATCH mptcp-next 3/4] mptcp: handle join requests via pernet listen socket

[Florian Westphal <fw@strlen.de>]

Kishen Maloor <kishen.maloor@intel.com> wrote:
> On 3/9/22 4:53 AM, Florian Westphal wrote:
> > Kishen Maloor <kishen.maloor@intel.com> wrote:
> >>>> Over a newly established MPTCP connection following listen(s1), the PM can issue an 
> >>>> ADD_ADDR with B. In light of this change there would be no listener created for B. 
> >>>> But if the remote endpoint immediately established a subflow in response (to the 
> >>>> ADD_ADDR), then that would create a subflow (connection) socket at B.
> >>>> It appears (and correct me if I'm wrong) that bind(s2, B) would fail after this point (?).
> >>>
> >>> Why would that fail? You can bind x:y even if there is an established
> >>> connection from x:y to q:r.
> >>
> >> If I establish an MPTCP connection using mptcp_connect individually as 
> >> Client and Server, then I am unable to bind a 3rd (new) Server process at the Client's 
> >> addr+port [1]. Why is this the case?
> > 
> > Whats [1]?
> > I suspect this patch series needs following addition in patch 3:
> > 
> > diff --git a/net/mptcp/ctrl.c b/net/mptcp/ctrl.c
> > --- a/net/mptcp/ctrl.c
> > +++ b/net/mptcp/ctrl.c
> > @@ -337,6 +337,8 @@ static int mptcp_init_join_sk(struct net *net, struct sock *sk, struct mptcp_joi
> >  	if (!tb)
> >  		return -ENOMEM;
> >  
> > +	ssock->sk->sk_reuse = 1;
> > +	ssock->sk->sk_reuseport = 1;
> >  	inet_csk(ssock->sk)->icsk_bind_hash = tb;
> >  	return 0;
> >  }
> > 
> > After that, follwing sequence should work:
> > 
> > 1. bind(0.0.0.0, p1) // listen, accept etc, initial subflow established
> > 2. announce p2
> > 3. receive join on addr, p2
> > 4. bind(0.0.0.0, p2)
> > 
> > 4) should work because sk used for endpoint in 3) has reuse flag set
> > and is not in listen state.
> > 
> > cf. include/net/inet_hashtables.h, line 47:
> > 2) If all sockets have sk->sk_reuse set, and none of them
> >    TCP_LISTEN state, the port may be shared.
> > 
> 
> Wouldn't 4) fail if the socket being bound at the time does not have the SO_REUSExxx flag(s) set?

Yes, it needs SO_REUSEADDR set.

> If so, that would be application level thing and in that situation we don't have a way to
> avoid a race.  Whereas when we require an explicit listener, we could have the kernel take a step
> back (and not create a listener) to break the race.

Uh, what?  Sorry, I am totally lost.  I have no idea what the problem is
that we're solving here.

EOD, I am out of ideas.  Feel free to toss this patchset, I have no idea
what to do.