1. Patchew
  2. MPTCP
  3. View series

[PATCH mptcp-net] mptcp: fix abba deadlock on fastopen

Paolo Abeni posted 1 patch 1 year, 6 months ago
Failed in applying to current master (apply log)
There is a newer version of this series
net/mptcp/protocol.c | 36 +++++++++++++++++++++++-------------
1 file changed, 23 insertions(+), 13 deletions(-)
[PATCH mptcp-net] mptcp: fix abba deadlock on fastopen
Posted by Paolo Abeni 1 year, 6 months ago 
Our CI reported lockdep splat in the fastopen code:
 ======================================================
 WARNING: possible circular locking dependency detected
 6.0.0.mptcp_f5e8bfe9878d+ #1558 Not tainted
 ------------------------------------------------------
 packetdrill/1071 is trying to acquire lock:
 ffff8881bd198140 (sk_lock-AF_INET){+.+.}-{0:0}, at: inet_wait_for_connect+0x19c/0x310

 but task is already holding lock:
 ffff8881b8346540 (k-sk_lock-AF_INET){+.+.}-{0:0}, at: mptcp_sendmsg+0xfdf/0x1740

 which lock already depends on the new lock.

 the existing dependency chain (in reverse order) is:

 -> #1 (k-sk_lock-AF_INET){+.+.}-{0:0}:
        __lock_acquire+0xb6d/0x1860
        lock_acquire+0x1d8/0x620
        lock_sock_nested+0x37/0xd0
        inet_stream_connect+0x3f/0xa0
        mptcp_connect+0x411/0x800
        __inet_stream_connect+0x3ab/0x800
        mptcp_stream_connect+0xac/0x110
        __sys_connect+0x101/0x130
        __x64_sys_connect+0x6e/0xb0
        do_syscall_64+0x59/0x90
        entry_SYSCALL_64_after_hwframe+0x63/0xcd

 -> #0 (sk_lock-AF_INET){+.+.}-{0:0}:
        check_prev_add+0x15e/0x2110
        validate_chain+0xace/0xdf0
        __lock_acquire+0xb6d/0x1860
        lock_acquire+0x1d8/0x620
        lock_sock_nested+0x37/0xd0
        inet_wait_for_connect+0x19c/0x310
        __inet_stream_connect+0x26c/0x800
        tcp_sendmsg_fastopen+0x341/0x650
        mptcp_sendmsg+0x109d/0x1740
        sock_sendmsg+0xe1/0x120
        __sys_sendto+0x1c7/0x2a0
        __x64_sys_sendto+0xdc/0x1b0
        do_syscall_64+0x59/0x90
        entry_SYSCALL_64_after_hwframe+0x63/0xcd

 other info that might help us debug this:

  Possible unsafe locking scenario:

        CPU0                    CPU1
        ----                    ----
   lock(k-sk_lock-AF_INET);
                                lock(sk_lock-AF_INET);
                                lock(k-sk_lock-AF_INET);
   lock(sk_lock-AF_INET);

  *** DEADLOCK ***

 1 lock held by packetdrill/1071:
  #0: ffff8881b8346540 (k-sk_lock-AF_INET){+.+.}-{0:0}, at: mptcp_sendmsg+0xfdf/0x1740
 ======================================================

The problem is caused by the blocking inet_wait_for_connect() releasing
and re-acquiring the msk socket lock while the subflow socket lock is
still held and the MPTCP socket requires that the msk socket lock must
be acquired before the subflow socket lock.

Address the issue always invoking tcp_sendmsg_fastopen() in an
unblocking manner, and later eventually complete the blocking
__inet_stream_connect() as needed.

Fixes: d98a82a6afc7 ("mptcp: handle defer connect in mptcp_sendmsg")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
commit ("mptcp: factor out mptcp_connect()") will need some
mangling on top of this one.

A possible alternative would be targeting even the above to -net.

Not strictly required, but it will also make more striaght-forward/
legit the __inet_stream_connect() invocation here.
---
 net/mptcp/protocol.c | 36 +++++++++++++++++++++++-------------
 1 file changed, 23 insertions(+), 13 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index f599ad44ed24..9ff28fa99f7c 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -1673,6 +1673,24 @@ static void mptcp_set_nospace(struct sock *sk)
 	set_bit(MPTCP_NOSPACE, &mptcp_sk(sk)->flags);
 }
 
+static int mptcp_sendmsg_fastopen(struct sock *sk, struct sock *ssk, struct msghdr *msg,
+				  size_t len, int *copied_syn)
+{
+	struct mptcp_sock *msk = mptcp_sk(sk);
+	int ret;
+
+	lock_sock(ssk);
+	ret = tcp_sendmsg_fastopen(ssk, msg, copied_syn, len, NULL);
+	release_sock(ssk);
+
+	/* do the blocking bits of inet_stream_connect outside the ssk socket lock*/
+	if (ret == -EINPROGRESS && !(msg->msg_flags & MSG_DONTWAIT))
+		ret = __inet_stream_connect(sk->sk_socket, msg->msg_name,
+					    msg->msg_namelen, O_NONBLOCK, 1);
+
+	return ret;
+}
+
 static int mptcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
 {
 	struct mptcp_sock *msk = mptcp_sk(sk);
@@ -1692,24 +1710,16 @@ static int mptcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
 	lock_sock(sk);
 
 	ssock = __mptcp_nmpc_socket(msk);
-	if (unlikely(ssock && inet_sk(ssock->sk)->defer_connect)) {
-		struct sock *ssk = ssock->sk;
+	if (unlikely(ssock && (inet_sk(ssock->sk)->defer_connect ||
+			       msg->msg_flags & MSG_FASTOPEN))) {
 		int copied_syn = 0;
 
-		lock_sock(ssk);
-
-		ret = tcp_sendmsg_fastopen(ssk, msg, &copied_syn, len, NULL);
+		ret = mptcp_sendmsg_fastopen(sk, ssock->sk, msg, len, &copied_syn);
 		copied += copied_syn;
-		if (ret == -EINPROGRESS && copied_syn > 0) {
-			/* reflect the new state on the MPTCP socket */
-			inet_sk_state_store(sk, inet_sk_state_load(ssk));
-			release_sock(ssk);
+		if (ret == -EINPROGRESS && copied_syn > 0)
 			goto out;
-		} else if (ret) {
-			release_sock(ssk);
+		else if (ret)
 			goto do_error;
-		}
-		release_sock(ssk);
 	}
 
 	timeo = sock_sndtimeo(sk, msg->msg_flags & MSG_DONTWAIT);
-- 
2.37.3
Re: [PATCH mptcp-net] mptcp: fix abba deadlock on fastopen
Posted by Paolo Abeni 1 year, 6 months ago 
On Tue, 2022-10-11 at 19:26 +0200, Paolo Abeni wrote:
> Our CI reported lockdep splat in the fastopen code:
>  ======================================================
>  WARNING: possible circular locking dependency detected
>  6.0.0.mptcp_f5e8bfe9878d+ #1558 Not tainted
>  ------------------------------------------------------
>  packetdrill/1071 is trying to acquire lock:
>  ffff8881bd198140 (sk_lock-AF_INET){+.+.}-{0:0}, at: inet_wait_for_connect+0x19c/0x310
> 
>  but task is already holding lock:
>  ffff8881b8346540 (k-sk_lock-AF_INET){+.+.}-{0:0}, at: mptcp_sendmsg+0xfdf/0x1740
> 
>  which lock already depends on the new lock.
> 
>  the existing dependency chain (in reverse order) is:
> 
>  -> #1 (k-sk_lock-AF_INET){+.+.}-{0:0}:
>         __lock_acquire+0xb6d/0x1860
>         lock_acquire+0x1d8/0x620
>         lock_sock_nested+0x37/0xd0
>         inet_stream_connect+0x3f/0xa0
>         mptcp_connect+0x411/0x800
>         __inet_stream_connect+0x3ab/0x800
>         mptcp_stream_connect+0xac/0x110
>         __sys_connect+0x101/0x130
>         __x64_sys_connect+0x6e/0xb0
>         do_syscall_64+0x59/0x90
>         entry_SYSCALL_64_after_hwframe+0x63/0xcd
> 
>  -> #0 (sk_lock-AF_INET){+.+.}-{0:0}:
>         check_prev_add+0x15e/0x2110
>         validate_chain+0xace/0xdf0
>         __lock_acquire+0xb6d/0x1860
>         lock_acquire+0x1d8/0x620
>         lock_sock_nested+0x37/0xd0
>         inet_wait_for_connect+0x19c/0x310
>         __inet_stream_connect+0x26c/0x800
>         tcp_sendmsg_fastopen+0x341/0x650
>         mptcp_sendmsg+0x109d/0x1740
>         sock_sendmsg+0xe1/0x120
>         __sys_sendto+0x1c7/0x2a0
>         __x64_sys_sendto+0xdc/0x1b0
>         do_syscall_64+0x59/0x90
>         entry_SYSCALL_64_after_hwframe+0x63/0xcd
> 
>  other info that might help us debug this:
> 
>   Possible unsafe locking scenario:
> 
>         CPU0                    CPU1
>         ----                    ----
>    lock(k-sk_lock-AF_INET);
>                                 lock(sk_lock-AF_INET);
>                                 lock(k-sk_lock-AF_INET);
>    lock(sk_lock-AF_INET);
> 
>   *** DEADLOCK ***
> 
>  1 lock held by packetdrill/1071:
>   #0: ffff8881b8346540 (k-sk_lock-AF_INET){+.+.}-{0:0}, at: mptcp_sendmsg+0xfdf/0x1740
>  ======================================================
> 
> The problem is caused by the blocking inet_wait_for_connect() releasing
> and re-acquiring the msk socket lock while the subflow socket lock is
> still held and the MPTCP socket requires that the msk socket lock must
> be acquired before the subflow socket lock.
> 
> Address the issue always invoking tcp_sendmsg_fastopen() in an
> unblocking manner, and later eventually complete the blocking
> __inet_stream_connect() as needed.
> 
> Fixes: d98a82a6afc7 ("mptcp: handle defer connect in mptcp_sendmsg")
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>

Whoops, sorry this is not the patch I tested. Self-nack

/P

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.