Re: [PATCH v10 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 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Unguarded hypervisor cleanup in ibmvfc_deregister_channel() leads to double-free of hypervisor resources and unconditional H_FREE_SUB_CRQ calls.
--

commit 6e9a49d8343f9077df9f1052cfd7a71c721c4b81
Author: Dave Marquardt <davemarq@linux.ibm.com>

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

This commit ensures scrq->irq is cleared to 0 after irq_dispose_mapping()
and guards free_irq() and irq_dispose_mapping() with a check for scrq->irq
in ibmvfc_deregister_channel() so unmapped or unregistered queues are
safely skipped.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> index a9cf1096e755d..c78e221f182a2 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> @@ -6890,6 +6890,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;

[Severity: High]
This isn't a bug introduced by this patch, but on the request_irq() error
path, the hypervisor resource is freed but scrq->cookie is not cleared to 0:

drivers/scsi/ibmvscsi/ibmvfc-core.c:ibmvfc_register_channel() {
    ...
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));
    ...
}

Could this cause a double-free when ibmvfc_deregister_channel() is later
called?

>  		goto irq_failed;
>  	}
>  
> @@ -6932,9 +6933,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 does this leave the hypervisor call
unguarded?

It appears this hypervisor call is executed unconditionally using whatever
stale cookie is present, even for queues that were never successfully
registered or have already been freed.

If the hypervisor reallocates the freed cookie to another queue or device,
could the double-free maliciously or accidentally destroy a live queue?

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