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

Yes, this is a potential problem. I'll be using a new routine to get the
first vhost that's referenced with scsi_host_get(), or NULL. The callers
(tests) then must use scsi_host_put() before returning.

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

Yes, this is another problem. I'll be adding device references to the
rport in the tests.

-Dave