Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 6 Jan 2023 06:00:35 +0900
From:   Shigeru Yoshida <syoshida@redhat.com>
To:     Mat Martineau <mathew.j.martineau@linux.intel.com>
Cc:     matthieu.baerts@tessares.net, davem@davemloft.net,
        edumazet@google.com, kuba@kernel.org, pabeni@redhat.com,
        dmytro@shytyi.net, netdev@vger.kernel.org, mptcp@lists.linux.dev,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] mptcp: Fix deadlock in mptcp_sendmsg()
Message-ID: <Y7c6c+5h25bEcn/A@kernel-devel>
References: <20230105201205.1087439-1-syoshida@redhat.com>
 <548fc9cf-b674-f37e-8ff7-9abae0bf15a8@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <548fc9cf-b674-f37e-8ff7-9abae0bf15a8@linux.intel.com>
Precedence: bulk

On Thu, Jan 05, 2023 at 12:26:29PM -0800, Mat Martineau wrote:
> On Fri, 6 Jan 2023, Shigeru Yoshida wrote:
> 
> > __mptcp_close_ssk() can be called from mptcp_sendmsg() with subflow
> > socket locked.  This can cause a deadlock as below:
> > 
> > mptcp_sendmsg()
> >  mptcp_sendmsg_fastopen() --> lock ssk
> >    tcp_sendmsg_fastopen()
> >       __inet_stream_connect()
> >          mptcp_disconnect()
> >             mptcp_destroy_common()
> >                __mptcp_close_ssk() --> lock ssk again
> > 
> > This patch fixes the issue by skipping locking for subflow socket
> > which is already locked.
> > 
> 
> Hi Shigeru -
> 
> I believe this has already been fixed by:
> 
> 7d803344fdc3 ("mptcp: fix deadlock in fastopen error path")
> 
> It is in the net repo:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git

Hi Mat,

Thank you so much for your prompt response.  I've missed above patch.
Yes, the patch fixed the same issue.

Thank you~

Shigeru

> 
> ...but hasn't been merged to net-next or Linus' tree yet. Jakub said to
> expect a net PR today, which should get the fix both upstream and in to
> net-next.
> 
> Thanks,
> 
> Mat
> 
> > Fixes: d98a82a6afc7 ("mptcp: handle defer connect in mptcp_sendmsg")
> > Signed-off-by: Shigeru Yoshida <syoshida@redhat.com>
> > ---
> > net/mptcp/protocol.c | 15 +++++++++------
> > net/mptcp/protocol.h |  4 ++--
> > 2 files changed, 11 insertions(+), 8 deletions(-)
> > 
> > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> > index f6f93957275b..979265f66082 100644
> > --- a/net/mptcp/protocol.c
> > +++ b/net/mptcp/protocol.c
> > @@ -1672,9 +1672,9 @@ static int mptcp_sendmsg_fastopen(struct sock *sk, struct sock *ssk, struct msgh
> > 	lock_sock(ssk);
> > 	msg->msg_flags |= MSG_DONTWAIT;
> > 	msk->connect_flags = O_NONBLOCK;
> > -	msk->is_sendmsg = 1;
> > +	msk->sendmsg_locked_sk = ssk;
> > 	ret = tcp_sendmsg_fastopen(ssk, msg, copied_syn, len, NULL);
> > -	msk->is_sendmsg = 0;
> > +	msk->sendmsg_locked_sk = NULL;
> > 	msg->msg_flags = saved_flags;
> > 	release_sock(ssk);
> > 
> > @@ -2319,7 +2319,8 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
> > 	if (dispose_it)
> > 		list_del(&subflow->node);
> > 
> > -	lock_sock_nested(ssk, SINGLE_DEPTH_NESTING);
> > +	if (msk->sendmsg_locked_sk != ssk)
> > +		lock_sock_nested(ssk, SINGLE_DEPTH_NESTING);
> > 
> > 	if (flags & MPTCP_CF_FASTCLOSE) {
> > 		/* be sure to force the tcp_disconnect() path,
> > @@ -2335,7 +2336,8 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
> > 		tcp_disconnect(ssk, 0);
> > 		msk->subflow->state = SS_UNCONNECTED;
> > 		mptcp_subflow_ctx_reset(subflow);
> > -		release_sock(ssk);
> > +		if (msk->sendmsg_locked_sk != ssk)
> > +			release_sock(ssk);
> > 
> > 		goto out;
> > 	}
> > @@ -2362,7 +2364,8 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
> > 		/* close acquired an extra ref */
> > 		__sock_put(ssk);
> > 	}
> > -	release_sock(ssk);
> > +	if (msk->sendmsg_locked_sk != ssk)
> > +		release_sock(ssk);
> > 
> > 	sock_put(ssk);
> > 
> > @@ -3532,7 +3535,7 @@ static int mptcp_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len)
> > 	/* if reaching here via the fastopen/sendmsg path, the caller already
> > 	 * acquired the subflow socket lock, too.
> > 	 */
> > -	if (msk->is_sendmsg)
> > +	if (msk->sendmsg_locked_sk)
> > 		err = __inet_stream_connect(ssock, uaddr, addr_len, msk->connect_flags, 1);
> > 	else
> > 		err = inet_stream_connect(ssock, uaddr, addr_len, msk->connect_flags);
> > diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> > index 955fb3d88eb3..43afc399e16b 100644
> > --- a/net/mptcp/protocol.h
> > +++ b/net/mptcp/protocol.h
> > @@ -294,8 +294,7 @@ struct mptcp_sock {
> > 	u8		mpc_endpoint_id;
> > 	u8		recvmsg_inq:1,
> > 			cork:1,
> > -			nodelay:1,
> > -			is_sendmsg:1;
> > +			nodelay:1;
> > 	int		connect_flags;
> > 	struct work_struct work;
> > 	struct sk_buff  *ooo_last_skb;
> > @@ -318,6 +317,7 @@ struct mptcp_sock {
> > 	u32 setsockopt_seq;
> > 	char		ca_name[TCP_CA_NAME_MAX];
> > 	struct mptcp_sock	*dl_next;
> > +	struct sock	*sendmsg_locked_sk;
> > };
> > 
> > #define mptcp_data_lock(sk) spin_lock_bh(&(sk)->sk_lock.slock)
> > -- 
> > 2.39.0
> > 
> > 
> 
> --
> Mat Martineau
> Intel
>