Re: [PATCH v6 3/8] scsi: ibmvfc: define asynchronous sub-queue

[email protected] writes:

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> --- Patch [3]: [PATCH v6 3/8] scsi: ibmvfc: define asynchronous sub-queue ---
> commit 4682ac13b0e9a18a4a0d6bd0390528a5ffb87d23
> Author: Dave Marquardt <[email protected]>
>
> scsi: ibmvfc: define asynchronous sub-queue
>
> Define data structures for asynchronous sub-queue support required for
> full and extended FPIN functionality.
>
>> Add ibmvfc_async_subq structure to represent async events received via
>> the sub-queue, including FPIN status, link state, event type, and WWPN
>> information.
>> 
>> Update ibmvfc_channel_setup structure to include async_subq_handle field
>> and reduce IBMVFC_MAX_CHANNELS from 502 to 501 to accommodate the async
>> sub-queue. Add async_scrq pointer to ibmvfc_channels structure.
>> 
>> Add capability flags IBMVFC_USE_ASYNC_SUBQ and IBMVFC_SUPPORT_ASYNC_SUBQ
>> for negotiating async sub-queue support with VIOS during login.
>
> [Severity: Medium]
> Are these changes missing from the patch? The commit message explicitly claims
> to define the ibmvfc_async_subq structure, update ibmvfc_channel_setup, reduce
> IBMVFC_MAX_CHANNELS, add an async_scrq pointer, and introduce several
> capability flags. However, almost none of these structural changes exist in the
> actual patch.

I missed rewriting this commit message after updates to integrate with
the recent ibmvfc NVMEOF work. Fixed in v7.

>> diff --git a/drivers/scsi/ibmvscsi/ibmvfc.h b/drivers/scsi/ibmvscsi/ibmvfc.h
>> index e8ca0d30f483f..d8880d02685b3 100644
>> --- a/drivers/scsi/ibmvscsi/ibmvfc.h
>> +++ b/drivers/scsi/ibmvscsi/ibmvfc.h
>
> [ ... ]
>
>> @@ -990,6 +990,7 @@ struct ibmvfc_host {
>>  	mempool_t *tgt_pool;
>>  	struct ibmvfc_queue crq;
>>  	struct ibmvfc_queue async_crq;
>> +	struct ibmvfc_queue async_sub_crq;
>
> [Severity: Medium]
> The commit message claims to add an async_scrq pointer to ibmvfc_channels,
> but only async_sub_crq is added to ibmvfc_host here.

Right. Also missed in the commit message. Rewritten in v7.

>
> [Severity: High]
> Does the adapter removal process introduce a NULL pointer dereference
> during teardown?
>
> If an FPIN async event interrupt arrives during the adapter removal process
> after destroy_workqueue() is called but before free_irq(), the interrupt
> handler will schedule the tasklet, which executes ibmvfc_handle_async().
>
> In drivers/scsi/ibmvscsi/ibmvfc-core.c:ibmvfc_remove():
>     destroy_workqueue(vhost->fpin_workq);
>     vhost->fpin_workq = NULL;
>
> Then later in drivers/scsi/ibmvscsi/ibmvfc-core.c:ibmvfc_release_crq_queue():
>     free_irq(vdev->irq, vhost);
>     tasklet_kill(&vhost->tasklet);
>
> And the tasklet dereferences the NULL fpin_workq:
>
> drivers/scsi/ibmvscsi/ibmvfc-core.c:ibmvfc_handle_async():
>     queue_work(vhost->fpin_workq, &aw->async_work_s);
>
> Because vhost->fpin_workq was destroyed and set to NULL early in
> ibmvfc_remove(), calling queue_work(NULL, ...) will immediately panic the
> kernel.

Yes, fixed in patch 1 of v7.

-Dave