* [PATCH v1] Bluetooth: MGMT: Fix discovery state race against cmd_sync worker
@ 2026-07-07 9:34 Xiuzhuo Shang
2026-07-07 14:09 ` Luiz Augusto von Dentz
0 siblings, 1 reply; 4+ messages in thread
From: Xiuzhuo Shang @ 2026-07-07 9:34 UTC (permalink / raw)
To: Bartosz Golaszewski, Marcel Holtmann, Luiz Augusto von Dentz
Cc: linux-arm-msm, linux-bluetooth, linux-kernel, cheng.jiang,
quic_chezhou, wei.deng, shuai.zhang, mengshi.wu, jinwang.li,
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v1] Bluetooth: MGMT: Fix discovery state race against cmd_sync worker
2026-07-07 9:34 [PATCH v1] Bluetooth: MGMT: Fix discovery state race against cmd_sync worker Xiuzhuo Shang
@ 2026-07-07 14:09 ` Luiz Augusto von Dentz
2026-07-07 15:17 ` Luiz Augusto von Dentz
0 siblings, 1 reply; 4+ messages in thread
From: Luiz Augusto von Dentz @ 2026-07-07 14:09 UTC (permalink / raw)
To: Xiuzhuo Shang
Cc: Bartosz Golaszewski, Marcel Holtmann, linux-arm-msm,
linux-bluetooth, linux-kernel, cheng.jiang, quic_chezhou,
wei.deng, shuai.zhang, mengshi.wu, jinwang.li
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v1] Bluetooth: MGMT: Fix discovery state race against cmd_sync worker
2026-07-07 14:09 ` Luiz Augusto von Dentz
@ 2026-07-07 15:17 ` Luiz Augusto von Dentz
2026-07-08 5:59 ` Xiuzhuo Shang
0 siblings, 1 reply; 4+ messages in thread
From: Luiz Augusto von Dentz @ 2026-07-07 15:17 UTC (permalink / raw)
To: Xiuzhuo Shang
Cc: Bartosz Golaszewski, Marcel Holtmann, linux-arm-msm,
linux-bluetooth, linux-kernel, cheng.jiang, quic_chezhou,
wei.deng, shuai.zhang, mengshi.wu, jinwang.li
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v1] Bluetooth: MGMT: Fix discovery state race against cmd_sync worker
2026-07-07 15:17 ` Luiz Augusto von Dentz
@ 2026-07-08 5:59 ` Xiuzhuo Shang
0 siblings, 0 replies; 4+ messages in thread
From: Xiuzhuo Shang @ 2026-07-08 5:59 UTC (permalink / raw)
To: Luiz Augusto von Dentz
Cc: Bartosz Golaszewski, Marcel Holtmann, linux-arm-msm,
linux-bluetooth, linux-kernel, cheng.jiang, quic_chezhou,
wei.deng, shuai.zhang, mengshi.wu, jinwang.li
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
>
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2026-07-08 5:59 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-07-07 9:34 [PATCH v1] Bluetooth: MGMT: Fix discovery state race against cmd_sync worker Xiuzhuo Shang
2026-07-07 14:09 ` Luiz Augusto von Dentz
2026-07-07 15:17 ` Luiz Augusto von Dentz
2026-07-08 5:59 ` Xiuzhuo Shang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®