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 for Android: free password hash cracker in your pocket

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

Message-ID: <aOtL_Y6HH5-qh2jD@wunner.de>
Date: Sun, 12 Oct 2025 08:34:37 +0200
From: Lukas Wunner <lukas@...ner.de>
To: Niklas Schnelle <schnelle@...ux.ibm.com>
Cc: Farhan Ali <alifm@...ux.ibm.com>, 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,
	mjrosato@...ux.ibm.com
Subject: Re: [PATCH v4 01/10] PCI: Avoid saving error values for config space

On Thu, Oct 09, 2025 at 11:12:03AM +0200, Niklas Schnelle wrote:
> On Wed, 2025-10-08 at 20:14 +0200, Lukas Wunner wrote:
> > And yet you're touching the device by trying to reset it.
> > 
> > The code you're introducing in patch [01/10] only becomes necessary
> > because you're not following the above-quoted protocol.  If you
> > follow the protocol, patch [01/10] becomes unnecessary.
> 
> I agree with your point above error_detected() should not touch the
> device. My understanding of Farhan's series though is that it follows
> that rule. As I understand it error_detected() is only used to inject
> the s390 specific PCI error event into the VM using the information
> stored in patch 7. As before vfio-pci returns
> PCI_ERS_RESULT_CAN_RECOVER from error_detected() but then with patch 7
> the pass-through case is detected and this gets turned into
> PCI_ERS_RESULT_RECOVERED and the rest of the s390 recovery code gets
> skipped. And yeah, writing it down I'm not super happy with this part,
> maybe it would be better to have an explicit
> PCI_ERS_RESULT_LEAVE_AS_IS.

Thanks, that's the high-level overview I was looking for.

It would be good to include something like this at least
in the cover letter or additionally in the commit messages
so that it's easier for reviewers to connect the dots.

I was expecting paravirtualized error handling, i.e. the
VM is aware it's virtualized and vfio essentially proxies
the pci_ers_result return value of the driver (e.g. nvme)
back to the host, thereby allowing the host to drive error
recovery normally.  I'm not sure if there are technical
reasons preventing such an approach.

If you do want to stick with your alternative approach,
maybe doing the error handling in the ->mmio_enabled() phase
instead of ->error_detected() would make more sense.
In that phase you're allowed to access the device,
you can also attempt a local reset and return
PCI_ERS_RESULT_RECOVERED on success.

You'd have to return PCI_ERS_RESULT_CAN_RECOVER though
from the ->error_detected() callback in order to progress
to the ->mmio_enabled() step.

Does that make sense?

Thanks,

Lukas