Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933462AbdCaWSY (ORCPT ); Fri, 31 Mar 2017 18:18:24 -0400 Received: from mx1.redhat.com ([209.132.183.28]:58344 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933149AbdCaWSW (ORCPT ); Fri, 31 Mar 2017 18:18:22 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 69934344DE5 Authentication-Results: ext-mx04.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx04.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=jeyu@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 69934344DE5 Date: Fri, 31 Mar 2017 15:18:18 -0700 From: Jessica Yu To: Vaneet Narang Cc: Miroslav Benes , Michal Hocko , Maninder Singh , "rusty@rustcorp.com.au" , "akpm@linux-foundation.org" , "chris@chris-wilson.co.uk" , "aryabinin@virtuozzo.com" , "joonas.lahtinen@linux.intel.com" , "keescook@chromium.org" , "pavel@ucw.cz" , "jinb.park7@gmail.com" , "anisse@astier.eu" , "rafael.j.wysocki@intel.com" , "zijun_hu@htc.com" , "mingo@kernel.org" , "mawilcox@microsoft.com" , "thgarnie@google.com" , "joelaf@google.com" , "kirill.shutemov@linux.intel.com" , "linux-mm@kvack.org" , "linux-kernel@vger.kernel.org" , PANKAJ MISHRA , Ajeet Kumar Yadav , =?utf-8?B?7J207ZWZ67SJ?= , AMIT SAHRAWAT , =?utf-8?B?656E66a/?= , CPGS Subject: Re: [PATCH v2] module: check if memory leak by module. Message-ID: <20170331221818.jc5werfzszwbjwbh@jeyu> References: <1490767322-9914-1-git-send-email-maninder1.s@samsung.com> <20170329074522.GB27994@dhcp22.suse.cz> <20170329092332epcms5p10ae8263c6e3ef14eac40e08a09eff9e6@epcms5p1> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline In-Reply-To: <20170329092332epcms5p10ae8263c6e3ef14eac40e08a09eff9e6@epcms5p1> X-OS: Linux jeyu 4.8.12-100.fc23.x86_64 x86_64 User-Agent: NeoMutt/20161126 (1.7.1) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.28]); Fri, 31 Mar 2017 22:18:22 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5670 Lines: 118 +++ Vaneet Narang [29/03/17 09:23 +0000]: >Hi, > >>> Hmm, how can you track _all_ vmalloc allocations done on behalf of the >>> module? It is quite some time since I've checked kernel/module.c but >>> from my vague understading your check is basically only about statically >>> vmalloced areas by module loader. Is that correct? If yes then is this >>> actually useful? Were there any bugs in the loader code recently? What >>> led you to prepare this patch? All this should be part of the changelog! > >First of all there is no issue in kernel/module.c. This patch add functionality >to detect scenario where some kernel module does some memory allocation but gets >unloaded without doing vfree. For example >static int kernel_init(void) >{ > char * ptr = vmalloc(400 * 1024); > return 0; >} > >static void kernel_exit(void) >{ >} > >Now in this case if we do rmmod then memory allocated by kernel_init >will not be freed but this patch will detect such kind of bugs in kernel module >code. kmemleak already detects leaks just like this, and it is not just limited to vmalloc (but also kmalloc, kmem_cache_alloc, etc). See mm/kmemleak-test.c, it is exactly like your example. Also, this patch is currently limited to direct vmalloc allocations from module core code (since you are only checking for vmalloc callers originating from mod->core_layout, not mod->init_layout, which is discarded at the end of do_init_module(). If we want to be complete, we'd have to do another leak check before module init code is freed. >Also We have seen bugs in some kernel modules where they allocate some memory and >gets removed without freeing them and if new module gets loaded in place >of removed module then /proc/vmallocinfo shows wrong information. vmalloc info will >show pages getting allocated by new module. So these logs will help in detecting >such issues. This is an unfortunate side effect of having dynamically loadable modules. After a module is gone, sprint_symbol() (which is used to display caller information in /proc/vmallocinfo) simply cannot trace an address back to a module that no longer exists, it is a natural limitation, and I'm not really sure if there's much we can do about it. When chasing leaks like this, one possibility might be to leave the module loaded so vmallocinfo can report accurate information, and then compare the reported information after the module unloads. And unfortunately, this patch also demonstrates the same problem you're describing: (1) Load leaky_module and read /proc/vmallocinfo: 0xffffa8570005d000-0xffffa8570005f000 8192 leaky_function+0x2f/0x75 [leaky_module] pages=1 vmalloc N0=1 (2) Unload leaky_module and read /proc/vmallocinfo: 0xffffa8570005d000-0xffffa8570005f000 8192 0xffffffffc038902f pages=1 vmalloc N0=1 ^^^ missing caller symbol since module is now gone On module unload, your patch prints: [ 289.834428] Module [leaky_module] is getting unloaded before doing vfree [ 289.835226] Memory still allocated: addr:0xffffa8570005d000 - 0xffffa8570005f000, pages 1 [ 289.836185] Allocating function leaky_function+0x2f/0x75 [leaky_module] Ok, so far that looks fine. But kmemleak also provides information about the same leak: unreferenced object 0xffffa8570005d000 (size 64): comm "insmod", pid 114, jiffies 4294673713 (age 208.968s) hex dump (first 32 bytes): e6 7e 00 00 00 00 00 00 0a 00 00 00 16 00 00 00 .~.............. 21 52 00 00 00 00 00 00 f4 7e 00 00 00 00 00 00 !R.......~...... backtrace: [] kmemleak_alloc+0x4a/0xa0 [] __vmalloc_node_range+0x1e4/0x300 [] vmalloc+0x54/0x60 [] leaky_function+0x2f/0x75 [leaky_module] [] 0xffffffffc038e00b [] do_one_initcall+0x53/0x1a0 [] do_init_module+0x5f/0x1ff [] load_module+0x273f/0x2b00 [] SYSC_init_module+0x166/0x180 [] SyS_init_module+0xe/0x10 [] entry_SYSCALL_64_fastpath+0x1a/0xa9 [] 0xffffffffffffffff (3) Load test_module, which happens to load where leaky_module used to reside in memory: 0xffffa8570005d000-0xffffa8570005f000 8192 test_module_exit+0x2f/0x1000 [test_module] pages=1 vmalloc N0=1 ^^^ incorrect caller, because test_module loaded where old caller used to be (4) Unload test_module and your patch prints: [ 459.140089] Module [test_module] is getting unloaded before doing vfree [ 459.140551] Memory still allocated: addr:0xffffa8570005d000 - 0xffffa8570005f000, pages 1 [ 459.141127] Allocating function test_module_exit+0x2f/0x1000 [test_module] <- incorrect caller So unfortunately this patch also runs into the same problem, reporting the incorrect caller, and I'm not really convinced that this patch adds new information that isn't already available with kmemleak and /proc/vmallocinfo. Jessica >> > static void free_module(struct module *mod) >> > { >> > + check_memory_leak(mod); >> > + > >>Of course, vfree() has not been called yet. It is the beginning of >>free_module(). vfree() is one of the last things you need to do. See >>module_memfree(). If I am not missing something, you get pr_err() >>everytime a module is unloaded. > >This patch is not to detect memory allocated by kernel. module_memfree >will allocated by kernel for kernel modules but our intent is to detect >memory allocated directly by kernel modules and not getting freed. > >Regards, >Vaneet Narang