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

[chenxiang (M)]

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;
>   }