Re: [PATCH v13 08/11] 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] scsi/ibmvfc: missing load-store memory barrier allows DMA ring data corruption
--

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

scsi: ibmvfc: add interrupt routine for asynchronous sub CRQ

This commit introduces an interrupt handler and a drain routine for
asynchronous sub-CRQ events. It adds direct traversal of the async
sub-CRQ ring and processes available entries while managing the queue lock
and hardware interrupts.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> index 6db9df91e9e68..32bc00a4be7ec 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c

[ ... ]

> +static void ibmvfc_drain_async_subq(struct ibmvfc_queue *scrq)
> +{
> +	struct ibmvfc_host *vhost = scrq->vhost;
> +	struct ibmvfc_async_crq_event ae = {
> +		.type = IBMVFC_ASYNC_CRQ_SUB,
> +	};
> +	struct ibmvfc_async_sub_crq *crq;
> +	unsigned long flags;
> +	int done = 0;
> +
> +	spin_lock_irqsave(&vhost->host->host_lock, flags);
> +	spin_lock(scrq->q_lock);
> +	while (!done) {
> +		while ((crq = ibmvfc_next_async_scrq(scrq)) != NULL) {
> +			ae.subq = *crq;
> +			ibmvfc_handle_async(&ae, scrq->vhost);
> +			crq->valid = 0;

[Severity: High]
Could the store to crq->valid be reordered before the reads from the payload
complete?

On weakly-ordered architectures like PowerPC, a load from memory can be
delayed and executed after a subsequent store unless separated by a memory
barrier.

While ibmvfc_handle_async() takes spinlocks in some paths, it does not
execute any spinlock or hardware barrier before returning in others
(such as IBMVFC_AE_LINK_UP or IBMVFC_AE_SCN_FABRIC).

If the hardware observes valid == 0 early, it might overwrite the new event
while the CPU is still reading it, causing the CPU to ingest corrupted data.
Does this require an smp_mb() or smp_store_release() before clearing the
valid bit?

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

[Severity: High]
Does this race-closure check also suffer from the same missing load-store
barrier before clearing the valid bit?

> +			wmb();	/* complete write */
> +		} else
> +			done = 1;
> +	}
> +	spin_unlock(scrq->q_lock);
> +	spin_unlock_irqrestore(&vhost->host->host_lock, flags);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261007-ibmvfc-fpin-support-v13-0-b816abfa4fcc@linux.ibm.com?part=8