Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1157792 > unrolled thread

[PATCH] mm: kmemleak: Fix crashing during kmemleak disabling

Back to article view | Back to linux.kernel

Contents

  [PATCH] mm: kmemleak: Fix crashing during kmemleak disabling Catalin Marinas <catalin.marinas@arm.com> - 2015-06-03 17:50 +0200
    Re: [PATCH] mm: kmemleak: Fix crashing during kmemleak disabling Andrew Morton <akpm@linux-foundation.org> - 2015-06-04 01:30 +0200

#1157792 — [PATCH] mm: kmemleak: Fix crashing during kmemleak disabling

With the current implementation, if kmemleak is disabled because of an
error condition (e.g. fails to allocate metadata), alloc/free calls are
no longer tracked. Usually this is not a problem since the kmemleak
metadata is being removed via kmemleak_do_cleanup(). However, if the
scanning thread is running at the time of disabling, kmemleak would no
longer notice a potential vfree() call and the freed/unmapped object may
still be accessed, causing a fault.

This patch separates the kmemleak_free() enabling/disabling from the
overall kmemleak_enabled nob so that we can defer the disabling of the
object freeing tracking until the scanning thread completed. The
kmemleak_free_part() is deliberately ignored by this patch since this is
only called during boot before the scanning thread started.

Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
Reported-by: Vignesh Radhakrishnan <vigneshr@codeaurora.org>
Tested-by: Vignesh Radhakrishnan <vigneshr@codeaurora.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: <stable@vger.kernel.org>
---
 mm/kmemleak.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/mm/kmemleak.c b/mm/kmemleak.c
index f0fe4f2c1fa7..11d6f8015896 100644
--- a/mm/kmemleak.c
+++ b/mm/kmemleak.c
@@ -195,6 +195,8 @@ static struct kmem_cache *scan_area_cache;
 
 /* set if tracing memory operations is enabled */
 static int kmemleak_enabled;
+/* same as above but only for the kmemleak_free() callback */
+static int kmemleak_free_enabled;
 /* set in the late_initcall if there were no errors */
 static int kmemleak_initialized;
 /* enables or disables early logging of the memory operations */
@@ -942,7 +944,7 @@ void __ref kmemleak_free(const void *ptr)
 {
 	pr_debug("%s(0x%p)\n", __func__, ptr);
 
-	if (kmemleak_enabled && ptr && !IS_ERR(ptr))
+	if (kmemleak_free_enabled && ptr && !IS_ERR(ptr))
 		delete_object_full((unsigned long)ptr);
 	else if (kmemleak_early_log)
 		log_early(KMEMLEAK_FREE, ptr, 0, 0);
@@ -982,7 +984,7 @@ void __ref kmemleak_free_percpu(const void __percpu *ptr)
 
 	pr_debug("%s(0x%p)\n", __func__, ptr);
 
-	if (kmemleak_enabled && ptr && !IS_ERR(ptr))
+	if (kmemleak_free_enabled && ptr && !IS_ERR(ptr))
 		for_each_possible_cpu(cpu)
 			delete_object_full((unsigned long)per_cpu_ptr(ptr,
 								      cpu));
@@ -1750,6 +1752,12 @@ static void kmemleak_do_cleanup(struct work_struct *work)
 	mutex_lock(&scan_mutex);
 	stop_scan_thread();
 
+	/*
+	 * Once the scan thread has stopped, it is safe to no longer track
+	 * object freeing.
+	 */
+	kmemleak_free_enabled = 0;
+
 	if (!kmemleak_found_leaks)
 		__kmemleak_do_cleanup();
 	else
@@ -1776,6 +1784,8 @@ static void kmemleak_disable(void)
 	/* check whether it is too early for a kernel thread */
 	if (kmemleak_initialized)
 		schedule_work(&cleanup_work);
+	else
+		kmemleak_free_enabled = 0;
 
 	pr_info("Kernel memory leak detector disabled\n");
 }
@@ -1840,8 +1850,10 @@ void __init kmemleak_init(void)
 	if (kmemleak_error) {
 		local_irq_restore(flags);
 		return;
-	} else
+	} else {
 		kmemleak_enabled = 1;
+		kmemleak_free_enabled = 1;
+	}
 	local_irq_restore(flags);
 
 	/*
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1158128

On Wed,  3 Jun 2015 16:42:56 +0100 Catalin Marinas <catalin.marinas@arm.com> wrote:

> With the current implementation, if kmemleak is disabled because of an
> error condition (e.g. fails to allocate metadata), alloc/free calls are
> no longer tracked. Usually this is not a problem since the kmemleak
> metadata is being removed via kmemleak_do_cleanup(). However, if the
> scanning thread is running at the time of disabling, kmemleak would no
> longer notice a potential vfree() call and the freed/unmapped object may
> still be accessed, causing a fault.
> 
> This patch separates the kmemleak_free() enabling/disabling from the
> overall kmemleak_enabled nob so that we can defer the disabling of the
> object freeing tracking until the scanning thread completed. The
> kmemleak_free_part() is deliberately ignored by this patch since this is
> only called during boot before the scanning thread started.

I'm having trouble with this.  afacit, kmemleak_free() can still be
called while kmemleak_scan() is running on another CPU. 
kmemleak_free_enabled hasn't been cleared yet so the races remain.

However your statement "if the scanning thread is running at the time
of disabling" implies that the race is between kmemleak_scan() and
kmemleak_disable().  Yet the race avoidance code is placed in
kmemleak_free().

All confused.  A more detailed description of the race would help.

Also, the words "kmemleak would no longer notice a potential vfree()
call" aren't sufficiently specific.  kmemleak is a big place - what
*part* of kmemleak are you referring to here?

Finally, I'm concerned that a bare

	kmemleak_free_enabled = 0;

lacks sufficient synchronization with respect to the
kmemleak_free_enabled readers from a locking/reordering point of view. 
What's the story here?

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web