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

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

[Catalin Marinas]

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, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Andrew Morton]

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, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Catalin Marinas]

Hi Andrew,

On Thu, Jun 04, 2015 at 12:29:36AM +0100, Andrew Morton wrote:
> 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.

It's not about kmemleak_free() racing with kmemleak_scan().
kmemleak_free() (and __delete_object()) is meant to race with the
scanning thread (which can run for minutes at a time), the locking is
done on individual kmemleak_object items.

> 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().

The race is indeed between kmemleak_disable() and kmemleak_scan(). Since
the former may be called in atomic contexts, we cannot issue a
kthread_stop() and wait for the scanning thread to finish. This is
deferred to the kmemleak_do_cleanup() work queue.

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

I'll try to improve it and re-post.

> 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?

What I meant is that without any patch, kmemleak_free() simply returns
on !kmemleak_enabled and kmemleak_scan() does not notice that objects it
is scanning are being freed (__delete_object() no longer called). This
is worse with vfree() as the object is no longer mapped.

There are other ways of fixing this like adding heavier locking but I
found that simply allowing kmemleak_free() to get through to
__delete_object() until the kmemleak_scan stopped is the simplest.

> 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. 

I thought about this as well and I didn't see an issue initially. From
an atomicity perspective, I'm not sure using atomic_t has any more
benefits (we used to have atomics here until commit 8910ae896c8c
"kmemleak: change some global variables to int").

As for the ordering, we need to ensure the visibility of the
kmemleak_free_enabled = 0 update to other CPUs in two cases:

1. after kmemleak_scan() is stopped. This is done by calling
   kthread_stop() -> put_task_struct() -> atomic_dec_and_test(). The
   latter implies barriers on each side of the operation, so I think
   this case is safe.

2. before __kmemleak_do_cleanup(). The risk here is that a
   delete_object_full() call from __kmemleak_do_cleanup() races with the
   same call from kmemleak_free(). The object_tree_root look-up (via
   find_get_object) is covered by the kmemleak_lock. However, it looks
   to me like two delete_object_full() calls on the same object can race
   to __delete_object() and call rb_erase() and list_del_rcu() twice on
   the same object.

Second case is trickier. A barrier after kmemleak_free_enabled = 0 does
not help, we need locking from object look-up all the way to removing
the object from the object list and tree. Alternatively, I could take
the object->lock for the whole __delete_object() function and use the
OBJECT_ALLOCATED flag to decide whether to call rb_erase and
list_del_rcu. Something like below, but untested yet (I'm off most of
the day); I would need to pass it through lock proving as well:

diff --git a/mm/kmemleak.c b/mm/kmemleak.c
index 11d6f8015896..27e2e0b688a9 100644
--- a/mm/kmemleak.c
+++ b/mm/kmemleak.c
@@ -607,20 +607,25 @@ static void __delete_object(struct kmemleak_object *object)
 {
 	unsigned long flags;
 
+	/*
+	 * Locking here ensures that the corresponding memory block cannot be
+	 * freed when it is being scanned. It also avoids __delete_object()
+	 * race when being called form __kmemleak_do_cleanup().
+	 */
+	spin_lock_irqsave(&object->lock, flags);
+	if (!(object->flags & OBJECT_ALLOCATED))
+		goto out;
+
 	write_lock_irqsave(&kmemleak_lock, flags);
 	rb_erase(&object->rb_node, &object_tree_root);
 	list_del_rcu(&object->object_list);
 	write_unlock_irqrestore(&kmemleak_lock, flags);
 
-	WARN_ON(!(object->flags & OBJECT_ALLOCATED));
 	WARN_ON(atomic_read(&object->use_count) < 2);
 
-	/*
-	 * Locking here also ensures that the corresponding memory block
-	 * cannot be freed when it is being scanned.
-	 */
-	spin_lock_irqsave(&object->lock, flags);
 	object->flags &= ~OBJECT_ALLOCATED;
+
+out:
 	spin_unlock_irqrestore(&object->lock, flags);
 	put_object(object);
 }

-- 
Catalin

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>