linux-kernel - Re: [PATCH v4 1/1] PCI/ERR: don't clobber status after reset

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: <20201112155912.GE74254@sarge.linuxathome.me>
Date:   Thu, 12 Nov 2020 15:59:12 +0000
From:   Hedi Berriche <hedi.berriche@....com>
To:     Kuppuswamy Sathyanarayanan 
        <sathyanarayanan.kuppuswamy@...ux.intel.com>,
        Ashok Raj <ashok.raj@...el.com>,
        Bjorn Helgaas <bhelgaas@...gle.com>
Cc:     linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org,
        Sinan Kaya <okaya@...nel.org>, Russ Anderson <rja@....com>,
        Joerg Roedel <jroedel@...e.com>, stable@...nel.org
Subject: Re: [PATCH v4 1/1] PCI/ERR: don't clobber status after reset_link()

On Mon, Nov 02, 2020 at 15:10 Hedi Berriche wrote:
>Commit 6d2c89441571 ("PCI/ERR: Update error status after reset_link()")
>broke pcie_do_recovery(): updating status after reset_link() has the ill
>side effect of causing recovery to fail if the error status is
>PCI_ERS_RESULT_CAN_RECOVER or PCI_ERS_RESULT_NEED_RESET as the following
>code will *never* run in the case of a successful reset_link()
>
>   177         if (status == PCI_ERS_RESULT_CAN_RECOVER) {
>   ...
>   181         }
>
>   183         if (status == PCI_ERS_RESULT_NEED_RESET) {
>   ...
>   192         }
>
>For instance in the case of PCI_ERS_RESULT_NEED_RESET we end up not
>calling ->slot_reset() (because we skip report_slot_reset()) thus
>breaking driver (re)initialisation.
>
>Don't clobber status with the return value of reset_link(); set status
>to PCI_ERS_RESULT_RECOVERED, in case of successful link reset, if and
>only if the initial value of error status is PCI_ERS_RESULT_DISCONNECT
>or PCI_ERS_RESULT_NO_AER_DRIVER.
>
>Fixes: 6d2c89441571 ("PCI/ERR: Update error status after reset_link()")
>Signed-off-by: Hedi Berriche <hedi.berriche@....com>
>
>Reviewed-by: Sinan Kaya <okaya@...nel.org>
>Cc: Russ Anderson <rja@....com>
>Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@...ux.intel.com>
>Cc: Bjorn Helgaas <bhelgaas@...gle.com>
>Cc: Ashok Raj <ashok.raj@...el.com>
>Cc: Joerg Roedel <jroedel@...e.com>
>
>Cc: stable@...nel.org # v5.7+
>---
> drivers/pci/pcie/err.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
>index c543f419d8f9..2730826cfd8a 100644
>--- a/drivers/pci/pcie/err.c
>+++ b/drivers/pci/pcie/err.c
>@@ -165,10 +165,13 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
> 	pci_dbg(dev, "broadcast error_detected message\n");
> 	if (state == pci_channel_io_frozen) {
> 		pci_walk_bus(bus, report_frozen_detected, &status);
>-		status = reset_link(dev);
>-		if (status != PCI_ERS_RESULT_RECOVERED) {
>+		if (reset_link(dev) != PCI_ERS_RESULT_RECOVERED) {
> 			pci_warn(dev, "link reset failed\n");
> 			goto failed;
>+		} else {
>+			if (status == PCI_ERS_RESULT_DISCONNECT ||
>+			    status == PCI_ERS_RESULT_NO_AER_DRIVER)
>+				status = PCI_ERS_RESULT_RECOVERED;
> 		}
> 	} else {
> 		pci_walk_bus(bus, report_normal_detected, &status);
>-- 
>2.28.0

Bjorn,

Sorry to bug you, but could you please cast your eyes on this patch and let me know whether you have
any concerns that might be barring it from inclusion.

Cheers,
Hedi.
-- 
Be careful of reading health books, you might die of a misprint.
	-- Mark Twain