Re: [PATCH] scsi: hisi_sas: Fix potential deadlock on &hisi_hba->lock

[Chengfeng Ye]

Thanks for the notice!

Yes, removing the lock acquisition inside hisi_sas_port_notify_formed()
can avoid the deadlock problem by the way.

Best Regards,
Chengfeng

chenxiang (M) <chenxiang66@xxxxxxxxxxxxx> 于2023年7月4日周二 13:55写道：
>
> Hi,
>
>
> 在 2023/6/28 星期三 23:30, Chengfeng Ye 写道:
> > As &hisi_hba->lock is acquired by hard irq int_abnormal_v1_hw(),
> > other acquisition of the same lock under process context should
> > disable irq, otherwise deadlock could happen if the
> > irq preempt the execution while the lock is held in process context
> > on the same CPU.
> >
> > [Interrupt]: int_abnormal_v1_hw()
> >      -->/root/linux/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c:1447
> >      -->/root/linux/drivers/scsi/hisi_sas/hisi_sas_main.c:2050
> >      -->/root/linux/drivers/scsi/hisi_sas/hisi_sas_main.c:1079
> >      -->spin_lock_irqsave(&hisi_hba->lock, flags);
> >
> > [Process Context]: hisi_sas_clear_nexus_ha()
> >      -->/root/linux/drivers/scsi/hisi_sas/hisi_sas_main.c:1932
> >      -->/root/linux/drivers/scsi/hisi_sas/hisi_sas_main.c:1135
> >      -->/root/linux/drivers/scsi/hisi_sas/hisi_sas_main.c:1116
> >      -->/root/linux/drivers/scsi/hisi_sas/hisi_sas_main.c:1105
> >      -->/root/linux/drivers/scsi/hisi_sas/hisi_sas_main.c:166
> >      -->spin_lock(&hisi_hba->lock);
> >
> > [Process Context]: hisi_sas_dev_found()
> >      -->/root/linux/drivers/scsi/hisi_sas/hisi_sas_main.c:665
> >      -->spin_lock(&hisi_hba->lock);
> >
> > [Process Context]: hisi_sas_queue_command()
> >      -->/root/linux/drivers/scsi/hisi_sas/hisi_sas_main.c:188
> >      -->spin_lock(&hisi_hba->lock);
> >
> > This flaw was found by an experimental static analysis tool I am
> > developing for irq-related deadlock, which reported the above
> > warning when analyzing the linux kernel 6.4-rc7 release.
> >
> > The tentative patch fix the potential deadlock by spin_lock_irqsave().
>
> Thank you for reporting it.
> But we consider about removing  hisi_hba->lock in function
> hisi_sas_port_notify_formed()
> which is called by int_abnormal_v1_hw(), as we think it is not necessary
> to add hisi_hba->lock in this function.
> So please ignore it and still thank you for pointing out the issue.
>
> Thanks,
> Shawn
>
> >
> > Signed-off-by: Chengfeng Ye <dg573847474@xxxxxxxxx>
> > ---
> >   drivers/scsi/hisi_sas/hisi_sas_main.c | 17 ++++++++++-------
> >   1 file changed, 10 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_sas/hisi_sas_main.c
> > index 412431c901a7..47c5062a732e 100644
> > --- a/drivers/scsi/hisi_sas/hisi_sas_main.c
> > +++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
> > @@ -161,11 +161,12 @@ static void hisi_sas_slot_index_clear(struct hisi_hba *hisi_hba, int slot_idx)
> >
> >   static void hisi_sas_slot_index_free(struct hisi_hba *hisi_hba, int slot_idx)
> >   {
> > +     unsigned long flags;
> >       if (hisi_hba->hw->slot_index_alloc ||
> >           slot_idx < HISI_SAS_RESERVED_IPTT) {
> > -             spin_lock(&hisi_hba->lock);
> > +             spin_lock_irqsave(&hisi_hba->lock, flags);
> >               hisi_sas_slot_index_clear(hisi_hba, slot_idx);
> > -             spin_unlock(&hisi_hba->lock);
> > +             spin_unlock_irqrestore(&hisi_hba->lock, flags);
> >       }
> >   }
> >
> > @@ -181,11 +182,12 @@ static int hisi_sas_slot_index_alloc(struct hisi_hba *hisi_hba,
> >   {
> >       int index;
> >       void *bitmap = hisi_hba->slot_index_tags;
> > +     unsigned long flags;
> >
> >       if (rq)
> >               return rq->tag + HISI_SAS_RESERVED_IPTT;
> >
> > -     spin_lock(&hisi_hba->lock);
> > +     spin_lock_irqsave(&hisi_hba->lock, flags);
> >       index = find_next_zero_bit(bitmap, HISI_SAS_RESERVED_IPTT,
> >                                  hisi_hba->last_slot_index + 1);
> >       if (index >= HISI_SAS_RESERVED_IPTT) {
> > @@ -193,13 +195,13 @@ static int hisi_sas_slot_index_alloc(struct hisi_hba *hisi_hba,
> >                               HISI_SAS_RESERVED_IPTT,
> >                               0);
> >               if (index >= HISI_SAS_RESERVED_IPTT) {
> > -                     spin_unlock(&hisi_hba->lock);
> > +                     spin_unlock_irqrestore(&hisi_hba->lock, flags);
> >                       return -SAS_QUEUE_FULL;
> >               }
> >       }
> >       hisi_sas_slot_index_set(hisi_hba, index);
> >       hisi_hba->last_slot_index = index;
> > -     spin_unlock(&hisi_hba->lock);
> > +     spin_unlock_irqrestore(&hisi_hba->lock, flags);
> >
> >       return index;
> >   }
> > @@ -658,11 +660,12 @@ static struct hisi_sas_device *hisi_sas_alloc_dev(struct domain_device *device)
> >   {
> >       struct hisi_hba *hisi_hba = dev_to_hisi_hba(device);
> >       struct hisi_sas_device *sas_dev = NULL;
> > +     unsigned long flags;
> >       int last = hisi_hba->last_dev_id;
> >       int first = (hisi_hba->last_dev_id + 1) % HISI_SAS_MAX_DEVICES;
> >       int i;
> >
> > -     spin_lock(&hisi_hba->lock);
> > +     spin_lock_irqsave(&hisi_hba->lock, flags);
> >       for (i = first; i != last; i %= HISI_SAS_MAX_DEVICES) {
> >               if (hisi_hba->devices[i].dev_type == SAS_PHY_UNUSED) {
> >                       int queue = i % hisi_hba->queue_count;
> > @@ -682,7 +685,7 @@ static struct hisi_sas_device *hisi_sas_alloc_dev(struct domain_device *device)
> >               i++;
> >       }
> >       hisi_hba->last_dev_id = i;
> > -     spin_unlock(&hisi_hba->lock);
> > +     spin_unlock_irqrestore(&hisi_hba->lock, flags);
> >
> >       return sas_dev;
> >   }
>