Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758749AbbFAE55 (ORCPT ); Mon, 1 Jun 2015 00:57:57 -0400 Received: from smtp.codeaurora.org ([198.145.29.96]:36380 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751475AbbFAE5t (ORCPT ); Mon, 1 Jun 2015 00:57:49 -0400 Message-ID: In-Reply-To: <20150522165108.GB29424@e104818-lin.cambridge.arm.com> References: <20150522165108.GB29424@e104818-lin.cambridge.arm.com> Date: Mon, 1 Jun 2015 04:57:48 -0000 Subject: Re: Crash in crc32_le during kmemleak_scan() From: vigneshr@codeaurora.org To: "Catalin Marinas" Cc: "vigneshr@codeaurora.org" , "linux-kernel@vger.kernel.org" , "bernd.schubert@itwm.fraunhofer.de" User-Agent: SquirrelMail/1.4.22-4.el6 MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7BIT X-Priority: 3 (Normal) Importance: Normal Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6173 Lines: 185 > 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. OOps ! Ya my bad. Sorry about that. > 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. Ah! Thanks for the explanation. This makes sense. > 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<----------------- > We have tested the patch provided above and it was clean report with no crashes that were seen earlier. I guess we can go ahead with this one if its okay with you. Tested-by: Vignesh Radhakrishnan Thanks and regards, Vignesh Radhakrishnan -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project -- 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/