Skip to contentSkip to conversation

parisc: Fix vmap memory leak in ioremap()/iounmap()

PATCH Superseded Helge Deller 3 participants 6 messages Sep 9, 2019 – Oct 11, 2019 PA-RISC architecture

lore
Share & tools
lore

Helge Deller

author maintainer
#0 lore raw

PATCH 1/1 parisc: Fix vmap memory leak in ioremap()/iounmap()

Sven noticed that calling ioremap() and iounmap() multiple times leads to a vmap memory leak:
vmap allocation for size 4198400 failed:
use vmalloc=<size> to increase size

It seems we missed calling remove_vm_area() for ioummap().

Signed-off-by Helge Deller <[email protected]>
Noticed-by Sven Schnelle <[email protected]>
Cc <[email protected]>
arch/parisc/mm/ioremap.c +23 -4
index 92a9b5f12f98..bcfa98aa134c 100644
@@ -3,13 +3,14 @@
  * arch/parisc/mm/ioremap.c
  *
  * (C) Copyright 1995 1996 Linus Torvalds
- * (C) Copyright 2001-2006 Helge Deller <[email protected]>
+ * (C) Copyright 2001-2019 Helge Deller <[email protected]>
  * (C) Copyright 2005 Kyle McMartin <[email protected]>
  */
 
 #include <linux/vmalloc.h>
 #include <linux/errno.h>
 #include <linux/module.h>
+#include <linux/slab.h>
 #include <linux/io.h>
 #include <asm/pgalloc.h>
 

@@ -84,7 +85,8 @@ void __iomem * __ioremap(unsigned long phys_addr, unsigned long size, unsigned l
 	addr = (void __iomem *) area->addr;
 	if (ioremap_page_range((unsigned long)addr, (unsigned long)addr + size,
 			       phys_addr, pgprot)) {
-		vfree(addr);
+		remove_vm_area((void __force *)addr);
+		kfree(area);
 		return NULL;
 	}
 

@@ -94,7 +96,24 @@ EXPORT_SYMBOL(__ioremap);
 
 void iounmap(const volatile void __iomem *addr)
 {
-	if (addr > high_memory)
-		return vfree((void *) (PAGE_MASK & (unsigned long __force) addr));
+	struct vm_struct *p, *o;
+
+	if (addr <= high_memory)
+		return;
+
+	addr = (volatile void __iomem *)
+		(PAGE_MASK & (unsigned long __force)addr);
+
+	p = find_vm_area((void __force *)addr);
+	if (!p) {
+		printk(KERN_ERR "iounmap: bad address %p\n", addr);
+		dump_stack();
+		return;
+	}
+
+	/* Finally remove it */
+	o = remove_vm_area((void __force *)addr);
+	BUG_ON(p != o || o == NULL);
+	kfree(p);
 }
 EXPORT_SYMBOL(iounmap);
#1 lore raw
Quoted 6 lines · On Mon, 9 Sep 2019 15:38:56 +0200 Helge Deller <[email protected]> wrote:

Sven noticed that calling ioremap() and iounmap() multiple times leads to a vmap memory leak:
vmap allocation for size 4198400 failed:
use vmalloc=<size> to increase size

It seems we missed calling remove_vm_area() for ioummap().

That patch seems to work (tested on a C8000 for a couple of weeks). I was surprised it wasn't part of the "parisc architecture updates for kernel v5.4" pull request.

Kind regards,
jer

#2 lore raw

has it already been tested on C36xx and C37xx?

Il giorno lun 30 set 2019 alle ore 08:22 Jeroen Roovers <[email protected]> ha scritto:

Quoted 18 lines
Quoted 6 lines · On Mon, 9 Sep 2019 15:38:56 +0200 Helge Deller <[email protected]> wrote:

Sven noticed that calling ioremap() and iounmap() multiple times leads to a vmap memory leak:
vmap allocation for size 4198400 failed:
use vmalloc=<size> to increase size

It seems we missed calling remove_vm_area() for ioummap().

That patch seems to work (tested on a C8000 for a couple of weeks). I was surprised it wasn't part of the "parisc architecture updates for kernel v5.4" pull request.

Kind regards,
jer

author maintainer
#3 lore raw
In reply to Jeroen Roovers
Quoted 11 lines · Jeroen Roovers
Quoted 6 lines · On Mon, 9 Sep 2019 15:38:56 +0200 Helge Deller <[email protected]> wrote:

Sven noticed that calling ioremap() and iounmap() multiple times leads to a vmap memory leak:
vmap allocation for size 4198400 failed:
use vmalloc=<size> to increase size

It seems we missed calling remove_vm_area() for ioummap().

That patch seems to work (tested on a C8000 for a couple of weeks).

Did you actually stress-tested it?
modprobe/rmmod is not used that often if you have the patch simply compiled in...

Quoted 2 lines

I was surprised it wasn't part of the "parisc architecture updates for kernel v5.4" pull request.

My hope was that some mm-knowledgeable person would doublecheck. It seems we could e.g. use vunmap() instead of remove_vm_area() & kfree()...

The patch is not forgotten, it just needs some more thoughts/discussions.

Helge

author maintainer
#4 lore raw

PATCH v2 1/1 parisc: Fix vmap memory leak in ioremap()/iounmap()

Sven noticed that calling ioremap() and iounmap() multiple times leads to a vmap memory leak:
vmap allocation for size 4198400 failed:
use vmalloc=<size> to increase size

It seems we missed calling vunmap() in ioummap().

Signed-off-by Helge Deller <[email protected]>
Noticed-by Sven Schnelle <[email protected]>
Cc <[email protected]>
arch/parisc/mm/ioremap.c +7 -5
index 92a9b5f12f98..f29f682352f0 100644
@@ -3,7 +3,7 @@
  * arch/parisc/mm/ioremap.c
  *
  * (C) Copyright 1995 1996 Linus Torvalds
- * (C) Copyright 2001-2006 Helge Deller <[email protected]>
+ * (C) Copyright 2001-2019 Helge Deller <[email protected]>
  * (C) Copyright 2005 Kyle McMartin <[email protected]>
  */
 

@@ -84,7 +84,7 @@ void __iomem * __ioremap(unsigned long phys_addr, unsigned long size, unsigned l
 	addr = (void __iomem *) area->addr;
 	if (ioremap_page_range((unsigned long)addr, (unsigned long)addr + size,
 			       phys_addr, pgprot)) {
-		vfree(addr);
+		vunmap(addr);
 		return NULL;
 	}
 

@@ -92,9 +92,11 @@ void __iomem * __ioremap(unsigned long phys_addr, unsigned long size, unsigned l
 }
 EXPORT_SYMBOL(__ioremap);
 
-void iounmap(const volatile void __iomem *addr)
+void iounmap(const volatile void __iomem *io_addr)
 {
-	if (addr > high_memory)
-		return vfree((void *) (PAGE_MASK & (unsigned long __force) addr));
+	unsigned long addr = (unsigned long)io_addr & PAGE_MASK;
+
+	if (is_vmalloc_addr((void *)addr))
+		vunmap((void *)addr);
 }
 EXPORT_SYMBOL(iounmap);
#5 lore raw

pATA Promise Technologies PDC20265 (datasheet)

In reply to Helge Deller

hi
perhaps a bit OT, but I really don't know where to ask. I googled a lot these days, but couldn't find a datasheet about the Promise Technologies PDC20265 chip.

does anyone happen to have a copy of it?

thanks
=)