kmmio/mmiotrace: fix double free of kmmio_fault_pages

After every iounmap mmiotrace has to free kmmio_fault_pages, but it can't do it directly, so it defers freeing by RCU.

It usually works, but when mmiotraced code calls ioremap-iounmap multiple times without sleeping between (so RCU won't kick in and start freeing) it can be given the same virtual address, so at every iounmap mmiotrace will schedule the same pages for release. Obviously it will explode on second free.

Fix it by marking kmmio_fault_pages which are scheduled for release and not adding them second time.

Signed-off-by Marcin Slusarz <[email protected]>

Cc Pekka Paalanen <[email protected]>

Cc Stuart Bennett <[email protected]>

---
 arch/x86/mm/kmmio.c |   16 +++++++++++++---
 1 files changed, 13 insertions(+), 3 deletions(-)

arch/x86/mm/kmmio.c +0 -0

index 5d0e67f..e5d5e2c 100644
@@ -45,6 +45,8 @@ struct kmmio_fault_page {
 	 * Protected by kmmio_lock, when linked into

kmmio_page_table. */

arch/x86/mm/kmmio.c +13 -3

 	int count;
+
+	bool scheduled_for_release;
 };
 
 struct kmmio_delayed_release {

@@ -398,8 +400,11 @@ static void
 release_kmmio_fault_page(unsigned long page, BUG_ON(f->count < 0);
 	if (!f->count) {
 		disarm_kmmio_fault_page(f);
-		f->release_next = *release_list;
-		*release_list = f;
+		if (!f->scheduled_for_release) {
+			f->release_next = *release_list;
+			*release_list = f;
+			f->scheduled_for_release = true;
+		}
 	}
 }
 

@@ -471,8 +476,10 @@ static void remove_kmmio_fault_pages(struct
 rcu_head *head) prevp = &f->release_next;
 		} else {
 			*prevp = f->release_next;
+			f->release_next = NULL;
+			f->scheduled_for_release = false;
 		}
-		f = f->release_next;
+		f = *prevp;
 	}
 	spin_unlock_irqrestore(&kmmio_lock, flags);
 

@@ -510,6 +517,9 @@ void unregister_kmmio_probe(struct
 kmmio_probe *p) kmmio_count--;
 	spin_unlock_irqrestore(&kmmio_lock, flags);
 
+	if (!release_list)
+		return;
+
 	drelease = kmalloc(sizeof(*drelease), GFP_ATOMIC);
 	if (!drelease) {
 		pr_crit("leaking kmmio_fault_page objects.\n");

Signature · 1 line

1.7.1

After every iounmap mmiotrace has to free kmmio_fault_pages, but it can't do it directly, so it defers freeing by RCU.

It usually works, but when mmiotraced code calls ioremap-iounmap multiple times without sleeping between (so RCU won't kick in and start freeing) it can be given the same virtual address, so at every iounmap mmiotrace will schedule the same pages for release. Obviously it will explode on second free.

Fix it by marking kmmio_fault_pages which are scheduled for release and not adding them second time.

Quoted 12 lines · Marcin Slusarz

After every iounmap mmiotrace has to free kmmio_fault_pages, but it can't do it directly, so it defers freeing by RCU.

It usually works, but when mmiotraced code calls ioremap-iounmap multiple times without sleeping between (so RCU won't kick in and start freeing) it can be given the same virtual address, so at every iounmap mmiotrace will schedule the same pages for release. Obviously it will explode on second free.

Fix it by marking kmmio_fault_pages which are scheduled for release and not adding them second time.

Attached patch for mmiotrace testing module allows to reliably reproduce the bug. It can be folded into the main patch.

---

(quoted diff) +0 -0

b/arch/x86/mm/testmmiotrace.c index 8565d94..5f0937b 100644

arch/x86/mm/testmmiotrace.c +13 -0

@@ -90,6 +90,19 @@ static void do_test(unsigned long size)
 	iounmap(p);
 }
 
+static void do_test2(void)
+{
+	void __iomem *p;
+	int i;
+	
+	for (i = 0; i < 10; ++i) {
+		p = ioremap_nocache(mmio_address, 4096);
+		if (p)
+			iounmap(p);
+	}
+	synchronize_rcu(); /* will freeing work? */
+}
+
 static int __init init(void)
 {
 	unsigned long size = (read_far) ? (8 << 20) : (16 << 10);

Quoted 12 lines · Marcin Slusarz

After every iounmap mmiotrace has to free kmmio_fault_pages, but it can't do it directly, so it defers freeing by RCU.

It usually works, but when mmiotraced code calls ioremap-iounmap multiple times without sleeping between (so RCU won't kick in and start freeing) it can be given the same virtual address, so at every iounmap mmiotrace will schedule the same pages for release. Obviously it will explode on second free.

Fix it by marking kmmio_fault_pages which are scheduled for release and not adding them second time.

Attached patch for mmiotrace testing module allows to reliably reproduce the bug. It can be folded into the main patch.

---

arch/x86/mm/testmmiotrace.c +0 -0

index 8565d94..5f0937b 100644

arch/x86/mm/testmmiotrace.c +6 -0

+		if (p)
+			iounmap(p);
+	}
+	synchronize_rcu(); /* will freeing work? */
+}
+
 static int __init init(void)
 {
 	unsigned long size = (read_far) ? (8 << 20) : (16 << 10);