Re: [PATCH mptcp-net] mptcp: avoid race on msk state changes

[Paolo Abeni <pabeni@redhat.com>]

On Wed, 2021-06-16 at 17:38 -0700, Mat Martineau wrote:
> On Wed, 16 Jun 2021, Paolo Abeni wrote:
> 
> > The msk socket state is currently updated in a few spots without
> > owning the msk socket lock itself.
> > 
> > Some of such operations are safe, as they happens before exposing
> > the msk socket to user-space and can't race with other changes.
> > 
> > A couple of them, at connect time, can actually race with close()
> > or shutdown(), leaving breaking the socket state machine.
> > 
> > This change addresses the issue moving such update under the msk
> > socket lock with the usual:
> > 
> > <acquire spinlock>
> > <check sk lock onwers>
> > <ev defer to release_cb>
> > 
> > scheme.
> > 
> > Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/56
> > Fixes: 8fd738049ac3 ("mptcp: fallback in case of simultaneous connect")
> > Fixes: c3c123d16c0e ("net: mptcp: don't hang in mptcp_sendmsg() after TCP fallbac")
> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > ---
> > net/mptcp/protocol.c |  2 ++
> > net/mptcp/protocol.h |  2 ++
> > net/mptcp/subflow.c  | 30 ++++++++++++++++++++++--------
> > 3 files changed, 26 insertions(+), 8 deletions(-)
> > 
> > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> > index 05c8382aafef..15c3b75516fb 100644
> > --- a/net/mptcp/protocol.c
> > +++ b/net/mptcp/protocol.c
> > @@ -2906,6 +2906,8 @@ static void mptcp_release_cb(struct sock *sk)
> > 		__mptcp_clean_una_wakeup(sk);
> > 	if (test_and_clear_bit(MPTCP_ERROR_REPORT, &mptcp_sk(sk)->flags))
> > 		__mptcp_error_report(sk);
> > +	if (test_and_clear_bit(MPTCP_CONNECTED, &mptcp_sk(sk)->flags))
> > +		__mptcp_set_connected(sk);
> 
> Is it worth it to move the MPTCP_CONNECTED handling to be first in 
> mptcp_release_cb()? Some of the other handlers would expect to have the 
> msk connection state set first

Good point! I placed it towards the bottom mainly to avoid processing
such bit before the MPTCP_PUSH_PENDING/MPTCP_RETRANSMIT loop, otherwise
we should check CONNECTED twice.

Double checking the existing code, it looks like
only MPTCP_ERROR_REPORT and MPTCP_CLEAN_UNA depends on it - and the
latter should never be set before MPTCP_CONNECTED, I think.

Moving the MPTCP_CONNECTED before ERROR_REPORT/CLEAN_UNA and
after PUSH_PENDING/RETRANSMIT should fit.

Thanks!

Paolo