Re: [PATCH v5 22/56] scsi: qla2xxx: Enable qla2x00_shutdown for 29xx

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Dropping a spinlock inside list_for_each_entry_safe exposes the cached next pointer to use-after-free.
- [High] Lockless iteration over fcport list exposes it to concurrent modifications.
--

--- Patch [22]: [PATCH v5 22/56] scsi: qla2xxx: Enable qla2x00_shutdown for 29xx ---
commit 6046de798bc7fa0a80928785cf9ec35ed974164d
Author: Nilesh Javali <njavali@marvell.com>

scsi: qla2xxx: Enable qla2x00_shutdown for 29xx

Enable qla2x00_shutdown for 29xx adapter by adding IS_QLA29XX check
to the shutdown path that performs firmware abort cleanup.

> diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
> index 146701445485f..5450c40259bf9 100644
> --- a/drivers/scsi/qla2xxx/qla_os.c
> +++ b/drivers/scsi/qla2xxx/qla_os.c
> @@ -3840,7 +3840,7 @@ qla2x00_shutdown(struct pci_dev *pdev)
>  		qla2x00_disable_eft_trace(vha);
>  
>  	if (IS_QLA25XX(ha) ||  IS_QLA2031(ha) || IS_QLA27XX(ha) ||
> -	    IS_QLA28XX(ha)) {
> +	    IS_QLA28XX(ha) || IS_QLA29XX(ha)) {
>  		if (ha->flags.fw_started)
>  			qla2x00_abort_isp_cleanup(vha);

[Severity: High]
This isn't a bug introduced by this patch, but it exposes 29xx adapters to a
pre-existing use-after-free issue in qla2x00_abort_isp_cleanup().

Dropping the vport_slock inside list_for_each_entry_safe() exposes the cached
next pointer (tvp) to concurrent removal. If a concurrent vport removal (e.g.,
via sysfs /sys/class/fc_host/hostX/vport_delete) acquires vport_slock, removes
tvp from vp_list, and frees it, the loop will dereference a freed pointer when
resuming:

drivers/scsi/qla2xxx/qla_init.c:qla2x00_abort_isp_cleanup() {
    ...
	spin_lock_irqsave(&ha->vport_slock, flags);
	list_for_each_entry_safe(vp, tvp, &ha->vp_list, list) {
		atomic_inc(&vp->vref_count);
		spin_unlock_irqrestore(&ha->vport_slock, flags);

		qla2x00_mark_all_devices_lost(vp);

		spin_lock_irqsave(&ha->vport_slock, flags);
		atomic_dec(&vp->vref_count);
	}
    ...
}

Can this lead to a use-after-free when the lock is reacquired and the loop
proceeds to tvp?

[Severity: High]
This is also a pre-existing issue, but this patch additionally exposes 29xx
adapters to a lockless list iteration bug in qla2x00_abort_isp_cleanup().

The vp_fcports list is iterated without holding any locks, which can race with
concurrent modifications like device discovery or target login/logout events
that modify vp_fcports under tgt.sess_lock.

For the primary host:

drivers/scsi/qla2xxx/qla_init.c:qla2x00_abort_isp_cleanup() {
    ...
	/* Clear all async request states across all VPs. */
	list_for_each_entry(fcport, &vha->vp_fcports, list) {
		fcport->flags &= ~(FCF_LOGIN_NEEDED | FCF_ASYNC_SENT);
		fcport->scan_state = 0;
	}
    ...
}

And for other vports where vport_slock is explicitly dropped before iterating:

drivers/scsi/qla2xxx/qla_init.c:qla2x00_abort_isp_cleanup() {
    ...
	list_for_each_entry_safe(vp, tvp, &ha->vp_list, list) {
		atomic_inc(&vp->vref_count);
		spin_unlock_irqrestore(&ha->vport_slock, flags);

		list_for_each_entry(fcport, &vp->vp_fcports, list)
			fcport->flags &= ~(FCF_LOGIN_NEEDED | FCF_ASYNC_SENT);
    ...
}

Does this lockless iteration risk list corruption or use-after-free if the
list is concurrently modified?

>  	} else {
>  		/* Stop currently executing firmware. */
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260717152904.3155730-1-njavali@marvell.com?part=22