Re: [PATCH v6 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] Unsafe lock drop inside `list_for_each_entry_safe` over `ha->vp_list` causes Use-After-Free during list traversal.
- [High] Unprotected list traversal over `vp_fcports` without holding `sess_lock` allows concurrent modifications to corrupt the traversal.
--

--- Patch [22]: [PATCH v6 22/56] scsi: qla2xxx: Enable qla2x00_shutdown for 29xx ---
commit 432bc00837ab9eb63a8687ca9bc265acf0d5bb67
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 since you are enabling
qla2x00_abort_isp_cleanup() for 29xx adapters, could this lead to a
use-after-free during list traversal?

Looking at qla2x00_abort_isp_cleanup() in drivers/scsi/qla2xxx/qla_init.c:

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

Is it safe to drop vport_slock inside the loop?

Since list_for_each_entry_safe() caches the next entry (tvp) prior to
dropping the lock, a concurrent thread (like qla24xx_vport_delete() via
sysfs) could remove and free tvp while the lock is dropped. When the lock
is reacquired, the loop would attempt to advance using the freed tvp.

[Severity: High]
This is a pre-existing issue, but does this function also lack necessary
locking when traversing vp_fcports?

Further down in qla2x00_abort_isp_cleanup():

	list_for_each_entry(fcport, &vha->vp_fcports, list) {
		fcport->flags &= ~(FCF_LOGIN_NEEDED | FCF_ASYNC_SENT);
		fcport->scan_state = 0;
	}

And similarly for vp->vp_fcports:

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

Can these iterations be corrupted by concurrent modifications?

This traversal doesn't appear to hold vha->hw->tgt.sess_lock. If concurrent
tasks (such as workqueues or softirqs executing qlt_create_sess() or
qla24xx_create_new_sess()) add to vp_fcports via list_add_tail(), or sysfs
tasks executing qla2x00_free_fcport() remove from it via list_del(), could
it corrupt the traversal state and cause a use-after-free?

>  	} else {
>  		/* Stop currently executing firmware. */
>  		qla2x00_try_to_stop_firmware(vha);

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