Re: [PATCH v4 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] Removal of `async->valid = 0;` leads to an infinite loop and stale message processing in the async CRQ tasklet.
- [High] Missing reference count acquisition on `tgt` in `ibmvfc_process_async_work` leads to Use-After-Free.
- [High] Improper devres lifecycle management of `fpin_workq` causes a double-free on probe failure and a Use-After-Free during device removal.
- [Medium] Data races and potential use-after-free in the new KUnit test module due to lockless access of global and host device lists.
--

--- Patch [1]: [PATCH v4 1/8] ibmvfc: add basic FPIN support ---
commit d8410cbccc8b5f74d804dc594b4dd758bd093c6e
Author: Dave Marquardt <davemarq@linux.ibm.com>

ibmvfc: add basic FPIN support

This patch implements basic Fabric Performance Impact Notifications (FPIN) 
support to enable monitoring of fabric congestion and link integrity events.
It adds async event handling that offloads FPIN processing to a workqueue
and introduces a KUnit test module for validation.

> 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 (list_entry_is_head(tgt, &vhost->targets, queue) || !tgt->rport) {
> +		dev_err_ratelimited(vhost->dev, "Invalid target for FPIN\n");
> +		goto end;
> +	}
> +
> +	fpin = ibmvfc_basic_fpin_to_desc(crq, tgt->wwpn);

[Severity: High]
Does this leave the target pointer vulnerable to a use-after-free?

The lock protecting the vhost->targets list is dropped before we access 
tgt->wwpn and tgt->rport, without acquiring a reference via kref_get().

If another thread handles a target logout or hotplug concurrently, it could 
remove and free the target structure, causing memory corruption when we 
attempt to access it here.

[ ... ]

> @@ -3803,8 +4031,6 @@ static void ibmvfc_tasklet(void *data)
>  		/* Pull all the valid messages off the async CRQ */
>  		while ((async = ibmvfc_next_async_crq(vhost)) != NULL) {
>  			ibmvfc_handle_async(async, vhost);
> -			async->valid = 0;
> -			wmb();
>  		}

[Severity: Critical]
Could removing this clearing operation lead to an infinite loop?

The hardware signals a valid entry by setting the valid field to 0x80. By 
removing the clearing of async->valid, the driver's ring-buffer polling logic 
will mistakenly see the old, uncleared entries as new valid entries once the 
circular queue index wraps around.

This would trap the system in an infinite loop within softirq context when 
processing any valid async event.

[ ... ]

> @@ -6364,9 +6588,15 @@ static int ibmvfc_probe(struct vio_dev *vdev, const struct vio_device_id *id)

[ ... ]

> -	if ((rc = ibmvfc_alloc_mem(vhost)))
> +	vhost->fpin_workq = devm_alloc_workqueue(vhost->dev, "%s-fpin-workq-%u", 0, 0,
> +						 IBMVFC_NAME, shost->host_no);
> +	if (vhost->fpin_workq == NULL)
>  		goto free_scsi_host;

[ ... ]

> @@ -6412,6 +6642,9 @@ static int ibmvfc_probe(struct vio_dev *vdev, const struct vio_device_id *id)
>  	kthread_stop(vhost->work_thread);
>  free_host_mem:
>  	ibmvfc_free_mem(vhost);
> +free_workq:
> +	destroy_workqueue(vhost->fpin_workq);
> +	vhost->fpin_workq = NULL;

[Severity: High]
Can this result in a double-free on probe failure, and a use-after-free 
during device removal?

The workqueue is allocated using devm_alloc_workqueue(). On probe failure, 
destroy_workqueue() is called explicitly, and then devres will automatically 
call it again during cleanup, causing a double-free.

Additionally, during device removal in ibmvfc_remove(), the host is manually 
freed before devres automatically cleans up the workqueue. If there are 
pending work items like ibmvfc_process_async_work(), they will execute and 
attempt to dereference host pointers that have already been freed.

[ ... ]

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc_kunit.c b/drivers/scsi/ibmvscsi/ibmvfc_kunit.c
> --- /dev/null
> +++ b/drivers/scsi/ibmvscsi/ibmvfc_kunit.c

[ ... ]

> +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(queue, struct ibmvfc_host, queue);
> +
> +	KUNIT_ASSERT_GE_MSG(test, vhost->num_targets, 1, "No targets");
> +	tgt = list_first_entry(&vhost->targets, struct ibmvfc_target, queue);

[Severity: Medium]
Is it safe to access the global ibmvfc_head list and vhost->targets list 
without holding the appropriate locks?

The test traverses the global ibmvfc_head without acquiring the 
ibmvfc_driver_lock, and then iterates over vhost->targets without acquiring 
the vhost->host->host_lock.

Since KUnit tests can run dynamically while other processes or the hypervisor 
interact with the driver, concurrently adding or removing devices could modify 
these lists and free structures, resulting in a use-after-free.

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