Re: [PATCH v3 50/88] scsi: qla2xxx: Hold vport reference in qla24xx_report_id_acquisition()

[sashiko-bot@kernel.org]

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

New issues:
- [High] The lockless atomic_dec(&vp->vref_count) lacks a memory barrier, allowing CPU reordering that leads to a Use-After-Free.

Pre-existing issues:
- [High] Failing to check the VPORT_DELETE flag before updating the vport allows a dying vport to be resurrected into the global B-tree, creating a dangling pointer.
--

--- Patch [50]: [PATCH v3 50/88] scsi: qla2xxx: Hold vport reference in qla24xx_report_id_acquisition() ---
commit cc05285288870e8f9cfc205d3a17b3cc31c7a415
Author: Nilesh Javali <njavali@marvell.com>

scsi: qla2xxx: Hold vport reference in qla24xx_report_id_acquisition()

This commit acquires a vport reference (vref_count) under vport_slock
during format 1 ID acquisition to prevent concurrent teardown. The
reference is dropped after the last use of the vp pointer.

> diff --git a/drivers/scsi/qla2xxx/qla_mbx.c b/drivers/scsi/qla2xxx/qla_mbx.c
> index 59023492c5a98..ba4a4764de1fb 100644
> --- a/drivers/scsi/qla2xxx/qla_mbx.c
> +++ b/drivers/scsi/qla2xxx/qla_mbx.c
> @@ -4267,6 +4267,7 @@ qla24xx_report_id_acquisition(scsi_qla_host_t *vha, void *pkt)
>  			list_for_each_entry(vp, &ha->vp_list, list) {
>  				if (vp_idx == vp->vp_idx) {
>  					found = 1;
> +					atomic_inc(&vp->vref_count);
>  					break;
>  				}
>  			}

[Severity: High]
This is a pre-existing issue, but should we check for the VPORT_DELETE flag
here before taking the reference?

If qla24xx_vport_delete() is running concurrently, it sets VPORT_DELETE and
clears vha->d_id.b24. If we don't check test_bit(VPORT_DELETE, &vp->dpc_flags),
we could take a reference on a dying vport.

When qla_update_host_map(vp, id) is subsequently called just below this block,
it might re-insert this dying vp back into the ha->host_map B-tree.

After the reference is dropped and the vport freed, could this leave a dangling
pointer in the B-tree?

Also, is it safe that qla_update_host_map(vp, id) is called without holding
ha->vport_slock?

[ ... ]

> @@ -4284,6 +4285,8 @@ qla24xx_report_id_acquisition(scsi_qla_host_t *vha, void *pkt)
>  			set_bit(VP_IDX_ACQUIRED, &vp->vp_flags);
>  			set_bit(REGISTER_FC4_NEEDED, &vp->dpc_flags);
>  			set_bit(REGISTER_FDMI_NEEDED, &vp->dpc_flags);
> +
> +			atomic_dec(&vp->vref_count);

[Severity: High]
Could the CPU reorder the set_bit() stores to happen after this atomic_dec()?

On weakly-ordered architectures like ARM64 or PowerPC, atomic_dec() does not
provide a memory barrier.

If qla24xx_deallocate_vp_id() is polling for atomic_read(&vp->vref_count) == 0
concurrently, it might observe the 0 and free the vp struct before the
set_bit() operations are globally visible.

Would it be safer to use atomic_dec_and_test() or add a memory barrier here
to ensure the bit flag updates complete before the reference is released?

>  		}
>  		set_bit(VP_DPC_NEEDED, &vha->dpc_flags);
>  		qla2xxx_wake_dpc(vha);

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