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

[Xiuzhuo Shang]

On 7/8/2026 9:51 PM, Luiz Augusto von Dentz wrote:
> Hi Xiuzhuo,
> 
> On Wed, Jul 8, 2026 at 5:38 AM Xiuzhuo Shang
> <xiuzhuo.shang@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 non-zero err, also reset the transient state
>>      to STOPPED". With (1) in place the state observed at completion
>>      time is always known.
>>
>>      For the stop path this also fixes a pre-existing wedge: when any
>>      sub-command issued from hci_stop_discovery_sync() returns an
>>      error, stop_discovery_complete() is invoked with err != 0. The
>>      existing "if (!err) set_state(STOPPED)" tail then skips the reset
>>      and the state machine sits in DISCOVERY_STOPPING forever.
>>
>> Fixes: abfeea476c68 ("Bluetooth: hci_sync: Convert MGMT_OP_START_DISCOVERY")
>> Signed-off-by: Xiuzhuo Shang <xiuzhuo.shang@xxxxxxxxxxxxxxxx>
>> ---
>> Changes in v3:
>>  - Replace inline patch title with lore.kernel.org URL in v2 link
>>    reference to fix GitLint B1 line-length check.
>>  - Link to v2:
>>    https://lore.kernel.org/all/20260708062009.3047447-1-xiuzhuo.shang@xxxxxxxxxxxxxxxx/
>>
>> Changes in v2:
>>  - Fix if (err < 0) to if (err) in both start_discovery_complete() and
>>    stop_discovery_complete() to also catch positive HCI status codes,
>>    flagged by Sashiko.
>>  - Add Fixes: tag for commit abfeea476c68 as requested.
>>  - Update commit message wording from "err < 0" to "non-zero err" to
>>    match the code change.
>>  - Link to v1:
>>    https://lore.kernel.org/all/20260707093426.372897-1-xiuzhuo.shang@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..81c09c24a14b 100644
>> --- a/net/bluetooth/mgmt.c
>> +++ b/net/bluetooth/mgmt.c
>> @@ -5975,15 +5975,38 @@ static void start_discovery_complete(struct hci_dev *hdev, void *data, int err)
>>
>>         bt_dev_dbg(hdev, "err %d", err);
>>
>> -       if (err == -ECANCELED || !mgmt_pending_valid(hdev, cmd))
>> +       if (err) {
>> +               /* The queued start-discovery work failed before the normal
>> +                * completion path could advance the state machine. The
>> +                * caller already moved the state to DISCOVERY_STARTING
>> +                * (under hdev->lock, before queueing). Reset it here so the
>> +                * gate in start_discovery_internal()/start_service_discovery()
>> +                * does not wedge in STARTING and reject every future Start
>> +                * (Service) Discovery with MGMT_STATUS_BUSY.
>> +                */
>> +               hci_dev_lock(hdev);
>> +               if (hdev->discovery.state == DISCOVERY_STARTING)
>> +                       hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
>> +               hci_dev_unlock(hdev);
>> +
>> +               if (err == -ECANCELED)
>> +                       return;
>> +       }
>> +
>> +       if (!mgmt_pending_valid(hdev, cmd))
>>                 return;
>>
>>         mgmt_cmd_complete(cmd->sk, cmd->hdev->id, cmd->opcode, mgmt_status(err),
>>                           cmd->param, 1);
>>         mgmt_pending_free(cmd);
>>
>> -       hci_discovery_set_state(hdev, err ? DISCOVERY_STOPPED:
>> +       /* Serialise discovery.state writes against any concurrent mgmt path
>> +        * holding hdev->lock; this callback runs on req_workqueue without it.
>> +        */
>> +       hci_dev_lock(hdev);
>> +       hci_discovery_set_state(hdev, err ? DISCOVERY_STOPPED :
>>                                 DISCOVERY_FINDING);
>> +       hci_dev_unlock(hdev);
>>  }
>>
>>  static int start_discovery_sync(struct hci_dev *hdev, void *data)
>> @@ -6051,15 +6074,23 @@ static int start_discovery_internal(struct sock *sk, struct hci_dev *hdev,
>>                 goto failed;
>>         }
>>
>> +       /* Publish the transient state BEFORE queueing the work. The
>> +        * completion callback runs on hdev->req_workqueue serialised by
>> +        * hci_req_sync_lock, which is independent of hdev->lock; setting
>> +        * the state after the queue allowed the worker to win the race
>> +        * and have its terminal STOPPED/FINDING write overwritten by this
>> +        * trailing STARTING write, wedging discovery in STARTING.
>> +        */
>> +       hci_discovery_set_state(hdev, DISCOVERY_STARTING);
> 
> I guess I will need to repeat myself, now that the callbacks do
> acquire hdev lock the comments above are invalid, the callback will
> attempt to hdev->lock making it wait the hci_dev_unlock bellow past
> setting DISCOVERY_STARTING, so the change above is no longer needed.
 Hi Luiz,
Thank you for the clarification. You are correct.

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

Will send v4.
> 
>>         err = hci_cmd_sync_queue(hdev, start_discovery_sync, cmd,
>>                                  start_discovery_complete);
>>         if (err < 0) {
>> +               hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
>>                 mgmt_pending_remove(cmd);
>>                 goto failed;
>>         }
>>
>> -       hci_discovery_set_state(hdev, DISCOVERY_STARTING);
>> -
>>  failed:
>>         hci_dev_unlock(hdev);
>>         return err;
>> @@ -6178,15 +6209,19 @@ static int start_service_discovery(struct sock *sk, struct hci_dev *hdev,
>>                 }
>>         }
>>
>> +       /* Publish the transient state BEFORE queueing; see the comment in
>> +        * start_discovery_internal() for the race details.
>> +        */
>> +       hci_discovery_set_state(hdev, DISCOVERY_STARTING);
>> +
>>         err = hci_cmd_sync_queue(hdev, start_discovery_sync, cmd,
>>                                  start_discovery_complete);
>>         if (err < 0) {
>> +               hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
>>                 mgmt_pending_remove(cmd);
>>                 goto failed;
>>         }
>>
>> -       hci_discovery_set_state(hdev, DISCOVERY_STARTING);
>> -
>>  failed:
>>         hci_dev_unlock(hdev);
>>         return err;
>> @@ -6196,17 +6231,40 @@ static void stop_discovery_complete(struct hci_dev *hdev, void *data, int err)
>>  {
>>         struct mgmt_pending_cmd *cmd = data;
>>
>> -       if (err == -ECANCELED || !mgmt_pending_valid(hdev, cmd))
>> -               return;
>> -
>>         bt_dev_dbg(hdev, "err %d", err);
>>
>> +       if (err) {
>> +               /* The queued stop-discovery work failed before the normal
>> +                * completion path could advance the state machine. The
>> +                * caller already moved the state to DISCOVERY_STOPPING
>> +                * (under hdev->lock, before queueing). Reset it here so
>> +                * the gate does not wedge in STOPPING.
>> +                */
>> +               hci_dev_lock(hdev);
>> +               if (hdev->discovery.state == DISCOVERY_STOPPING)
>> +                       hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
>> +               hci_dev_unlock(hdev);
>> +
>> +               if (err == -ECANCELED)
>> +                       return;
>> +       }
>> +
>> +       if (!mgmt_pending_valid(hdev, cmd))
>> +               return;
>> +
>>         mgmt_cmd_complete(cmd->sk, cmd->hdev->id, cmd->opcode, mgmt_status(err),
>>                           cmd->param, 1);
>>         mgmt_pending_free(cmd);
>>
>> -       if (!err)
>> +       if (!err) {
>> +               /* Serialise discovery.state writes against any concurrent
>> +                * mgmt path holding hdev->lock; this callback runs on
>> +                * req_workqueue without it.
>> +                */
>> +               hci_dev_lock(hdev);
>>                 hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
>> +               hci_dev_unlock(hdev);
>> +       }
>>  }
>>
>>  static int stop_discovery_sync(struct hci_dev *hdev, void *data)
>> @@ -6248,15 +6306,19 @@ static int stop_discovery(struct sock *sk, struct hci_dev *hdev, void *data,
>>                 goto unlock;
>>         }
>>
>> +       /* Publish the transient state BEFORE queueing; see the comment in
>> +        * start_discovery_internal() for the race details.
>> +        */
>> +       hci_discovery_set_state(hdev, DISCOVERY_STOPPING);
>> +
>>         err = hci_cmd_sync_queue(hdev, stop_discovery_sync, cmd,
>>                                  stop_discovery_complete);
>>         if (err < 0) {
>> +               hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
>>                 mgmt_pending_remove(cmd);
>>                 goto unlock;
>>         }
>>
>> -       hci_discovery_set_state(hdev, DISCOVERY_STOPPING);
>> -
>>  unlock:
>>         hci_dev_unlock(hdev);
>>         return err;
>> --
>> 2.43.0
>>
> 
>