Messages in this thread | ![/](/images/icornerl.gif) | | Subject | Re: [PATCH] scsi: hisi_sas: Fix potential deadlock on &hisi_hba->lock | From | "chenxiang (M)" <> | Date | Tue, 4 Jul 2023 13:54:59 +0800 |
| |
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@gmail.com> > --- > 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; > }
| ![\](/images/icornerr.gif) |