lkml.org 
[lkml]   [2023]   [Jul]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] scsi: hisi_sas: Fix potential deadlock on &hisi_hba->lock
From
Date
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;
> }

\
 
 \ /
  Last update: 2023-07-04 07:55    [W:0.034 / U:0.260 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site