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

[Farhan Ali <alifm@linux.ibm.com>]

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@smtp.kernel.org/

>      - [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@smtp.kernel.org
>
>    [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@smtp.kernel.org

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@smtp.kernel.org

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