Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757923AbbEVQvQ (ORCPT ); Fri, 22 May 2015 12:51:16 -0400 Received: from foss.arm.com ([217.140.101.70]:44435 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757564AbbEVQvM (ORCPT ); Fri, 22 May 2015 12:51:12 -0400 Date: Fri, 22 May 2015 17:51:08 +0100 From: Catalin Marinas To: "vigneshr@codeaurora.org" Cc: "linux-kernel@vger.kernel.org" , "bernd.schubert@itwm.fraunhofer.de" Subject: Re: Crash in crc32_le during kmemleak_scan() Message-ID: <20150522165108.GB29424@e104818-lin.cambridge.arm.com> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5498 Lines: 165 Hi Vignesh, Thanks for testing this. On Thu, May 21, 2015 at 06:45:17AM +0100, vigneshr@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 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/