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

[Xiuzhuo Shang <xiuzhuo.shang@oss.qualcomm.com>]

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