Re: [PATCH v10 3/5] s390/pci: Improve function measurement state management

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] s390/pci: broken FMB retry and status check when zpci_fmb_enable() fails
- [Medium] s390/pci: zpci_reenable_device() enables FMB on disabled PCI devices
- [Medium] s390/pci: zpci_fmb_request_off() clears statistics_on before disable succeeds
--

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

s390/pci: Improve function measurement state management

This commit decouples the user's desired function measurement state in
zdev->statistics_on from the firmware enablement state via
zpci_fmb_request_on(), zpci_fmb_request_off(), zpci_fmb_enable(), and
zpci_fmb_disable(). It also preserves and restores function measurement
across device enable, disable, and re-enable paths.

[ ... ]

> diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
> index 2a7e1ca0fa12f..98a4e938865ae 100644
> --- a/arch/s390/pci/pci.c
> +++ b/arch/s390/pci/pci.c
> @@ -165,7 +165,37 @@ int zpci_unregister_ioat(struct zpci_dev *zdev, u8 dmaas)
>  }
>  
>  /* Modify PCI: Set PCI function measurement parameters */
> -int zpci_fmb_enable_device(struct zpci_dev *zdev)
> +int zpci_fmb_request_on(struct zpci_dev *zdev)
> +{
> +	lockdep_assert_held(&zdev->fmb_lock);
> +
> +	if (zdev->statistics_on && zdev->fmb)
> +		return -EINVAL;
> +
> +	/*
> +	 * Record the user's intent first. If the enable fails, the flag stays set
> +	 * so we can retry enabling the FMB upon the next device (re-)enable. The
> +	 * consumer can detect that a measurement sample was never written if the
> +	 * timestamp is still zero, or if the FMB pointer is null.
> +	 */
> +	zdev->statistics_on = true;
> +	return zpci_fmb_enable(zdev);
> +}

[Severity: Medium]
In arch/s390/pci/pci.c, zpci_fmb_enable() allocates (or reuses and zeroes)
zdev->fmb via kmem_cache_zalloc() before calling zpci_mod_fc(), and leaves
zdev->fmb non-NULL if zpci_mod_fc() fails and returns -EIO:

	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));
	}
	...
	cc = zpci_mod_fc(req, &fib, &status);
	if (cc)
		return -EIO;

If zpci_fmb_enable() fails in zpci_mod_fc() during pcibios_enable_device(),
zpci_reenable_device(), or pci_perf_seq_write() -> zpci_fmb_request_on(),
both zdev->statistics_on and zdev->fmb remain set while function
measurement is disabled in firmware.

When that happens, won't the (zdev->statistics_on && zdev->fmb) check in
zpci_fmb_request_on() reject any subsequent retry from pci_perf_seq_write()
with -EINVAL instead of calling zpci_fmb_enable()?

Also, in arch/s390/pci/pci_debug.c, pci_perf_show() only checks
zdev->statistics_on and zdev->fmb:

	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;
	}

Wouldn't pci_perf_show() report zeroed or stale counters instead of
"FMB statistics disabled" when zpci_mod_fc() fails after zdev->fmb has been
allocated?

> +
> +/* Modify PCI: Disable PCI function measurement */
> +int zpci_fmb_request_off(struct zpci_dev *zdev)
> +{
> +	lockdep_assert_held(&zdev->fmb_lock);
> +
> +	if (!zdev->statistics_on)
> +		return -EINVAL;
> +
> +	zdev->statistics_on = false;
> +	return zpci_fmb_disable(zdev);
> +}

[Severity: Medium]
What happens in zpci_fmb_request_off() in arch/s390/pci/pci.c if
zpci_fmb_disable(zdev) fails with -EIO when zpci_mod_fc() returns cc == 1?

Because zdev->statistics_on is set to false before zpci_fmb_disable(zdev)
succeeds, a failed disable leaves function measurement active in firmware
while zdev->statistics_on is false. A subsequent retry via
pci_perf_seq_write() -> zpci_fmb_request_off() will then fail with -EINVAL
at the (!zdev->statistics_on) check, and pcibios_disable_device() will also
skip calling zpci_fmb_disable(zdev).

[ ... ]

> @@ -647,7 +672,8 @@ int pcibios_enable_device(struct pci_dev *pdev, int mask)
>  
>  	zpci_debug_init_device(zdev, dev_name(&pdev->dev));
>  	mutex_lock(&zdev->fmb_lock);
> -	zpci_fmb_enable_device(zdev);
> +	if (zdev->statistics_on)
> +		zpci_fmb_enable(zdev);
>  	mutex_unlock(&zdev->fmb_lock);
>  
>  	return pci_enable_resources(pdev, mask);
> @@ -658,7 +684,8 @@ void pcibios_disable_device(struct pci_dev *pdev)
>  	struct zpci_dev *zdev = to_zpci(pdev);
>  
>  	mutex_lock(&zdev->fmb_lock);
> -	zpci_fmb_disable_device(zdev);
> +	if (zdev->statistics_on)
> +		zpci_fmb_disable(zdev);
>  	mutex_unlock(&zdev->fmb_lock);
>  	zpci_debug_exit_device(zdev);
>  }
> @@ -740,8 +767,14 @@ int zpci_reenable_device(struct zpci_dev *zdev)
>  	}
>  
>  	rc = zpci_iommu_register_ioat(zdev, &status);
> -	if (rc)
> +	if (rc) {
>  		zpci_disable_device(zdev);
> +		return rc;
> +	}
> +
> +	guard(mutex)(&zdev->fmb_lock);
> +	if (zdev->statistics_on)
> +		zpci_fmb_enable(zdev);
>  
>  	return rc;
>  }

[Severity: Medium]
Could zpci_reenable_device() in arch/s390/pci/pci.c enable function
measurement while the upper-layer pci_dev is disabled or removed?

In zpci_create_device(), zdev->statistics_on is initialized to true, and
pcibios_disable_device() preserves zdev->statistics_on = true when disabling
measurement.

In arch/s390/pci/pci_sysfs.c, recover_store() -> _do_recover() removes the
pci_dev before calling zpci_reenable_device():

	pci_stop_and_remove_bus_device(pdev);
	if (zdev_enabled(zdev)) {
		ret = zpci_disable_device(zdev);
		if (ret == -EINVAL)
			ret = 0;
		if (ret)
			return ret;
	}

	ret = zpci_reenable_device(zdev);

Here pci_stop_and_remove_bus_device(pdev) unbinds the driver and calls
pcibios_disable_device(pdev), leaving zdev->statistics_on set to true.
_do_recover() then calls zpci_reenable_device(zdev) (and similarly
reset_slot() -> zpci_hot_reset_device() -> zpci_reenable_device() can run
when no driver has enabled the pci_dev), which immediately starts firmware
measurement updates to zdev->fmb while the pci_dev is not enabled.

When recover_store() subsequently calls pci_rescan_bus() and a driver probes
and enables the device via pci_enable_device() -> pcibios_enable_device(),
pcibios_enable_device() sees zdev->statistics_on is true and calls
zpci_fmb_enable(zdev) again, executing memset() on zdev->fmb while firmware
measurement updates to zdev->fmb are already active.

[ ... ]

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