Re: [PATCH v7 5/8] 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] 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?

I agree. Adding use of host lock in next revision.

>> +			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);
>> +}

Yes.

-Dave