[PATCH v3] Bluetooth: MGMT: Fix discovery state race against cmd_sync worker
From: Xiuzhuo Shang
Date: Wed Jul 08 2026 - 05:40:43 EST
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@xxxxxxxxxxxxxxxx>
---
Changes in v3:
- Replace inline patch title with lore.kernel.org URL in v2 link
reference to fix GitLint B1 line-length check.
- Link to v2:
https://lore.kernel.org/all/20260708062009.3047447-1-xiuzhuo.shang@xxxxxxxxxxxxxxxx/
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:
https://lore.kernel.org/all/20260707093426.372897-1-xiuzhuo.shang@xxxxxxxxxxxxxxxx/
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