RE: [PATCH v1] Bluetooth: Fix race condition in handling NOP command

Hi Manish, 

From: Manish Mandlik <redacted> 
Sent: Saturday, August 14, 2021 5:20 AM
To: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Cc: K, Kiran <redacted>; Marcel Holtmann <marcel@holtmann.org>; BlueZ <redacted>; Srivatsa, Ravishankar <redacted>; Tumkur Narayan, Chethan <redacted>
Subject: Re: [PATCH v1] Bluetooth: Fix race condition in handling NOP command

Hi Luiz,

This patch looks ok to me, it'll not break ncmd timeout handling. 

@Kiran: Just one nit on the patch: we can get rid of the argument 'u16 opcode' of the function 'handle_cmd_cnt_and_timer()' as it won't be used anymore in this case.

Thanks for the comment. I fix and send out an updated version.

Regards,
Manish.


On Thu, Aug 12, 2021 at 10:31 AM Luiz Augusto von Dentz <mailto:luiz.dentz@gmail.com> wrote:
Hi Manish,

On Thu, Aug 12, 2021 at 3:58 AM K, Kiran <mailto:kiran.k@intel.com> wrote:

quoted

Hi Marcel,

quoted

-----Original Message-----
From: K, Kiran
Sent: Friday, August 6, 2021 8:14 PM
To: 'Marcel Holtmann' <mailto:marcel@holtmann.org>
Cc: BlueZ <mailto:linux-bluetooth@vger.kernel.org>; Srivatsa, Ravishankar
<mailto:ravishankar.srivatsa@intel.com>; Tumkur Narayan, Chethan
<mailto:chethan.tumkur.narayan@intel.com>
Subject: RE: [PATCH v1] Bluetooth: Fix race condition in handling NOP
command

Hi Marcel,

quoted

-----Original Message-----
From: Marcel Holtmann <mailto:marcel@holtmann.org>
Sent: Thursday, August 5, 2021 6:41 PM
To: K, Kiran <mailto:kiran.k@intel.com>
Cc: BlueZ <mailto:linux-bluetooth@vger.kernel.org>; Srivatsa, Ravishankar
<mailto:ravishankar.srivatsa@intel.com>; Tumkur Narayan, Chethan
<mailto:chethan.tumkur.narayan@intel.com>
Subject: Re: [PATCH v1] Bluetooth: Fix race condition in handling NOP
command

Hi Kiran,

quoted

For NOP command, need to cancel work scheduled on cmd_timer, on
receiving command status or commmand complete event.

Below use case might lead to race condition multiple when NOP
commands are queued sequentially:

hci_cmd_work() {
  if (atomic_read(&hdev->cmd_cnt) {
           .
           .
           .
     atomic_dec(&hdev->cmd_cnt);
     hci_send_frame(hdev,...);
     schedule_delayed_work(&hdev->cmd_timer,...);
  }
}

On receiving event for first NOP, the work scheduled on
hdev->cmd_timer is not cancelled and  second NOP is dequeued and
hdev->sent

to controller.

quoted

While waiting for an event for second NOP command, work scheduled on
cmd_timer for first NOP can get scheduled, resulting in sending
third NOP command not waiting for an event for second NOP. This
might cause issues at controller side (like memory overrun,
controller going
unresponsive) resulting in hci tx timeouts, hardware errors etc.

Signed-off-by: Kiran K <mailto:kiran.k@intel.com>
Reviewed-by: Chethan T N <mailto:chethan.tumkur.narayan@intel.com>
Reviewed-by: Srivatsa Ravishankar <mailto:ravishankar.srivatsa@intel.com>
---
net/bluetooth/hci_event.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index ea7fc09478be..14dfbdc8b81b 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c

@@ -3271,8 +3271,7 @@ static void hci_remote_features_evt(struct

hci_dev *hdev, static inline void handle_cmd_cnt_and_timer(struct
hci_dev

*hdev,

quoted

                                      u16 opcode, u8 ncmd)
{
- if (opcode != HCI_OP_NOP)
-         cancel_delayed_work(&hdev->cmd_timer);
+ cancel_delayed_work(&hdev->cmd_timer);

  if (!test_bit(HCI_RESET, &hdev->flags)) {
          if (ncmd) {

so this is conflicting with the patch introducing the ncmd timeout handling.

My patch specifically addresses the issue observed in case of NOP command.
It prevents the issue by handling NOP same as any other SIG command.

It looks commit de75cd0d9b2f3250d5f25846bb5632ccce6275f4 tries to
recover when controller goes bad.

Do you have any further comments here ? Waiting for your input.

quoted

quoted

commit de75cd0d9b2f3250d5f25846bb5632ccce6275f4
Author: Manish Mandlik <mailto:mmandlik@google.com>
Date:   Thu Apr 29 10:24:22 2021 -0700

    Bluetooth: Add ncmd=0 recovery handling

    During command status or command complete event, the controller
may set
    ncmd=0 indicating that it is not accepting any more commands. In such a
    case, host holds off sending any more commands to the controller. If the
    controller doesn't recover from such condition, host will wait forever,
    until the user decides that the Bluetooth is broken and may power cycles
    the Bluetooth.

    This patch triggers the hardware error to reset the controller and
    driver when it gets into such state as there is no other wat out.

Nowhere in your commit description you are addressing why is this the
right to do.

Will fix it in the next version if you are OK with the current fix. Please let me
know.

Can you confirm this change doesn't break your patch above?

quoted

quoted

quoted

Regards

Marcel

Thanks,
Kiran

Thanks,
Kiran

-- 
Luiz Augusto von Dentz

Regards,
Kiran