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

[sashiko-bot@kernel.org]

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.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc_kunit.c b/drivers/scsi/ibmvscsi/ibmvfc_kunit.c
> new file mode 100644
> index 0000000000000..8a73ea1b1da8c
> --- /dev/null
> +++ b/drivers/scsi/ibmvscsi/ibmvfc_kunit.c
[ ... ]
> @@ -0,0 +1,146 @@
[ ... ]
> +static void ibmvfc_async_fpin_test(struct kunit *test)
> +{
[ ... ]
> +	headp = ibmvfc_get_headp();
> +	if (list_empty(headp))
> +		kunit_skip(test, "No ibmvfc devices available");
> +	queue = headp->next;
> +	vhost = container_of_const(queue, struct ibmvfc_host, queue);

[Severity: Medium]
Could this lockless traversal of the global ibmvfc_head list lead to a
use-after-free regression?

If ibmvfc_remove() executes concurrently (for instance, during a device
hot-unplug), it could remove and free the vhost structure while the KUnit test
is still traversing the list.

> +
> +	spin_lock_irqsave(vhost->host->host_lock, flags);
> +	if (vhost->scsi_scrqs.num_targets < 1) {
> +		spin_unlock_irqrestore(vhost->host->host_lock, flags);
> +		kunit_skip(test, "No targets");
> +	}
> +	tgt = list_first_entry(&vhost->scsi_scrqs.targets, struct ibmvfc_target, queue);
> +	if (!tgt->rport) {
> +		spin_unlock_irqrestore(vhost->host->host_lock, flags);
> +		kunit_skip(test, "No rport");
> +	}
> +	kref_get(&tgt->kref);
> +	spin_unlock_irqrestore(vhost->host->host_lock, flags);
> +
> +	fc_host = shost_to_fc_host(vhost->host);
> +
> +	pre[IBMVFC_AE_FPIN_LINK_CONGESTED] = READ_ONCE(fc_host->fpin_stats.cn_device_specific);
> +	pre[IBMVFC_AE_FPIN_PORT_CONGESTED] = READ_ONCE(tgt->rport->fpin_stats.cn_device_specific);

[Severity: Medium]
Is there a potential use-after-free regression here if tgt->rport is
dereferenced outside of the host_lock?

The test grabs a reference to tgt and drops vhost->host->host_lock, but then
reads tgt->rport->fpin_stats locklessly. If an event like a link down triggers
ibmvfc_del_tgt() concurrently, it can schedule the removal of the remote port.
The worker thread would then call fc_remote_port_delete(rport), freeing the
rport while the test is still dereferencing it.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831-ibmvfc-fpin-support-v7-0-77d0b9809080@linux.ibm.com?part=1