Re: [PATCH v10 5/5] vfio-pci/zdev: Add VFIO FMB device features

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] vfio-pci/zdev: race with firmware FMB update on nested enable in zpci_fmb_enable()
--

commit 9b8c0a84d66e0a84f5421ca2e3d89abbb0e740d4
Author: Omar Elghoul <oelghoul@linux.ibm.com>

vfio-pci/zdev: Add VFIO FMB device features

This commit introduces the VFIO_DEVICE_FEATURE_ZPCI_FMB_ENABLE and
VFIO_DEVICE_FEATURE_ZPCI_FMB_READ device features to allow userspace to
enable, disable, and read the Function Measurement Block (FMB) on s390 zPCI
devices.

> diff --git a/drivers/vfio/pci/vfio_pci_zdev.c b/drivers/vfio/pci/vfio_pci_zdev.c
> index f47f36314a1cc..c2afe60111d73 100644
> --- a/drivers/vfio/pci/vfio_pci_zdev.c
> +++ b/drivers/vfio/pci/vfio_pci_zdev.c
> @@ -219,3 +219,63 @@ void vfio_pci_zdev_close_device(struct vfio_pci_core_device *vdev)
>  	if (zpci_kvm_hook.kvm_unregister)
>  		zpci_kvm_hook.kvm_unregister(zdev);
>  }
> +
> +int vfio_pci_zdev_feature_fmb_enable(struct vfio_pci_core_device *vdev, u32 flags,
> +				     void __user *arg, size_t argsz)
> +{
> +	struct zpci_dev *zdev;
> +	struct vfio_device_feature_zpci_fmb_enable fmb_enable;
> +	int ret;
> +
> +	ret = vfio_check_feature(flags, argsz, VFIO_DEVICE_FEATURE_SET, sizeof(fmb_enable));
> +	if (ret != 1)
> +		return ret;
> +
> +	zdev = to_zpci(vdev->pdev);
> +	if (!zdev)
> +		return -ENODEV;
> +
> +	if (copy_from_user(&fmb_enable, arg, sizeof(fmb_enable)))
> +		return -EFAULT;
> +
> +	guard(mutex)(&zdev->fmb_lock);
> +
> +	if (fmb_enable.enabled) {
> +		zdev->statistics_on = true;
> +		return zpci_fmb_enable(zdev);
> +	}

[Severity: Medium]
When VFIO_DEVICE_FEATURE_ZPCI_FMB_ENABLE is invoked with enabled != 0 while
FMB measurement is already active in firmware, can
vfio_pci_zdev_feature_fmb_enable() -> zpci_fmb_enable() race with a periodic
firmware FMB DMA update and leave stale pre-reset or torn counters in
zdev->fmb?

In arch/s390/pci/pci.c, existing callers such as zpci_fmb_request_on() only
call zpci_fmb_enable() when FMB measurement is not already active:

arch/s390/pci/pci.c:zpci_fmb_request_on() {
	if (zdev->statistics_on && zdev->fmb)
		return -EINVAL;
	...
	zdev->statistics_on = true;
	return zpci_fmb_enable(zdev);
}

Here, vfio_pci_zdev_feature_fmb_enable() in drivers/vfio/pci/vfio_pci_zdev.c
calls zpci_fmb_enable() directly on a nested enable so that FMB counters are
reset to zero as documented for VFIO_DEVICE_FEATURE_ZPCI_FMB_ENABLE in
include/uapi/linux/vfio.h. However, zpci_fmb_enable() zeroes the live DMA
buffer before issuing the ZPCI_MOD_FC_SET_MEASURE command via zpci_mod_fc():

arch/s390/pci/pci.c:zpci_fmb_enable() {
	...
	if (!zdev->fmb) {
		zdev->fmb = kmem_cache_zalloc(zdev_fmb_cache, GFP_KERNEL);
		if (!zdev->fmb)
			return -ENOMEM;
	} else {
		/* reuse the same FMB buffer for as long as the zdev lives */
		memset(zdev->fmb, 0, sizeof(*zdev->fmb));
	}
	...
	fib.fmb_addr = virt_to_phys(zdev->fmb);
	fib.gd = zdev->gisa;
	cc = zpci_mod_fc(req, &fib, &status);
}

Because virt_to_phys(zdev->fmb) is still registered with firmware during the
memset() and up until zpci_mod_fc() completes, a periodic firmware FMB update
firing in that window can overwrite zdev->fmb with pre-reset counters and a
non-zero timestamp. Since firmware only writes to zdev->fmb at fmb_update
intervals, subsequent VFIO_DEVICE_FEATURE_ZPCI_FMB_READ calls would then
return those stale values instead of zeroed counters.

Should FMB measurement be disabled in firmware before zeroing zdev->fmb, or
zdev->fmb cleared again after stopping measurement?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020212427.2561-1-oelghoul@linux.ibm.com?part=5