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

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

[Xiuzhuo Shang]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 6852 bytes --]

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 two parts:

  1. 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; the caller holds hdev->lock across hci_cmd_sync_queue()
     and the trailing set_state(STARTING / STOPPING), so the callback's
     hci_dev_lock() blocks until the caller has published the transient
     state and released the lock. This serialises the state writes and
     closes the race window without needing to reorder the set_state
     call relative to hci_cmd_sync_queue().

  2. Generalise the "ignore -ECANCELED" early return in both completion
     callbacks to "on any non-zero err, also reset the transient state
     to STOPPED".

     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 v4:
 - Drop the "move set_state before hci_cmd_sync_queue" change (Part 1
   in v1-v3): now that the completion callbacks acquire hci_dev_lock,
   they block until the caller releases hdev->lock — which happens only
   after set_state(STARTING/STOPPING) has been written. The lock
   acquisition therefore serialises the state writes without reordering
   the set_state call.
 - Update commit message from "Fix it in three parts" to "two parts"
   and revise Part 1 description to explain the locking argument.
 - Link to v3:
   https://lore.kernel.org/all/20260708093822.3495633-1-xiuzhuo.shang@oss.qualcomm.com/

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@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:
   https://lore.kernel.org/all/20260707093426.372897-1-xiuzhuo.shang@oss.qualcomm.com/

 net/bluetooth/mgmt.c | 58 +++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 52 insertions(+), 6 deletions(-)

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 733a4b70e10c..2295042234f8 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; the callback's hci_dev_lock() blocked
+		 * until the caller wrote the transient state and released
+		 * the lock). 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)
@@ -6196,17 +6219,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; the callback's hci_dev_lock() blocked
+		 * until the caller wrote the transient state and released
+		 * the lock). 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)
-- 
2.43.0

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

[Xiuzhuo Shang]

On 7/10/2026 1:19 PM, Xiuzhuo Shang wrote:
> 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 two parts:
> 
>   1. 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; the caller holds hdev->lock across hci_cmd_sync_queue()
>      and the trailing set_state(STARTING / STOPPING), so the callback's
>      hci_dev_lock() blocks until the caller has published the transient
>      state and released the lock. This serialises the state writes and
>      closes the race window without needing to reorder the set_state
>      call relative to hci_cmd_sync_queue().
> 
>   2. Generalise the "ignore -ECANCELED" early return in both completion
>      callbacks to "on any non-zero err, also reset the transient state
>      to STOPPED".
> 
>      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 v4:
>  - Drop the "move set_state before hci_cmd_sync_queue" change (Part 1
>    in v1-v3): now that the completion callbacks acquire hci_dev_lock,
>    they block until the caller releases hdev->lock — which happens only
>    after set_state(STARTING/STOPPING) has been written. The lock
>    acquisition therefore serialises the state writes without reordering
>    the set_state call.
>  - Update commit message from "Fix it in three parts" to "two parts"
>    and revise Part 1 description to explain the locking argument.
>  - Link to v3:
>    https://lore.kernel.org/all/20260708093822.3495633-1-xiuzhuo.shang@oss.qualcomm.com/
> 
> 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@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:
>    https://lore.kernel.org/all/20260707093426.372897-1-xiuzhuo.shang@oss.qualcomm.com/
> 
>  net/bluetooth/mgmt.c | 58 +++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 52 insertions(+), 6 deletions(-)
> 
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 733a4b70e10c..2295042234f8 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; the callback's hci_dev_lock() blocked
> +		 * until the caller wrote the transient state and released
> +		 * the lock). 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)
> @@ -6196,17 +6219,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; the callback's hci_dev_lock() blocked
> +		 * until the caller wrote the transient state and released
> +		 * the lock). 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);
> +	}
>  }
>  
Hi, Luiz, do you still have suggestions in this change? 
>  static int stop_discovery_sync(struct hci_dev *hdev, void *data)