Re: [PATCH v6 7/8] scsi: ibmvfc: register and use asynchronous sub CRQ for events

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

New issues:
- [High] The newly added async_sub_crq is leaked and leaves its IRQ registered if ibmvfc_alloc_channels() fails during SCSI channel initialization.
- [High] The async_sub_crq is completely omitted from the CRQ reset and re-enable sequences, breaking its lifecycle.

Pre-existing issues:
- [Critical] NULL pointer dereference in ibmvfc_channel_setup() when mq_enabled == 0.
- [High] Missing bounds checks on hypervisor-provided queue counts can lead to out-of-bounds array accesses.
- [High] Error codes from request_irq() are unconditionally clobbered, silently masking critical failures.
--

--- Patch [7]: [PATCH v6 7/8] scsi: ibmvfc: register and use asynchronous sub CRQ for events ---
commit 10af0529db6ade6b831181ddc695f2e8713b38ee
Author: Dave Marquardt <[email protected]>

scsi: ibmvfc: register and use asynchronous sub CRQ for events
    
Wire the async sub-CRQ into the sub-CRQ lifecycle so it is allocated,
registered, and freed alongside the SCSI channel queues.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
[ ... ]
> @@ -5792,6 +5794,7 @@ static void ibmvfc_channel_setup_done(struct ibmvfc_event *evt)
>  		for (i = 0; i < nvme->active_queues; i++)
>  			nvme->scrqs[i].vios_cookie =
>  				be64_to_cpu(setup->channel_handles[scsi->active_queues + i]);

[Severity: High]
This is a pre-existing issue, but is it possible for an unexpectedly large
response from the hypervisor to cause out-of-bounds array accesses here?

Looking at drivers/scsi/ibmvscsi/ibmvfc-core.c:ibmvfc_channel_setup_done(),
the unvalidated active_queues count returned by the VIOS is used directly
as a loop limit to write into the driver's nvme->scrqs and scsi->scrqs
arrays.

Could a malformed or compromised response corrupt adjacent memory?

> +		vhost->async_sub_crq.vios_cookie = be64_to_cpu(setup->async_sub_crq_handle);
>  
>  		ibmvfc_dbg(vhost, "Using %u SCSI channels\n",
>  			   scsi->active_queues);
[ ... ]
> @@ -5851,6 +5854,7 @@ static void ibmvfc_channel_setup(struct ibmvfc_host *vhost)
>  		for (i = 0; i < nvme_channels; i++)
>  			setup_buf->channel_handles[scsi_channels + i] =
>  				cpu_to_be64(nvme->scrqs[i].cookie);

[Severity: Critical]
This is a pre-existing issue, but does this loop risk a NULL pointer
dereference when mq_enabled is 0?

Looking at drivers/scsi/ibmvscsi/ibmvfc-core.c:ibmvfc_channel_setup(),
if the driver is loaded with mq_enabled=0, ibmvfc_init_sub_crqs() returns
early without allocating scsi_scrqs.scrqs. 

However, if a modern VIOS supports channels, the enquiry succeeds and sets
max_vios_scsi_channels > 0.

This would cause ibmvfc_channel_setup() to compute scsi_channels > 0 and
unconditionally execute this loop, accessing the unallocated
scsi->scrqs[i].cookie array.

> +		setup_buf->async_sub_crq_handle = cpu_to_be64(vhost->async_sub_crq.cookie);
>  	}
>  
>  	ibmvfc_init_event(evt, ibmvfc_channel_setup_done, IBMVFC_MAD_FORMAT);
[ ... ]
> @@ -6949,7 +6953,9 @@ static void ibmvfc_reg_sub_crqs(struct ibmvfc_host *vhost,
>  	for (i = 0; i < channels->max_queues; i++) {
>  		if (ibmvfc_register_channel(vhost, channels, i)) {

[Severity: High]
This is a pre-existing issue, but does ibmvfc_register_channel() silently
mask critical failures?

Looking at drivers/scsi/ibmvscsi/ibmvfc-core.c:ibmvfc_register_channel(),
if request_irq() fails, the code jumps to the irq_failed label where it
executes:

rc = plpar_hcall_norets(H_FREE_SUB_CRQ, vdev->unit_address, scrq->cookie);

If the hypervisor successfully frees the sub-CRQ, rc is overwritten with 0
(H_SUCCESS). 

The function then returns 0, falsely indicating success to the caller,
which leaves the channel marked active but without an interrupt handler.

>  			for (j = i; j > 0; j--)
> -				ibmvfc_deregister_channel(vhost, channels, j - 1);
> +				ibmvfc_deregister_channel(
> +					vhost, channels, j - 1);
> +
>  			vhost->do_enquiry = 0;
>  			return;
>  		}
[ ... ]
> @@ -7004,10 +7010,27 @@ static int ibmvfc_alloc_channels(struct ibmvfc_host *vhost,
>  
>  static void ibmvfc_init_sub_crqs(struct ibmvfc_host *vhost)
>  {
[ ... ]
> +	/* register async_sub_crq channel */
> +	if (ibmvfc_register_channel(vhost, &vhost->scsi_scrqs, -1)) {
> +		ibmvfc_free_queue(vhost, &vhost->async_sub_crq);
> +		vhost->do_enquiry = 0;
> +		vhost->mq_enabled = 0;
> +		return;
> +	}
> +
>  	if (ibmvfc_alloc_channels(vhost, &vhost->scsi_scrqs)) {
>  		vhost->do_enquiry = 0;
>  		vhost->mq_enabled = 0;
>  		return;
>  	}

[Severity: High]
Does this error path leak the newly added async_sub_crq?

Looking at drivers/scsi/ibmvscsi/ibmvfc-core.c:ibmvfc_init_sub_crqs(),
if ibmvfc_alloc_channels() fails during SCSI channel initialization, the
function returns early.

This skips deregistering or freeing the successfully allocated
async_sub_crq.

> @@ -7050,6 +7073,9 @@ static void ibmvfc_release_sub_crqs(struct ibmvfc_host *vhost)
>  	if (!vhost->scsi_scrqs.scrqs)
>  		return;

[Severity: High]
Could this early return bypass async_sub_crq cleanup entirely?

Looking at drivers/scsi/ibmvscsi/ibmvfc-core.c:ibmvfc_release_sub_crqs(),
if scsi_scrqs allocation failed earlier, !vhost->scsi_scrqs.scrqs evaluates
to true, causing an immediate return. 

When the module unloads, the memory for async_sub_crq is freed but its
IRQ remains registered, potentially causing a use-after-free panic if the
hardware triggers an interrupt.

> +	ibmvfc_deregister_channel(vhost, &vhost->scsi_scrqs, -1);
> +	ibmvfc_free_queue(vhost, &vhost->async_sub_crq);
> +
>  	ibmvfc_dereg_sub_crqs(vhost, &vhost->scsi_scrqs);

[Severity: High]
Are there lifecycle regressions during host adapter resets for async_sub_crq?

Looking at drivers/scsi/ibmvscsi/ibmvfc-core.c:ibmvfc_reset_crq(),
when the host adapter undergoes a reset, ibmvfc_reset_crq() and
ibmvfc_reenable_crq_queue() explicitly deregister and re-register the SCSI
and NVMe sub-CRQs. 

However, async_sub_crq appears to be omitted from these sequences.

Across a reset, the async_sub_crq registration with the hypervisor becomes
stale, which would permanently break the receipt of asynchronous events
until the driver is reloaded. 

Should async_sub_crq be included in the reset path?

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