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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The `ibmvfc_drain_async_subq()` function calls `ibmvfc_handle_async()` without holding the primary SCSI host lock (`vhost->host->host_lock`). This leads to severe data races and list corruption because `ibmvfc_handle_async()` mutates shared state and iterates over lists that are concurrently modified by other threads holding the lock.
--

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

scsi: ibmvfc: add interrupt routine for asynchronous sub CRQ

Add ibmvfc_interrupt_async_subq(), an IRQ handler dedicated to
asynchronous sub-CRQ events from the adapter. The handler disables
the sub-CRQ IRQ and then calls ibmvfc_drain_async_subq() to consume
all pending entries before re-enabling interrupts.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> index 229b06effd4cf..a28875c1f9830 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> @@ -4369,6 +4369,56 @@ 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_crq *crq;
> +	int done = 0;
> +
> +	spin_lock(scrq->q_lock);
> +	while (!done) {
> +		while ((crq = ibmvfc_next_scrq(scrq)) != NULL) {
> +			struct ibmvfc_async_crq_event ae = {
> +				.type = IBMVFC_ASYNC_CRQ_SUB,
> +				.subq = *(struct ibmvfc_async_sub_crq *)crq,
> +			};
> +			ibmvfc_handle_async(&ae, scrq->vhost);

[Severity: High]
Does this code need to hold the host_lock before calling ibmvfc_handle_async()?

Calling ibmvfc_handle_async() with only the local queue lock (scrq->q_lock)
held can lead to unprotected list iterations:

drivers/scsi/ibmvscsi/ibmvfc-core.c:ibmvfc_handle_async() {
    ...
		list_for_each_entry(tgt, &vhost->scsi_scrqs.targets, queue) {
    ...
}

It can also lead to unprotected read-modify-write operations on shared state:

drivers/scsi/ibmvscsi/ibmvfc-core.c:ibmvfc_handle_async() {
    ...
			vhost->events_to_log |= IBMVFC_AE_LINKUP;
    ...
}

If it races with normal target initialization, addition, or removal routines
running on other CPUs holding the host_lock, could it result in list corruption
or use-after-free?

> +			crq->valid = 0;
> +			wmb();	/* complete write */
> +		}
> +
> +		ibmvfc_toggle_scrq_irq(scrq, 1);
> +		crq = ibmvfc_next_scrq(scrq);
> +		if (crq != NULL) {
> +			struct ibmvfc_async_crq_event ae = {
> +				.type = IBMVFC_ASYNC_CRQ_SUB,
> +				.subq = *(struct ibmvfc_async_sub_crq *)crq,
> +			};
> +			ibmvfc_toggle_scrq_irq(scrq, 0);
> +			ibmvfc_handle_async(&ae, scrq->vhost);

[Severity: High]
Does this call to ibmvfc_handle_async() also need the host_lock for the same
reasons as above?

> +			crq->valid = 0;
> +			wmb();	/* complete write */
> +		} else
> +			done = 1;
> +	}
> +	spin_unlock(scrq->q_lock);
> +}

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