Re: [PATCH v4 1/4] s390/pci: Hold fmb_lock when enabling or disabling PCI devices

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Missing cleanup in pcibios_enable_device() error path causes memory leak and a critical Use-After-Free via leaked debugfs files.
- [Critical] Dangling debugfs pointers and FMB leak if device is removed while enabled.
- [High] Race condition between pcibios_disable_device() and debugfs write causes a permanent FMB memory leak.
--

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

s390/pci: Hold fmb_lock when enabling or disabling PCI devices

Ensure that fmb_lock is held by pcibios_enable_device() and
pcibios_disable_device() when calling zpci_fmb_enable_device() or
zpci_fmb_disable_device(), respectively. Additionally, assert that the
fmb_lock is held within the latter two functions to prevent future race
conditions regarding new callers.

> diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
> index 39bd2adfc240f..2910d4038d39c 100644
> --- a/arch/s390/pci/pci.c
> +++ b/arch/s390/pci/pci.c
[ ... ]
> @@ -639,7 +643,9 @@ int pcibios_enable_device(struct pci_dev *pdev, int mask)
>  	struct zpci_dev *zdev = to_zpci(pdev);
>  
>  	zpci_debug_init_device(zdev, dev_name(&pdev->dev));
> +	mutex_lock(&zdev->fmb_lock);
>  	zpci_fmb_enable_device(zdev);
> +	mutex_unlock(&zdev->fmb_lock);
>  
>  	return pci_enable_resources(pdev, mask);

[Severity: Critical]
This is a pre-existing issue, but does this leak resources and potentially
cause a use-after-free if pci_enable_resources() fails?

If pci_enable_resources() returns an error, pcibios_enable_device() returns
the error directly without rolling back the debugfs creation or freeing the
FMB. Since pcibios_disable_device() is only invoked by the PCI core if
enablement succeeds, these resources appear to be permanently leaked.

Later, when the device is unregistered, zpci_release_device() frees the zdev
structure via kfree_rcu(). Because the error path bypassed cleanup, the
debugfs files remain active. If userspace then reads or writes to these
orphaned debugfs files, such as in arch/s390/pci/pci_debug.c:pci_perf_show():

	struct zpci_dev *zdev = m->private;

	if (!zdev)
		return 0;

	mutex_lock(&zdev->fmb_lock);

won't it dereference the freed zdev pointer?

>  }
> @@ -648,7 +654,9 @@ 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);
> +	mutex_unlock(&zdev->fmb_lock);
>  	zpci_debug_exit_device(zdev);

[Severity: High]
This isn't a bug introduced by this patch, but does releasing the lock
before destroying the debugfs endpoints create a race window?

If a concurrent userspace write to the debugfs statistics file in
arch/s390/pci/pci_debug.c:pci_perf_seq_write() is blocked on fmb_lock:

	mutex_lock(&zdev->fmb_lock);
	switch (val) {
	case 0:
		rc = zpci_fmb_disable_device(zdev);
		break;
	case 1:
		rc = zpci_fmb_enable_device(zdev);
		break;
	}
	mutex_unlock(&zdev->fmb_lock);

it will wake up immediately after pcibios_disable_device() drops the lock.
It will then execute zpci_fmb_enable_device(), which allocates a new
zdev->fmb buffer and re-enables hardware statistics.

After pci_perf_seq_write() releases the lock, pcibios_disable_device()
proceeds to call zpci_debug_exit_device() and permanently deletes the
debugfs files.

Could this race leave the hardware FMB silently active and the zdev->fmb
memory permanently leaked because userspace can no longer disable it?

[Severity: Critical]
This is also a pre-existing issue, but what happens if a zPCI device is
hot-removed while its enable_cnt is greater than 0?

If the device is manually enabled via the PCI sysfs enable attribute while
unbound, or if a bound driver fails to call pci_disable_device(), the PCI
subsystem's teardown path (pci_stop_and_remove_bus_device) does not
automatically invoke pci_disable_device().

As a result, pcibios_disable_device() is never executed. When the device is
subsequently unregistered, zpci_release_device() invokes kfree_rcu() to
free the zdev structure:

arch/s390/pci/pci.c:zpci_release_device() {
    ...
	zpci_bus_device_unregister(zdev);
	zpci_destroy_iommu(zdev);
	zpci_dbg(3, "rem fid:%x\n", zdev->fid);
	kfree_rcu(zdev, rcu);
    ...
}

Because the debugfs teardown step in pcibios_disable_device() was bypassed,
will the active debugfs endpoints remain registered with pointers to the
freed zdev structure, leading to a use-after-free upon access?

>  }

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