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

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

[Eric Dumazet]

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 ?
> 
> BTW, I wrote a simple test code to emulate net/core/flow.c behavior at
> this stage which triggers the report, and it could also make kmemleak
> give similar report, please check below test code:
> 
> kernel version:
> #uname -a
> Linux 3.1.0-rc7 #22 SMP Tue Sep 26 05:43:01 CST 2011 x86_64 x86_64
> x86_64 GNU/Linux
> 
> memory leak report:
> -------------------------------------------------------------------------------------------
> unreferenced object 0xffff880073a70000 (size 8192):
>   comm "swapper", pid 1, jiffies 4294937832 (age 445.740s)
>   hex dump (first 32 bytes):
>     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>   backtrace:
>     [<ffffffff8124db64>] create_object+0x144/0x360
>     [<ffffffff8191192e>] kmemleak_alloc+0x7e/0x110
>     [<ffffffff81235b26>] __kmalloc_node+0x156/0x3a0
>     [<ffffffff81935512>] flow_cache_cpu_prepare.clone.1+0x58/0xc0
>     [<ffffffff8214c361>] flow_cache_init_global+0xb6/0x1af
>     [<ffffffff8100225d>] do_one_initcall+0x4d/0x260
>     [<ffffffff820ec2e9>] kernel_init+0x161/0x23a
>     [<ffffffff8194ab04>] kernel_thread_helper+0x4/0x10
>     [<ffffffffffffffff>] 0xffffffffffffffff
> unreferenced object 0xffff880073a74290 (size 8192):
>   comm "swapper", pid 1, jiffies 4294937832 (age 445.740s)
>   hex dump (first 32 bytes):
>     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>   backtrace:
>     [<ffffffff8124db64>] create_object+0x144/0x360
>     [<ffffffff8191192e>] kmemleak_alloc+0x7e/0x110
>     [<ffffffff81235b26>] __kmalloc_node+0x156/0x3a0
>     [<ffffffff81935512>] flow_cache_cpu_prepare.clone.1+0x58/0xc0
>     [<ffffffff8214c361>] flow_cache_init_global+0xb6/0x1af
>     [<ffffffff8100225d>] do_one_initcall+0x4d/0x260
>     [<ffffffff820ec2e9>] kernel_init+0x161/0x23a
>     [<ffffffff8194ab04>] kernel_thread_helper+0x4/0x10
>     [<ffffffffffffffff>] 0xffffffffffffffff
> 
> 
> 
> Simple test code to reproduce a similar report:
> -----------------------------------------------------------------------------------------
> MODULE_LICENSE("GPL");
> 
> struct test {
>         int *pt;


	char spaceholder[30000];

> };
> 
> static struct test __percpu *percpu;
> 
> static int __init test_init(void)
> {
>         int i;
> 
>         percpu = alloc_percpu(struct test);
>         if (!percpu)
>                 return -ENOMEM;
> 
>         for_each_online_cpu(i) {
>                 struct test *p = per_cpu_ptr(percpu, i);
>                 p->pt = kmalloc(sizeof(int), GFP_KERNEL);
>         }
> 
>         return 0;
> }
> 
> static void __exit test_exit(void)
> {
>         int i;
> 
>         for_each_possible_cpu(i) {
>                 struct test *p = per_cpu_ptr(percpu, i);
>                 if (p->pt)
>                         kfree(p->pt);
>         }
> 
>         if (percpu)
>                 free_percpu(percpu);
> }
> module_init(test_init);
> module_exit(test_exit);


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.

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

[Catalin Marinas]

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@arm.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@gmail.com>
Signed-off-by: Catalin Marinas <catalin.marinas@arm.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);

-- 
Catalin

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

[Eric Dumazet]

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@arm.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@gmail.com>
> Signed-off-by: Catalin Marinas <catalin.marinas@arm.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:

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

[Huajun Li]

2011/9/27 Eric Dumazet <eric.dumazet@gmail.com>:
> 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@arm.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@gmail.com>
>> Signed-off-by: Catalin Marinas <catalin.marinas@arm.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:
>
>

About this one,  memory leak detector disabled(actually I enable it
while config the kernel) while booting system, and and found following
info in dmesg:
---------------------------------------------------------------------------------------------------------------
[    0.370000] smpboot cpu 1: start_ip = 9a000
[    0.010000] numa_add_cpu cpu 1 node 0: mask now 0-1
[    0.530124] NMI watchdog enabled, takes one hw-pmu counter.
[    0.530344] Brought up 2 CPUs
[    0.530348] Total of 2 processors activated (10639.45 BogoMIPS).
[    0.533083] kmemleak: Cannot insert 0xffff88007a3d7448 into the
object search tree (already existing)
[    0.533083] Pid: 1, comm: swapper Tainted: G          I 3.1.0-rc7+ #25
[    0.533083] Call Trace:
[    0.533083]  [<ffffffff81256512>] create_object+0x2c2/0x360
[    0.533083]  [<ffffffff81920c5e>] kmemleak_alloc+0x7e/0x110
[    0.533083]  [<ffffffff811fd8cd>] pcpu_alloc+0xd2d/0x10f0
[    0.533083]  [<ffffffff81243129>] ? __kmalloc_track_caller+0x139/0x310
[    0.533083]  [<ffffffff811fdca8>] __alloc_percpu+0x18/0x30
[    0.533083]  [<ffffffff8128dded>] alloc_vfsmnt+0x11d/0x250
[    0.533083]  [<ffffffff8128dfd6>] vfs_kern_mount+0x46/0x130
[    0.533083]  [<ffffffff8194a283>] ? _raw_write_unlock+0x43/0x60
[    0.533083]  [<ffffffff82117cec>] shmem_init+0xb9/0x13b
[    0.533083]  [<ffffffff820ea2c6>] kernel_init+0x10e/0x23a
[    0.533083]  [<ffffffff8195a484>] kernel_thread_helper+0x4/0x10
[    0.533083]  [<ffffffff8194ac20>] ? _raw_spin_unlock_irq+0x50/0x80
[    0.533083]  [<ffffffff8194b834>] ? retint_restore_args+0x13/0x13
[    0.533083]  [<ffffffff820ea1b8>] ? start_kernel+0x6ce/0x6ce
[    0.533083]  [<ffffffff8195a480>] ? gs_change+0x13/0x13
[    0.533083] kmemleak: Kernel memory leak detector disabled
[    0.533083] kmemleak: Object 0xffff88007a200000 (size 1949696):
[    0.533083] kmemleak:   comm "swapper", pid 0, jiffies 4294937296
[    0.533083] kmemleak:   min_count = 0
[    0.533083] kmemleak:   count = 0
[    0.533083] kmemleak:   flags = 0x1
[    0.533083] kmemleak:   checksum = 0
[    0.533083] kmemleak:   backtrace:
[    0.533083]      [<ffffffff81256394>] create_object+0x144/0x360
[    0.533083]      [<ffffffff819214c8>] kmemleak_free_part+0x138/0x1d0
[    0.533083]      [<ffffffff8211e4e8>] kmemleak_init+0x30b/0x3fd
[    0.533083]      [<ffffffff820ea01c>] start_kernel+0x532/0x6ce
[    0.533083]      [<ffffffff820e938d>] x86_64_start_reservations+0x178/0x183
[    0.533083]      [<ffffffff820e94fb>] x86_64_start_kernel+0x163/0x179
[    0.533083]      [<ffffffffffffffff>] 0xffffffffffffffff
[    0.533083] devtmpfs: initialized
[    0.533083] gcov: version magic: 0x3430352a
[    0.533083] PM: Registering ACPI NVS region at 7d2afe00 (8352 bytes)
[    0.533083] print_constraints: dummy:
[    0.533083] RTC time: 12:40:24, date: 09/27/11
[    0.533083] NET: Registered protocol family 16

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

[Eric Dumazet]

Le mardi 27 septembre 2011 à 13:29 +0800, Huajun Li a écrit :
> 2011/9/27 Eric Dumazet <eric.dumazet@gmail.com>:
> > 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@arm.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@gmail.com>
> >> Signed-off-by: Catalin Marinas <catalin.marinas@arm.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:
> >
> >
> 
> About this one,  memory leak detector disabled(actually I enable it
> while config the kernel) while booting system, and and found following
> info in dmesg:

Yes, it was not a patch, but the general idea for Catalin ;)

You hit the fact that same zone (embedded percpu space) is now in a
mixed state.

In current kernels, the embedded percpu zone is already known by
kmemleak, but with a large granularity. kmemleak is not aware of
individual allocations/freeing in this large zone.

Once kmemleak and percpu allocator are cooperating, we might find more
kmemleaks. Right now, kmemleak can find pointers in percpu chunks that
are not anymore reachable (they were freed), and therefore doesnt warn
of possible memory leaks.

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

[Catalin Marinas]

On Tue, Sep 27, 2011 at 06:55:18AM +0100, Eric Dumazet wrote:
> Yes, it was not a patch, but the general idea for Catalin ;)
> 
> You hit the fact that same zone (embedded percpu space) is now in a
> mixed state.
> 
> In current kernels, the embedded percpu zone is already known by
> kmemleak, but with a large granularity. kmemleak is not aware of
> individual allocations/freeing in this large zone.
> 
> Once kmemleak and percpu allocator are cooperating, we might find more
> kmemleaks. Right now, kmemleak can find pointers in percpu chunks that
> are not anymore reachable (they were freed), and therefore doesnt warn
> of possible memory leaks.

Thanks for suggestions. I need to understand the percpu code a bit
better as it looks that kmemleak is told about some memory blocks twice.

-- 
Catalin

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

[Catalin Marinas]

On Tue, Sep 27, 2011 at 06:55:18AM +0100, Eric Dumazet wrote:
> Yes, it was not a patch, but the general idea for Catalin ;)
> 
> You hit the fact that same zone (embedded percpu space) is now in a
> mixed state.
> 
> In current kernels, the embedded percpu zone is already known by
> kmemleak, but with a large granularity. kmemleak is not aware of
> individual allocations/freeing in this large zone.

It looks like this comes via the bootmem allocator. Maybe we could
simply call kmemleak_free() on the embedded percpu space and just track
those via the standard percpu API.

-- 
Catalin

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

[Catalin Marinas]

On Mon, Sep 26, 2011 at 08:46:35PM +0100, Eric Dumazet wrote:
> Le lundi 26 septembre 2011 à 17:50 +0100, Catalin Marinas a écrit :
> > 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:
...
> Hmm, you need to call kmemleak_alloc() for each chunk allocated per
> possible cpu.

I tried this but it's tricky. The problem is that the percpu pointer
returned by alloc_percpu() does not directly point to the per-cpu chunks
and kmemleak would report most percpu allocations as leaks. So far the
workaround is to simply mark the alloc_percpu() objects as never leaking
and at least we avoid false positives in other areas. See the patch
below (note that you have to increase the CONFIG_KMEMLEAK_EARLY_LOG_SIZE
as there are many alloc_percpu() calls before kmemleak is fully
initialised):

------------8<------------------------------------

kmemleak: Handle percpu memory allocation

From: Catalin Marinas <catalin.marinas@arm.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@gmail.com>
Cc: Tejun Heo <tj@kernel.org>,
Cc: Christoph Lameter <cl@linux-foundation.org>
Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
---
 mm/percpu.c |   22 +++++++++++++++++++++-
 1 files changed, 21 insertions(+), 1 deletions(-)

diff --git a/mm/percpu.c b/mm/percpu.c
index bf80e55..ece9f85 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>
@@ -709,6 +710,8 @@ static void __percpu *pcpu_alloc(size_t size, size_t align, bool reserved)
 	const char *err;
 	int slot, off, new_alloc;
 	unsigned long flags;
+	void __percpu *ptr;
+	unsigned int cpu;
 
 	if (unlikely(!size || size > PCPU_MIN_UNIT_SIZE || align > PAGE_SIZE)) {
 		WARN(true, "illegal size (%zu) or align (%zu) for "
@@ -801,7 +804,16 @@ area_found:
 	mutex_unlock(&pcpu_alloc_mutex);
 
 	/* return address relative to base address */
-	return __addr_to_pcpu_ptr(chunk->base_addr + off);
+	ptr = __addr_to_pcpu_ptr(chunk->base_addr + off);
+
+	/*
+	 * Percpu allocations are currently reported as leaks (kmemleak false
+	 * positives). To avoid this, just set min_count to 0.
+	 */
+	for_each_possible_cpu(cpu)
+		kmemleak_alloc(per_cpu_ptr(ptr, cpu), size, 0, GFP_KERNEL);
+
+	return ptr;
 
 fail_unlock:
 	spin_unlock_irqrestore(&pcpu_lock, flags);
@@ -911,10 +923,14 @@ void free_percpu(void __percpu *ptr)
 	struct pcpu_chunk *chunk;
 	unsigned long flags;
 	int off;
+	unsigned int cpu;
 
 	if (!ptr)
 		return;
 
+	for_each_possible_cpu(cpu)
+		kmemleak_free(per_cpu_ptr(ptr, cpu));
+
 	addr = __pcpu_ptr_to_addr(ptr);
 
 	spin_lock_irqsave(&pcpu_lock, flags);
@@ -1619,6 +1635,8 @@ int __init pcpu_embed_first_chunk(size_t reserved_size, size_t dyn_size,
 			rc = -ENOMEM;
 			goto out_free_areas;
 		}
+		/* kmemleak tracks the percpu allocations separately */
+		kmemleak_free(ptr);
 		areas[group] = ptr;
 
 		base = min(ptr, base);
@@ -1733,6 +1751,8 @@ int __init pcpu_page_first_chunk(size_t reserved_size,
 					   "for cpu%u\n", psize_str, cpu);
 				goto enomem;
 			}
+			/* kmemleak tracks the percpu allocations separately */
+			kmemleak_free(ptr);
 			pages[j++] = virt_to_page(ptr);
 		}
 

-- 
Catalin

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

[Christoph Lameter]

On Wed, 28 Sep 2011, Catalin Marinas wrote:

> I tried this but it's tricky. The problem is that the percpu pointer
> returned by alloc_percpu() does not directly point to the per-cpu chunks
> and kmemleak would report most percpu allocations as leaks. So far the
> workaround is to simply mark the alloc_percpu() objects as never leaking
> and at least we avoid false positives in other areas. See the patch
> below (note that you have to increase the CONFIG_KMEMLEAK_EARLY_LOG_SIZE
> as there are many alloc_percpu() calls before kmemleak is fully
> initialised):

Seems that kernel.org is out and so tejon wont be seeing these.

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

[Catalin Marinas]

On Thu, Sep 29, 2011 at 03:08:47PM +0100, Christoph Lameter wrote:
> On Wed, 28 Sep 2011, Catalin Marinas wrote:
> 
> > I tried this but it's tricky. The problem is that the percpu pointer
> > returned by alloc_percpu() does not directly point to the per-cpu chunks
> > and kmemleak would report most percpu allocations as leaks. So far the
> > workaround is to simply mark the alloc_percpu() objects as never leaking
> > and at least we avoid false positives in other areas. See the patch
> > below (note that you have to increase the CONFIG_KMEMLEAK_EARLY_LOG_SIZE
> > as there are many alloc_percpu() calls before kmemleak is fully
> > initialised):
> 
> Seems that kernel.org is out and so tejon wont be seeing these.

That's ok, I don't aim this at the upcoming merging window. I don't have
an alternative email address for him.

-- 
Catalin

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

[Tejun Heo]

On Thu, Sep 29, 2011 at 03:18:10PM +0100, Catalin Marinas wrote:
> On Thu, Sep 29, 2011 at 03:08:47PM +0100, Christoph Lameter wrote:
> > On Wed, 28 Sep 2011, Catalin Marinas wrote:
> > 
> > > I tried this but it's tricky. The problem is that the percpu pointer
> > > returned by alloc_percpu() does not directly point to the per-cpu chunks
> > > and kmemleak would report most percpu allocations as leaks. So far the
> > > workaround is to simply mark the alloc_percpu() objects as never leaking
> > > and at least we avoid false positives in other areas. See the patch
> > > below (note that you have to increase the CONFIG_KMEMLEAK_EARLY_LOG_SIZE
> > > as there are many alloc_percpu() calls before kmemleak is fully
> > > initialised):
> > 
> > Seems that kernel.org is out and so tejon wont be seeing these.
> 
> That's ok, I don't aim this at the upcoming merging window. I don't have
> an alternative email address for him.

That's htejun@gmail.com but as long as lkml is cc'd I can read the
emails.  Lack of replies is more due to the month long vacation which
just ended. ;)

Thank you.

-- 
tejun

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

[Huajun Li]

2011/9/27 Catalin Marinas <Catalin.Marinas@arm.com>:
> 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@arm.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@gmail.com>
> Signed-off-by: Catalin Marinas <catalin.marinas@arm.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);
>
> --
> Catalin
>

Applied the patch and found following msg after system boot up :
--------------------------------------------------------------------------------------------------------
...
[   48.040027] eth0: no IPv6 routers present
[   69.240960] BUG: unable to handle kernel paging request at 00000000001d7270
[   69.241010] IP: [<ffffffff81256c62>] scan_block+0x62/0x220
[   69.241045] PGD 5f289067 PUD 5f292067 PMD 0
[   69.241074] Oops: 0000 [#1] SMP
[   69.241098] CPU 0
[   69.241110] Modules linked in: binfmt_misc dm_crypt
snd_hda_codec_analog snd_hda_intel snd_hda_codec usbhid hid snd_hwdep
snd_pcm snd_seq_midi snd_rawmidi i915 snd_seq_midi_event hp_wmi ppdev
sparse_keymap snd_seq snd_timer snd_seq_device ehci_hcd uhci_hcd
drm_kms_helper usbcore tpm_infineon psmouse snd e1000e drm serio_raw
soundcore tpm_tis snd_page_alloc tpm floppy tpm_bios parport_pc
i2c_algo_bit video lp parport
[   69.249303]
[   69.250246] Pid: 47, comm: kmemleak Tainted: G          I
3.1.0-rc7+ #24 Hewlett-Packard HP Compaq dc7800p Convertible
Minitower/0AACh
[   69.250246] RIP: 0010:[<ffffffff81256c62>]  [<ffffffff81256c62>]
scan_block+0x62/0x220
[   69.250246] RSP: 0018:ffff880073f69d90  EFLAGS: 00010012
[   69.250246] RAX: 0000000000000000 RBX: ffff880078750858 RCX: 0000000000000000
[   69.250246] RDX: ffff880078750858 RSI: 0000000000000001 RDI: 00000000001d7270
[   69.250246] RBP: ffff880073f69dd0 R08: 0000000000000000 R09: 0000000000000000
[   69.250246] R10: 0000000000000001 R11: 0000000000000000 R12: ffff8800787508b8
[   69.250246] R13: 00000000001d7270 R14: 00000000001d7431 R15: 0000000000000000
[   69.250246] FS:  0000000000000000(0000) GS:ffff88007a200000(0000)
knlGS:0000000000000000
[   69.250246] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[   69.250246] CR2: 00000000001d7270 CR3: 000000006d735000 CR4: 00000000000006f0
[   69.250246] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[   69.250246] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
[   69.250246] Process kmemleak (pid: 47, threadinfo ffff880073f68000,
task ffff880073e4a460)
[   69.250246] Stack:
[   69.250246]  ffff880073f69dd0 ffffffff8194b49a ffffffff81256ed1
ffff880078750858
[   69.250246]  ffff8800787508b8 00000000001d8270 00000000001d7438
0000000000000206
[   69.250246]  ffff880073f69e10 ffffffff8125702a 000000000007d2af
ffffffff81e0d120
[   69.250246] Call Trace:
[   69.250246]  [<ffffffff8194b49a>] ? _raw_spin_lock_irqsave+0xfa/0x110
[   69.250246]  [<ffffffff81256ed1>] ? scan_gray_list+0xb1/0x280
[   69.250246]  [<ffffffff8125702a>] scan_gray_list+0x20a/0x280
[   69.250246]  [<ffffffff8125787a>] kmemleak_scan+0x44a/0xbd0
[   69.250246]  [<ffffffff81257430>] ? kmemleak_do_cleanup+0x210/0x210
[   69.250246]  [<ffffffff81258630>] ? kmemleak_write+0x630/0x630
[   69.250246]  [<ffffffff812586bf>] kmemleak_scan_thread+0x8f/0x150
[   69.250246]  [<ffffffff810dc4e6>] kthread+0xe6/0x100
[   69.250246]  [<ffffffff8195a4c4>] kernel_thread_helper+0x4/0x10
[   69.250246]  [<ffffffff8194ac50>] ? _raw_spin_unlock_irq+0x50/0x80
[   69.250246]  [<ffffffff8194b874>] ? retint_restore_args+0x13/0x13
[   69.250246]  [<ffffffff810dc400>] ? __init_kthread_worker+0x80/0x80
[   69.250246]  [<ffffffff8195a4c0>] ? gs_change+0x13/0x13
[   69.250246] Code: 00 00 e9 d2 01 00 00 66 90 e8 ab f4 ff ff 48 83
05 33 ef 06 02 01 85 c0 0f 85 bb 01 00 00 48 83 05 2b ef 06 02 01 be
01 00 00 00
[   69.250246]  8b 7d 00 e8 c5 fc ff ff 48 85 c0 49 89 c4 0f 84 69 01 00 00
[   69.250246] RIP  [<ffffffff81256c62>] scan_block+0x62/0x220
[   69.250246]  RSP <ffff880073f69d90>
[   69.250246] CR2: 00000000001d7270
[   69.360917] ---[ end trace a7919e7f17c0a727 ]---