[PATCH] fork: fix kmemleak false positive due to thread stacks caching

[PATCH] fork: fix kmemleak false positive due to thread stacks caching

[Luis Henriques]

kmemleak has been reporting memory leaks since commit ac496bf48d97 ("fork:
Optimize task creation by caching two thread stacks per CPU if
CONFIG_VMAP_STACK=y"):

unreferenced object 0xffffc900002b0000 (size 16384):
  comm "init", pid 147, jiffies 4294893306 (age 11.292s)
  hex dump (first 32 bytes):
    9d 6e ac 57 00 00 00 00 00 00 00 00 00 00 00 00  .n.W............
    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
  backtrace:
    [<ffffffff815b481e>] kmemleak_alloc+0x4e/0xb0
    [<ffffffff8112a8b0>] __vmalloc_node_range+0x160/0x240
    [<ffffffff8104a328>] copy_process.part.8+0x478/0x1630
    [<ffffffff8104b69a>] _do_fork+0xca/0x330
    [<ffffffff8104b9a9>] SyS_clone+0x19/0x20
    [<ffffffff8100199c>] do_syscall_64+0x4c/0xb0
    [<ffffffff815b8d06>] return_from_SYSCALL_64+0x0/0x6a
    [<ffffffffffffffff>] 0xffffffffffffffff

This is because this commit started caching 2 thread stacks per CPU, and
kmemleak assumes its memory is never freed.  Report these stacks as not
being memory leaks using kmemleak_not_leak().

Cc: stable@vger.kernel.org
Fixes: ac496bf48d97 ("fork: Optimize task creation by caching two thread stacks per CPU if CONFIG_VMAP_STACK=y")
Signed-off-by: Luis Henriques <lhenriques@suse.com>
---
 kernel/fork.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/fork.c b/kernel/fork.c
index aa1076c5e4a9..c4d79ad0f5bc 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -255,6 +255,7 @@ static inline void free_thread_stack(struct task_struct *tsk)
 
 			this_cpu_write(cached_stacks[i], tsk->stack_vm_area);
 			local_irq_restore(flags);
+			kmemleak_not_leak(tsk->stack);
 			return;
 		}
 		local_irq_restore(flags);

Re: [PATCH] fork: fix kmemleak false positive due to thread stacks caching

[Catalin Marinas]

On Fri, May 26, 2017 at 02:49:49PM +0100, Luis Henriques wrote:
> kmemleak has been reporting memory leaks since commit ac496bf48d97 ("fork:
> Optimize task creation by caching two thread stacks per CPU if
> CONFIG_VMAP_STACK=y"):
> 
> unreferenced object 0xffffc900002b0000 (size 16384):
>   comm "init", pid 147, jiffies 4294893306 (age 11.292s)
>   hex dump (first 32 bytes):
>     9d 6e ac 57 00 00 00 00 00 00 00 00 00 00 00 00  .n.W............
>     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>   backtrace:
>     [<ffffffff815b481e>] kmemleak_alloc+0x4e/0xb0
>     [<ffffffff8112a8b0>] __vmalloc_node_range+0x160/0x240
>     [<ffffffff8104a328>] copy_process.part.8+0x478/0x1630
>     [<ffffffff8104b69a>] _do_fork+0xca/0x330
>     [<ffffffff8104b9a9>] SyS_clone+0x19/0x20
>     [<ffffffff8100199c>] do_syscall_64+0x4c/0xb0
>     [<ffffffff815b8d06>] return_from_SYSCALL_64+0x0/0x6a
>     [<ffffffffffffffff>] 0xffffffffffffffff
> 
> This is because this commit started caching 2 thread stacks per CPU, and
> kmemleak assumes its memory is never freed.  Report these stacks as not
> being memory leaks using kmemleak_not_leak().
> 
> Cc: stable@vger.kernel.org
> Fixes: ac496bf48d97 ("fork: Optimize task creation by caching two thread stacks per CPU if CONFIG_VMAP_STACK=y")
> Signed-off-by: Luis Henriques <lhenriques@suse.com>

I guess that's meant as a 4.12 and earlier fix until the
kmemleak_vmalloc() patches go in
(http://lkml.kernel.org/r/1495726937-23557-1-git-send-email-catalin.marinas@arm.com)

> diff --git a/kernel/fork.c b/kernel/fork.c
> index aa1076c5e4a9..c4d79ad0f5bc 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -255,6 +255,7 @@ static inline void free_thread_stack(struct task_struct *tsk)
>  
>  			this_cpu_write(cached_stacks[i], tsk->stack_vm_area);
>  			local_irq_restore(flags);
> +			kmemleak_not_leak(tsk->stack);

I would add a comment here on why the annotation is needed.

The disadvantage is that such objects would no longer be detected as
leaks since kmemleak doesn't have a way to "unignore" an object (I have
a patch but not worth merging since we'll fix the above with a dedicated
kmemleak_vmalloc() API). We could however work around this with the
current kmemleak API as in this patch:

http://lkml.kernel.org/r/20170516133925.GA9453@e104818-lin.cambridge.arm.com

Thanks.

-- 
Catalin

Re: [PATCH] fork: fix kmemleak false positive due to thread stacks caching

[Luis Henriques]

On Fri, May 26, 2017 at 03:22:54PM +0100, Catalin Marinas wrote:
> On Fri, May 26, 2017 at 02:49:49PM +0100, Luis Henriques wrote:
> > kmemleak has been reporting memory leaks since commit ac496bf48d97 ("fork:
> > Optimize task creation by caching two thread stacks per CPU if
> > CONFIG_VMAP_STACK=y"):
> > 
> > unreferenced object 0xffffc900002b0000 (size 16384):
> >   comm "init", pid 147, jiffies 4294893306 (age 11.292s)
> >   hex dump (first 32 bytes):
> >     9d 6e ac 57 00 00 00 00 00 00 00 00 00 00 00 00  .n.W............
> >     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
> >   backtrace:
> >     [<ffffffff815b481e>] kmemleak_alloc+0x4e/0xb0
> >     [<ffffffff8112a8b0>] __vmalloc_node_range+0x160/0x240
> >     [<ffffffff8104a328>] copy_process.part.8+0x478/0x1630
> >     [<ffffffff8104b69a>] _do_fork+0xca/0x330
> >     [<ffffffff8104b9a9>] SyS_clone+0x19/0x20
> >     [<ffffffff8100199c>] do_syscall_64+0x4c/0xb0
> >     [<ffffffff815b8d06>] return_from_SYSCALL_64+0x0/0x6a
> >     [<ffffffffffffffff>] 0xffffffffffffffff
> > 
> > This is because this commit started caching 2 thread stacks per CPU, and
> > kmemleak assumes its memory is never freed.  Report these stacks as not
> > being memory leaks using kmemleak_not_leak().
> > 
> > Cc: stable@vger.kernel.org
> > Fixes: ac496bf48d97 ("fork: Optimize task creation by caching two thread stacks per CPU if CONFIG_VMAP_STACK=y")
> > Signed-off-by: Luis Henriques <lhenriques@suse.com>
> 
> I guess that's meant as a 4.12 and earlier fix until the
> kmemleak_vmalloc() patches go in
> (http://lkml.kernel.org/r/1495726937-23557-1-git-send-email-catalin.marinas@arm.com)
> 
> > diff --git a/kernel/fork.c b/kernel/fork.c
> > index aa1076c5e4a9..c4d79ad0f5bc 100644
> > --- a/kernel/fork.c
> > +++ b/kernel/fork.c
> > @@ -255,6 +255,7 @@ static inline void free_thread_stack(struct task_struct *tsk)
> >  
> >  			this_cpu_write(cached_stacks[i], tsk->stack_vm_area);
> >  			local_irq_restore(flags);
> > +			kmemleak_not_leak(tsk->stack);
> 
> I would add a comment here on why the annotation is needed.
> 
> The disadvantage is that such objects would no longer be detected as
> leaks since kmemleak doesn't have a way to "unignore" an object (I have
> a patch but not worth merging since we'll fix the above with a dedicated
> kmemleak_vmalloc() API). We could however work around this with the
> current kmemleak API as in this patch:
> 
> http://lkml.kernel.org/r/20170516133925.GA9453@e104818-lin.cambridge.arm.com

Ok, looks like I need to improve my lkml search-fu -- I totally missed
this discussion and the patchset with the new API!

So, I guess it only makes sense to work around this issue with the current
API when backporting the fix to stable kernels, in case you think it's
worth fixing this in older kernels, of course.  (Or maybe if this new API
doesn't make it into 4.12...)

Anyway, thanks for pointing me at this patchset Catalin.  I'll have a look
at it.

Cheers,
--
Lu�s