[MPTCP] Re: [PATCH 2/9] mptcp: fix race from mptcp_close/mptcp join

[MPTCP] Re: [PATCH 2/9] mptcp: fix race from mptcp_close/mptcp join

[Florian Westphal]

[-- Attachment #1: Type: text/plain, Size: 682 bytes --]

Paolo Abeni <pabeni(a)redhat.com> wrote:
> > +	/* mptcp socket already closing? */
> > +	if (parent->sk_state != TCP_ESTABLISHED)
> > +		goto out;
> > +
> > +	ret = true;
> >  	list_add_tail(&subflow->node, &msk->conn_list);
> 
> 
> This relies on mptcp_close() setting msk status before acquiring the
> msk socket lock, right? If so perhaps we can add a comment there to
> document the locking schema?

No, if mptcp_close would set its state after acquiring the lock this
will still work, either the above check triggers (mptcp_close already
finished) or mptcp_close is still blocked on the lock.

The second case is no different from "mptcp_close was not called".

[MPTCP] Re: [PATCH 2/9] mptcp: fix race from mptcp_close/mptcp join

[Paolo Abeni]

[-- Attachment #1: Type: text/plain, Size: 903 bytes --]

On Mon, 2019-12-02 at 14:29 +0100, Florian Westphal wrote:
> Paolo Abeni <pabeni(a)redhat.com> wrote:
> > > +	/* mptcp socket already closing? */
> > > +	if (parent->sk_state != TCP_ESTABLISHED)
> > > +		goto out;
> > > +
> > > +	ret = true;
> > >  	list_add_tail(&subflow->node, &msk->conn_list);
> > 
> > This relies on mptcp_close() setting msk status before acquiring the
> > msk socket lock, right? If so perhaps we can add a comment there to
> > document the locking schema?
> 
> No, if mptcp_close would set its state after acquiring the lock this
> will still work, either the above check triggers (mptcp_close already
> finished) or mptcp_close is still blocked on the lock.
> 
> The second case is no different from "mptcp_close was not called".

Ah, I see mptcp_close still have to acquire the ssk socket lock! 

Yep, no additional changes required.

Thanks,

Paolo

[MPTCP] Re: [PATCH 2/9] mptcp: fix race from mptcp_close/mptcp join

[Paolo Abeni]

[-- Attachment #1: Type: text/plain, Size: 2616 bytes --]

On Sat, 2019-11-30 at 23:48 +0100, Florian Westphal wrote:
> squashto: mptcp: Add handling of incoming MP_JOIN requests
> 
> When we get join request, we can run into following problem:
> 
> cpu0                        cpu1
> 
> mptcp_close()               mptcp_finish_join()
>   lock_mptcp_sk()
>   close all ssk
>   unlock_mptcp_sk()
>                            lock_mptcp_sk()
> 	                    ... add to empty list
> 			    ... graft ssk to invalid parent
> 
> Fix this by indicating via mptcp_finish_join if the join was ok or not.
> We can do so by testing the mptcp sk_state from mptcp_finish_join after
> we've obtained the msk socket lock.
> 
> If the state is established, mptcp_close wasn't called yet (or, its
> waiting on the msk lock -- the latter makes no difference to the
> former).
> 
> If the state is different, then don't don't graft the ssk, don't
> add it the the connection list, and let the caller close the ssk.
> 
> Also, do NOT advance the ssk to established state.  The tcp stack
> already does this a bit later on, after the full tcp_sk state has been
> set up.  Doing this here can trigger a divide-by-0 bug when the ssk
> receives data right away, because tp->->rcvq_space.space has not been
> initialized yet.
> 
> Signed-off-by: Florian Westphal <fw(a)strlen.de>
> ---
>  net/mptcp/protocol.c | 13 +++++++++++--
>  net/mptcp/protocol.h |  2 +-
>  net/mptcp/subflow.c  |  8 +++++---
>  3 files changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 72e6950fbbc2..529a08095512 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -1174,19 +1174,28 @@ void mptcp_finish_connect(struct sock *sk, int mp_capable)
>  	inet_sk_state_store(sk, TCP_ESTABLISHED);
>  }
>  
> -void mptcp_finish_join(struct sock *sk)
> +bool mptcp_finish_join(struct sock *sk)
>  {
>  	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
>  	struct mptcp_sock *msk = mptcp_sk(subflow->conn);
> +	struct sock *parent = (void *)msk;
> +	bool ret = false;
>  
>  	pr_debug("msk=%p, subflow=%p", msk, subflow);
>  
>  	local_bh_disable();
>  	bh_lock_sock_nested(subflow->conn);
> +
> +	/* mptcp socket already closing? */
> +	if (parent->sk_state != TCP_ESTABLISHED)
> +		goto out;
> +
> +	ret = true;
>  	list_add_tail(&subflow->node, &msk->conn_list);


This relies on mptcp_close() setting msk status before acquiring the
msk socket lock, right? If so perhaps we can add a comment there to
document the locking schema?
 
The patch LGTM! 

/P