Re: [PATCH v3 78/88] scsi: qla2xxx: Hold vport_slock for host map update in report ID acquisition

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Incomplete fix leaves physical port `host_map` updates unprotected against concurrent btree corruption.
- [High] Global lock mismatch for `ha->host_map` and `vp_map` structures, allowing concurrent btree corruption.
--

--- Patch [78]: [PATCH v3 78/88] scsi: qla2xxx: Hold vport_slock for host map update in report ID acquisition ---
commit e3ac99c05ebbead772a6f7fee7f4f320422f988b
Author: Nilesh Javali <njavali@marvell.com>

scsi: qla2xxx: Hold vport_slock for host map update in report ID acquisition
    
This commit adds a missing vport_slock around qla_update_host_map() in
the format-1 handling path of qla24xx_report_id_acquisition() to prevent
concurrent host_map updates from corrupting the btree.

> diff --git a/drivers/scsi/qla2xxx/qla_mbx.c b/drivers/scsi/qla2xxx/qla_mbx.c
> index e88c3a989a51b..39544deab576a 100644
> --- a/drivers/scsi/qla2xxx/qla_mbx.c
> +++ b/drivers/scsi/qla2xxx/qla_mbx.c
> @@ -4280,7 +4280,9 @@ qla24xx_report_id_acquisition(scsi_qla_host_t *vha, void *pkt)
>  			if (!found)
>  				return;
>  
> +			spin_lock_irqsave(&ha->vport_slock, flags);
>  			qla_update_host_map(vp, id);
> +			spin_unlock_irqrestore(&ha->vport_slock, flags);

[Severity: High]
This isn't a bug introduced by this patch, but does the physical port also
need to be protected by vport_slock?

Earlier in qla24xx_report_id_acquisition(), when handling format == 0 or
when vp_idx == 0 for format == 1, qla_update_host_map(vha, id) is called
without locks.

If qla24xx_report_id_acquisition() is called from
qlt_24xx_process_atio_queue(vha, 0) where no locks are held, could these
unprotected physical port updates race and corrupt the btree?

[Severity: High]
This is a pre-existing issue, but does using vport_slock here conflict with
other contexts that update the host_map?

Looking at other updaters, it seems they hold hardware_lock instead of
vport_slock:

drivers/scsi/qla2xxx/qla_init.c:qla2x00_configure_hba() {
    ...
    spin_lock_irqsave(&ha->hardware_lock, flags);
    ...
    qla_update_host_map(vha, id);
    spin_unlock_irqrestore(&ha->hardware_lock, flags);
    ...
}

drivers/scsi/qla2xxx/qla_mid.c:qla24xx_disable_vp() {
    ...
    spin_lock_irqsave(&vha->hw->hardware_lock, flags);
    qla_update_vp_map(vha, RESET_AL_PA);
    spin_unlock_irqrestore(&vha->hw->hardware_lock, flags);
    ...
}

Because these contexts use disjoint locks to serialize modifications to the
exact same btree (ha->host_map), could this lead to concurrent btree
corruption?

>  
>  			/*
>  			 * Cannot configure here as we are still sitting on the

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