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

[email protected] writes:

> 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 <[email protected]>
>
> 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.

I don't see an actual problem here, other than a false-positive.

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

No, it does not 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++) {

I can't find any definitions of or references to shwqs in the existing
code.

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

The CRQ Valid bit is now cleared by callers of ibmvfc_handle_async() in v7.

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

Not yet. In v7 I added the __maybe_unused attribute for this new
function, and remove it in the the next patch where its actually used.

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

Already fixed in previous patches in v7.

-Dave