diff mbox series

mptcp: Fix deadlock in mptcp_sendmsg()

Message ID 20230105201205.1087439-1-syoshida@redhat.com (mailing list archive)
State Awaiting Upstream
Delegated to: Netdev Maintainers
Headers show
Series mptcp: Fix deadlock in mptcp_sendmsg() | expand

Checks

Context Check Description
netdev/tree_selection success Guessed tree name to be net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix warning Target tree name not specified in the subject
netdev/cover_letter success Single patches do not need cover letters
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 5 this patch: 5
netdev/cc_maintainers success CCed 9 of 9 maintainers
netdev/build_clang success Errors and warnings before: 3 this patch: 3
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 5 this patch: 5
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 62 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Shigeru Yoshida Jan. 5, 2023, 8:12 p.m. UTC 
__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.

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(-)

Comments

Mat Martineau Jan. 5, 2023, 8:26 p.m. UTC | #1 
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

...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
Shigeru Yoshida Jan. 5, 2023, 9 p.m. UTC | #2 
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
>
diff mbox series

Patch

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)