netdev - Re: Question about memory leak detector giving false positive report for net/core/flow.c

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1317066395.2796.11.camel@edumazet-laptop>
Date:	Mon, 26 Sep 2011 21:46:35 +0200
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	Catalin Marinas <Catalin.Marinas@....com>
Cc:	Huajun Li <huajun.li.lee@...il.com>,
	"linux-mm@...ck.org" <linux-mm@...ck.org>,
	netdev <netdev@...r.kernel.org>,
	linux-kernel <linux-kernel@...r.kernel.org>,
	Tejun Heo <tj@...nel.org>,
	Christoph Lameter <cl@...ux-foundation.org>
Subject: Re: Question about memory leak detector giving false positive
 report for net/core/flow.c

Le lundi 26 septembre 2011 à 17:50 +0100, Catalin Marinas a écrit :
> On Mon, Sep 26, 2011 at 05:32:54PM +0100, Eric Dumazet wrote:
> > Le lundi 26 septembre 2011 à 23:17 +0800, Huajun Li a écrit :
> > > Memory leak detector gives following memory leak report, it seems the
> > > report is triggered by net/core/flow.c, but actually, it should be a
> > > false positive report.
> > > So, is there any idea from kmemleak side to fix/disable this false
> > > positive report like this?
> > > Yes, kmemleak_not_leak(...) could disable it, but is it suitable for this case ?
> ...
> > CC lkml and percpu maintainers (Tejun Heo & Christoph Lameter ) as well
> > 
> > AFAIK this false positive only occurs if percpu data is allocated
> > outside of embedded pcu space. 
> > 
> >  (grep pcpu_get_vm_areas /proc/vmallocinfo)
> > 
> > I suspect this is a percpu/kmemleak cooperation problem (a missing
> > kmemleak_alloc() ?)
> > 
> > I am pretty sure kmemleak_not_leak() is not the right answer to this
> > problem.
> 
> kmemleak_not_leak() definitely not the write answer. The alloc_percpu()
> call does not have any kmemleak_alloc() callback, so it doesn't scan
> them.
> 
> Huajun, could you please try the patch below:
> 
> 8<--------------------------------
> kmemleak: Handle percpu memory allocation
> 
> From: Catalin Marinas <catalin.marinas@....com>
> 
> This patch adds kmemleak callbacks from the percpu allocator, reducing a
> number of false positives caused by kmemleak not scanning such memory
> blocks.
> 
> Reported-by: Huajun Li <huajun.li.lee@...il.com>
> Signed-off-by: Catalin Marinas <catalin.marinas@....com>
> ---
>  mm/percpu.c |   11 +++++++++--
>  1 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/percpu.c b/mm/percpu.c
> index bf80e55..c47a90b 100644
> --- a/mm/percpu.c
> +++ b/mm/percpu.c
> @@ -67,6 +67,7 @@
>  #include <linux/spinlock.h>
>  #include <linux/vmalloc.h>
>  #include <linux/workqueue.h>
> +#include <linux/kmemleak.h>
>  
>  #include <asm/cacheflush.h>
>  #include <asm/sections.h>
> @@ -833,7 +834,9 @@ fail_unlock_mutex:
>   */
>  void __percpu *__alloc_percpu(size_t size, size_t align)
>  {
> -	return pcpu_alloc(size, align, false);
> +	void __percpu *ptr = pcpu_alloc(size, align, false);
> +	kmemleak_alloc(ptr, size, 1, GFP_KERNEL);
> +	return ptr;
>  }
>  EXPORT_SYMBOL_GPL(__alloc_percpu);
>  
> @@ -855,7 +858,9 @@ EXPORT_SYMBOL_GPL(__alloc_percpu);
>   */
>  void __percpu *__alloc_reserved_percpu(size_t size, size_t align)
>  {
> -	return pcpu_alloc(size, align, true);
> +	void __percpu *ptr = pcpu_alloc(size, align, true);
> +	kmemleak_alloc(ptr, size, 1, GFP_KERNEL);
> +	return ptr;
>  }
>  
>  /**
> @@ -915,6 +920,8 @@ void free_percpu(void __percpu *ptr)
>  	if (!ptr)
>  		return;
>  
> +	kmemleak_free(ptr);
> +
>  	addr = __pcpu_ptr_to_addr(ptr);
>  
>  	spin_lock_irqsave(&pcpu_lock, flags);
> 

Hmm, you need to call kmemleak_alloc() for each chunk allocated per
possible cpu.

Here is the (untested) patch for the allocation phase, need the same at
freeing time

diff --git a/mm/percpu-km.c b/mm/percpu-km.c
index 89633fe..5061ac5 100644
--- a/mm/percpu-km.c
+++ b/mm/percpu-km.c
@@ -37,9 +37,12 @@ static int pcpu_populate_chunk(struct pcpu_chunk *chunk, int off, int size)
 {
 	unsigned int cpu;
 
-	for_each_possible_cpu(cpu)
-		memset((void *)pcpu_chunk_addr(chunk, cpu, 0) + off, 0, size);
+	for_each_possible_cpu(cpu) {
+		void *chunk_addr = (void *)pcpu_chunk_addr(chunk, cpu, 0) + off;
 
+		kmemleak_alloc(chunk_addr, size, 1, GFP_KERNEL);
+		memset(chunk_addr, 0, size);
+	}
 	return 0;
 }
 
diff --git a/mm/percpu-vm.c b/mm/percpu-vm.c
index ea53496..0d397cc 100644
--- a/mm/percpu-vm.c
+++ b/mm/percpu-vm.c
@@ -342,8 +342,12 @@ static int pcpu_populate_chunk(struct pcpu_chunk *chunk, int off, int size)
 	/* commit new bitmap */
 	bitmap_copy(chunk->populated, populated, pcpu_unit_pages);
 clear:
-	for_each_possible_cpu(cpu)
-		memset((void *)pcpu_chunk_addr(chunk, cpu, 0) + off, 0, size);
+	for_each_possible_cpu(cpu) {
+		void *chunk_addr = (void *)pcpu_chunk_addr(chunk, cpu, 0) + off;
+
+		kmemleak_alloc(chunk_addr, size, 1, GFP_KERNEL);
+		memset(chunk_addr, 0, size);
+	}
 	return 0;
 
 err_unmap:



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html