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

[PATCH v1] 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 err < 0, 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.
Signed-off-by: Xiuzhuo Shang <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..25ad9c10740d 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 < 0) {
+		/* 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 < 0) {
+		/* 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 v1] Bluetooth: MGMT: Fix discovery state race against cmd_sync worker

[Luiz Augusto von Dentz]

Hi Xiuzhuo,

On Tue, Jul 7, 2026 at 5:34 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 err < 0, 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.

Needs the Fixes tag since we might want to backport.

> Signed-off-by: Xiuzhuo Shang <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..25ad9c10740d 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 < 0) {
> +               /* 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);

Hmm, do we really need to do this if hdev lock is taken at
start_discovery_sync? Because that should then synchronize the
callback won't be able to change the state before it is set to
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);

Ditto.

>         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 < 0) {
> +               /* 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);

Ditto.

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

[Luiz Augusto von Dentz]

Hi Xiuzhuo,

On Tue, Jul 7, 2026 at 10:09 AM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Xiuzhuo,
>
> On Tue, Jul 7, 2026 at 5:34 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 err < 0, 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.
>
> Needs the Fixes tag since we might want to backport.
>
> > Signed-off-by: Xiuzhuo Shang <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..25ad9c10740d 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 < 0) {

Sashiko flags the above check as an issue because it tests only for
negative errors, not HCI (positive) errors:

https://sashiko.dev/#/patchset/20260707093426.372897-1-xiuzhuo.shang%40oss.qualcomm.com

> > +               /* 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);
>
> Hmm, do we really need to do this if hdev lock is taken at
> start_discovery_sync? Because that should then synchronize the
> callback won't be able to change the state before it is set to
> 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);
>
> Ditto.
>
> >         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 < 0) {
> > +               /* 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);
>
> Ditto.
>
> >         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



-- 
Luiz Augusto von Dentz

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

[Xiuzhuo Shang]

On 7/7/2026 11:17 PM, Luiz Augusto von Dentz wrote:
> Hi Xiuzhuo,
> 
> On Tue, Jul 7, 2026 at 10:09 AM Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
>>
>> Hi Xiuzhuo,
>>
>> On Tue, Jul 7, 2026 at 5:34 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 err < 0, 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.
>>
>> Needs the Fixes tag since we might want to backport.
Acknowledged. The bug was introduced by:                                    
                                                                              
  Fixes: abfeea476c68 ("Bluetooth: hci_sync: Convert MGMT_OP_START_DISCOVERY")
                                                                              
  Will add in v2.
>>
>>> Signed-off-by: Xiuzhuo Shang <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..25ad9c10740d 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 < 0) {
> 
> Sashiko flags the above check as an issue because it tests only for
> negative errors, not HCI (positive) errors:
> 
> https://sashiko.dev/#/patchset/20260707093426.372897-1-xiuzhuo.shang%40oss.qualcomm.com

  The check should be if (err) rather than if (err < 0). The existing tail already uses:                    
                                                                                                            
  hci_discovery_set_state(hdev, err ? DISCOVERY_STOPPED : DISCOVERY_FINDING);                               
                                                                                                            
  which catches both negative errno values and positive HCI status codes. The if (err < 0) guard in the new 
  block is inconsistent and would miss positive HCI errors, leaving the state wedged in STARTING. Will fix  
  in v2. 
> 
>>> +               /* 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);
>>
>> Hmm, do we really need to do this if hdev lock is taken at
>> start_discovery_sync? Because that should then synchronize the
>> callback won't be able to change the state before it is set to
>> DISCOVERY_STARTING.
  Yes, the move is necessary. The old discov_update worker was safe for a     
  different reason: it never wrote the transient states (STARTING/STOPPING) — 
  those were always written by the caller before queue_work(), and the caller 
  wrote nothing after. The worker only wrote the terminal states              
  (FINDING/STOPPED) after completing the HCI work. There was no conflict      
  because only one side wrote each state.                                     
                                                                              
  With hci_cmd_sync_queue(), the completion callback now writes the terminal  
  state, but the caller still writes STARTING/STOPPING after the queue call.  
  Two threads write discovery.state in an uncoordinated way.                  
                                                                              
  The reason hdev->lock does not prevent the race is that the completion
  callback runs under hci_req_sync_lock, not hdev->lock. Looking at
  hci_cmd_sync_work() in hci_sync.c:    
 
  hci_req_sync_lock(hdev);
  err = entry->func(hdev, entry->data);       /* start_discovery_sync */
  if (entry->destroy)                   
      entry->destroy(hdev, entry->data, err); /* start_discovery_complete */
  hci_req_sync_unlock(hdev);

  hci_dev_lock is mutex_lock(&hdev->lock) and hci_req_sync_lock is
  mutex_lock(&hdev->req_sync_lock) — two independent mutexes. The caller holds
   hdev->lock when it calls hci_cmd_sync_queue(), but the worker only needs
  hci_req_sync_lock to run, so it can proceed and complete concurrently:

  Caller (holds hdev->lock)            Worker (holds hci_req_sync_lock)

  hci_cmd_sync_queue() ──────────────► enqueued, returns immediately
    [still holds hdev->lock]           start_discovery_sync()
                                       start_discovery_complete()
                                         set_state(FINDING)  ← ① written first
                                       hci_req_sync_unlock()

  set_state(STARTING)  ← ② overwrites FINDING → WEDGE
  hci_dev_unlock()

  We have a debug-instrumented kernel log from a QCS8300 device that captures
  the race directly:  
                  
  [T8668] stop-discovery complete: err=0  state=2  <- FINDING; caller has not
  yet written STOPPING                  
  [T8668] set_state(STOPPED)                       <- worker writes terminal
  state                                 
  [T2216] set_state(STOPPING)                      <- caller overwrites
  STOPPED                               
  [T2216] REJECT BUSY: state=4                     <- every subsequent
  StartDiscovery refused                

  state=2 (FINDING) at the completion callback entry proves the caller had not
   yet executed the trailing set_state(STOPPING). Moving set_state to before
  hci_cmd_sync_queue() — while hdev->lock is still held — ensures the
  transient state is always visible before the worker can call the completion.

  Will send v2 with the above corrections.

>>
>>>         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);
>>
>> Ditto.
>>
>>>         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 < 0) {
>>> +               /* 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);
>>
>> Ditto.
>>
>>>         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
> 
> 
>