Re: [PATCH v5 29/56] scsi: qla2xxx: Update handling of ELS IOCBs for 29xx series

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Endianness portability bug in `qla_els_set_vp_sof()` due to native C bitfield assignments on a `__le16` hardware struct field.
- [High] Incomplete 29xx hardware support in `qla24xx_process_abts()` uses the legacy 24xx struct layout for the ABTS response, truncating the virtual port index and corrupting the descriptor.
- [Medium] Partial zeroing of the reused DMA buffer in `qla24xx_process_abts()` leaves upper 64 bytes uninitialized for 29xx adapters.
--

--- Patch [29]: [PATCH v5 29/56] scsi: qla2xxx: Update handling of ELS IOCBs for 29xx series ---
commit 0508a9076a015bdb2b8eca42fabd1c2776bb924b
Author: Nilesh Javali <njavali@marvell.com>

scsi: qla2xxx: Update handling of ELS IOCBs for 29xx series

This commit updates ELS IOCB handling to support the extended 128-byte
els_entry_24xx_ext structure for 29xx series adapters. It modifies several
functions to take generic void pointers and introduces a helper to handle
24xx vs 29xx bitfield encoding.

> diff --git a/drivers/scsi/qla2xxx/qla_inline.h b/drivers/scsi/qla2xxx/qla_inline.h
> --- a/drivers/scsi/qla2xxx/qla_inline.h
> +++ b/drivers/scsi/qla2xxx/qla_inline.h
[ ... ]
> @@ -863,3 +863,31 @@ qla_sts_fwi2_extract(struct qla_hw_data *ha, void *pkt,
>  		host_to_fcp_swap(s->data, sizeof(s->data));
>  	}
>  }
> +
> +/*
> + * qla_els_set_vp_sof() - write the vp_index / sof_type pair into an ELS
> + *    pass-through IOCB (els_entry_24xx{,_ext}).
> + *
> + * Both layouts have the same 16-bit slot at offset 14, but it is encoded
> + * differently:
> + *   - 24xx: separate u8 vp_index + u8 sof_type with EST_SOFI3 (1 << 4)
> + *   - 29xx: __le16 with bitfields { vp_index:9, reserved_1_sof:3,
> + *           sof_type:4 } and ELS_EXT_EST_SOFI3
> + * so this is the single point in the driver that knows about that
> + * encoding split.
> + */
> +static inline void
> +qla_els_set_vp_sof(struct scsi_qla_host *vha, void *pkt, u16 vp_idx)
> +{
> +	if (IS_QLA29XX(vha->hw)) {
> +		struct els_entry_24xx_ext *ext = pkt;
> +
> +		ext->vp_index = vp_idx;
> +		ext->sof_type = ELS_EXT_EST_SOFI3;

[Severity: High]
Does this code introduce an endianness bug on big-endian architectures?

The 29xx extended ELS struct defines these fields as C bitfields inside a
__le16 type. On big-endian architectures like s390x or PowerPC, the compiler
will place these bitfields in the most significant bits of the 16-bit word,
reversing the layout expected by the little-endian hardware.

Could this corrupt the descriptor DMA'd to the firmware, causing ELS
commands to be rejected or misrouted?

> +	} else {
> +		struct els_entry_24xx *e = pkt;
> +
> +		e->vp_index = vp_idx;
> +		e->sof_type = EST_SOFI3;
> +	}
> +}
[ ... ]
> diff --git a/drivers/scsi/qla2xxx/qla_isr.c b/drivers/scsi/qla2xxx/qla_isr.c
> --- a/drivers/scsi/qla2xxx/qla_isr.c
> +++ b/drivers/scsi/qla2xxx/qla_isr.c
[ ... ]
> @@ -131,7 +140,7 @@ qla24xx_process_abts(struct scsi_qla_host *vha, struct purex_item *pkt)
>  	}
>  
>  	/* send ABTS response */
> -	abts_rsp = (void *)rsp_els;
> +	abts_rsp = rsp_pkt;
>  	memset(abts_rsp, 0, sizeof(*abts_rsp));

[Severity: Medium]
Will this memset only clear the first 64 bytes of the newly allocated
128-byte buffer on 29xx hardware?

Since abts_rsp is cast to the legacy 64-byte struct abts_entry_24xx,
sizeof(*abts_rsp) evaluates to 64. The remaining 64 bytes of the 128-byte
buffer may contain stale data from the prior firmware interaction, which
could then be sent back to the hardware.

[Severity: High]
Is the 29xx hardware fully supported by the ABTS response logic here?

While the buffer is allocated dynamically for 128 bytes, the struct parsing
and formatting logic casts the buffer to the legacy struct abts_entry_24xx.
For 29xx hardware, vp_idx and sof_type are packed into a 16-bit field, but
the legacy struct uses two 8-bit fields.

Writing to the 8-bit sof_type and masking it:

abts_rsp->vp_idx = abts->vp_idx;
abts_rsp->sof_type = abts->sof_type & 0xf0;

will overwrite the 9th bit of vp_index, silently truncating it to 8 bits.

Could this cause NPIV port traffic for virtual ports with an index > 255 to
be misrouted or dropped?

>  	abts_rsp->entry_type = ABTS_RSP_TYPE;
>  	abts_rsp->entry_count = 1;

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