Re: [PATCH 6.1.y] scsi: hisi_sas: Grab sas_dev lock when traversing the members of sas_dev.list

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scsi: hisi_sas: AB-BA lock inversion between sas_dev->lock and hisi_hba->lock
--

commit f12017bc77f1c8af65ce94588bd971006d10081f
Author: Xingui Yang <yangxingui@huawei.com>
scsi: hisi_sas: Grab sas_dev lock when traversing the members of sas_dev.list

This commit fixes a NULL pointer exception during slot completion by grabbing
sas_dev->lock when traversing the members of sas_dev.list. It adds a need_lock
parameter to hisi_sas_slot_task_free() to avoid re-acquiring the lock when
it is already held by hisi_sas_release_task() or dereg_device_v3_hw().

Link: https://lore.kernel.org/r/1679283265-115066-2-git-send-email-chenxiang66@hisilicon.com

> diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_sas/hisi_sas_main.c
> index 10ea1d434c48d..290c9915fb9b2 100644
> --- a/drivers/scsi/hisi_sas/hisi_sas_main.c
> +++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
> @@ -239,9 +239,13 @@ void hisi_sas_slot_task_free(struct hisi_hba *hisi_hba, struct sas_task *task,
>  		}
>  	}
>  
> -	spin_lock(&sas_dev->lock);
> -	list_del_init(&slot->entry);
> -	spin_unlock(&sas_dev->lock);
> +	if (need_lock) {
> +		spin_lock(&sas_dev->lock);
> +		list_del_init(&slot->entry);
> +		spin_unlock(&sas_dev->lock);
> +	} else {
> +		list_del_init(&slot->entry);
> +	}

[Severity: High]
Does this code introduce an AB-BA lock inversion deadlock?

When need_lock is false, this code executes while sas_dev->lock is already
held by the caller (hisi_sas_release_task). Shortly after this snippet,
hisi_sas_slot_task_free() calls hisi_sas_slot_index_free() which takes
hisi_hba->lock, establishing a lock dependency of sas_dev->lock ->
hisi_hba->lock.

However, on v1 hardware, the completion interrupt handler cq_interrupt_v1_hw()
acquires hisi_hba->lock before calling slot_complete_v1_hw(), which then calls
hisi_sas_slot_task_free(..., need_lock=true). This inner call acquires
sas_dev->lock, establishing a reverse lock order of hisi_hba->lock ->
sas_dev->lock.

If these two paths run concurrently on different CPUs during device error
handling, could this freeze the system? Also, if they run on the same CPU,
could an AA deadlock occur since the EH thread uses spin_lock without disabling
IRQs?

[ ... ]

> @@ -1085,8 +1089,11 @@ static void hisi_sas_release_task(struct hisi_hba *hisi_hba,
>  	struct hisi_sas_slot *slot, *slot2;
>  	struct hisi_sas_device *sas_dev = device->lldd_dev;
>  
> +	spin_lock(&sas_dev->lock);
>  	list_for_each_entry_safe(slot, slot2, &sas_dev->list, entry)
> -		hisi_sas_do_release_task(hisi_hba, slot->task, slot);
> +		hisi_sas_do_release_task(hisi_hba, slot->task, slot, false);
> +
> +	spin_unlock(&sas_dev->lock);
>  }

[Severity: High]
This is where the new scope of sas_dev->lock is introduced. Since
hisi_sas_do_release_task() eventually calls hisi_sas_slot_index_free(),
which acquires hisi_hba->lock, we are nesting hisi_hba->lock under
sas_dev->lock.

Could this traversal be modified to avoid holding sas_dev->lock during
the call to hisi_sas_slot_index_free()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260925195622.31558-1-artem@trailofbits.com?part=1