Re: [PATCH v3 77/88] scsi: qla2xxx: Drop vport reference under lock in report ID acquisition

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] The `QLA_VHA_MARK_NOT_BUSY()` and `qla_vha_mark_busy()` macros drop the vport reference count without synchronization, risking a use-after-free.
- [High] Unprotected btree modifications in `qla_update_host_map` due to missing `vport_slock`.
- [High] Missing `sess_lock` when traversing `vp_fcports` and calling `qla2x00_find_fcport_by_wwpn` in format 2 handling.
--

--- Patch [77]: [PATCH v3 77/88] scsi: qla2xxx: Drop vport reference under lock in report ID acquisition ---
commit 5bf158aa54ed892e70c7c619b33f75270e50d57a
Author: Nilesh Javali <njavali@marvell.com>

scsi: qla2xxx: Drop vport reference under lock in report ID acquisition

qla24xx_report_id_acquisition() format-1 handling takes the vport
reference under vport_slock but drops it outside the lock, after setting
flags. This patch drops the reference under vport_slock to ensure the
flag stores are ordered before the decrement becomes visible.

> diff --git a/drivers/scsi/qla2xxx/qla_mbx.c b/drivers/scsi/qla2xxx/qla_mbx.c
> index affcd87893cda..e88c3a989a51b 100644
> --- a/drivers/scsi/qla2xxx/qla_mbx.c
> +++ b/drivers/scsi/qla2xxx/qla_mbx.c
> @@ -4290,7 +4290,9 @@ qla24xx_report_id_acquisition(scsi_qla_host_t *vha, void *pkt)
>  			set_bit(REGISTER_FC4_NEEDED, &vp->dpc_flags);
>  			set_bit(REGISTER_FDMI_NEEDED, &vp->dpc_flags);
>  
> +			spin_lock_irqsave(&ha->vport_slock, flags);
>  			atomic_dec(&vp->vref_count);

[Severity: Critical]
This is a pre-existing issue, but while we're fixing the vref_count decrement
here, I noticed a similar pattern in drivers/scsi/qla2xxx/qla_def.h.

In QLA_VHA_MARK_NOT_BUSY() and qla_vha_mark_busy(), the vport reference count
is dropped without synchronization:

drivers/scsi/qla2xxx/qla_def.h:QLA_VHA_MARK_NOT_BUSY() {
    ...
    atomic_dec(&__vha->vref_count);
    wake_up(&__vha->vref_waitq);
    ...
}

If the count reaches 0, the concurrent polling thread in
qla24xx_deallocate_vp_id() might observe it, delete the vport, and free it.
Will the subsequent wake_up(&__vha->vref_waitq) then result in a
use-after-free by dereferencing the freed __vha?


> +			spin_unlock_irqrestore(&ha->vport_slock, flags);
>  		}
>  		set_bit(VP_DPC_NEEDED, &vha->dpc_flags);
>  		qla2xxx_wake_dpc(vha);

[Severity: High]
This isn't a bug introduced by this patch, but I noticed another unprotected
operation earlier in this same function.

When handling the physical port (vp_idx == 0) in
qla24xx_report_id_acquisition(), qla_update_host_map() is called without
any lock:

drivers/scsi/qla2xxx/qla_mbx.c:qla24xx_report_id_acquisition() {
    ...
    if (vp_idx == 0) {
        if (vp_status == VP_STAT_COMPL) {
            ...
            qla_update_host_map(vha, id);
        }
    ...
}

Since other paths modify the shared ha->host_map b-tree under ha->vport_slock
or ha->hardware_lock, will modifying it here without vport_slock lead to
b-tree corruption?


[Severity: High]
This is a pre-existing issue, but looking further down in the format 2
handling of qla24xx_report_id_acquisition(), the vp_fcports list is traversed
without holding a lock:

drivers/scsi/qla2xxx/qla_mbx.c:qla24xx_report_id_acquisition() {
    ...
    } else if (rptid_entry->format == 2) {
        ...
        list_for_each_entry(fcport, &vha->vp_fcports, list) {
            fcport->scan_state = QLA_FCPORT_SCAN;
            fcport->n2n_flag = 0;
        }

        fcport = qla2x00_find_fcport_by_wwpn(vha,
            rptid_entry->u.f2.port_name, 1);
    ...
}

The sess_lock is typically acquired when traversing vp_fcports. If the list
is concurrently modified by session addition or deletion, can this
unprotected traversal race and cause list corruption or a use-after-free?

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