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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Use-After-Free / NULL Pointer Dereference (Lifecycle Mismatch)
- [Medium] Incomplete Implementation / Commit Message Mismatch
--

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

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


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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3