diff mbox series

[v6,1/4] lib/pci_iomap.c: fix cleanup bug in pci_iounmap()

Message ID 20240131090023.12331-2-pstanner@redhat.com
State New
Headers
Series Regather scattered PCI-Code |

Commit Message

Philipp Stanner Jan. 31, 2024, 9 a.m. UTC 
  The #ifdef for the ioport-ranges accidentally also guards iounmap(),
potentially compiling an empty function. This would cause the mapping to
be leaked.

Move the guard so that iounmap() will always be part of the function.

CC: <stable@vger.kernel.org> # v5.15+
Fixes: 316e8d79a095 ("pci_iounmap'2: Electric Boogaloo: try to make sense of it all")
Reported-by: Danilo Krummrich <dakr@redhat.com>
Suggested-by: Arnd Bergmann <arnd@kernel.org>
Signed-off-by: Philipp Stanner <pstanner@redhat.com>
Reviewed-by: Arnd Bergmann <arnd@arndb.de>
---
 lib/pci_iomap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Bjorn Helgaas Jan. 31, 2024, 9:09 p.m. UTC | #1 
On Wed, Jan 31, 2024 at 10:00:20AM +0100, Philipp Stanner wrote:
> The #ifdef for the ioport-ranges accidentally also guards iounmap(),
> potentially compiling an empty function. This would cause the mapping to
> be leaked.
> 
> Move the guard so that iounmap() will always be part of the function.

I tweaked the subject and commit log to be more explicit about what
the bug is.  Let me know if I got it wrong:

  pci_iounmap(): Fix MMIO mapping leak

  The #ifdef ARCH_HAS_GENERIC_IOPORT_MAP accidentally also guards iounmap(),
  which means MMIO mappings are leaked.

  Move the guard so we call iounmap() for MMIO mappings.

Bjorn
Philipp Stanner Feb. 6, 2024, 9:36 a.m. UTC | #2 
On Wed, 2024-01-31 at 15:09 -0600, Bjorn Helgaas wrote:
> On Wed, Jan 31, 2024 at 10:00:20AM +0100, Philipp Stanner wrote:
> > The #ifdef for the ioport-ranges accidentally also guards
> > iounmap(),
> > potentially compiling an empty function. This would cause the
> > mapping to
> > be leaked.
> > 
> > Move the guard so that iounmap() will always be part of the
> > function.
> 
> I tweaked the subject and commit log to be more explicit about what
> the bug is.  Let me know if I got it wrong:

Mostly correct IMO

> 
>   pci_iounmap(): Fix MMIO mapping leak
> 
>   The #ifdef ARCH_HAS_GENERIC_IOPORT_MAP accidentally also guards
> iounmap(),
>   which means MMIO mappings are leaked.

nit: I wasn't entirely sure when they are actually leaked, just that
they _could_ be leaked. To know for sure we'd need to search who sets
ARCH_WANTS_GENERIC_PCI_IOUNMAP without setting
ARCH_HAS_GENERIC_IOPORT_MAP.

I think your formulation should be fine, though, since it's definitely
a bug.

P.

> 
>   Move the guard so we call iounmap() for MMIO mappings.
> 
> Bjorn
>
diff mbox series

Patch

diff --git a/lib/pci_iomap.c b/lib/pci_iomap.c
index ce39ce9f3526..2829ddb0e316 100644
--- a/lib/pci_iomap.c
+++ b/lib/pci_iomap.c
@@ -170,8 +170,8 @@  void pci_iounmap(struct pci_dev *dev, void __iomem *p)
 
 	if (addr >= start && addr < start + IO_SPACE_LIMIT)
 		return;
-	iounmap(p);
 #endif
+	iounmap(p);
 }
 EXPORT_SYMBOL(pci_iounmap);