Re: [PATCH v6 5/8] scsi: ibmvfc: add interrupt routine for asynchronous sub CRQ

[sashiko-bot@kernel.org]

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

New issues:
- [High] Double clearing of the CRQ valid byte creates a race condition causing silently dropped hardware events.
- [High] The fpin_workq is destroyed before unregistering the CRQ/Sub-CRQ IRQ handlers, leading to a NULL pointer dereference.
- [Medium] Nested acquisition of queue locks sharing the same lockdep class triggers a recursive locking warning.
- [Low] The newly added interrupt handler `ibmvfc_interrupt_async_subq` is never registered.
- [Low] ibmvfc_handle_async() attempts to dereference a void pointer, breaking compilation.

Pre-existing issues:
- [High] Uninitialized variable 'shwqs' causes out-of-bounds access and kernel panic in ibmvfc_purge_requests.
--

--- Patch [5]: [PATCH v6 5/8] scsi: ibmvfc: add interrupt routine for asynchronous sub CRQ ---
commit 0805a2f1bf2f7cec43403295f1715fe78c1224dd
Author: Dave Marquardt <davemarq@linux.ibm.com>

scsi: ibmvfc: add interrupt routine for asynchronous sub CRQ

This commit adds an interrupt handler for asynchronous sub-CRQ events,
which drains pending entries and re-enables interrupts.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> index 4d1630d2ab1b5..3b5732f2fb6ec 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
[ ... ]
> @@ -4372,6 +4372,52 @@ static struct ibmvfc_crq *ibmvfc_next_scrq(struct ibmvfc_queue *scrq)
>  	return crq;
>  }
>  
> +static void ibmvfc_drain_async_subq(struct ibmvfc_queue *scrq)
> +{
> +	struct ibmvfc_host *vhost = scrq->vhost;
> +	struct ibmvfc_crq *crq;
> +	unsigned long flags;
> +	int done = 0;
> +
> +	spin_lock_irqsave(vhost->host->host_lock, flags);
> +	spin_lock(scrq->q_lock);

[Severity: Medium]
Does acquiring this lock trigger a recursive locking warning?

Both async_sub_crq.q_lock and scsi_q[i].q_lock appear to be initialized
from the same spin_lock_init(&queue->_lock) in ibmvfc_alloc_queue(),
meaning they share the same lockdep class.

If an event handled here triggers a host reset, the call chain could be:
ibmvfc_drain_async_subq() -> ibmvfc_handle_async() ->
ibmvfc_hard_reset_host() -> ibmvfc_purge_requests()

ibmvfc_purge_requests() then acquires scsi_q[i].q_lock for all active
channels, potentially causing lockdep to flag a false-positive recursive
locking splat.

> +	while (!done) {
> +		while ((crq = ibmvfc_next_scrq(scrq)) != NULL) {
> +			ibmvfc_handle_async(crq, scrq->vhost, true);

[Severity: Low]
Does changing ibmvfc_handle_async() to accept a void *crq cause
a compilation error?

It looks like the NVMe target iteration loop in ibmvfc_handle_async()
still attempts to dereference the pointer directly:

if (!crq->scsi_id && !crq->wwpn && !crq->node_name)

Dereferencing a void pointer will break the build.

[Severity: High]
This is a pre-existing issue, but does ibmvfc_purge_requests() use an
uninitialized variable when called from this path?

If a host reset occurs, the chain ibmvfc_handle_async() ->
ibmvfc_hard_reset_host() -> ibmvfc_purge_requests() is triggered.

In ibmvfc_purge_requests(), shwqs is declared but not initialized:
int shwqs, nhwqs = 0;

If vhost->using_channels is false, shwqs remains uninitialized.
The loop then uses it as a bound:
for (i = 0; i < shwqs; i++) {

Could this cause an out-of-bounds access on the scsi_q array?

> +			crq->valid = 0;
> +			wmb();	/* complete write */

[Severity: High]
Does clearing the valid byte here create a race condition?

When processing an event, ibmvfc_handle_async() already hands ownership
back to the adapter by clearing the valid byte and issuing a write
memory barrier.

If the adapter writes a new event into this CRQ slot exactly between
the clear in ibmvfc_handle_async() and the clear here, will the newly
arrived event be permanently overwritten and lost?

> +		}
> +
> +		ibmvfc_toggle_scrq_irq(scrq, 1);
> +		crq = ibmvfc_next_scrq(scrq);
> +		if (crq != NULL) {
> +			ibmvfc_toggle_scrq_irq(scrq, 0);
> +			ibmvfc_handle_async(crq, scrq->vhost, true);
> +			crq->valid = 0;
> +			wmb();	/* complete write */
> +		} else
> +			done = 1;
> +	}
> +	spin_unlock(scrq->q_lock);
> +	spin_unlock_irqrestore(vhost->host->host_lock, flags);
> +}
> +
> +/**
> + * ibmvfc_interrupt_async_subq - Handle an async event from the adapter
> + * @irq:           interrupt request
> + * @scrq_instance: async subq
> + *
> + **/
> +static irqreturn_t ibmvfc_interrupt_async_subq(int irq, void *scrq_instance)

[Severity: Low]
Is this interrupt handler ever registered?

It appears ibmvfc_interrupt_async_subq is defined to handle asynchronous
sub-CRQ events, but it is never passed to request_irq() during queue
initialization, leaving this function as dead code.

[Severity: High]
Can an async event arriving during driver teardown cause a NULL pointer
dereference?

During ibmvfc_remove(), vhost->fpin_workq is destroyed and set to NULL
before device interrupts are disabled via ibmvfc_release_crq_queue().

If an async FPIN event arrives from the adapter in this window, this
interrupt handler will run and eventually call ibmvfc_handle_async(),
which executes:
queue_work(vhost->fpin_workq, &aw->async_work_s);

Could this trigger a panic by queuing work to a NULL workqueue?

> +{
> +	struct ibmvfc_queue *scrq = (struct ibmvfc_queue *)scrq_instance;
> +
> +	ibmvfc_toggle_scrq_irq(scrq, 0);
> +	ibmvfc_drain_async_subq(scrq);
> +
> +	return IRQ_HANDLED;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260817-ibmvfc-fpin-support-v6-0-62a2d525e70d@linux.ibm.com?part=5