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

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1903 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=1122959

---Test result---

Test Summary:
CheckPatch                    PASS      1.25 seconds
VerifyFixes                   PASS      0.13 seconds
VerifySignedoff               PASS      0.13 seconds
GitLint                       PASS      0.33 seconds
SubjectPrefix                 PASS      0.12 seconds
BuildKernel                   PASS      27.05 seconds
CheckAllWarning               PASS      29.58 seconds
CheckSparse                   PASS      28.81 seconds
BuildKernel32                 PASS      26.80 seconds
CheckKernelLLVM               SKIP      0.00 seconds
TestRunnerSetup               PASS      499.12 seconds
TestRunner_mgmt-tester        FAIL      219.48 seconds
TestRunner_mesh-tester        FAIL      24.52 seconds
IncrementalBuild              PASS      29.02 seconds

Details
##############################
Test: CheckKernelLLVM - SKIP
Desc: Build kernel with LLVM + context analysis
Output:
Clang not found
##############################
Test: TestRunner_mgmt-tester - FAIL
Desc: Run mgmt-tester with test-runner
Output:
Total: 494, Passed: 489 (99.0%), Failed: 1, Not Run: 4

Failed Test Cases
Read Exp Feature - Success                           Failed       0.247 seconds
##############################
Test: TestRunner_mesh-tester - FAIL
Desc: Run mesh-tester with test-runner
Output:
Total: 10, Passed: 8 (80.0%), Failed: 2, Not Run: 0

Failed Test Cases
Mesh - Send cancel - 1                               Timed out    2.436 seconds
Mesh - Send cancel - 2                               Failed       0.316 seconds


https://github.com/bluez/bluetooth-next/pull/407

---
Regards,
Linux Bluetooth

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