Re: [PATCH v5 1/7] scsi: ibmvfc: add basic FPIN support

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] Out-of-bounds read and `kref` corruption when evaluating `!tgt->rport` on the list head.
- [High] Double-free on devm-managed workqueue during probe error path.
- [High] Use-after-free of `vhost` on device removal due to delayed devres workqueue flush.
- [Medium] KUnit test relies on `msleep(1U)` for workqueue synchronization, leading to flaky test failures.
--

--- Patch [1]: [PATCH v5 1/7] scsi: ibmvfc: add basic FPIN support ---
commit 4d09e7f6a4d0d71dfbc3b5f16409ed908239bf5d
Author: Dave Marquardt <[email protected]>

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.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> index dfa231b6cc478..2829f4e4b5636 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
[ ... ]
> @@ -3261,16 +3265,249 @@ static const char *ibmvfc_get_link_state(enum ibmvfc_ae_link_state state)
> +static void ibmvfc_process_async_work(struct work_struct *work)
> +{
> +	struct ibmvfc_async_work *aw;
> +	struct ibmvfc_async_crq *crq;
> +	struct ibmvfc_target *tgt;
> +	struct ibmvfc_host *vhost;
> +	struct fc_els_fpin *fpin;
> +	unsigned long flags;
> +
> +	aw = container_of_const(work, struct ibmvfc_async_work, async_work_s);
> +	vhost = aw->vhost;
> +	crq = &aw->crq;
> +
> +	if (!crq->scsi_id && !crq->wwpn && !crq->node_name)
> +		goto free;
> +
> +	spin_lock_irqsave(vhost->host->host_lock, flags);
> +	list_for_each_entry(tgt, &vhost->scsi_scrqs.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;
> +	}
> +
> +	if (!list_entry_is_head(tgt, &vhost->scsi_scrqs.targets, queue) ||
> +	    !tgt->rport) {
> +		kref_get(&tgt->kref);

[Severity: Critical]
Does this logic perform an out-of-bounds read if the target isn't found?

If the loop finishes without finding a match, tgt will point to the list
head. The check evaluates !list_entry_is_head(tgt, ...), which is false,
so it falls through to the OR condition and evaluates !tgt->rport.

Since tgt is the list head, this reads past the bounds of the list_head
structure. If that garbage memory evaluates to NULL, it could proceed to
corrupt memory via kref_get(&tgt->kref). Should this use && instead of ||
to ensure it only checks tgt->rport on valid entries?

[ ... ]
> @@ -6874,9 +7123,15 @@ static int ibmvfc_probe(struct vio_dev *vdev, const struct vio_device_id *id)
>  	INIT_WORK(&vhost->rport_add_work_q, ibmvfc_rport_add_thread);
>  	mutex_init(&vhost->passthru_mutex);
>  
> -	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);

[Severity: High]
Can this devm-managed workqueue cause a use-after-free on module removal?

During device removal, if the driver tears down and calls
scsi_host_put(vhost->host), the vhost structure is freed. Since the devres
cleanup runs after the device remove function returns, the workqueue is
flushed and destroyed after vhost is already gone.

If there is pending work in the queue, ibmvfc_process_async_work() might
execute and dereference the freed vhost.

[ ... ]
>  	vhost->work_thread = kthread_run(ibmvfc_work, vhost, "%s_%d", IBMVFC_NAME,
>  					 shost->host_no);
>  
> @@ -6922,6 +7177,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;
>  free_scsi_host:

[Severity: High]
Does this cause a double-free of the workqueue on the error path?

Since fpin_workq was allocated with devm_alloc_workqueue(), the driver
core's devres cleanup will automatically call destroy_workqueue() when
unwinding resources. Manually calling it here in the error path could lead
to a double free.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc_kunit.c b/drivers/scsi/ibmvscsi/ibmvfc_kunit.c
> new file mode 100644
> index 0000000000000..64a517a74af8e
> --- /dev/null
> +++ b/drivers/scsi/ibmvscsi/ibmvfc_kunit.c
[ ... ]
> +	for (fs = IBMVFC_AE_FPIN_LINK_CONGESTED; fs <= IBMVFC_AE_FPIN_CONGESTION_CLEARED; fs++) {
> +		crq[fs].valid = 0x80;
> +		crq[fs].link_state = IBMVFC_AE_LS_LINK_UP;
> +		crq[fs].fpin_status = fs;
> +		crq[fs].event = cpu_to_be64(IBMVFC_AE_FPIN);
> +		crq[fs].scsi_id = cpu_to_be64(tgt->scsi_id);
> +		crq[fs].wwpn = cpu_to_be64(tgt->wwpn);
> +		crq[fs].node_name = cpu_to_be64(tgt->ids.node_name);
> +		ibmvfc_handle_async(&crq[fs], vhost);
> +		msleep(1U);
> +	}

[Severity: Medium]
Is a 1 millisecond sleep sufficient to guarantee the background work has
completed?

Workqueue execution latency depends on system load and scheduler behavior.
Using msleep(1U) as a synchronization primitive could lead to intermittent
KUnit test failures if the assertions check statistics before the work
has run. Would flush_workqueue() be a safer way to wait?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1