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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] Use-After-Free (UAF) on target structure during asynchronous FPIN processing.
- [High] Out-of-bounds Memory Access / Type Confusion due to misused list iterator.
- [Medium] Incorrect lifecycle cleanup ordering allows FPIN work item to run on a removed transport device.
- [Medium] The KUnit test unconditionally fails on systems without the IBM Virtual FC hardware instead of gracefully skipping.
--

commit ef1762da1f48d8a096031c317c4a9bf573f1b39e
Author: Dave Marquardt <davemarq@linux.ibm.com>

ibmvfc: add basic FPIN support

This commit adds basic FPIN message support to the ibmvfc driver. It adds
FPIN handling to the async event handler, offloads processing to a work
queue, and converts the VIOS FPIN messages to struct fc_els_fpin.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc.c b/drivers/scsi/ibmvscsi/ibmvfc.c
> --- a/drivers/scsi/ibmvscsi/ibmvfc.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc.c
[ ... ]
> @@ -3185,16 +3189,228 @@ static const char *ibmvfc_get_link_state(enum ibmvfc_ae_link_state state)
[ ... ]
> +static void ibmvfc_process_async_work(struct work_struct *work)
> +{
[ ... ]
> +	spin_lock_irqsave(vhost->host->host_lock, flags);
> +	list_for_each_entry_safe(tgt, next, &vhost->targets, queue) {
> +		if (crq->scsi_id && cpu_to_be64(tgt->scsi_id) != crq->scsi_id)
> +			continue;
> +		if (crq->wwpn && cpu_to_be64(tgt->ids.port_name) != crq->wwpn)
> +			continue;
> +		if (crq->node_name && cpu_to_be64(tgt->ids.node_name) != crq->node_name)
> +			continue;
> +		if (!tgt->rport)
> +			continue;
> +		break;
> +	}
> +	spin_unlock_irqrestore(vhost->host->host_lock, flags);
> +
> +	if (!tgt || !tgt->rport) {

[Severity: High]
Does this list iterator evaluation correctly determine if the target was found?

If an asynchronous FPIN event is received with a wwpn or scsi_id that does not
match any active target (for example, immediately after a target disconnects),
the list_for_each_entry_safe() loop finishes without executing a break.
At this point, the list iterator tgt points to the list head, not NULL.

The validation check if (!tgt || !tgt->rport) is bypassed because tgt is a
valid pointer, and its rport member maps to vhost->crq.msgs which contains a
valid initialized pointer. This causes the function to mistakenly treat the
vhost struct fields as a target struct, which can result in an out-of-bounds
read and a crash.

[Severity: Critical]
Is it possible for a concurrent system event to free the target structure here?

When the target is successfully matched in the loop, the spinlock
vhost->host->host_lock is immediately dropped. The function proceeds to access
tgt->wwpn and tgt->vhost->host to populate the FPIN descriptor without acquiring
a reference count via kref_get(&tgt->kref).

Because ibmvfc_process_async_work() runs asynchronously in fpin_workq, a
concurrent event (such as a link down or fabric logout) could acquire the
host lock, delete the target, and invoke kref_put(), freeing the memory for
tgt. The subsequent reads by this worker thread would then result in a
use-after-free.

> +		dev_err_ratelimited(vhost->dev, "Invalid target for FPIN\n");
> +		goto end;
> +	}
[ ... ]
> @@ -6453,6 +6686,8 @@ static void ibmvfc_remove(struct vio_dev *vdev)
>  	ibmvfc_release_crq_queue(vhost);
>  
>  	ibmvfc_free_mem(vhost);
> +	destroy_workqueue(vhost->fpin_workq);
> +	vhost->fpin_workq = NULL;
>  	spin_lock(&ibmvfc_driver_lock);

[Severity: Medium]
What prevents pending FPIN tasks from executing on removed infrastructure?

The destroy_workqueue(vhost->fpin_workq) call occurs at the very end of
ibmvfc_remove(), long after fc_remove_host(vhost->host) and
scsi_remove_host(vhost->host) have been called earlier in the lifecycle.

Because destroy_workqueue() flushes and executes all pending work items, any
queued asynchronous FPIN messages will be processed against a host whose rports
are deleted and whose sysfs transport representation has been torn down.

[ ... ]
> diff --git a/drivers/scsi/ibmvscsi/ibmvfc_kunit.c b/drivers/scsi/ibmvscsi/ibmvfc_kunit.c
> new file mode 100644
> index 0000000000000..e41e2a49e5497
> --- /dev/null
> +++ b/drivers/scsi/ibmvscsi/ibmvfc_kunit.c
[ ... ]
> @@ -0,0 +1,131 @@
[ ... ]
> +static void ibmvfc_async_fpin_test(struct kunit *test)
> +{
[ ... ]
> +	headp = ibmvfc_get_headp();
> +	KUNIT_ASSERT_FALSE_MSG(test, list_empty(headp), "No ibmvfc devices available\n");

[Severity: Medium]
Could this KUnit test gracefully skip instead of forcing a hard failure on
systems without the IBM Virtual FC hardware?

The test unconditionally asserts that ibmvfc_head is not empty. Executing this
module on architectures or systems lacking the specific IBM POWER Virtual FC
hardware directly invokes a failure instead of yielding via kunit_skip().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702-ibmvfc-fpin-support-v3-0-d95b9547cf88@linux.ibm.com?part=1