[v1] Bluetooth: L2CAP: Fix deadlock

Commit Message

Luiz Augusto von Dentz June 24, 2024, 1:46 p.m. UTC

From: Luiz Augusto von Dentz <[e-mail]>

This fixes the following deadlock introduced by e3203b177717
("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: e3203b177717 ("bluetooth/l2cap: sync sock recv cb and release")
Signed-off-by: Luiz Augusto von Dentz <[e-mail]>
---
 net/bluetooth/l2cap_core.c |  3 +++
 net/bluetooth/l2cap_sock.c | 13 +------------
 2 files changed, 4 insertions(+), 12 deletions(-)

Comments

[e-mail] June 24, 2024, 2:34 p.m. UTC | #1

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=864924

---Test result---

Test Summary:
CheckPatch                    PASS      0.74 seconds
GitLint                       PASS      0.24 seconds
SubjectPrefix                 PASS      0.08 seconds
BuildKernel                   PASS      30.23 seconds
CheckAllWarning               PASS      32.83 seconds
CheckSparse                   PASS      39.01 seconds
CheckSmatch                   PASS      104.85 seconds
BuildKernel32                 PASS      29.39 seconds
TestRunnerSetup               PASS      529.98 seconds
TestRunner_l2cap-tester       PASS      20.66 seconds
TestRunner_iso-tester         PASS      43.64 seconds
TestRunner_bnep-tester        PASS      5.10 seconds
TestRunner_mgmt-tester        PASS      114.35 seconds
TestRunner_rfcomm-tester      PASS      7.63 seconds
TestRunner_sco-tester         PASS      15.29 seconds
TestRunner_ioctl-tester       PASS      8.05 seconds
TestRunner_mesh-tester        PASS      6.12 seconds
TestRunner_smp-tester         PASS      7.05 seconds
TestRunner_userchan-tester    PASS      5.11 seconds
IncrementalBuild              PASS      28.12 seconds



---
Regards,
Linux Bluetooth

Luiz Augusto von Dentz June 24, 2024, 2:56 p.m. UTC | #2

On Mon, Jun 24, 2024 at 10:34 AM <[e-mail]> wrote:
>
> This is automated email and please do not reply to this email!
>
> Dear submitter,
>
> Thank you for submitting the patches to the linux bluetooth mailing list.
> This is a CI test results with your patch series:
> PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=864924
>
> ---Test result---
>
> Test Summary:
> CheckPatch                    PASS      0.74 seconds
> GitLint                       PASS      0.24 seconds
> SubjectPrefix                 PASS      0.08 seconds
> BuildKernel                   PASS      30.23 seconds
> CheckAllWarning               PASS      32.83 seconds
> CheckSparse                   PASS      39.01 seconds
> CheckSmatch                   PASS      104.85 seconds
> BuildKernel32                 PASS      29.39 seconds
> TestRunnerSetup               PASS      529.98 seconds
> TestRunner_l2cap-tester       PASS      20.66 seconds
> TestRunner_iso-tester         PASS      43.64 seconds
> TestRunner_bnep-tester        PASS      5.10 seconds
> TestRunner_mgmt-tester        PASS      114.35 seconds
> TestRunner_rfcomm-tester      PASS      7.63 seconds
> TestRunner_sco-tester         PASS      15.29 seconds
> TestRunner_ioctl-tester       PASS      8.05 seconds
> TestRunner_mesh-tester        PASS      6.12 seconds
> TestRunner_smp-tester         PASS      7.05 seconds
> TestRunner_userchan-tester    PASS      5.11 seconds
> IncrementalBuild              PASS      28.12 seconds
>
>
>
> ---
> Regards,
> Linux Bluetooth
>

[e-mail] June 26, 2024, 8 p.m. UTC | #3

Hello:

This patch was applied to bluetooth/bluetooth-next.git (master)
by Luiz Augusto von Dentz <[e-mail]>:

On Mon, 24 Jun 2024 09:46:37 -0400 you wrote:
> From: Luiz Augusto von Dentz <[e-mail]>
> 
> This fixes the following deadlock introduced by e3203b177717
> ("bluetooth/l2cap: sync sock recv cb and release"):
> 
> ============================================
> WARNING: possible recursive locking detected
> 6.10.0-rc3-g4029dba6b6f1 #6823 Not tainted
> 
> [...]

Here is the summary with links:
  - [v1] Bluetooth: L2CAP: Fix deadlock
    https://git.kernel.org/bluetooth/bluetooth-next/c/d8abca53bb2b

You are awesome, thank you!

Patch

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index aed025734d04..c3c26bbb5dda 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -6761,6 +6761,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;
 
@@ -6777,6 +6779,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 962aa11ce3de..ba437c6f6ee5 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -1489,18 +1489,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);
@@ -1552,8 +1543,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;
 }