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

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

Hi Marcel,

> -----Original Message-----
> From: K, Kiran
> Sent: Friday, August 6, 2021 8:14 PM
> To: 'Marcel Holtmann' <marcel@holtmann.org>
> Cc: 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 Marcel,
> 
> > -----Original Message-----
> > From: Marcel Holtmann <marcel@holtmann.org>
> > Sent: Thursday, August 5, 2021 6:41 PM
> > To: K, Kiran <kiran.k@intel.com>
> > Cc: 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 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 <kiran.k@intel.com>
> > > Reviewed-by: Chethan T N <chethan.tumkur.narayan@intel.com>
> > > Reviewed-by: Srivatsa Ravishankar <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 <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.
> 
> > Regards
> >
> > Marcel
> 
> Thanks,
> Kiran

Thanks,
Kiran