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@xxxxxxxxx> wrote:
>
> Hi Xiuzhuo,
>
> On Tue, Jul 7, 2026 at 5:34 AM Xiuzhuo Shang
> <xiuzhuo.shang@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxx>
> > ---
> >  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