Bluetooth: L2CAP: Fix deadlock

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>

diff --git a/include/net/bluetooth/hci_sync.h b/include/net/bluetooth/hci_sync.h
index 6a9d063e9f472dad520031f2b1ba4f1119731a72..534c3386e714f9e46984e9596f0216ba5fc192d9 100644 (file)
--- a/include/net/bluetooth/hci_sync.h
+++ b/include/net/bluetooth/hci_sync.h
@@ -38,6 +38,8 @@ int __hci_cmd_sync_status(struct hci_dev *hdev, u16 opcode, u32 plen,
 int __hci_cmd_sync_status_sk(struct hci_dev *hdev, u16 opcode, u32 plen,
                             const void *param, u8 event, u32 timeout,
                             struct sock *sk);
+int hci_cmd_sync_status(struct hci_dev *hdev, u16 opcode, u32 plen,
+                       const void *param, u32 timeout);
 
 void hci_cmd_sync_init(struct hci_dev *hdev);
 void hci_cmd_sync_clear(struct hci_dev *hdev);

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index dbbe5e2da21070c0b68eb5029f2814323d1853fe..c644b30977bd8e41b65c20ee04949e21d01c2a16 100644 (file)
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -63,50 +63,6 @@ DEFINE_MUTEX(hci_cb_list_lock);
 /* HCI ID Numbering */
 static DEFINE_IDA(hci_index_ida);
 
-static int hci_scan_req(struct hci_request *req, unsigned long opt)
-{
-       __u8 scan = opt;
-
-       BT_DBG("%s %x", req->hdev->name, scan);
-
-       /* Inquiry and Page scans */
-       hci_req_add(req, HCI_OP_WRITE_SCAN_ENABLE, 1, &scan);
-       return 0;
-}
-
-static int hci_auth_req(struct hci_request *req, unsigned long opt)
-{
-       __u8 auth = opt;
-
-       BT_DBG("%s %x", req->hdev->name, auth);
-
-       /* Authentication */
-       hci_req_add(req, HCI_OP_WRITE_AUTH_ENABLE, 1, &auth);
-       return 0;
-}
-
-static int hci_encrypt_req(struct hci_request *req, unsigned long opt)
-{
-       __u8 encrypt = opt;
-
-       BT_DBG("%s %x", req->hdev->name, encrypt);
-
-       /* Encryption */
-       hci_req_add(req, HCI_OP_WRITE_ENCRYPT_MODE, 1, &encrypt);
-       return 0;
-}
-
-static int hci_linkpol_req(struct hci_request *req, unsigned long opt)
-{
-       __le16 policy = cpu_to_le16(opt);
-
-       BT_DBG("%s %x", req->hdev->name, policy);
-
-       /* Default link policy */
-       hci_req_add(req, HCI_OP_WRITE_DEF_LINK_POLICY, 2, &policy);
-       return 0;
-}
-
 /* Get HCI device by index.
  * Device is held on return. */
 struct hci_dev *hci_dev_get(int index)
@@ -735,6 +691,7 @@ int hci_dev_cmd(unsigned int cmd, void __user *arg)
 {
        struct hci_dev *hdev;
        struct hci_dev_req dr;
+       __le16 policy;
        int err = 0;
 
        if (copy_from_user(&dr, arg, sizeof(dr)))
@@ -761,8 +718,8 @@ int hci_dev_cmd(unsigned int cmd, void __user *arg)
 
        switch (cmd) {
        case HCISETAUTH:
-               err = hci_req_sync(hdev, hci_auth_req, dr.dev_opt,
-                                  HCI_INIT_TIMEOUT, NULL);
+               err = __hci_cmd_sync_status(hdev, HCI_OP_WRITE_AUTH_ENABLE,
+                                           1, &dr.dev_opt, HCI_CMD_TIMEOUT);
                break;
 
        case HCISETENCRYPT:
@@ -773,19 +730,23 @@ int hci_dev_cmd(unsigned int cmd, void __user *arg)
 
                if (!test_bit(HCI_AUTH, &hdev->flags)) {
                        /* Auth must be enabled first */
-                       err = hci_req_sync(hdev, hci_auth_req, dr.dev_opt,
-                                          HCI_INIT_TIMEOUT, NULL);
+                       err = __hci_cmd_sync_status(hdev,
+                                                   HCI_OP_WRITE_AUTH_ENABLE,
+                                                   1, &dr.dev_opt,
+                                                   HCI_CMD_TIMEOUT);
                        if (err)
                                break;
                }
 
-               err = hci_req_sync(hdev, hci_encrypt_req, dr.dev_opt,
-                                  HCI_INIT_TIMEOUT, NULL);
+               err = __hci_cmd_sync_status(hdev, HCI_OP_WRITE_ENCRYPT_MODE,
+                                           1, &dr.dev_opt,
+                                           HCI_CMD_TIMEOUT);
                break;
 
        case HCISETSCAN:
-               err = hci_req_sync(hdev, hci_scan_req, dr.dev_opt,
-                                  HCI_INIT_TIMEOUT, NULL);
+               err = __hci_cmd_sync_status(hdev, HCI_OP_WRITE_SCAN_ENABLE,
+                                           1, &dr.dev_opt,
+                                           HCI_CMD_TIMEOUT);
 
                /* Ensure that the connectable and discoverable states
                 * get correctly modified as this was a non-mgmt change.
@@ -795,8 +756,11 @@ int hci_dev_cmd(unsigned int cmd, void __user *arg)
                break;
 
        case HCISETLINKPOL:
-               err = hci_req_sync(hdev, hci_linkpol_req, dr.dev_opt,
-                                  HCI_INIT_TIMEOUT, NULL);
+               policy = cpu_to_le16(dr.dev_opt);
+
+               err = __hci_cmd_sync_status(hdev, HCI_OP_WRITE_DEF_LINK_POLICY,
+                                           2, &policy,
+                                           HCI_CMD_TIMEOUT);
                break;
 
        case HCISETLINKMODE:

diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
index a8a7d2b368701b70585ea6cd7a24d5a0c3d5fed1..eea34e6a236fd17763bfce47aeeb2f7152510dfe 100644 (file)
--- a/net/bluetooth/hci_sync.c
+++ b/net/bluetooth/hci_sync.c
@@ -280,6 +280,19 @@ int __hci_cmd_sync_status(struct hci_dev *hdev, u16 opcode, u32 plen,
 }
 EXPORT_SYMBOL(__hci_cmd_sync_status);
 
+int hci_cmd_sync_status(struct hci_dev *hdev, u16 opcode, u32 plen,
+                       const void *param, u32 timeout)
+{
+       int err;
+
+       hci_req_sync_lock(hdev);
+       err = __hci_cmd_sync_status(hdev, opcode, plen, param, timeout);
+       hci_req_sync_unlock(hdev);
+
+       return err;
+}
+EXPORT_SYMBOL(hci_cmd_sync_status);
+
 static void hci_cmd_sync_work(struct work_struct *work)
 {
        struct hci_dev *hdev = container_of(work, struct hci_dev, cmd_sync_work);

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index aed025734d04798a8e0f6cf040ce57f031e4d35e..c3c26bbb5ddaebd2c4faa0da887774a32688b71c 100644 (file)
--- 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 962aa11ce3de7f88accd37b83819599e993c7dc0..ba437c6f6ee591a5624f5fbfbf28f2a80d399372 100644 (file)
--- 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;
 }