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

[PATCH v3] 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 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 | 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: [PATCH v3] Bluetooth: MGMT: Fix discovery state race against cmd_sync worker

[Luiz Augusto von Dentz]

Hi Xiuzhuo,

On Wed, Jul 8, 2026 at 5:38 AM Xiuzhuo Shang
<xiuzhuo.shang@oss.qualcomm.com> 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 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 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 | 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);

I guess I will need to repeat myself, now that the callbacks do
acquire hdev lock the comments above are invalid, the callback will
attempt to hdev->lock making it wait the hci_dev_unlock bellow past
setting DISCOVERY_STARTING, so the change above is no longer needed.

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


-- 
Luiz Augusto von Dentz

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

[Xiuzhuo Shang]

On 7/8/2026 9:51 PM, Luiz Augusto von Dentz wrote:
> Hi Xiuzhuo,
> 
> On Wed, Jul 8, 2026 at 5:38 AM Xiuzhuo Shang
> <xiuzhuo.shang@oss.qualcomm.com> 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 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 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 | 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);
> 
> I guess I will need to repeat myself, now that the callbacks do
> acquire hdev lock the comments above are invalid, the callback will
> attempt to hdev->lock making it wait the hci_dev_unlock bellow past
> setting DISCOVERY_STARTING, so the change above is no longer needed.
 Hi Luiz,
Thank you for the clarification. You are correct.

With the hci_dev_lock() / hci_dev_unlock() added to the completion callbacks in Part 2, the callbacks are forced to
wait until the caller releases hdev->lock. Since the caller holds hdev->lock across the entire hci_cmd_sync_queue()
call and the trailing hci_discovery_set_state(STARTING / STOPPING), by the time any callback can acquire hci_dev_lock,
the transient state is already published. Part 1 is therefore redundant and has been removed in v4.
Changes in v4:
- Drop the set_state-before-queue reordering (Part 1 in v1–v3)
- Update commit message from "Fix it in three parts" to "two parts" and revise the description to explain the locking

Will send v4.
> 
>>         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
>>
> 
>