Re: [PATCH v11 04/11] scsi: ibmvfc: add basic FPIN support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] scsi: ibmvfc: missing scsi_host_put() in test success path
- [Medium] scsi: ibmvfc: missing host_lock around ibmvfc_handle_async() in KUnit test
--

commit 29ab8a9a78c94679b8187ae357d2978336830599
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. This includes async event
handling and KUnit test infrastructure.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc_kunit.c b/drivers/scsi/ibmvscsi/ibmvfc_kunit.c
> new file mode 100644
> index 0000000000000..ec52285b0aa22
> --- /dev/null
> +++ b/drivers/scsi/ibmvscsi/ibmvfc_kunit.c
> @@ -0,0 +1,149 @@
[ ... ]
> +static void ibmvfc_async_fpin_test(struct kunit *test)
> +{
[ ... ]
> +	vhost = ibmvfc_get_first_vhost();
> +	if (!vhost)
> +		kunit_skip(test, "No ibmvfc devices available");
[ ... ]
> +	for (fs = IBMVFC_AE_FPIN_LINK_CONGESTED; fs <= IBMVFC_AE_FPIN_CONGESTION_CLEARED; fs++) {
> +		ae[fs].valid = 0x80;
> +		ae[fs].link_state = IBMVFC_AE_LS_LINK_UP;
> +		ae[fs].fpin_status = fs;
> +		ae[fs].event = cpu_to_be64(IBMVFC_AE_FPIN);
> +		ae[fs].scsi_id = cpu_to_be64(tgt->scsi_id);
> +		ae[fs].wwpn = cpu_to_be64(tgt->wwpn);
> +		ae[fs].node_name = cpu_to_be64(tgt->ids.node_name);
> +		ibmvfc_handle_async(&ae[fs], vhost);

[Severity: Medium]
Does this invocation of ibmvfc_handle_async() need to be protected by
the host_lock?

If a concurrent device removal via ibmvfc_remove() sets vhost->state to
IBMVFC_HOST_OFFLINE and destroys fpin_workq just before this handles the
async event, could queue_work() be called on a freed workqueue?

> +		ae[fs].valid = 0;
> +		wmb(); /* ensure valid bit clear is visible before checking stats */
> +	}
> +	flush_workqueue(vhost->fpin_workq);
[ ... ]
> +	ae[0].valid = 0x80;
> +	ae[0].link_state = IBMVFC_AE_LS_LINK_UP;
> +	ae[0].fpin_status = 0; /* bad value */
> +	ae[0].event = cpu_to_be64(IBMVFC_AE_FPIN);
> +	ae[0].scsi_id = cpu_to_be64(tgt->scsi_id);
> +	ae[0].wwpn = cpu_to_be64(tgt->wwpn);
> +	ae[0].node_name = cpu_to_be64(tgt->ids.node_name);
> +	ibmvfc_handle_async(&ae[0], vhost);

[Severity: Medium]
Is the host_lock similarly required around this call to
ibmvfc_handle_async() to prevent a use-after-free if the device is
concurrently unbound?

> +	ae[0].valid = 0;
> +	wmb(); /* ensure valid bit clear is visible before checking stats */
> +	flush_workqueue(vhost->fpin_workq);
[ ... ]
> +	kref_put(&tgt->kref, ibmvfc_release_tgt);
> +	put_device(&rport->dev);
> +}

[Severity: Medium]
Does this successful exit path leak the Scsi_Host reference?

The call to ibmvfc_get_first_vhost() at the beginning of the test acquires
a reference via scsi_host_get(), but unlike the error return paths, there
doesn't appear to be a corresponding scsi_host_put() call here.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260925044900.414340-1-tyreld@linux.ibm.com?part=4