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

Pre-existing issues:
- [Critical] Asynchronous timer deletion during teardown allows the timer callback to execute after the event pool memory has been freed.
- [High] Unknown channel protocol error path skips IRQ mapping disposal and falsely reports success, causing an IRQ leak and a subsequent kernel warning on teardown.
- [High] Sub-CRQ cookies are not zeroed after being freed via H_FREE_SUB_CRQ, leading to double-freeing the cookies during adapter resets or teardowns.
- [High] The adapter initialization state machine stalls indefinitely if channel registration fails.
- [Medium] ibmvfc_deregister_channel() unconditionally executes H_FREE_SUB_CRQ for unmapped or unregistered queues, causing spurious hypervisor errors and kernel log spam.
- [Medium] Event pool memory is permanently leaked if queue allocation fails midway in ibmvfc_alloc_queue().
--

commit 9f8024026aa5c4ef26997be7255cb4acbd34dd0f
Author: Dave Marquardt <davemarq@linux.ibm.com>

    scsi: ibmvfc: fix IRQ leak and guard deregister on channel reg failure
    
    If request_irq() fails during ibmvfc_register_channel(), the error path
    disposes of the IRQ mapping via irq_dispose_mapping() but leaves scrq->irq
    populated with the stale virq number. A subsequent call to
    ibmvfc_deregister_channel() during teardown or reset cleanup then
    unconditionally invokes free_irq() and irq_dispose_mapping() on the stale
    IRQ descriptor, triggering kernel warnings.
    
    Fix this by:
    1. Explicitly setting scrq->irq to 0 after irq_dispose_mapping() in the
       ibmvfc_register_channel() error path.
    2. Guarding 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 1c660edcf608a..f88262ffad134 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> @@ -6888,6 +6888,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;
>  	}
>  
> @@ -6930,9 +6931,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: Medium]
This is a pre-existing issue, but does this unconditional hypercall cause
spurious hypervisor errors and kernel log spam?

During adapter teardown or reset, ibmvfc_dereg_sub_crqs() iteratively calls
ibmvfc_deregister_channel() on all allocated sub-CRQs up to max_queues,
regardless of whether they were successfully registered.

This results in H_FREE_SUB_CRQ being invoked on uninitialized queues where
scrq->cookie is 0.

[Severity: High]
This is a pre-existing issue, but is it possible for sub-CRQ cookies to be
double-freed during adapter resets or teardowns?

After calling H_FREE_SUB_CRQ above, scrq->cookie is not zeroed. If iterative
teardown functions call this again, could the double-free destroy an active
channel on the VIOS side if the hypervisor has reallocated that cookie?

This same issue occurs in the error path of ibmvfc_register_channel():

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));
...
}

Where the cookie is similarly freed but not zeroed.

drivers/scsi/ibmvscsi/ibmvfc-core.c:ibmvfc_register_channel() {
...
		default:
			dev_err(dev, "Unknown channel protocol (%d)\n",
				channels->protocol);
			goto irq_failed;
		}
...
}

[Severity: High]
This is a pre-existing issue, but does the unknown channel protocol error path
skip IRQ mapping disposal and falsely report success?

If channels->protocol hits the default case, it jumps to irq_failed. Because
rc was initialized to -ENOMEM but overwritten to 0 by a successful
h_reg_sub_crq(), the function falsely returns 0.

This leaves scrq->irq non-zero and causes an IRQ leak and a subsequent kernel
warning on teardown when free_irq() is called on an unrequested IRQ.

drivers/scsi/ibmvscsi/ibmvfc-core.c:ibmvfc_alloc_queue() {
...
	if (dma_mapping_error(dev, queue->msg_token)) {
		free_page((unsigned long)queue->msgs.handle);
		queue->msgs.handle = NULL;
		return -ENOMEM;
	}
...
}

[Severity: Medium]
This is a pre-existing issue, but does this error path leak the event pool
memory?

If get_zeroed_page() or dma_map_single() fail, the function returns -ENOMEM
without calling ibmvfc_free_event_pool() to clean up the event pool allocated
at the beginning of the function.

When handling the failure in ibmvfc_alloc_channels():

drivers/scsi/ibmvscsi/ibmvfc-core.c:ibmvfc_alloc_channels() {
...
		if (rc) {
			for (j = i; j > 0; j--) {
				scrq = &channels->scrqs[j - 1];
				ibmvfc_free_queue(vhost, scrq);
			}
			kfree(channels->scrqs);
...
}

The array is freed, losing the reference to the partially-allocated queue i
and permanently leaking its event pool.

drivers/scsi/ibmvscsi/ibmvfc-core.c:ibmvfc_fail_request() {
...
	timer_delete(&evt->timer);
...
}

[Severity: Critical]
This is a pre-existing issue, but does asynchronous timer deletion during
teardown allow the timer callback to execute after the event pool memory has
been freed?

During adapter removal, ibmvfc_purge_requests() calls ibmvfc_fail_request()
which executes timer_delete() instead of timer_delete_sync(). Teardown then
quickly progresses to ibmvfc_free_event_pool().

If a command timeout occurs concurrently:

drivers/scsi/ibmvscsi/ibmvfc-core.c:ibmvfc_timeout() {
...
	struct ibmvfc_event *evt = timer_container_of(evt, t, timer);
	struct ibmvfc_host *vhost = evt->vhost;
...
}

Can the timer callback access fields on an event structure that has already
been freed, leading to a use-after-free and potential kernel crash?

drivers/scsi/ibmvscsi/ibmvfc-core.c:ibmvfc_npiv_login_done() {
...
	if (ibmvfc_check_caps(vhost, IBMVFC_CAN_SUPPORT_CHANNELS)) {
		if (ibmvfc_check_caps(vhost, IBMVFC_SUPPORT_SCSI))
			vhost->do_scsi_login = 1;
		if (ibmvfc_check_caps(vhost, IBMVFC_SUPPORT_NVMEOF))
			vhost->do_nvme_login = 1;
		if (vhost->do_enquiry)
			ibmvfc_channel_enquiry(vhost);
	} else {
		vhost->do_enquiry = 0;
		ibmvfc_set_host_action(vhost, IBMVFC_HOST_ACTION_QUERY);
		wake_up(&vhost->work_wait_q);
	}
...
}

[Severity: High]
This is a pre-existing issue, but can the adapter initialization state machine
stall indefinitely if channel registration fails?

If ibmvfc_alloc_queue() or ibmvfc_register_channel() fail,
vhost->do_enquiry is set to 0. 

Later, in ibmvfc_npiv_login_done(), if the adapter supports channels but
vhost->do_enquiry is 0, the code skips ibmvfc_channel_enquiry() but doesn't
fall into the else branch.

Because it does not advance vhost->action or wake up the work queue, does the
state machine remain stuck in IBMVFC_HOST_ACTION_INIT_WAIT, rendering the
storage adapter unusable?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909-ibmvfc-fpin-support-v8-0-b27183b055af@linux.ibm.com?part=7