linux-kernel - Re: [PATCH v3 1/2] PCI/AER: Fix missing uevent on recovery when a reset is requested

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] [day] [month] [year] [list]

Message-ID: <3e356ac60da0c9b9a3b54e7ebbea404f72f2b7a3.camel@linux.ibm.com>
Date: Mon, 04 Aug 2025 13:04:47 +0200
From: Niklas Schnelle <schnelle@...ux.ibm.com>
To: Lukas Wunner <lukas@...ner.de>
Cc: Bjorn Helgaas <bhelgaas@...gle.com>,
        Mahesh J Salgaonkar	
 <mahesh@...ux.ibm.com>,
        Linas Vepstas <linasvepstas@...il.com>,
        Ilpo
 Järvinen <ilpo.jarvinen@...ux.intel.com>,
        Manivannan
 Sadhasivam <mani@...nel.org>,
        Gerald Schaefer	
 <gerald.schaefer@...ux.ibm.com>,
        Heiko Carstens <hca@...ux.ibm.com>, Vasily
 Gorbik <gor@...ux.ibm.com>,
        Alexander Gordeev <agordeev@...ux.ibm.com>,
        Christian Borntraeger	 <borntraeger@...ux.ibm.com>,
        Sven Schnelle
 <svens@...ux.ibm.com>,
        Peter Oberparleiter <oberpar@...ux.ibm.com>,
        Matthew
 Rosato <mjrosato@...ux.ibm.com>,
        "Oliver O'Halloran"	 <oohall@...il.com>, Sinan Kaya <okaya@...nel.org>,
        linuxppc-dev@...ts.ozlabs.org, linux-s390@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org,
        Keith Busch	 <kbusch@...nel.org>
Subject: Re: [PATCH v3 1/2] PCI/AER: Fix missing uevent on recovery when a
 reset is requested

On Thu, 2025-07-31 at 15:01 +0200, Lukas Wunner wrote:
> On Wed, Jul 30, 2025 at 10:24:07PM +0200, Lukas Wunner wrote:
> > On Wed, Jul 30, 2025 at 10:01:50PM +0200, Lukas Wunner wrote:
> > > On Wed, Jul 30, 2025 at 01:20:57PM +0200, Niklas Schnelle wrote:
> > > > Since commit 7b42d97e99d3 ("PCI/ERR: Always report current recovery
> > > > status for udev") AER uses the result of error_detected() as parameter
> > > > to pci_uevent_ers(). As pci_uevent_ers() however does not handle
> > > > PCI_ERS_RESULT_NEED_RESET this results in a missing uevent for the
> > > > beginning of recovery if drivers request a reset. Fix this by treating
> > > > PCI_ERS_RESULT_NEED_RESET as beginning recovery.
> > > [...]
> > > > +++ b/drivers/pci/pci-driver.c
> > > > @@ -1592,6 +1592,7 @@ void pci_uevent_ers(struct pci_dev *pdev, enum pci_ers_result err_type)
> > > >  	switch (err_type) {
> > > >  	case PCI_ERS_RESULT_NONE:
> > > >  	case PCI_ERS_RESULT_CAN_RECOVER:
> > > > +	case PCI_ERS_RESULT_NEED_RESET:
> > > >  		envp[idx++] = "ERROR_EVENT=BEGIN_RECOVERY";
> > > >  		envp[idx++] = "DEVICE_ONLINE=0";
> > > >  		break;
> > > 
> > > I note that PCI_ERS_RESULT_NO_AER_DRIVER is also missing in that
> > > switch/case statement.  I guess for the patch to be complete,
> > > it needs to be added to the PCI_ERS_RESULT_DISCONNECT case.
> > > Do you agree?

As far as I can see PCI_ERS_RESULT_NO_AER_DRIVER only occurs in the AER
code and leads to abandoning all recovery attempts for the whole
subtree with a disconnect. So my thinking is that the uevent is just
disconnect. That also matches your proposal below. Thankfully it looks
like there are still kernel messages indicating the reason of the
failure.

> > 
> > I realize now there's a bigger problem here:  In pcie_do_recovery(),
> > when control reaches the "failed:" label, a uevent is only signaled
> > for the *bridge*.  Shouldn't a uevent instead be signaled for every
> > device *below* the bridge?  (And possibly the bridge itself if it was
> > the device reporting the error.)
> 
> The small patch below should resolve this issue.
> Please let me know what you think.

The patch makes sense to me I agree one should get uevents for each
downstream device. Please Cc me when you send it.

> 
> > In that case you don't need to add PCI_ERS_RESULT_NO_AER_DRIVER to
> > the switch/case statement because we wouldn't want to have multiple
> > uevents reporting disconnect, so the one emitted below the "failed:"
> > label would be sufficient.
> 
> I'll send a separate Reviewed-by for your original patch as the small
> patch below should resolve my concern about PCI_ERS_RESULT_NO_AER_DRIVER.
> 
> > This all looks so broken that I'm starting to wonder if there's any
> > user space application at all that takes advantage of these uevents?
> 
> I'd still be interested to know which user space application you're
> using to track these uevents?
> 
> Thanks,
> 
> Lukas

Thanks for the R-b! And yes I agree we should minimize the differences
between the behavior of the implementations. I'll see if I can sync up
on this with Mahesh too.

I only tested this with udevadm so far. That said I had multiple
projects ask for ways to monitor for errors/recovery from user-space so
at least for s390 there is interest for these events. There is also a
bit of mainframe specifics here of course, for example our machines
have the ability to fail over a broken PCI link to an I/O drawer with
an alternate path and that piggybacks on these recovery mechanisms. And
generally, we're looking at expanding our support for and use of PCI
error recovery and will hopefully be sharing some more of that soon.

Also note that with the recently released IBM z17 and IBM LinuxONE 5
with Linux we're significantly expanding our reliance on native
PCI[0][1].

[0]
https://www.ibm.com/docs/en/linux-on-systems?topic=networking-network-express
[1]
https://vmworkshop.org/2025/present/9175tovr.pdf
>