[PATCH v2] Bluetooth: MGMT: Fix discovery state race against cmd_sync worker

[PATCH v2] Bluetooth: MGMT: Fix discovery state race against cmd_sync worker

[Xiuzhuo Shang]

start_discovery_internal(), start_service_discovery() and stop_discovery()
queue a cmd_sync work item and only then move the discovery state machine
into its transient value (DISCOVERY_STARTING / DISCOVERY_STOPPING):

  err = hci_cmd_sync_queue(hdev, ..._sync, cmd, ..._complete);
  if (err < 0) { ... }
  hci_discovery_set_state(hdev, DISCOVERY_STARTING /* or STOPPING */);

The matching completion callbacks run on hdev->req_workqueue serialised
by hci_req_sync_lock, which is independent of hdev->lock. So once the
work has been queued, the worker can be scheduled, run the sync function
and invoke the completion before the caller has executed the trailing
hci_discovery_set_state(). The completion's success path writes the
terminal state (DISCOVERY_STOPPED for stop, DISCOVERY_FINDING for start);
the caller then overwrites it with the transient value, and the state
machine is wedged: every subsequent Start (Service) Discovery is
rejected by the DISCOVERY_STOPPED gate with MGMT_STATUS_BUSY (0x0a),
with no HCI traffic generated, until bluetoothd or the adapter is
restarted.

Fix it in three parts:

  1. In all three call sites move hci_discovery_set_state(STARTING /
     STOPPING) to before hci_cmd_sync_queue(). The transient state is
     therefore always published before any worker can run the
     completion. On queue-submit failure, roll back to
     DISCOVERY_STOPPED.

  2. In start_discovery_complete() and stop_discovery_complete(), wrap
     the terminal hci_discovery_set_state() call with
     hci_dev_lock() / hci_dev_unlock(). These callbacks run without
     hdev->lock; serialising the state write matches the pattern used
     by mgmt_set_powered_complete() and removes any residual ordering
     hazard against a concurrent mgmt path holding hdev->lock.

  3. Generalise the "ignore -ECANCELED" early return in both completion
     callbacks to "on any non-zero err, also reset the transient state
     to STOPPED". With (1) in place the state observed at completion
     time is always known.

     For the stop path this also fixes a pre-existing wedge: when any
     sub-command issued from hci_stop_discovery_sync() returns an
     error, stop_discovery_complete() is invoked with err != 0. The
     existing "if (!err) set_state(STOPPED)" tail then skips the reset
     and the state machine sits in DISCOVERY_STOPPING forever.

Fixes: abfeea476c68 ("Bluetooth: hci_sync: Convert MGMT_OP_START_DISCOVERY")
Signed-off-by: Xiuzhuo Shang <xiuzhuo.shang@oss.qualcomm.com>
---
Changes in v2:
 - Fix if (err < 0) to if (err) in both start_discovery_complete() and
   stop_discovery_complete() to also catch positive HCI status codes,
   flagged by Sashiko.
 - Add Fixes: tag for commit abfeea476c68 as requested.
 - Update commit message wording from "err < 0" to "non-zero err" to
   match the code change.
 - Link to v1
   [PATCH v1] Bluetooth: MGMT: Fix discovery state race against cmd_sync worker - Xiuzhuo Shang

 net/bluetooth/mgmt.c | 86 +++++++++++++++++++++++++++++++++++++-------
 1 file changed, 74 insertions(+), 12 deletions(-)

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 733a4b70e10c..81c09c24a14b 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -5975,15 +5975,38 @@ static void start_discovery_complete(struct hci_dev *hdev, void *data, int err)
 
 	bt_dev_dbg(hdev, "err %d", err);
 
-	if (err == -ECANCELED || !mgmt_pending_valid(hdev, cmd))
+	if (err) {
+		/* The queued start-discovery work failed before the normal
+		 * completion path could advance the state machine. The
+		 * caller already moved the state to DISCOVERY_STARTING
+		 * (under hdev->lock, before queueing). Reset it here so the
+		 * gate in start_discovery_internal()/start_service_discovery()
+		 * does not wedge in STARTING and reject every future Start
+		 * (Service) Discovery with MGMT_STATUS_BUSY.
+		 */
+		hci_dev_lock(hdev);
+		if (hdev->discovery.state == DISCOVERY_STARTING)
+			hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
+		hci_dev_unlock(hdev);
+
+		if (err == -ECANCELED)
+			return;
+	}
+
+	if (!mgmt_pending_valid(hdev, cmd))
 		return;
 
 	mgmt_cmd_complete(cmd->sk, cmd->hdev->id, cmd->opcode, mgmt_status(err),
 			  cmd->param, 1);
 	mgmt_pending_free(cmd);
 
-	hci_discovery_set_state(hdev, err ? DISCOVERY_STOPPED:
+	/* Serialise discovery.state writes against any concurrent mgmt path
+	 * holding hdev->lock; this callback runs on req_workqueue without it.
+	 */
+	hci_dev_lock(hdev);
+	hci_discovery_set_state(hdev, err ? DISCOVERY_STOPPED :
 				DISCOVERY_FINDING);
+	hci_dev_unlock(hdev);
 }
 
 static int start_discovery_sync(struct hci_dev *hdev, void *data)
@@ -6051,15 +6074,23 @@ static int start_discovery_internal(struct sock *sk, struct hci_dev *hdev,
 		goto failed;
 	}
 
+	/* Publish the transient state BEFORE queueing the work. The
+	 * completion callback runs on hdev->req_workqueue serialised by
+	 * hci_req_sync_lock, which is independent of hdev->lock; setting
+	 * the state after the queue allowed the worker to win the race
+	 * and have its terminal STOPPED/FINDING write overwritten by this
+	 * trailing STARTING write, wedging discovery in STARTING.
+	 */
+	hci_discovery_set_state(hdev, DISCOVERY_STARTING);
+
 	err = hci_cmd_sync_queue(hdev, start_discovery_sync, cmd,
 				 start_discovery_complete);
 	if (err < 0) {
+		hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
 		mgmt_pending_remove(cmd);
 		goto failed;
 	}
 
-	hci_discovery_set_state(hdev, DISCOVERY_STARTING);
-
 failed:
 	hci_dev_unlock(hdev);
 	return err;
@@ -6178,15 +6209,19 @@ static int start_service_discovery(struct sock *sk, struct hci_dev *hdev,
 		}
 	}
 
+	/* Publish the transient state BEFORE queueing; see the comment in
+	 * start_discovery_internal() for the race details.
+	 */
+	hci_discovery_set_state(hdev, DISCOVERY_STARTING);
+
 	err = hci_cmd_sync_queue(hdev, start_discovery_sync, cmd,
 				 start_discovery_complete);
 	if (err < 0) {
+		hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
 		mgmt_pending_remove(cmd);
 		goto failed;
 	}
 
-	hci_discovery_set_state(hdev, DISCOVERY_STARTING);
-
 failed:
 	hci_dev_unlock(hdev);
 	return err;
@@ -6196,17 +6231,40 @@ static void stop_discovery_complete(struct hci_dev *hdev, void *data, int err)
 {
 	struct mgmt_pending_cmd *cmd = data;
 
-	if (err == -ECANCELED || !mgmt_pending_valid(hdev, cmd))
-		return;
-
 	bt_dev_dbg(hdev, "err %d", err);
 
+	if (err) {
+		/* The queued stop-discovery work failed before the normal
+		 * completion path could advance the state machine. The
+		 * caller already moved the state to DISCOVERY_STOPPING
+		 * (under hdev->lock, before queueing). Reset it here so
+		 * the gate does not wedge in STOPPING.
+		 */
+		hci_dev_lock(hdev);
+		if (hdev->discovery.state == DISCOVERY_STOPPING)
+			hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
+		hci_dev_unlock(hdev);
+
+		if (err == -ECANCELED)
+			return;
+	}
+
+	if (!mgmt_pending_valid(hdev, cmd))
+		return;
+
 	mgmt_cmd_complete(cmd->sk, cmd->hdev->id, cmd->opcode, mgmt_status(err),
 			  cmd->param, 1);
 	mgmt_pending_free(cmd);
 
-	if (!err)
+	if (!err) {
+		/* Serialise discovery.state writes against any concurrent
+		 * mgmt path holding hdev->lock; this callback runs on
+		 * req_workqueue without it.
+		 */
+		hci_dev_lock(hdev);
 		hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
+		hci_dev_unlock(hdev);
+	}
 }
 
 static int stop_discovery_sync(struct hci_dev *hdev, void *data)
@@ -6248,15 +6306,19 @@ static int stop_discovery(struct sock *sk, struct hci_dev *hdev, void *data,
 		goto unlock;
 	}
 
+	/* Publish the transient state BEFORE queueing; see the comment in
+	 * start_discovery_internal() for the race details.
+	 */
+	hci_discovery_set_state(hdev, DISCOVERY_STOPPING);
+
 	err = hci_cmd_sync_queue(hdev, stop_discovery_sync, cmd,
 				 stop_discovery_complete);
 	if (err < 0) {
+		hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
 		mgmt_pending_remove(cmd);
 		goto unlock;
 	}
 
-	hci_discovery_set_state(hdev, DISCOVERY_STOPPING);
-
 unlock:
 	hci_dev_unlock(hdev);
 	return err;
-- 
2.43.0

RE: [v2] Bluetooth: MGMT: Fix discovery state race against cmd_sync worker

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 2191 bytes --]

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=1123569

---Test result---

Test Summary:
CheckPatch                    PASS      0.79 seconds
VerifyFixes                   PASS      0.13 seconds
VerifySignedoff               PASS      0.13 seconds
GitLint                       FAIL      0.32 seconds
SubjectPrefix                 PASS      0.12 seconds
BuildKernel                   PASS      26.26 seconds
CheckAllWarning               PASS      28.82 seconds
CheckSparse                   PASS      27.61 seconds
BuildKernel32                 PASS      25.34 seconds
CheckKernelLLVM               SKIP      0.00 seconds
TestRunnerSetup               PASS      486.94 seconds
TestRunner_mgmt-tester        FAIL      204.24 seconds
TestRunner_mesh-tester        FAIL      24.96 seconds
IncrementalBuild              PASS      24.46 seconds

Details
##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
[v2] Bluetooth: MGMT: Fix discovery state race against cmd_sync worker

59: B1 Line exceeds max length (95>80): "   [PATCH v1] Bluetooth: MGMT: Fix discovery state race against cmd_sync worker - Xiuzhuo Shang"
##############################
Test: CheckKernelLLVM - SKIP
Desc: Build kernel with LLVM + context analysis
Output:
Clang not found
##############################
Test: TestRunner_mgmt-tester - FAIL
Desc: Run mgmt-tester with test-runner
Output:
Total: 494, Passed: 489 (99.0%), Failed: 1, Not Run: 4

Failed Test Cases
Read Exp Feature - Success                           Failed       0.234 seconds
##############################
Test: TestRunner_mesh-tester - FAIL
Desc: Run mesh-tester with test-runner
Output:
Total: 10, Passed: 8 (80.0%), Failed: 2, Not Run: 0

Failed Test Cases
Mesh - Send cancel - 1                               Timed out    2.045 seconds
Mesh - Send cancel - 2                               Timed out    1.991 seconds


https://github.com/bluez/bluetooth-next/pull/409

---
Regards,
Linux Bluetooth