Re: [PATCH 07/14] scsi: qla2xxx: Skip vport DPC processing during vport deletion

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scsi: qla2xxx: wrong vha delete_progress check in qla2x00_do_dpc_all_vps()
--

commit 4fbe8fb96efa7d2224539e8ec6860bb2e6e4ec2d
Author: Manish Rangankar <mrangankar@marvell.com>

scsi: qla2xxx: Skip vport DPC processing during vport deletion

This commit attempts to prevent a race between virtual port dpc processing in
qla2x00_do_dpc_all_vps() and virtual port deletion by checking the
delete_progress flag before invoking qla2x00_do_dpc_vp(). A later commit in
the series ("scsi: qla2xxx: Fix use-after-free races during vport deletion")
updates this check from vha to vp, and another follow-up commit ("scsi:
qla2xxx: Re-acquire vport_slock and refresh tvp during vport DPC") further
updates the loop.

> diff --git a/drivers/scsi/qla2xxx/qla_mid.c b/drivers/scsi/qla2xxx/qla_mid.c
> index 4ad23d206add2..f7a08bc0093a5 100644
> --- a/drivers/scsi/qla2xxx/qla_mid.c
> +++ b/drivers/scsi/qla2xxx/qla_mid.c
> @@ -439,7 +439,7 @@ qla2x00_do_dpc_all_vps(scsi_qla_host_t *vha)
>  
>  	spin_lock_irqsave(&ha->vport_slock, flags);
>  	list_for_each_entry_safe(vp, tvp, &ha->vp_list, list) {
> -		if (vp->vp_idx) {
> +		if (vp->vp_idx && !vha->flags.delete_progress) {

[Severity: High]
Should this check vp->flags.delete_progress instead of
vha->flags.delete_progress?

In drivers/scsi/qla2xxx/qla_mid.c:qla2x00_do_dpc_all_vps(), vha is always the
base physical host:

	if (vha->vp_idx)
		return;

Each virtual port on ha->vp_list is represented by the loop iterator vp, and
drivers/scsi/qla2xxx/qla_attr.c:qla24xx_vport_delete() only sets
flags.delete_progress on the virtual port being deleted, never on the base
physical host. Checking vha->flags.delete_progress here causes the condition
to remain true for deleting virtual ports, allowing qla2x00_do_dpc_vp(vp) to
continue running concurrently with teardown and potentially causing a
use-after-free or null pointer dereference.

Although the follow-up commit "scsi: qla2xxx: Fix use-after-free races during
vport deletion" changes this to check !vp->flags.delete_progress, this commit
is broken on its own and is tagged for stable.

Additionally, even when checking vp->flags.delete_progress, is delete_progress
set early enough in drivers/scsi/qla2xxx/qla_attr.c:qla24xx_vport_delete() to
prevent races with qla2x00_do_dpc_vp()?

In drivers/scsi/qla2xxx/qla_attr.c:qla24xx_vport_delete():

	set_bit(VPORT_DELETE, &vha->dpc_flags);

	while (test_bit(LOOP_RESYNC_ACTIVE, &vha->dpc_flags))
		msleep(1000);


	qla24xx_disable_vp(vha);
	qla2x00_wait_for_sess_deletion(vha);

	qla_nvme_delete(vha);
	qla_enode_stop(vha);
	qla_edb_stop(vha);

	vha->flags.delete_progress = 1;

	qlt_remove_target(ha, vha);

	fc_remove_host(vha->host);

	scsi_remove_host(vha->host);

	/* Allow timer to run to drain queued items, when removing vp */
	qla24xx_deallocate_vp_id(vha);

Before flags.delete_progress is set, qla24xx_vport_delete() has already
waited for resync, disabled the vport, deleted sessions, and stopped
nvme/enode/edb. Could qla2x00_do_dpc_vp(vp) still enter during that window to
start a new loop resync, configure the vport, process purex items, or queue
relogin work, and then continue executing concurrently with
qlt_remove_target(), fc_remove_host(), and scsi_remove_host() before
qla24xx_deallocate_vp_id() waits for vref_count to drain?

Would it be safer to check test_bit(VPORT_DELETE, &vp->dpc_flags) here, matching
drivers/scsi/qla2xxx/qla_mid.c:qla2x00_alert_all_vps()?

>  			atomic_inc(&vp->vref_count);
>  			spin_unlock_irqrestore(&ha->vport_slock, flags);
>

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