Hi Vignesh,
Thanks for testing this.
On Thu, May 21, 2015 at 06:45:17AM +0100, vigne...@codeaurora.org wrote:
> diff --git a/mm/kmemleak.c b/mm/kmemleak.c
> index 5ec8b71..4455bb8 100644
> --- a/mm/kmemleak.c
> +++ b/mm/kmemleak.c
> @@ -959,7 +959,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_enabled && ptr && !IS_ERR(ptr) && !kmemleak_error)
> delete_object_full((unsigned long)ptr);
> else if (kmemleak_early_log)
> log_early(KMEMLEAK_FREE, ptr, 0, 0);
That's the problem we try to avoid, if we block kmemleak_free on
kmemleak_error (that was the same as the kmemleak_enabled case before),
scanning may still be in progress for an object but the object unmapped
by something like vfree.
So for the error case, we want:
1. Allow object freeing during a memory scan
2. Block kmemleak_free() being entered once the scanning stops and the
clean-up starts
What I missed is that the clean-up calls delete_object_full() and this
can race with a kmemleak_free() on the same object. The same could
probably happen if buggy kernel code would call kfree() on the same
object from different CPUs. Covering this case is more complicated, I
have to properly think of the locking.
But assuming that the callers are safe, we need to disable kmemleak
before the clean-up starts. We can safely set kmemleak_enabled to 0
after the scanning thread is stopped. So on top of my previous patch:
diff --git a/mm/kmemleak.c b/mm/kmemleak.c
index dcba05812678..52a38eed50e2 100644
--- a/mm/kmemleak.c
+++ b/mm/kmemleak.c
@@ -1757,16 +1757,15 @@ static void kmemleak_do_cleanup(struct work_struct
*work)
mutex_lock(&scan_mutex);
stop_scan_thread();
+ /* stop any memory operation tracing */
+ kmemleak_enabled = 0;
+
if (!kmemleak_found_leaks)
__kmemleak_do_cleanup();
else
pr_info("Kmemleak disabled without freeing internal data. "
"Reclaim the memory with \"echo clear >
/sys/kernel/debug/kmemleak\"\n");
mutex_unlock(&scan_mutex);
-
- /* stop any memory operation tracing */
- kmemleak_enabled = 0;
-
}
static DECLARE_WORK(cleanup_work, kmemleak_do_cleanup);
> @@ -1019,7 +1019,7 @@ void __ref kmemleak_not_leak(const void *ptr)
> {
> pr_debug("%s(0x%p)\n", __func__, ptr);
>
> - if (kmemleak_enabled && ptr && !IS_ERR(ptr))
> + if (kmemleak_enabled && ptr && !IS_ERR(ptr) && !kmemleak_error)
> make_gray_object((unsigned long)ptr);
> else if (kmemleak_early_log)
> log_early(KMEMLEAK_NOT_LEAK, ptr, 0, 0);
That's needed as well. Actually, all the kmemleak entry points apart
from kmemleak_free() need to bail out on kmemleak_error (e.g.
kmemleak_ignore).
So I think we need a separate kmemleak_free_enabled. Can you try the
patch below against mainline please (so revert the previous one)? I
haven't bothered with kmemleak_free_part() since this is only called
during early memboot allocations, so we don't have any scanning thread
running.
BTW, I'll be on holiday for a week, back on the 1st of June.
----8<------------------
diff --git a/mm/kmemleak.c b/mm/kmemleak.c
index 5405aff5a590..7913386ca506 100644
--- a/mm/kmemleak.c
+++ b/mm/kmemleak.c
@@ -194,6 +194,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 */
@@ -941,7 +943,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);
@@ -981,7 +983,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));
@@ -1749,6 +1751,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
@@ -1775,6 +1783,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");
}
@@ -1839,8 +1849,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);
/*
-------------------8<-----------------
Thanks,
Catalin
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
