Skip to contentSkip to conversation

kmmio/mmiotrace: fix double free of kmmio_fault_pages

PATCH In review Marcin Slusarz 1 participant 3 messages Jun 13, 2010 – Jun 18, 2010

Subsystems (4)
x86 Arm STM32 x86 MM Tracing MMIO accesses (mmiotrace)
lore
Share & tools
lore

Marcin Slusarz

author
#0 lore raw

PATCH v2 1/1 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]>
Acked-by Pekka Paalanen <[email protected]>
Cc Stuart Bennett <[email protected]>
Tested-by Marcin Kościelnicki <[email protected]>
Tested-by Shinpei KATO <[email protected]>

---
v2: added test to testmmiotrace.c

---
 arch/x86/mm/kmmio.c         |   16 +++++++++++++---
 arch/x86/mm/testmmiotrace.c |   22 ++++++++++++++++++++++
 2 files changed, 35 insertions(+), 3 deletions(-)
arch/x86/mm/kmmio.c +13 -3
index 5d0e67f..e5d5e2c 100644
@@ -45,6 +45,8 @@ struct kmmio_fault_page {
 	 * Protected by kmmio_lock, when linked into kmmio_page_table.
 	 */
 	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");
arch/x86/mm/testmmiotrace.c +22 -0
index 8565d94..e6cc5be 100644
@@ -90,6 +90,27 @@ static void do_test(unsigned long size)
 	iounmap(p);
 }
 
+/*
+ * Tests how mmiotrace behaves in face of multiple ioremap / iounmaps in
+ * a short time. We had a bug in deferred freeing procedure which tried
+ * to free this region multiple times (ioremap can reuse the same address
+ * for many mappings).
+ */
+static void do_test_bulk_ioremapping(void)
+{
+	void __iomem *p;
+	int i;
+	
+	for (i = 0; i < 10; ++i) {
+		p = ioremap_nocache(mmio_address, PAGE_SIZE);
+		if (p)
+			iounmap(p);
+	}
+
+	/* Force freeing. If it will crash we will know why. */
+	synchronize_rcu();
+}
+
 static int __init init(void)
 {
 	unsigned long size = (read_far) ? (8 << 20) : (16 << 10);

@@ -104,6 +125,7 @@ static int __init init(void)
 		   "and writing 16 kB of rubbish in there.\n",
 		   size >> 10, mmio_address);
 	do_test(size);
+	do_test_bulk_ioremapping();
 	pr_info("All done.\n");
 	return 0;
 }
Signature · 1 line
1.7.1
author
#1 lore raw
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.

Quoted 6 lines
Signed-off-by Marcin Slusarz <[email protected]>
Acked-by Pekka Paalanen <[email protected]>
Cc Stuart Bennett <[email protected]>
Tested-by Marcin Kościelnicki <[email protected]>
Tested-by Shinpei KATO <[email protected]>

---

It would be good to apply it to -stable too. Sometimes people has to test some earlier kernels because of unfixable constraints (you know, these uncooperative vendors not releasing their binary drivers for latest kernels)

Marcin

author
#2 lore raw

[tip:x86/mm] x86, kmmio/mmiotrace: Fix double free of kmmio_fault_pages

In reply to Marcin Slusarz
Commit-ID:  8b8f79b927b6b302bb65fb8c56e7a19be5fbdbef
Gitweb:     http://git.kernel.org/tip/8b8f79b927b6b302bb65fb8c56e7a19be5fbdbef
Author:     Marcin Slusarz <[email protected]>
AuthorDate: Sun, 13 Jun 2010 23:56:54 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Fri, 18 Jun 2010 11:30:09 +0200

x86, 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]>
Tested-by Marcin Kocielnicki <[email protected]>
Tested-by Shinpei KATO <[email protected]>
Acked-by Pekka Paalanen <[email protected]>
Cc Stuart Bennett <[email protected]>
Cc Marcin Kocielnicki <[email protected]>
Cc [email protected]
Cc <[email protected]>

LKML-Reference: <[email protected]>

Signed-off-by Ingo Molnar <[email protected]>
---
 arch/x86/mm/kmmio.c         |   16 +++++++++++++---
 arch/x86/mm/testmmiotrace.c |   22 ++++++++++++++++++++++
 2 files changed, 35 insertions(+), 3 deletions(-)
arch/x86/mm/kmmio.c +13 -3
index 5d0e67f..e5d5e2c 100644
@@ -45,6 +45,8 @@ struct kmmio_fault_page {
 	 * Protected by kmmio_lock, when linked into kmmio_page_table.
 	 */
 	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");
arch/x86/mm/testmmiotrace.c +22 -0
index 8565d94..38868ad 100644
@@ -90,6 +90,27 @@ static void do_test(unsigned long size)
 	iounmap(p);
 }
 
+/*
+ * Tests how mmiotrace behaves in face of multiple ioremap / iounmaps in
+ * a short time. We had a bug in deferred freeing procedure which tried
+ * to free this region multiple times (ioremap can reuse the same address
+ * for many mappings).
+ */
+static void do_test_bulk_ioremapping(void)
+{
+	void __iomem *p;
+	int i;
+
+	for (i = 0; i < 10; ++i) {
+		p = ioremap_nocache(mmio_address, PAGE_SIZE);
+		if (p)
+			iounmap(p);
+	}
+
+	/* Force freeing. If it will crash we will know why. */
+	synchronize_rcu();
+}
+
 static int __init init(void)
 {
 	unsigned long size = (read_far) ? (8 << 20) : (16 << 10);

@@ -104,6 +125,7 @@ static int __init init(void)
 		   "and writing 16 kB of rubbish in there.\n",
 		   size >> 10, mmio_address);
 	do_test(size);
+	do_test_bulk_ioremapping();
 	pr_info("All done.\n");
 	return 0;
 }