* [PATCH] scsi: hisi_sas: Fix potential deadlock on &hisi_hba->lock
@ 2023-06-28 15:30 Chengfeng Ye
2023-07-04 5:54 ` chenxiang (M)
0 siblings, 1 reply; 3+ messages in thread
From: Chengfeng Ye @ 2023-06-28 15:30 UTC (permalink / raw)
To: chenxiang66, jejb, martin.petersen; +Cc: linux-scsi, linux-kernel, 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().
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;
}
--
2.17.1
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] scsi: hisi_sas: Fix potential deadlock on &hisi_hba->lock
2023-06-28 15:30 [PATCH] scsi: hisi_sas: Fix potential deadlock on &hisi_hba->lock Chengfeng Ye
@ 2023-07-04 5:54 ` chenxiang (M)
2023-07-05 10:06 ` Chengfeng Ye
0 siblings, 1 reply; 3+ messages in thread
From: chenxiang (M) @ 2023-07-04 5:54 UTC (permalink / raw)
To: Chengfeng Ye, jejb, martin.petersen; +Cc: linux-scsi, linux-kernel
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;
> }
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] scsi: hisi_sas: Fix potential deadlock on &hisi_hba->lock
2023-07-04 5:54 ` chenxiang (M)
@ 2023-07-05 10:06 ` Chengfeng Ye
0 siblings, 0 replies; 3+ messages in thread
From: Chengfeng Ye @ 2023-07-05 10:06 UTC (permalink / raw)
To: chenxiang (M); +Cc: jejb, martin.petersen, linux-scsi, linux-kernel
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@hisilicon.com> 于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@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;
> > }
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-07-05 10:06 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-06-28 15:30 [PATCH] scsi: hisi_sas: Fix potential deadlock on &hisi_hba->lock Chengfeng Ye
2023-07-04 5:54 ` chenxiang (M)
2023-07-05 10:06 ` Chengfeng Ye
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®