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

[Dave Marquardt <davemarq@linux.ibm.com>]

sashiko-bot@kernel.org writes:

> 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?

Yes, this routine should acquire the host_lock before calling ibmvfc_handle_async.

>> +		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?

Yes.

>> +	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.

Yes, this needs a scsi_host_put() call.

-Dave