Re: [PATCH v10 3/9] scsi: ibmvfc: add FPIN extended flag and async sub-CRQ queue handle

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] Reference count leaks for `Scsi_Host` and `fc_rport` device on the KUnit test success path.

Pre-existing issues:
- [High] Missing synchronous cancellation of event timers (`evt->timer`) during adapter teardown.
--

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

scsi: ibmvfc: add FPIN extended flag and async sub-CRQ queue handle

This patch adds the IBMVFC_ASYNC_IS_FPIN_EXT flag bit to distinguish
extended FPIN events, and introduces a dedicated queue handle for
asynchronous sub-CRQs. The associated series also implements basic Fabric
Performance Impact Notifications (FPIN) support, offloads event handling
to a new workqueue, and provides 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..67bd3922a49f9
> --- /dev/null
> +++ b/drivers/scsi/ibmvscsi/ibmvfc_kunit.c
> @@ -0,0 +1,148 @@
[ ... ]
> +static void ibmvfc_async_fpin_test(struct kunit *test)
> +{
[ ... ]
> +	vhost = ibmvfc_get_first_vhost();
> +	if (!vhost)
> +		kunit_skip(test, "No ibmvfc devices available");
> +
> +	spin_lock_irqsave(&vhost->host->host_lock, flags);
> +	if (vhost->scsi_scrqs.num_targets < 1) {
> +		spin_unlock_irqrestore(&vhost->host->host_lock, flags);
> +		scsi_host_put(vhost->host);
> +		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);
> +		scsi_host_put(vhost->host);
> +		kunit_skip(test, "No rport");
> +	}
> +	rport = tgt->rport;
> +	get_device(&rport->dev);
> +	kref_get(&tgt->kref);
> +	spin_unlock_irqrestore(&vhost->host->host_lock, flags);
[ ... ]
> +	KUNIT_EXPECT_EQ(test, pre[IBMVFC_AE_FPIN_CONGESTION_CLEARED],
> +			post[IBMVFC_AE_FPIN_CONGESTION_CLEARED]);
> +
> +	kref_put(&tgt->kref, ibmvfc_release_tgt);
> +}

[Severity: Medium]
Does this success path leak the Scsi_Host and fc_rport device reference
counts?

The underlying devices vhost->host and rport->dev were acquired earlier in this
function with ibmvfc_get_first_vhost() and get_device().

However, the required cleanup calls scsi_host_put() and put_device() appear
to be entirely omitted before returning.


While reviewing the adapter teardown sequence, I noticed a pre-existing issue
in the event timer cleanup:

drivers/scsi/ibmvscsi/ibmvfc-core.c:ibmvfc_fail_request() {
    ...
    timer_delete(&evt->timer);
}

[Severity: High]
This isn't a bug introduced by this patch, but does this asynchronous timer
deletion leave a race window where the timer callback could still run and
cause a use-after-free?

During the adapter teardown sequence:

ibmvfc_remove()
  -> ibmvfc_purge_requests()
    -> ibmvfc_fail_request()

The event timer is deleted asynchronously without waiting for any concurrently
executing callbacks.

The driver then proceeds to free the event pool and release the host structure.

If the timer callback executes concurrently:

drivers/scsi/ibmvscsi/ibmvfc-core.c:ibmvfc_timeout() {
    struct ibmvfc_host *vhost = evt->vhost;
    dev_err(vhost->dev, "Command timed out (%p). Resetting connection\n", evt);
    ...
}

It accesses the event and host structures which may have already been freed by
the teardown thread.

Could this lead to memory corruption or a kernel crash upon adapter removal,
hotplug, or connection reset?

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