Re: [PATCH v7 1/8] scsi: ibmvfc: add basic FPIN support

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

sashiko-bot@kernel.org writes:

> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [High] Teardown ordering race allows FPIN works to execute on an unregistered `fc_host`.
> - [Medium] Lockless traversal of global `ibmvfc_head` list in KUnit test risks use-after-free.
> - [Medium] The KUnit test accesses `tgt->rport` outside of the `host_lock`.
> --
>
> commit 6a259111a28ef932ad14496cabbef71084312dfe
> Author: Dave Marquardt <davemarq@linux.ibm.com>
>
> scsi: ibmvfc: add basic FPIN support
>
> Implement support for a basic level of Fabric Performance Impact
> Notifications (FPIN) in the ibmvfc driver to enable monitoring of
> fabric congestion and link integrity events. Offloads FPIN processing
> to a dedicated workqueue and adds a KUnit test module for validation.
>
>> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
>> index b3bc3ce872d6d..78be00d31a064 100644
>> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
>> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> [ ... ]
>> @@ -6962,6 +7231,8 @@ static void ibmvfc_remove(struct vio_dev *vdev)
>>  	ibmvfc_complete_purge(&purge);
>>  	ibmvfc_release_sub_crqs(vhost);
>>  	ibmvfc_release_crq_queue(vhost);
>> +	destroy_workqueue(vhost->fpin_workq);
>> +	vhost->fpin_workq = NULL;
>>  
>>  	ibmvfc_free_mem(vhost);
>>  	spin_lock(&ibmvfc_driver_lock);
>
> [Severity: High]
> Could this teardown ordering race cause a regression by allowing FPIN works to
> execute on an unregistered fc_host?
>
> Earlier in ibmvfc_remove(), fc_remove_host() is called which destroys
> fc_host->work_q and sets it to NULL. This happens before disabling FPIN
> interrupts via ibmvfc_release_sub_crqs() and ibmvfc_release_crq_queue(), and
> before destroying the new fpin_workq here.
>
> If an FPIN interrupt arrives concurrently before the queues are released,
> it can queue a work item to fpin_workq. When that work executes
> fc_host_fpin_rcv(), it calls fc_queue_event(). Because scsi_remove_host() has
> not yet executed to clear state_in_sysfs, fc_queue_event() may attempt to
> queue work to the already destroyed fc_host->work_q via queue_work(NULL, ...),
> resulting in a kernel crash.

Yes, this is a problem. I plan to solve this by moving the calls to
fc_remove_host() and scsi_remove_host() to just after the call to
ibmvfc_release_crq_queue().

-Dave