Re: [PATCH v13 08/11] scsi: ibmvfc: add interrupt routine for asynchronous sub CRQ

[Dave Marquardt <davemarq@linux.ibm.com>]

sashiko-bot@kernel.org writes:

> 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?

Note that the IRQ is disabled in ibmvfc_interrupt_async_subq() before
calling ibmvfc_drain_async_subq():

	ibmvfc_toggle_scrq_irq(scrq, 0);
	ibmvfc_drain_async_subq(scrq);

So I'm unclear how the hardware would be writing to *crq. Certainly not
through an interrupt.

I don't think this is a valid concern.

>> +			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?

Same issue here: interrupts are disabled.

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