linux-kernel - [PATCH] PCI: Fix devres regression in pci

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

Message-ID: <20240725120729.59788-2-pstanner@redhat.com>
Date: Thu, 25 Jul 2024 14:07:30 +0200
From: Philipp Stanner <pstanner@...hat.com>
To: Bjorn Helgaas <bhelgaas@...gle.com>,
	Krzysztof Wilczyński <kwilczynski@...nel.org>,
	Philipp Stanner <pstanner@...hat.com>
Cc: linux-pci@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	Damien Le Moal <dlemoal@...nel.org>
Subject: [PATCH] PCI: Fix devres regression in pci_intx()

pci_intx() is a function that becomes managed if pcim_enable_device()
has been called in advance. Commit 25216afc9db5 ("PCI: Add managed
pcim_intx()") changed this behavior so that pci_intx() always leads to
creation of a separate device resource for itself, whereas earlier, a
shared resource was used for all PCI devres operations.

Unfortunately, pci_intx() seems to be used in some drivers' remove()
paths; in the managed case this causes a device resource to be created
on driver detach.

Fix the regression by only redirecting pci_intx() to its managed twin
pcim_intx() if the pci_command changes.

Fixes: 25216afc9db5 ("PCI: Add managed pcim_intx()")
Reported-by: Damien Le Moal <dlemoal@...nel.org>
Closes: https://lore.kernel.org/all/b8f4ba97-84fc-4b7e-ba1a-99de2d9f0118@kernel.org/
Signed-off-by: Philipp Stanner <pstanner@...hat.com>
---
Alright, I reproduced this with QEMU as Damien described and this here
fixes the issue on my side. Feedback welcome. Thank you very much,
Damien.

It seems that this might yet again be the issue of drivers not being
aware that pci_intx() might become managed, so they use it in their
unwind path (rightfully so; there probably was no alternative back
then).

That will make the long term cleanup difficult. But I think this for now
is the most elegant possible workaround.
---
 drivers/pci/pci.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index e3a49f66982d..ffaaca0978cb 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4477,12 +4477,6 @@ void pci_intx(struct pci_dev *pdev, int enable)
 {
 	u16 pci_command, new;
 
-	/* Preserve the "hybrid" behavior for backwards compatibility */
-	if (pci_is_managed(pdev)) {
-		WARN_ON_ONCE(pcim_intx(pdev, enable) != 0);
-		return;
-	}
-
 	pci_read_config_word(pdev, PCI_COMMAND, &pci_command);
 
 	if (enable)
@@ -4490,8 +4484,15 @@ void pci_intx(struct pci_dev *pdev, int enable)
 	else
 		new = pci_command | PCI_COMMAND_INTX_DISABLE;
 
-	if (new != pci_command)
+	if (new != pci_command) {
+		/* Preserve the "hybrid" behavior for backwards compatibility */
+		if (pci_is_managed(pdev)) {
+			WARN_ON_ONCE(pcim_intx(pdev, enable) != 0);
+			return;
+		}
+
 		pci_write_config_word(pdev, PCI_COMMAND, new);
+	}
 }
 EXPORT_SYMBOL_GPL(pci_intx);
 
-- 
2.45.2