Re: [PATCH v21 1/4] PCI: Allow per function PCI slots to fix slot reset on s390

[sashiko-bot@kernel.org]

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

New issues:
- [High] Unconditional enablement of `per_func_slot` on S390 breaks standard PCI hotplug (e.g., pciehp, shpchp) slot matching and resets.

Pre-existing issues:
- [High] Lockless access to `dev->slot` in `pci_dev_reset_slot_function` can lead to Use-After-Free if a hotplug driver is concurrently unbound.
--

commit e76ff052218a061a873ef34162524387e5e10c78
Author: Farhan Ali <alifm@linux.ibm.com>
Subject: PCI: Allow per function PCI slots to fix slot reset on s390

This commit addresses an issue on s390 systems where the hotplug driver
creates slots for individual PCI functions. It introduces a per_func_slot
flag to struct pci_slot to allow per-function PCI slots and changes the
slot number to u16 to accommodate special placeholder and all-devices
values.

Fixes: 44510d6fa0c0 ("s390/pci: Handling multifunctions")
Cc: stable@vger.kernel.org

> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 77b17b13ee615..350bae907ebf8 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4897,8 +4897,9 @@ static int pci_reset_hotplug_slot(struct hotplug_slot *hotplug, bool probe)
>  
>  static int pci_dev_reset_slot_function(struct pci_dev *dev, bool probe)
>  {
> -	if (dev->multifunction || dev->subordinate || !dev->slot ||
> -	    dev->dev_flags & PCI_DEV_FLAGS_NO_BUS_RESET)
> +	if (dev->subordinate || !dev->slot ||
> +	    dev->dev_flags & PCI_DEV_FLAGS_NO_BUS_RESET ||
> +	    (dev->multifunction && !dev->slot->per_func_slot))

[Severity: High]
This is a pre-existing issue, but does this lockless access to dev->slot risk
a use-after-free if a hotplug driver is concurrently unbound?

The patch expands this access by adding the check for
!dev->slot->per_func_slot. If a concurrent sysfs operation unbinds the
pciehp service driver from the parent bridge, it takes the device lock on the
bridge and executes:

pci_hp_deregister() -> pci_destroy_slot()

This invokes pci_slot_release(), clearing dev->slot = NULL under
down_read(&pci_bus_sem) and freeing the slot memory. Since
pci_dev_reset_slot_function() does not hold pci_bus_sem or pci_slot_mutex,
could it check !dev->slot, get preempted, and then dereference freed memory
upon resuming to evaluate dev->slot->per_func_slot?

>  		return -ENOTTY;
>  
>  	return pci_reset_hotplug_slot(dev->slot->hotplug, probe);

[ ... ]

> diff --git a/drivers/pci/slot.c b/drivers/pci/slot.c
> index 6d5cd37bfb1e7..894d6213ed306 100644
> --- a/drivers/pci/slot.c
> +++ b/drivers/pci/slot.c
[ ... ]
> @@ -72,6 +72,23 @@ static ssize_t cur_speed_read_file(struct pci_slot *slot, char *buf)
>  	return bus_speed_read(slot->bus->cur_bus_speed, buf);
>  }
>  
> +static bool pci_dev_matches_slot(struct pci_dev *dev, struct pci_slot *slot)
> +{
> +	if (slot->per_func_slot)
> +		return dev->devfn == slot->number;

[Severity: High]
Does enabling per_func_slot unconditionally on S390 break standard PCI
hotplug slot matching?

When standard PCI hotplug drivers (like pciehp) create a slot on an S390
system, pci_slot_enabled_per_func() globally forces per_func_slot to 1.

This preempts the wildcard checking logic in pci_dev_matches_slot().
For drivers creating bus-wide slots using PCI_SLOT_ALL_DEVICES (0xfeff),
this rigid comparison forces dev->devfn == 0xfeff, which will always fail
since devfn is an 8-bit value.

Additionally, for drivers passing a standard slot number N,
pci_dev_matches_slot() now compares it directly against devfn instead of
calculating PCI_SLOT(dev->devfn). A slot number of 1 will match Device 0,
Function 1 instead of Device 1, Functions 0-7.

Should per_func_slot be an explicitly opted-in property provided by the
specific hotplug driver, rather than an architecture-wide global override?

> +
> +	return slot->number == PCI_SLOT_ALL_DEVICES ||
> +		PCI_SLOT(dev->devfn) == slot->number;
> +}
> +
> +static bool pci_slot_enabled_per_func(void)
> +{
> +	if (IS_ENABLED(CONFIG_S390))
> +		return true;
> +
> +	return false;
> +}
> +

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