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

[Xiuzhuo Shang <xiuzhuo.shang@oss.qualcomm.com>]

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)