Re: [PATCH 2/5] scsi: elx: efct: free the HIO when the WQE cannot be built

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

Pre-existing issues:
- [Critical] `sli_xmit_sequence64_wqe()` treats the provided SGL array as a direct payload buffer, leading to an out-of-bounds DMA read and transmitting kernel memory contents over Fibre Channel.
- [High] If asynchronous task scheduling fails due to memory pressure, the IO request is leaked and never completed to the upper layer.
- [High] Missing `efct_hw_reqtag_free` on error paths leaks WQ callback context, eventually leading to exhaustion of the request tag pool.
--

--- Patch [2]: [PATCH 2/5] scsi: elx: efct: free the HIO when the WQE cannot be built ---
commit c236a09d90fd9d31b4fd7efd9ef66e408be03d17
Author: Ali Ahmet Memis <[email protected]>

scsi: elx: efct: free the HIO when the WQE cannot be built
    
efct_els_hw_srrs_send() and efct_hw_bls_send() allocate an HIO, ask sli4
to build a WQE into it, and give up when that fails without putting the
HIO back:

[ ... ]

> diff --git a/drivers/scsi/elx/efct/efct_hw.c b/drivers/scsi/elx/efct/efct_hw.c
> index 15c37ff1d52e4..6cc48fa3e656a 100644
> --- a/drivers/scsi/elx/efct/efct_hw.c
> +++ b/drivers/scsi/elx/efct/efct_hw.c

[ ... ]

> @@ -2820,24 +2821,27 @@ efct_els_hw_srrs_send(struct efc *efc, struct efc_disc_io *io)
>  		rc = -EIO;
>  	}
>  
> -	if (rc == 0) {
> -		hio->xbusy = true;
> +	if (rc) {
> +		efct_hw_io_free(hw, hio);
> +		return rc;
> +	}

[Severity: Critical]
This is a pre-existing issue, but does sli_xmit_sequence64_wqe() incorrectly
receive hio->sgl instead of the actual data buffer send?

In efct_els_hw_srrs_send() for EFC_DISC_IO_CT_RESP:

	if (sli_xmit_sequence64_wqe(&hw->sli, hio->wqe.wqebuf, hio->sgl,
				    &ct_params)){

If sli_xmit_sequence64_wqe() treats the provided SGL array as a direct
payload buffer:

libefc_sli/sli4.c:sli_xmit_sequence64_wqe() {
	...
	xmit->bde.bde_type_buflen =
		cpu_to_le32((SLI4_BDE_TYPE_VAL(64)) |
			(params->rsp_len & SLI4_BDE_LEN_MASK));
	xmit->bde.u.data.low  =
			cpu_to_le32(lower_32_bits(payload->phys));
	xmit->bde.u.data.high =
			cpu_to_le32(upper_32_bits(payload->phys));
	xmit->sequence_payload_len = cpu_to_le32(params->rsp_len);
	...
}

Could this lead to an out-of-bounds DMA read and transmit kernel memory
contents over Fibre Channel?

[ ... ]

[Severity: High]
This is a pre-existing issue, but does efct_hw_send_frame() leak the WQ
callback context on error paths?

drivers/scsi/elx/efct/efct_hw.c:efct_hw_send_frame() {
	...
	rc = sli_send_frame_wqe(&hw->sli, wqe->wqebuf,
				sof, eof, (u32 *)hdr, payload, payload->len,
				EFCT_HW_SEND_FRAME_TIMEOUT, xri,
				ctx->wqcb->instance_index);
	if (rc) {
		efc_log_err(hw->os, "sli_send_frame_wqe failed: %d\n", rc);
		return -EIO;
	}

	/* Write to WQ */
	rc = efct_hw_wq_write(wq, wqe);
	if (rc) {
		efc_log_err(hw->os, "efct_hw_wq_write failed: %d\n", rc);
		return -EIO;
	}
	...
}

If sli_send_frame_wqe() or efct_hw_wq_write() fail, the function returns -EIO,
but shouldn't it call efct_hw_reqtag_free() to avoid exhausting the request
tag pool?

[Severity: High]
This is a pre-existing issue, but do efct_scsi_dispatch_pending() and
efct_scsi_check_pending() leak the IO request if asynchronous task scheduling
fails?

drivers/scsi/elx/efct/efct_scsi.c:efct_scsi_dispatch_pending() {
	...
		if (efct_hw_async_call(&efct->hw,
				       efct_scsi_check_pending_async_cb, io)) {
			efc_log_debug(efct, "call hw async failed\n");
		}
	...
}

drivers/scsi/elx/efct/efct_scsi.c:efct_scsi_check_pending() {
	...
			if (efct_hw_async_call(&efct->hw,
					efct_scsi_check_pending_async_cb, io)) {
				efc_log_debug(efct, "hw async failed\n");
			}
	...
}

If efct_hw_async_call() fails due to memory pressure, it logs an error but
doesn't call the callback or return the IO to the pool. Will this silently drop
the SCSI IO request and cause a command timeout?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2