linux-kernel - Re: [PATCH v4 01/10] PCI: Avoid saving error values for config space

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aOQX6ZTMvekd6gWy@wunner.de>
Date: Mon, 6 Oct 2025 21:26:33 +0200
From: Lukas Wunner <lukas@...ner.de>
To: Farhan Ali <alifm@...ux.ibm.com>
Cc: Benjamin Block <bblock@...ux.ibm.com>, linux-s390@...r.kernel.org,
	kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-pci@...r.kernel.org, alex.williamson@...hat.com,
	helgaas@...nel.org, clg@...hat.com, schnelle@...ux.ibm.com,
	mjrosato@...ux.ibm.com
Subject: Re: [PATCH v4 01/10] PCI: Avoid saving error values for config space

On Mon, Oct 06, 2025 at 10:54:51AM -0700, Farhan Ali wrote:
> On 10/4/2025 7:54 AM, Lukas Wunner wrote:
> > I believe this also makes patch [01/10] in your series unnecessary.
> 
> I tested your patches + patches 2-10 of this series. It unfortunately didn't
> completely help with the s390x use case. We still need the check to in
> pci_save_state() from this patch to make sure we are not saving error
> values, which can be written back to the device in pci_restore_state().

What's the caller of pci_save_state() that needs this?

Can you move the check for PCI_POSSIBLE_ERROR() to the caller?
I think plenty of other callers don't need this, so it adds
extra overhead for them and down the road it'll be difficult
to untangle which caller needs it and which doesn't.

> As part of the error recovery userspace can use the VFIO_DEVICE_RESET to
> reset the device (pci_try_reset_function()). The function call for this is:
> 
> pci_dev_save_and_disable <https://elixir.bootlin.com/linux/v6.17.1/C/ident/pci_dev_save_and_disable>();
> 
> __pci_reset_function_locked <https://elixir.bootlin.com/linux/v6.17.1/C/ident/__pci_reset_function_locked>();
> 
> pci_dev_restore
> <https://elixir.bootlin.com/linux/v6.17.1/C/ident/pci_dev_restore>();
> 
> So we can end up overwriting the initial saved state (added by you in
> pci_bus_add_device()). Do we need to update the pci_dev_save_and_disable()
> not to save the state?

The state saved on device addition is just the initial state and
it is fine if later on it gets updated (which is a nicer term than
"overwritten").  E.g. when portdrv.c instantiates port services
and drivers are bound to them, various registers in Config Space
are changed, hence pcie_portdrv_probe() calls pci_save_state()
again.

However we can discuss whether pci_save_state() is still needed
in pci_dev_save_and_disable().

Thanks,

Lukas