Re: [PATCH 08/14] scsi: qla2xxx: Add validation check for base vport index during delete

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scsi: qla2xxx: qla24xx_vport_delete() returns 0 on invalid vp_idx error
--

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

scsi: qla2xxx: Add validation check for base vport index during delete

This commit adds a check in qla24xx_vport_delete() to log an error and return
early if vha->vp_idx is zero.

> diff --git a/drivers/scsi/qla2xxx/qla_attr.c b/drivers/scsi/qla2xxx/qla_attr.c
> index a4ca22024edeb..e6fc47c3f87f8 100644
> --- a/drivers/scsi/qla2xxx/qla_attr.c
> +++ b/drivers/scsi/qla2xxx/qla_attr.c
> @@ -3276,6 +3276,12 @@ qla24xx_vport_delete(struct fc_vport *fc_vport)
>  	struct qla_hw_data *ha = vha->hw;
>  	uint16_t id = vha->vp_idx;
>  
> +	if (!vha->vp_idx) {
> +		ql_log(ql_log_fatal, vha, 0x700f,
> +		       "failed to delete Vport idx [%d]\n", vha->vp_idx);
> +		return 0;
> +	}

[Severity: High]
Should qla24xx_vport_delete() in drivers/scsi/qla2xxx/qla_attr.c return a
non-zero error code here instead of 0 when !vha->vp_idx?

qla24xx_vport_delete() is the .vport_delete callback invoked by
fc_vport_terminate() in drivers/scsi/scsi_transport_fc.c:

	if (i->f->vport_delete)
		stat = i->f->vport_delete(vport);
	else
		stat = -ENOENT;

	spin_lock_irqsave(shost->host_lock, flags);
	vport->flags &= ~FC_VPORT_DELETING;
	if (!stat) {
		vport->flags |= FC_VPORT_DELETED;
		list_del(&vport->peers);
		fc_host->npiv_vports_inuse--;
		scsi_host_put(shost);		/* for fc_host->vport list */
	}
	spin_unlock_irqrestore(shost->host_lock, flags);

	if (stat)
		return stat;
	...
	device_del(dev);
	transport_destroy_device(dev);

	put_device(dev);			/* for self-reference */

When qla24xx_vport_delete() returns 0, fc_vport_terminate() treats the
deletion as successful and unregisters and frees struct fc_vport, even though
qla24xx_vport_delete() skipped tearing down vha, removing vha->host, stopping
the vport timer, freeing vha->gnl.l and vha->scan.l, removing vha from
ha->vp_list in qla24xx_deallocate_vp_id(), and decrementing
ha->cur_vport_count.

Does this leave vha active on ha->vp_list with a dangling vha->fc_vport
pointer, and cause qla2x00_delete_all_vps() in drivers/scsi/qla2xxx/qla_os.c
to loop infinitely on the freed vha->fc_vport during driver unload:

	while (ha->cur_vport_count) {
		spin_lock_irqsave(&ha->vport_slock, flags);

		BUG_ON(base_vha->list.next == &ha->vp_list);
		/* This assumes first entry in ha->vp_list is always base vha */
		vha = list_first_entry(&base_vha->list, scsi_qla_host_t, list);
		scsi_host_get(vha->host);

		spin_unlock_irqrestore(&ha->vport_slock, flags);
		mutex_unlock(&ha->vport_lock);

		qla_nvme_delete(vha);

		fc_vport_terminate(vha->fc_vport);
		scsi_host_put(vha->host);

		mutex_lock(&ha->vport_lock);
	}

because vha is never removed from base_vha->list and ha->cur_vport_count is
never decremented?

> +
>  	set_bit(VPORT_DELETE, &vha->dpc_flags);
>  
>  	while (test_bit(LOOP_RESYNC_ACTIVE, &vha->dpc_flags))

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