[PATCH 5.10.y] Bluetooth: L2CAP: Fix deadlock

[PATCH 5.10.y] Bluetooth: L2CAP: Fix deadlock

[Karl Mehltretter]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

[ Upstream commit f1a8f402f13f94263cf349216c257b2985100927 ]

This fixes the following deadlock introduced by 39a92a55be13
("bluetooth/l2cap: sync sock recv cb and release")

============================================
WARNING: possible recursive locking detected
6.10.0-rc3-g4029dba6b6f1 #6823 Not tainted
--------------------------------------------
kworker/u5:0/35 is trying to acquire lock:
ffff888002ec2510 (&chan->lock#2/1){+.+.}-{3:3}, at:
l2cap_sock_recv_cb+0x44/0x1e0

but task is already holding lock:
ffff888002ec2510 (&chan->lock#2/1){+.+.}-{3:3}, at:
l2cap_get_chan_by_scid+0xaf/0xd0

other info that might help us debug this:
 Possible unsafe locking scenario:

       CPU0
       ----
  lock(&chan->lock#2/1);
  lock(&chan->lock#2/1);

 *** DEADLOCK ***

 May be due to missing lock nesting notation

3 locks held by kworker/u5:0/35:
 #0: ffff888002b8a940 ((wq_completion)hci0#2){+.+.}-{0:0}, at:
process_one_work+0x750/0x930
 #1: ffff888002c67dd0 ((work_completion)(&hdev->rx_work)){+.+.}-{0:0},
at: process_one_work+0x44e/0x930
 #2: ffff888002ec2510 (&chan->lock#2/1){+.+.}-{3:3}, at:
l2cap_get_chan_by_scid+0xaf/0xd0

To fix the original problem this introduces l2cap_chan_lock at
l2cap_conless_channel to ensure that l2cap_sock_recv_cb is called with
chan->lock held.

Fixes: 89e856e124f9 ("bluetooth/l2cap: sync sock recv cb and release")
Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
[ Karl Mehltretter: only the l2cap_core.c and l2cap_sock.c hunks apply
  to this tree. hci_sync.c and hci_sync.h do not exist here, and the
  hci_core.c change is an unrelated conversion of hci_dev_cmd() off
  the old hci_request API. The changes to both L2CAP files apply
  unmodified.

  The lock this adds to l2cap_conless_channel() is also the one that
  commit c531e63871c0 ("Bluetooth: l2cap: always unlock channel in
  l2cap_conless_channel()") was backported without, so it additionally
  pairs the l2cap_chan_unlock() that function currently calls on a
  mutex it never acquired. ]
Assisted-by: LLM
Signed-off-by: Karl Mehltretter <kmehltretter@gmail.com>
---

Notes:
    This is intended to replace the queued revert of
    commit 2243127db6ba ("bluetooth/l2cap: sync sock recv cb and release"),
    rather than to be applied on top of it.
    
    The revert fixes the deadlock but removes the NULL guard from
    l2cap_sock_recv_cb(), while l2cap_sock_destruct() still sets chan->data to
    NULL.  Under KASAN, closing receiving sockets while L2CAP data is inbound
    then gives a fatal NULL dereference in hci_rx_work, reproduced 3/3 on
    v5.10.270 with the queued revert applied.  This patch keeps the guard and
    fixes the deadlock in one step, and uses the two applicable upstream
    L2CAP hunks unmodified.
    
    Tested in QEMU with two virtual BR/EDR controllers, PROVE_LOCKING,
    DEBUG_MUTEXES and KASAN:
    
      v5.10.270 as released         recursive chan->lock deadlock
      v5.10.270 + the queued revert fatal NULL deref, 3/3
      v5.10.270 + this patch        clean 3/3, 300 close cycles each
    
    BlueZ's own l2cap-tester also deadlocks hci_rx_work on v5.10.270 and never
    completes.  With this patch all six tester suites run to completion.
    
    Also on a Raspberry Pi 400 (BCM2711, onboard CYW43455) with a second board
    as the L2CAP peer, each test from its own boot so lockdep was armed for
    each:
    
      v5.10.270 as released, connectionless   bad unlock balance
      v5.10.270 as released, connected        possible recursive locking
      v5.10.270 + this patch, connectionless  clean, debug_locks still 1
      v5.10.270 + this patch, connected       clean, debug_locks still 1
    
    The same board with an A2DP speaker shows the user-visible effect.  On
    v5.10.270 as released, connecting to the speaker deadlocks hci_rx_work and
    playback cannot start at all:
    
      bluetoothd: a2dp-source profile connect failed: Device or resource busy
    
      task:kworker/u9:0 state:D
      Workqueue: hci0 hci_rx_work [bluetooth]
        __mutex_lock
        l2cap_sock_recv_cb
        l2cap_recv_frame
        l2cap_recv_acldata
        hci_rx_work
    
    With this patch the same speaker connects and plays the full track with no
    kernel warning and debug_locks still 1.
    
    If the queued revert is kept instead, the same end state is reachable with
    two patches on top of it, which I can send.

 net/bluetooth/l2cap_core.c |  3 +++
 net/bluetooth/l2cap_sock.c | 13 +------------
 2 files changed, 4 insertions(+), 12 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 1122566c4b50..7de512843f06 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -8044,6 +8044,8 @@ static void l2cap_conless_channel(struct l2cap_conn *conn, __le16 psm,
 
 	BT_DBG("chan %p, len %d", chan, skb->len);
 
+	l2cap_chan_lock(chan);
+
 	if (chan->state != BT_BOUND && chan->state != BT_CONNECTED)
 		goto drop;
 
@@ -8061,6 +8063,7 @@ static void l2cap_conless_channel(struct l2cap_conn *conn, __le16 psm,
 	}
 
 drop:
+	l2cap_chan_unlock(chan);
 	l2cap_chan_put(chan);
 free_skb:
 	kfree_skb(skb);
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index 9d834f225462..018b5a0c87ce 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -1548,18 +1548,9 @@ static int l2cap_sock_recv_cb(struct l2cap_chan *chan, struct sk_buff *skb)
 	struct l2cap_pinfo *pi;
 	int err;
 
-	/* To avoid race with sock_release, a chan lock needs to be added here
-	 * to synchronize the sock.
-	 */
-	l2cap_chan_hold(chan);
-	l2cap_chan_lock(chan);
 	sk = chan->data;
-
-	if (!sk) {
-		l2cap_chan_unlock(chan);
-		l2cap_chan_put(chan);
+	if (!sk)
 		return -ENXIO;
-	}
 
 	pi = l2cap_pi(sk);
 	lock_sock(sk);
@@ -1611,8 +1602,6 @@ static int l2cap_sock_recv_cb(struct l2cap_chan *chan, struct sk_buff *skb)
 
 done:
 	release_sock(sk);
-	l2cap_chan_unlock(chan);
-	l2cap_chan_put(chan);
 
 	return err;
 }

base-commit: 1797d8bf8d0c2e74defad605d14e3553d43a3caf
-- 
2.53.0

Re: [PATCH 5.10.y] Bluetooth: L2CAP: Fix deadlock

[Sasha Levin]

> [ Upstream commit f1a8f402f13f94263cf349216c257b2985100927 ]
>
> This fixes the following deadlock introduced by 39a92a55be13
> ("bluetooth/l2cap: sync sock recv cb and release")

Queued for 5.10, thanks.

89e856e124f9 ("bluetooth/l2cap: sync sock recv cb and release") is back
on 5.10 as well, so the tree now has both halves rather than the revert.

-- 
Thanks,
Sasha