Re: [PATCH v11 3/5] s390/pci: Reuse FMB buffer across enable and disable cycles

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] s390/pci: stale FMB enabled state when zpci_fmb_enable() fails
--

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

s390/pci: Reuse FMB buffer across enable and disable cycles

This commit allocates the FMB buffer once on first enable and reuses it across
disable and enable cycles for the lifetime of the zdev, deferring deallocation
until zpci_release_device().

> diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
> index 5889dd227009b..a7e548a83bc15 100644
> --- a/arch/s390/pci/pci.c
> +++ b/arch/s390/pci/pci.c
> @@ -211,13 +211,17 @@ int zpci_fmb_enable(struct zpci_dev *zdev)
>  
>  	lockdep_assert_held(&zdev->fmb_lock);
>  
> -	if (zdev->fmb || sizeof(*zdev->fmb) < zdev->fmb_length)
> +	if (sizeof(*zdev->fmb) < zdev->fmb_length)
>  		return -EINVAL;
>  
> -	zdev->fmb = kmem_cache_zalloc(zdev_fmb_cache, GFP_KERNEL);
> -	if (!zdev->fmb)
> -		return -ENOMEM;
> -	WARN_ON((u64) zdev->fmb & 0xf);
> +	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));
> +	}
>  
>  	/* reset software counters */
>  	spin_lock_irqsave(&zdev->dom_lock, flags);
> @@ -235,10 +239,6 @@ int zpci_fmb_enable(struct zpci_dev *zdev)
>  	fib.fmb_addr = virt_to_phys(zdev->fmb);
>  	fib.gd = zdev->gisa;
>  	cc = zpci_mod_fc(req, &fib, &status);
> -	if (cc) {
> -		kmem_cache_free(zdev_fmb_cache, zdev->fmb);
> -		zdev->fmb = NULL;
> -	}
>  	return cc ? -EIO : 0;
>  }

[Severity: Medium]
Can keeping zdev->fmb non-NULL when zpci_mod_fc() fails in zpci_fmb_enable()
leave the FMB enablement state inconsistent?

zdev->statistics_on is initialized to true in zpci_create_device(). When
pcibios_enable_device() or zpci_reenable_device() calls zpci_fmb_enable(),
the return value is ignored and zdev->statistics_on remains true:

arch/s390/pci/pci.c:pcibios_enable_device() {
	...
	mutex_lock(&zdev->fmb_lock);
	if (zdev->statistics_on)
		zpci_fmb_enable(zdev);
	mutex_unlock(&zdev->fmb_lock);
	...
}

arch/s390/pci/pci.c:zpci_reenable_device() {
	...
	guard(mutex)(&zdev->fmb_lock);
	if (zdev->statistics_on)
		zpci_fmb_enable(zdev);
	...
}

Previously, zpci_fmb_enable() freed zdev->fmb and reset it to NULL on
zpci_mod_fc() failure, and zpci_fmb_disable() reset zdev->fmb to NULL on
disable. With zdev->fmb kept allocated both across disable/re-enable cycles
and when zpci_mod_fc() fails (or if zpci_reenable_device() returns early on an
error after a prior allocation), both zdev->statistics_on and zdev->fmb remain
non-zero even though hardware measurement is not active.

In arch/s390/pci/pci_debug.c, pci_perf_show() checks whether FMB is disabled
using both fields:

arch/s390/pci/pci_debug.c:pci_perf_show() {
	...
	mutex_lock(&zdev->fmb_lock);
	if (!zdev->statistics_on || !zdev->fmb) {
		mutex_unlock(&zdev->fmb_lock);
		seq_puts(m, "FMB statistics disabled\n");
		return 0;
	}
	...
}

Similarly, the later commit "vfio-pci/zdev: Add VFIO FMB device features"
relies on the same (!zdev->statistics_on || !zdev->fmb) check in
vfio_pci_zdev_feature_fmb_read(), and zpci_fmb_request_on() rejects subsequent
enable attempts with -EINVAL when zdev->statistics_on is still true.

Would this cause pci_perf_show() to report zeroed or stale FMB counters instead
of "FMB statistics disabled" when enabling FMB fails?

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