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

[Farhan Ali]

On 7/22/2026 3:32 PM, Bjorn Helgaas wrote:
> On Thu, Jul 16, 2026 at 11:15:33AM -0700, Farhan Ali wrote:
> > On 7/15/2026 4:34 PM, Bjorn Helgaas wrote:
> > > > On s390 systems, which use a machine level hypervisor, PCI devices are
> > > > always accessed through a form of PCI pass-through which fundamentally
> > > > operates on a per PCI function granularity. This is also reflected in the
> > > > s390 PCI hotplug driver which creates hotplug slots for individual PCI
> > > > functions. Its reset_slot() function, which is a wrapper for
> > > > zpci_hot_reset_device(), thus also resets individual functions.
> > > >
> > > > Currently, the kernel's PCI_SLOT() macro assigns the same pci_slot object
> > > > to multifunction devices.
> > > PCI_SLOT() doesn't assign pci_slot objects; I guess they're assigned
> > > by some code that*uses* PCI_SLOT().  Since this says "currently," I
> > > assume you're changing that code, so we should mention where it is to
> > > help readers out.
> > Thanks for your response! I can re-word the commit message, how about
> > something like this:
> >
> > Currently, the pci_create_slot() assigns the same pci_slot object to
> > multifunction devices.
> >
> > > I see some Sashiko comments; those also need to be addressed or
> > > explained away.
> > Regarding Sashiko's comments for this patch, it mentions 2 issues:
> >
> > New issues: - [High] Unconditional enablement of `per_func_slot` on S390
> > breaks standard PCI hotplug (e.g., pciehp, shpchp) slot matching and resets.
> >
> > I believe this is not applicable as on s390 we don't support any other PCI
> > hotplug drivers given the unique nature of zPCI architecture.
> Makes sense.
>
> > 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.
> >
> > Sashiko identified this as a pre-existing issue, so I don't think should be
> > addressed with this patch.
> Right, we don't need to fix pre-existing issues in this series, but I
> meant there were Sashiko comments on other patches in this series that
> look like they *should* be addressed, e.g.,
>
>    [PATCH v21 2/4] PCI: Avoid saving config space state if inaccessible
>
>      - [High] pci_dev_save_and_disable() skips disabling the device if
>        config space is momentarily inaccessible, potentially leaving
>        DMA and interrupts enabled.

I am not sure if this is an issue? If we want to check if the device is 
momentarily inaccessible, then we would need to poll and do something 
similar to pci_dev_wait(). Please correct me if I am wrong. FWIW this 
doesn't even show up as an error anymore in v22 even though its the same 
code.

https://lore.kernel.org/all/20260720194254.E6B811F000E9@xxxxxxxxxxxxxxx/

>      - [Low] String literal passed to a non-const `char *` pointer in
>        `pci_dev_config_accessible()`.

I wasn't sure if this is something that is strictly enforced. But I can 
fix this if we want to enforce it.


>      https://lore.kernel.org/all/20260630170754.093021F00A3A@xxxxxxxxxxxxxxx
>
>    [PATCH v21 3/4] PCI: Fail FLR when config space is inaccessible
>
>      - [High] Un-ratelimited pci_warn() in pci_dev_config_accessible()
>        allows an attacker to flood the host kernel log.

Since we bailout early with -ENOTTY, we should at least try the other 
reset methods such as bus reset. So AFAIU we shouldn't be spamming the logs.

>      - [High] Early bailout in pcie_flr() when config space is
>        inaccessible skips the Function Level Reset and the subsequent
>        wait, allowing device assignment to continue without reset,
>        leaking state.
>
>      https://lore.kernel.org/all/20260630171310.F25D41F000E9@xxxxxxxxxxxxxxx

The idea with early bailout was to make sure we can try the other reset 
methods as currently if FLR is inaccessible/fails we don't try any other 
reset methods. So we should be trying the other reset methods and it 
shouldn't leak state. I think we probably are leaking the state today, 
as we don't try any other reset methods to reset the device.


>    [PATCH v21 4/4] PCI/MSI: Enable memory decoding before restoring MSI-X messages
>
>      - [Medium] Blindly restoring a stale PCI_COMMAND value introduces
>        a TOCTOU race window that can erase concurrent modifications to
>        the register.
>
>      https://lore.kernel.org/all/20260630171255.20BD91F000E9@xxxxxxxxxxxxxxx

I am not sure if this something we should fix?


> Annoyingly, on v22, Sashiko complained about *different* things and
> didn't complain about some of these even though the v22 patches are
> identical to v21.  Sigh.

Yeah, for this series Sashiko has been a little inconsistent in the 
issues it identifies :)

Thanks

Farhan