diff mbox series

[v2] Bluetooth: hci_sync: Push sync command cancellation to workqueue

Message ID 20211217152809.671966-1-benjamin@sipsolutions.net (mailing list archive)
State Accepted
Commit 5b2004d13a5a8b5ad0cf0836a75bafed5b293ab6
Headers show
Series [v2] Bluetooth: hci_sync: Push sync command cancellation to workqueue | expand

Checks

Context Check Description
tedd_an/checkpatch fail [v2] Bluetooth: hci_sync: Push sync command cancellation to workqueue\WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line) #72: Reported-and-tested-by: syzbot+485cc00ea7cf41dfdbf1@syzkaller.appspotmail.com total: 0 errors, 1 warnings, 0 checks, 80 lines checked NOTE: For some of the reported defects, checkpatch may be able to mechanically convert to the typical style using --fix or --fix-inplace. /github/workspace/src/12685071.patch has style problems, please review. NOTE: Ignored message types: UNKNOWN_COMMIT_ID NOTE: If any of the errors are false positives, please report them to the maintainer, see CHECKPATCH in MAINTAINERS.
tedd_an/gitlint fail [v2] Bluetooth: hci_sync: Push sync command cancellation to workqueue 14: B1 Line exceeds max length (85>80): "Fixes: c97a747efc93 ("Bluetooth: btusb: Cancel sync commands for certain URB errors")"
tedd_an/buildkernel success Build Kernel PASS
tedd_an/incremental_build success Pass
tedd_an/testrunnersetup success Test Runner Setup PASS
tedd_an/testrunnerl2cap-tester success Total: 40, Passed: 40 (100.0%), Failed: 0, Not Run: 0
tedd_an/testrunnerbnep-tester success Total: 1, Passed: 1 (100.0%), Failed: 0, Not Run: 0
tedd_an/testrunnermgmt-tester success Total: 493, Passed: 493 (100.0%), Failed: 0, Not Run: 0
tedd_an/testrunnerrfcomm-tester success Total: 9, Passed: 9 (100.0%), Failed: 0, Not Run: 0
tedd_an/testrunnersco-tester success Total: 12, Passed: 12 (100.0%), Failed: 0, Not Run: 0
tedd_an/testrunnersmp-tester success Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0
tedd_an/testrunneruserchan-tester success Total: 4, Passed: 4 (100.0%), Failed: 0, Not Run: 0

Commit Message

Benjamin Berg Dec. 17, 2021, 3:28 p.m. UTC 
From: Benjamin Berg <bberg@redhat.com>

syzbot reported that hci_cmd_sync_cancel may sleep from the wrong
context. To avoid this, create a new work item that pushes the relevant
parts into a different context.

Note that we keep the old implementation with the name
__hci_cmd_sync_cancel as the sleeping behaviour is desired in some
cases.

Reported-and-tested-by: syzbot+485cc00ea7cf41dfdbf1@syzkaller.appspotmail.com
Fixes: c97a747efc93 ("Bluetooth: btusb: Cancel sync commands for certain URB errors")
Signed-off-by: Benjamin Berg <bberg@redhat.com>

---

v2:
 - Add Signed-off-by and changed email address
---
 include/net/bluetooth/hci_core.h |  1 +
 include/net/bluetooth/hci_sync.h |  1 +
 net/bluetooth/hci_core.c         |  2 +-
 net/bluetooth/hci_request.c      |  2 +-
 net/bluetooth/hci_sync.c         | 27 ++++++++++++++++++++++++++-
 5 files changed, 30 insertions(+), 3 deletions(-)

Comments

Marcel Holtmann Dec. 22, 2021, 8:39 a.m. UTC | #1 
Hi Benjamin,

> syzbot reported that hci_cmd_sync_cancel may sleep from the wrong
> context. To avoid this, create a new work item that pushes the relevant
> parts into a different context.
> 
> Note that we keep the old implementation with the name
> __hci_cmd_sync_cancel as the sleeping behaviour is desired in some
> cases.
> 
> Reported-and-tested-by: syzbot+485cc00ea7cf41dfdbf1@syzkaller.appspotmail.com
> Fixes: c97a747efc93 ("Bluetooth: btusb: Cancel sync commands for certain URB errors")
> Signed-off-by: Benjamin Berg <bberg@redhat.com>
> 
> ---
> 
> v2:
> - Add Signed-off-by and changed email address
> ---
> include/net/bluetooth/hci_core.h |  1 +
> include/net/bluetooth/hci_sync.h |  1 +
> net/bluetooth/hci_core.c         |  2 +-
> net/bluetooth/hci_request.c      |  2 +-
> net/bluetooth/hci_sync.c         | 27 ++++++++++++++++++++++++++-
> 5 files changed, 30 insertions(+), 3 deletions(-)

patch has been applied to bluetooth-next tree.

Regards

Marcel
diff mbox series

Patch

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 4d69dcfebd63..6509109c2413 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -480,6 +480,7 @@  struct hci_dev {
 	struct work_struct	cmd_sync_work;
 	struct list_head	cmd_sync_work_list;
 	struct mutex		cmd_sync_work_lock;
+	struct work_struct	cmd_sync_cancel_work;
 
 	__u16			discov_timeout;
 	struct delayed_work	discov_off;
diff --git a/include/net/bluetooth/hci_sync.h b/include/net/bluetooth/hci_sync.h
index f4034bf8f1ce..435674cf388e 100644
--- a/include/net/bluetooth/hci_sync.h
+++ b/include/net/bluetooth/hci_sync.h
@@ -38,6 +38,7 @@  int __hci_cmd_sync_status_sk(struct hci_dev *hdev, u16 opcode, u32 plen,
 void hci_cmd_sync_init(struct hci_dev *hdev);
 void hci_cmd_sync_clear(struct hci_dev *hdev);
 void hci_cmd_sync_cancel(struct hci_dev *hdev, int err);
+void __hci_cmd_sync_cancel(struct hci_dev *hdev, int err);
 
 int hci_cmd_sync_queue(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
 		       void *data, hci_cmd_sync_work_destroy_t destroy);
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 38063bf1fdc5..2b7bd3655b07 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -3859,7 +3859,7 @@  static void hci_cmd_work(struct work_struct *work)
 
 			res = hci_send_frame(hdev, skb);
 			if (res < 0)
-				hci_cmd_sync_cancel(hdev, -res);
+				__hci_cmd_sync_cancel(hdev, -res);
 
 			if (test_bit(HCI_RESET, &hdev->flags))
 				cancel_delayed_work(&hdev->cmd_timer);
diff --git a/net/bluetooth/hci_request.c b/net/bluetooth/hci_request.c
index 329c66456cf1..ef5ced467f75 100644
--- a/net/bluetooth/hci_request.c
+++ b/net/bluetooth/hci_request.c
@@ -2692,7 +2692,7 @@  void hci_request_setup(struct hci_dev *hdev)
 
 void hci_request_cancel_all(struct hci_dev *hdev)
 {
-	hci_cmd_sync_cancel(hdev, ENODEV);
+	__hci_cmd_sync_cancel(hdev, ENODEV);
 
 	cancel_work_sync(&hdev->discov_update);
 	cancel_work_sync(&hdev->scan_update);
diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
index fd15fb37a52a..2fb8bc496d18 100644
--- a/net/bluetooth/hci_sync.c
+++ b/net/bluetooth/hci_sync.c
@@ -313,11 +313,24 @@  static void hci_cmd_sync_work(struct work_struct *work)
 	}
 }
 
+static void hci_cmd_sync_cancel_work(struct work_struct *work)
+{
+	struct hci_dev *hdev = container_of(work, struct hci_dev, cmd_sync_cancel_work);
+
+	cancel_delayed_work_sync(&hdev->cmd_timer);
+	cancel_delayed_work_sync(&hdev->ncmd_timer);
+	atomic_set(&hdev->cmd_cnt, 1);
+
+	wake_up_interruptible(&hdev->req_wait_q);
+}
+
 void hci_cmd_sync_init(struct hci_dev *hdev)
 {
 	INIT_WORK(&hdev->cmd_sync_work, hci_cmd_sync_work);
 	INIT_LIST_HEAD(&hdev->cmd_sync_work_list);
 	mutex_init(&hdev->cmd_sync_work_lock);
+
+	INIT_WORK(&hdev->cmd_sync_cancel_work, hci_cmd_sync_cancel_work);
 }
 
 void hci_cmd_sync_clear(struct hci_dev *hdev)
@@ -335,7 +348,7 @@  void hci_cmd_sync_clear(struct hci_dev *hdev)
 	}
 }
 
-void hci_cmd_sync_cancel(struct hci_dev *hdev, int err)
+void __hci_cmd_sync_cancel(struct hci_dev *hdev, int err)
 {
 	bt_dev_dbg(hdev, "err 0x%2.2x", err);
 
@@ -350,6 +363,18 @@  void hci_cmd_sync_cancel(struct hci_dev *hdev, int err)
 		wake_up_interruptible(&hdev->req_wait_q);
 	}
 }
+
+void hci_cmd_sync_cancel(struct hci_dev *hdev, int err)
+{
+	bt_dev_dbg(hdev, "err 0x%2.2x", err);
+
+	if (hdev->req_status == HCI_REQ_PEND) {
+		hdev->req_result = err;
+		hdev->req_status = HCI_REQ_CANCELED;
+
+		queue_work(hdev->workqueue, &hdev->cmd_sync_cancel_work);
+	}
+}
 EXPORT_SYMBOL(hci_cmd_sync_cancel);
 
 int hci_cmd_sync_queue(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,