Re: [PATCH v7 6/8] scsi: ibmvfc: extend channel registration and deregistration for async subq

[sashiko-bot@kernel.org]

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

New issues:
- [High] Race condition and lockless list access in async sub-CRQ handler.

Pre-existing issues:
- [Medium] Stale IRQ mapping handle left on error path causes free_irq kernel WARNING.
--

commit 99a10b7da4dba7f584bda8695781cb27e1b928b0
Author: Dave Marquardt <davemarq@linux.ibm.com>

scsi: ibmvfc: extend channel registration and deregistration for async subq

This commit extends ibmvfc_register_channel() and
ibmvfc_deregister_channel() to handle the async sub-CRQ by accepting a
negative index. It also removes the __maybe_unused attribute from the async
interrupt handler to enable its use.

> 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
> @@ -4409,7 +4409,7 @@ static void ibmvfc_drain_async_subq(struct ibmvfc_queue *scrq)
>   * @scrq_instance: async subq
>   *
>   **/
> -static irqreturn_t __maybe_unused ibmvfc_interrupt_async_subq(int irq, void *scrq_instance)
> +static irqreturn_t ibmvfc_interrupt_async_subq(int irq, void *scrq_instance)
>  {
>  	struct ibmvfc_queue *scrq = (struct ibmvfc_queue *)scrq_instance;
>  

[Severity: High]
Now that the async sub-CRQ handler is enabled by this patch, can its
execution race with target teardown or host reset? 

In drivers/scsi/ibmvscsi/ibmvfc-core.c, the interrupt handler calls:
ibmvfc_interrupt_async_subq() -> ibmvfc_drain_async_subq() ->
ibmvfc_handle_async().

The ibmvfc_handle_async() function expects to hold vhost->host->host_lock,
but this call chain only holds scrq->q_lock. Consequently, it iterates over
and modifies vhost->scsi_scrqs.targets without the proper lock. 

Could this lockless list traversal cause a use-after-free or data corruption
when handling dynamic async events sent by the hypervisor?

[ ... ]

> @@ -6825,36 +6841,49 @@ static int ibmvfc_register_channel(struct ibmvfc_host *vhost,
[ ... ]
>  	rc = request_irq(scrq->irq, scrq->handler, 0, scrq->name, scrq);
>  
>  	if (rc) {
> -		dev_err(dev, "Couldn't register sub-crq[%d] irq\n", index);
> +		if (!is_async)
> +			dev_err(dev, "Couldn't register sub-crq[%d] irq\n", index);
> +		else
> +			dev_err(dev, "Couldn't register async sub-crq irq\n");
>  		irq_dispose_mapping(scrq->irq);
>  		goto irq_failed;
>  	}

[Severity: Medium]
This is a pre-existing issue, but does leaving scrq->irq populated with a
stale value after irq_dispose_mapping() cause a kernel warning during
teardown?

If request_irq() fails in ibmvfc_register_channel(), the error path calls
irq_dispose_mapping(scrq->irq) but does not clear the irq field to 0. Later,
when ibmvfc_deregister_channel() is unconditionally called for the async
sub-CRQ during teardown, it might call free_irq(scrq->irq, scrq) on this
stale mapping ID (or on 0 if it was never registered). 

Should scrq->irq be explicitly cleared to 0 in this error path to prevent
invalid IRQ descriptor warnings?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831-ibmvfc-fpin-support-v7-0-77d0b9809080@linux.ibm.com?part=6