Re: [PATCH v3 74/88] scsi: qla2xxx: Clamp max_npiv_vports to VP_CTRL bitmap capacity

[sashiko-bot@kernel.org]

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

New issues:
- [High] The introduced cap on `ha->max_npiv_vports` is immediately overwritten by firmware values, leaving the intended resource leak completely unpatched.

Pre-existing issues:
- [High] A pre-existing off-by-one error in `qla24xx_control_vp()` guarantees that the vport at the highest valid index will always leak.
- [High] Data race on bitfields in the `ha->flags` structure leading to lost mailbox interrupts and adapter hangs.
--

--- Patch [74]: [PATCH v3 74/88] scsi: qla2xxx: Clamp max_npiv_vports to VP_CTRL bitmap capacity ---
commit 200dabdd0444e358cb943332543c7d1ad6735f45
Author: Nilesh Javali <njavali@marvell.com>
scsi: qla2xxx: Clamp max_npiv_vports to VP_CTRL bitmap capacity

This commit attempts to prevent a virtual port resource leak by capping the
maximum number of NPIV vports to the capacity of the VP_CTRL IOCB index
bitmap. It updates the maximum count during chip setup and modifies the
bounds check during VP control operations.

[ ... ]

> diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c
> index d678e27213a94..1f20ab386003d 100644
> --- a/drivers/scsi/qla2xxx/qla_init.c
> +++ b/drivers/scsi/qla2xxx/qla_init.c
> @@ -4412,6 +4412,19 @@ qla2x00_setup_chip(scsi_qla_host_t *vha)
>  					    MIN_MULTI_ID_FABRIC))
>  						ha->max_npiv_vports =
>  						    MIN_MULTI_ID_FABRIC - 1;

[Severity: High]
This isn't a bug introduced by this patch, but can the initialization of
ha->flags in this block cause a data race that drops mailbox interrupts?

The ha->flags structure is a collection of bitfields sharing a 32-bit word.
During qla2x00_setup_chip(), lockless writes occur right before this logic,
such as setting npiv_supported:

qla2x00_setup_chip()
    ha->flags.npiv_supported = 1;

If the hardware issues a concurrent mailbox completion interrupt, the ISR
will acquire the hardware lock and set the mbox_int bit:

qla24xx_intr_handler() -> qla2x00_mbx_completion()
    ha->flags.mbox_int = 1;

Could the lockless read-modify-write phase of setting npiv_supported
overwrite and clear the newly set mbox_int bit, leading to a mailbox command
timeout?

> +
> +					/*
> +					 * The VP_CTRL IOCB selects target VPs
> +					 * through the fixed vp_idx_map bitmap,
> +					 * so a vp_index beyond it can be enabled
> +					 * via VP_CONFIG but never disabled via
> +					 * VP_CTRL, leaking the VP.  Cap the count
> +					 * to the bitmap capacity.
> +					 */
> +					if (ha->max_npiv_vports >=
> +					    VP_CTRL_IDX_MAP_BITS)
> +						ha->max_npiv_vports =
> +						    VP_CTRL_IDX_MAP_BITS - 1;
>  				}
>  				qlt_config_nvram_with_fw_version(vha);
>  				qla2x00_get_resource_cnts(vha);

[Severity: High]
Does the subsequent call to qla2x00_get_resource_cnts() immediately overwrite
this newly calculated cap?

Looking at qla2x00_get_resource_cnts(), it unconditionally assigns the raw
firmware value back to max_npiv_vports:

qla2x00_get_resource_cnts() {
    ...
    if (ha->flags.npiv_supported)
        ha->max_npiv_vports = mcp->mb[11];
    ...
}

Will this cause the intended cap to be lost before it can prevent the
resource leak?

[ ... ]

> diff --git a/drivers/scsi/qla2xxx/qla_mid.c b/drivers/scsi/qla2xxx/qla_mid.c
> index 33bfc61d81652..4ad23d206add2 100644
> --- a/drivers/scsi/qla2xxx/qla_mid.c
> +++ b/drivers/scsi/qla2xxx/qla_mid.c
> @@ -996,7 +996,7 @@ int qla24xx_control_vp(scsi_qla_host_t *vha, int cmd)
>  	 * (16-byte) vp_idx_map bitmap, so vp_index must fit within it even
>  	 * if firmware advertises more NPIV vports.
>  	 */
> -	if (vp_index > sizeof_field(struct vp_ctrl_entry_24xx, vp_idx_map) * 8)
> +	if (vp_index > VP_CTRL_IDX_MAP_BITS)
>  		return QLA_PARAMETER_ERROR;

[Severity: High]
This is a pre-existing issue, but does the strict inequality check on
max_npiv_vports earlier in this function create an off-by-one error?

In qla24xx_allocate_vp_id(), the allocator searches up to max_npiv_vports + 1:

qla24xx_allocate_vp_id() {
    ...
    vp_id = find_first_zero_bit(ha->vp_idx_map, ha->max_npiv_vports + 1);
    if (vp_id > ha->max_npiv_vports) {
    ...
}

This permits allocating an ID exactly equal to ha->max_npiv_vports.

However, earlier in qla24xx_control_vp(), the check rejects values greater
than or equal to max_npiv_vports:

    if (vp_index == 0 || vp_index >= ha->max_npiv_vports)
        return QLA_PARAMETER_ERROR;

Will this cause the virtual port at the highest index to be successfully
created, but silently rejected during deletion, leaving it permanently
active in the firmware?

>  
>  	/* ref: INIT */

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