Skip to content

Commit

Permalink
Bluetooth: L2CAP: Fix deadlock
Browse files Browse the repository at this point in the history 
commit f1a8f40 upstream.

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: 89e856e ("bluetooth/l2cap: sync sock recv cb and release")
Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
  • Loading branch information
@Vudentz @gregkh
Vudentz authored and gregkh committed Jul 23, 2024
1 parent 40c4e57 commit ccb743e
Show file tree
Hide file tree
Showing 5 changed files with 37 additions and 66 deletions.
2 changes: 2 additions & 0 deletions include/net/bluetooth/hci_sync.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
72 changes: 18 additions & 54 deletions net/bluetooth/hci_core.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -728,6 +684,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)))
Expand All @@ -754,8 +711,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:
Expand All @@ -766,19 +723,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.
Expand All @@ -788,8 +749,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:
Expand Down
13 changes: 13 additions & 0 deletions net/bluetooth/hci_sync.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
3 changes: 3 additions & 0 deletions net/bluetooth/l2cap_core.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6762,6 +6762,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;

Expand All @@ -6778,6 +6780,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);
Expand Down
13 changes: 1 addition & 12 deletions net/bluetooth/l2cap_sock.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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;
}
Expand Down

0 comments on commit ccb743e

Please sign in to comment.