Re: [PATCH v9 7/9] scsi: ibmvfc: fix IRQ leak and guard deregister on channel reg failure

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Error code from request_irq failure is swallowed, causing initialization functions to falsely return success.
- [High] Unconditional execution of H_FREE_SUB_CRQ and failure to clear `scrq->cookie` leads to hypervisor double-free and use-after-free of sub-CRQs.
--

commit 2c9e0fcaebe0c140397143556c20f5137db7357d
Author: Dave Marquardt <davemarq@linux.ibm.com>

scsi: ibmvfc: fix IRQ leak and guard deregister on channel reg failure

This commit fixes an IRQ leak where the error path leaves scrq->irq
populated, causing a subsequent deregister call to double-free the IRQ.
It explicitly sets scrq->irq to 0 on failure and guards the teardown
process against unregistered queues.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> index fa312a58471fa..8eeb125ee13b8 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> @@ -6891,6 +6891,7 @@ static int ibmvfc_register_channel(struct ibmvfc_host *vhost,
>  		else
>  			dev_err(dev, "Couldn't register async sub-crq irq\n");
>  		irq_dispose_mapping(scrq->irq);
> +		scrq->irq = 0;
>  		goto irq_failed;
>  	}
>  
> @@ -6933,9 +6934,11 @@ static void ibmvfc_deregister_channel(struct ibmvfc_host *vhost,
>  
>  	ENTER;
>  
> -	free_irq(scrq->irq, scrq);
> -	irq_dispose_mapping(scrq->irq);
> -	scrq->irq = 0;
> +	if (scrq->irq) {
> +		free_irq(scrq->irq, scrq);
> +		irq_dispose_mapping(scrq->irq);
> +		scrq->irq = 0;
> +	}
>  
>  	do {
>  		rc = plpar_hcall_norets(H_FREE_SUB_CRQ, vdev->unit_address,

[Severity: High]
This is a pre-existing issue, but by clearing the irq here, should we also be
clearing scrq->cookie?

If ibmvfc_reg_sub_crqs() fails, it triggers a rollback that calls
ibmvfc_deregister_channel(), which executes H_FREE_SUB_CRQ but leaves
scrq->cookie populated.

Since ibmvfc_dereg_sub_crqs() unconditionally loops over channels->max_queues
and calls H_FREE_SUB_CRQ on everything during teardown, couldn't this lead to
passing stale cookies to the hypervisor?

If those cookies were reused for NVMe queues, would this destroy active queues
and cause use-after-free corruption?

Similarly, in ibmvfc_register_channel(), if request_irq() fails, the error
path executes H_FREE_SUB_CRQ without clearing scrq->cookie:

irq_failed:
	do {
		hcall_rc = plpar_hcall_norets(H_FREE_SUB_CRQ, vdev->unit_address, scrq->cookie);
	} while (hcall_rc == H_BUSY || H_IS_LONG_BUSY(hcall_rc));
reg_failed:


[Severity: High]
This is a pre-existing issue, but while reviewing the initialization code,
does ibmvfc_init_crq() swallow the error code from request_irq() failures?

In ibmvfc_init_crq(), retrc is set to 0 upon successful CRQ registration:

	retrc = 0;
	...
	if ((rc = request_irq(vdev->irq, ibmvfc_interrupt, 0, IBMVFC_NAME, vhost))) {
		dev_err(dev, "Couldn't register irq 0x%x. rc=%d\n", vdev->irq, rc);
		goto req_irq_failed;
	}
	...
req_irq_failed:
	tasklet_kill(&vhost->tasklet);
	do {
		rc = plpar_hcall_norets(H_FREE_CRQ, vdev->unit_address);
	} while (rc == H_BUSY || H_IS_LONG_BUSY(rc));
reg_crq_failed:
	ibmvfc_free_queue(vhost, crq);
	return retrc;

Since retrc is already 0, the error path unconditionally returns success
instead of propagating rc.

Could this cause the probe function to consider the adapter successfully
initialized, even though it lacks a functional IRQ handler?

Would this lead to a silent hang during IO operations since no completion
interrupts will be processed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911021910.1038128-1-tyreld@linux.ibm.com?part=7