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

["K, Kiran" <kiran.k@intel.com>]

Hi Manish, 
> 
> From: Manish Mandlik <mmandlik@google.com> 
> Sent: Saturday, August 14, 2021 5:20 AM
> To: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
> Cc: K, Kiran <kiran.k@intel.com>; Marcel Holtmann <marcel@holtmann.org>; BlueZ <linux-bluetooth@vger.kernel.org>; Srivatsa, Ravishankar <ravishankar.srivatsa@intel.com>; Tumkur Narayan, Chethan <chethan.tumkur.narayan@intel.com>
> 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:
> >
> > Hi Marcel,
> >
> > > -----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,
> > >
> > > > -----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,
> > > >
> > > > > 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.
> > > > >
> > > > > 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,
> > > > >                                       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.
> >
> > > > 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?
> 
> > >
> > > > Regards
> > > >
> > > > Marcel
> > >
> > > Thanks,
> > > Kiran
> >
> > Thanks,
> > Kiran
> >
> >
> 
> 
> -- 
> Luiz Augusto von Dentz
>

Regards,
Kiran