* kmemleak not always catching stuff
@ 2017-09-01 22:33 Steven Rostedt
2017-09-02 10:35 ` Dmitry Vyukov
2017-09-04 10:09 ` Catalin Marinas
0 siblings, 2 replies; 8+ messages in thread
From: Steven Rostedt @ 2017-09-01 22:33 UTC (permalink / raw)
To: LKML; +Cc: Catalin Marinas, Andrey Ryabinin, kasan-dev
Hi,
Recently kmemleak discovered a bug in my code where an allocated
trampoline for a ftrace function tracer wasn't freed due to an exit
path. The thing is, kmemleak was able to catch this 100% when it was
triggered by one of my ftrace selftests that happen at bootup. But when
I trigger the issue from user space after bootup finished, it would not
catch it.
Now I was thinking that it may be due to the fact that the trampoline
is allocated with module_alloc(), and that has some magic kasan goo in
it. But when forcing the issue with adding the following code:
void **pblah;
void *blah;
pblah = kmalloc(sizeof(*pblah), GFP_KERNEL);
blah = module_alloc(PAGE_SIZE);
*pblah = blah;
printk("allocated blah %p\n", blah);
kfree(pblah);
in a path that I could control, it would catch it only after doing it
several times. I was never able to have kmemleak catch the actual bug
from user space no matter how many times I triggered it.
# dmesg |grep kmemleak
[ 16.746832] kmemleak: Kernel memory leak detector initialized
[ 16.746888] kmemleak: Automatic memory scanning thread started
And then I would do:
# echo scan=on > /sys/kernel/debug/kmemleak
[do the test]
# echo scan > /sys/kernel/debug/kmemleak
Most of the times it found nothing. Even when I switched the above from
module_alloc() to kmalloc().
Is this normal?
-- Steve
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: kmemleak not always catching stuff 2017-09-01 22:33 kmemleak not always catching stuff Steven Rostedt @ 2017-09-02 10:35 ` Dmitry Vyukov 2017-09-04 10:09 ` Catalin Marinas 1 sibling, 0 replies; 8+ messages in thread From: Dmitry Vyukov @ 2017-09-02 10:35 UTC (permalink / raw) To: Steven Rostedt; +Cc: LKML, Catalin Marinas, Andrey Ryabinin, kasan-dev On Sat, Sep 2, 2017 at 12:33 AM, Steven Rostedt <rostedt@goodmis.org> wrote: > Hi, > > Recently kmemleak discovered a bug in my code where an allocated > trampoline for a ftrace function tracer wasn't freed due to an exit > path. The thing is, kmemleak was able to catch this 100% when it was > triggered by one of my ftrace selftests that happen at bootup. But when > I trigger the issue from user space after bootup finished, it would not > catch it. > > Now I was thinking that it may be due to the fact that the trampoline > is allocated with module_alloc(), and that has some magic kasan goo in > it. But when forcing the issue with adding the following code: > > void **pblah; > void *blah; > > pblah = kmalloc(sizeof(*pblah), GFP_KERNEL); > blah = module_alloc(PAGE_SIZE); > *pblah = blah; > printk("allocated blah %p\n", blah); > kfree(pblah); > > in a path that I could control, it would catch it only after doing it > several times. I was never able to have kmemleak catch the actual bug > from user space no matter how many times I triggered it. > > # dmesg |grep kmemleak > [ 16.746832] kmemleak: Kernel memory leak detector initialized > [ 16.746888] kmemleak: Automatic memory scanning thread started > > And then I would do: > > # echo scan=on > /sys/kernel/debug/kmemleak > > [do the test] > > # echo scan > /sys/kernel/debug/kmemleak > > Most of the times it found nothing. Even when I switched the above from > module_alloc() to kmalloc(). > > Is this normal? Hi, We've caught some leaks triggered from userspace, so generally it works. But I never tried to do analysis of false negatives, it's generally hard because you don't know where are they to begin with. For such tools it's generally useful to look at false negatives once they come to light, because frequently that allows to fix bugs. Having said that, kmemleak has inherent false positives due to the fact that it does not have precise information about live data. It, of course, looks at status of heap objects (allocated/freed), but still it will treat as live data paddings on stack, paddings in heap objects, uninit parts of heap objects, dead slots on stack, etc. So I guess your pointer just stays in one of these dead slots, but kmemleak still discovers it and does not report leak. I assume that the task that triggered the leak has exited by the time you do scan, right? Stack is a common place for these dead pointers. I don't know how much of proactive zeroing kmemleak enables. E.g. does it zero heap blocks on allocation? Does it zero task stacks on creation? Perhaps we can do more of this. Also, since you CCed kasan-dev, is it related to KASAN? Does it happen only when KASAN enabled? Does it happen without KASAN? I suspect that KASAN's quarantine can have very negative effect on kmemleak. I think we need to do better integration there and tell kmemleak that quarantied objects are not live. Also, does kmemleak know about actual size of heap objects (what user asked for)? If not, then KASAN has that info and could pass to kmemleak. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: kmemleak not always catching stuff 2017-09-01 22:33 kmemleak not always catching stuff Steven Rostedt 2017-09-02 10:35 ` Dmitry Vyukov @ 2017-09-04 10:09 ` Catalin Marinas 2017-09-05 14:15 ` Steven Rostedt 2017-09-05 14:23 ` Dmitry Vyukov 1 sibling, 2 replies; 8+ messages in thread From: Catalin Marinas @ 2017-09-04 10:09 UTC (permalink / raw) To: Steven Rostedt; +Cc: LKML, Andrey Ryabinin, kasan-dev Hi Steve, On Fri, Sep 01, 2017 at 06:33:11PM -0400, Steven Rostedt wrote: > Recently kmemleak discovered a bug in my code where an allocated > trampoline for a ftrace function tracer wasn't freed due to an exit > path. The thing is, kmemleak was able to catch this 100% when it was > triggered by one of my ftrace selftests that happen at bootup. But when > I trigger the issue from user space after bootup finished, it would not > catch it. Is this the create_filter() fix that went in recently? > Now I was thinking that it may be due to the fact that the trampoline > is allocated with module_alloc(), and that has some magic kasan goo in > it. But when forcing the issue with adding the following code: > > void **pblah; > void *blah; > > pblah = kmalloc(sizeof(*pblah), GFP_KERNEL); > blah = module_alloc(PAGE_SIZE); > *pblah = blah; > printk("allocated blah %p\n", blah); > kfree(pblah); > > in a path that I could control, it would catch it only after doing it > several times. I was never able to have kmemleak catch the actual bug > from user space no matter how many times I triggered it. module_alloc() uses vmalloc_exec(), so it is tracked by kmemleak but you probably hit a false negative with the blah pointer lingering somewhere on some stack. > # dmesg |grep kmemleak > [ 16.746832] kmemleak: Kernel memory leak detector initialized > [ 16.746888] kmemleak: Automatic memory scanning thread started > > And then I would do: > > # echo scan=on > /sys/kernel/debug/kmemleak scan=on is not necessary since this just enables the scanning thread (already started as per dmesg). > [do the test] > > # echo scan > /sys/kernel/debug/kmemleak Some heuristics in kmemleak cause the first leak of an object not to be reported (too many false positives). You'd need to do "echo scan" at least twice after an allocation. I tried the same test code you have above triggered with an echo ... > /sys from user space. After the second scan it shows the leak, both with and without KASan. > Most of the times it found nothing. Even when I switched the above from > module_alloc() to kmalloc(). > > Is this normal? In general, a leak would eventually appear after a few scans or in time when some memory location is overridden. Yet another heuristics in kmemleak is to treat pointers at some offset inside an object as valid references (because of the container_of tricks). However, the downside is that the bigger the object, the greater chances of finding some random data that looks like a pointer. We could change this logic to require precise pointers above a certain size (e.g. PAGE_SIZE) where the use of container_of() is less likely. Kmemleak doesn't have a way to inspect false negatives but if you are interested in digging further, I could add a "find=0x..." command to print all references to an object during scanning. I also need to find some time to implement a "stopscan" command which uses stop_machine() and skips the heuristics for reducing false positives. -- Catalin ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: kmemleak not always catching stuff 2017-09-04 10:09 ` Catalin Marinas @ 2017-09-05 14:15 ` Steven Rostedt 2017-09-08 17:45 ` Catalin Marinas 2017-09-05 14:23 ` Dmitry Vyukov 1 sibling, 1 reply; 8+ messages in thread From: Steven Rostedt @ 2017-09-05 14:15 UTC (permalink / raw) To: Catalin Marinas; +Cc: LKML, Andrey Ryabinin, kasan-dev [-- Attachment #1: Type: text/plain, Size: 5135 bytes --] On Mon, 4 Sep 2017 11:09:05 +0100 Catalin Marinas <catalin.marinas@arm.com> wrote: > Hi Steve, > > On Fri, Sep 01, 2017 at 06:33:11PM -0400, Steven Rostedt wrote: > > Recently kmemleak discovered a bug in my code where an allocated > > trampoline for a ftrace function tracer wasn't freed due to an exit > > path. The thing is, kmemleak was able to catch this 100% when it was > > triggered by one of my ftrace selftests that happen at bootup. But when > > I trigger the issue from user space after bootup finished, it would not > > catch it. > > Is this the create_filter() fix that went in recently? No. I haven't pushed it. I just finished testing it and will include it in my pull request to Linus. I just pushed it to my linux-next branch (here's the link: http://lkml.kernel.org/r/20170905133217.883468521@goodmis.org) > > > Now I was thinking that it may be due to the fact that the trampoline > > is allocated with module_alloc(), and that has some magic kasan goo in > > it. But when forcing the issue with adding the following code: > > > > void **pblah; > > void *blah; > > > > pblah = kmalloc(sizeof(*pblah), GFP_KERNEL); > > blah = module_alloc(PAGE_SIZE); > > *pblah = blah; > > printk("allocated blah %p\n", blah); > > kfree(pblah); > > > > in a path that I could control, it would catch it only after doing it > > several times. I was never able to have kmemleak catch the actual bug > > from user space no matter how many times I triggered it. > > module_alloc() uses vmalloc_exec(), so it is tracked by kmemleak but you > probably hit a false negative with the blah pointer lingering somewhere > on some stack. Hmm, could this also be what causes the miss of catching the lingering ftrace trampoline? > > > # dmesg |grep kmemleak > > [ 16.746832] kmemleak: Kernel memory leak detector initialized > > [ 16.746888] kmemleak: Automatic memory scanning thread started > > > > And then I would do: > > > > # echo scan=on > /sys/kernel/debug/kmemleak > > scan=on is not necessary since this just enables the scanning thread > (already started as per dmesg). Yeah, but I found doing that appeared to catch things quicker when I repeated the test. > > > [do the test] > > > > # echo scan > /sys/kernel/debug/kmemleak > > Some heuristics in kmemleak cause the first leak of an object not to be > reported (too many false positives). You'd need to do "echo scan" at > least twice after an allocation. OK, is this documented somewhere? > > I tried the same test code you have above triggered with an echo ... > > /sys from user space. After the second scan it shows the leak, both with > and without KASan. > > > Most of the times it found nothing. Even when I switched the above from > > module_alloc() to kmalloc(). > > > > Is this normal? > > In general, a leak would eventually appear after a few scans or in time > when some memory location is overridden. > > Yet another heuristics in kmemleak is to treat pointers at some offset > inside an object as valid references (because of the container_of > tricks). However, the downside is that the bigger the object, the > greater chances of finding some random data that looks like a pointer. > We could change this logic to require precise pointers above a certain > size (e.g. PAGE_SIZE) where the use of container_of() is less likely. > > Kmemleak doesn't have a way to inspect false negatives but if you are > interested in digging further, I could add a "find=0x..." command to > print all references to an object during scanning. I also need to find > some time to implement a "stopscan" command which uses stop_machine() > and skips the heuristics for reducing false positives. > Without the patch in the link above, there's a memory leak with the ftrace trampoline with the following commands: Requires: CONFIG_FUNCTION_TRACER and CONFIG_SCHED_TRACER # cd /sys/kernel/debug/tracing # mkdir instances/foo # echo wakeup > instances/foo/current_tracer # echo 0 > /proc/sys/kernel/ftrace_enabled # echo nop > instances/foo/current_tracer # rmdir instances/foo What the above does, is creates a new (allocated/non-static) buffer in the instance directory. Then we enable the wakeup tracer which enables function tracing and also creates a dynamic ftrace trampoline for it. We disable function tracing for all tracers with the proc sysctl ftrace_enabled set to zero. The nop removes the wakeup tracer and unregisters its function tracing handler. This is where the leak happens. The unregister path sees that function tracing is disabled and exits out early, without releasing the trampoline. To make sure nothing else may be accessing it, I even remove the trace instance with the rmdir. I tried the above over and over and kmemleak never catches it. The attached patch is what I added to debug this to see that it was not being freed. This patch can be added on top of the above commit, as it reverts the fix. Note, this is basically the same printks I used to debug what was happening with the original triggering of kmemleak (which only caught this on boot up, via the ftrace self tests). -- Steve [-- Attachment #2: kmemleak-debug.patch --] [-- Type: text/x-patch, Size: 4082 bytes --] Index: linux-trace.git/arch/x86/kernel/ftrace.c =================================================================== --- linux-trace.git.orig/arch/x86/kernel/ftrace.c +++ linux-trace.git/arch/x86/kernel/ftrace.c @@ -687,12 +687,18 @@ static unsigned char *ftrace_jmp_replace /* Module allocation simplifies allocating memory for code */ static inline void *alloc_tramp(unsigned long size) { - return module_alloc(size); + void *ret; + + ret = module_alloc(size); + printk("alloc tramp %p %ld\n", ret, size); + return ret; + } static inline void tramp_free(void *tramp, int size) { int npages = PAGE_ALIGN(size) >> PAGE_SHIFT; + printk("free tramp %p %d\n", tramp, size); set_memory_nx((unsigned long)tramp, npages); set_memory_rw((unsigned long)tramp, npages); module_memfree(tramp); @@ -947,6 +953,7 @@ void arch_ftrace_trampoline_free(struct if (!ops || !(ops->flags & FTRACE_OPS_FL_ALLOC_TRAMP)) return; + printk("free %pS at %p size=%d\n", ops, (void *)ops->trampoline, ops->trampoline_size); tramp_free((void *)ops->trampoline, ops->trampoline_size); ops->trampoline = 0; } Index: linux-trace.git/kernel/trace/ftrace.c =================================================================== --- linux-trace.git.orig/kernel/trace/ftrace.c +++ linux-trace.git/kernel/trace/ftrace.c @@ -2801,10 +2801,12 @@ static int ftrace_shutdown(struct ftrace if (unlikely(ftrace_disabled)) return -ENODEV; + printk("unreg %pS %d\n", ops, __LINE__); ret = __unregister_ftrace_function(ops); if (ret) return ret; + printk("unreg %pS %d\n", ops, __LINE__); ftrace_start_up--; /* * Just warn in case of unbalance, no need to kill ftrace, it's not @@ -2828,17 +2830,20 @@ static int ftrace_shutdown(struct ftrace if (!command || !ftrace_enabled) { /* - * If these are dynamic or per_cpu ops, they still - * need their data freed. Since, function tracing is + * If these are per_cpu ops, they still need their + * per_cpu field freed. Since, function tracing is * not currently active, we can just free them * without synchronizing all CPUs. */ - if (ops->flags & (FTRACE_OPS_FL_DYNAMIC | FTRACE_OPS_FL_PER_CPU)) - goto free_ops; - + printk("%pS ops->flags=%x tramp=%pS (leaving)\n", ops, ops->flags, + (void *)ops->trampoline); + if (ops->flags & FTRACE_OPS_FL_PER_CPU) + per_cpu_ops_free(ops); + printk("unreg %pS %d leaving\n", ops, __LINE__); return 0; } + printk("unreg %pS %d\n", ops, __LINE__); /* * If the ops uses a trampoline, then it needs to be * tested first on update. @@ -2880,6 +2885,7 @@ static int ftrace_shutdown(struct ftrace * The same goes for freeing the per_cpu data of the per_cpu * ops. */ + printk("%pS ops->flags=%x\n", ops, ops->flags); if (ops->flags & (FTRACE_OPS_FL_DYNAMIC | FTRACE_OPS_FL_PER_CPU)) { /* * We need to do a hard force of sched synchronization. @@ -2901,7 +2907,6 @@ static int ftrace_shutdown(struct ftrace if (IS_ENABLED(CONFIG_PREEMPT)) synchronize_rcu_tasks(); - free_ops: arch_ftrace_trampoline_free(ops); if (ops->flags & FTRACE_OPS_FL_PER_CPU) @@ -5535,6 +5540,7 @@ void ftrace_create_filter_files(struct f void ftrace_destroy_filter_files(struct ftrace_ops *ops) { mutex_lock(&ftrace_lock); + printk("destroy %pS %x (%x)\n", ops, ops->flags, ops->flags & FTRACE_OPS_FL_ENABLED); if (ops->flags & FTRACE_OPS_FL_ENABLED) ftrace_shutdown(ops, 0); ops->flags |= FTRACE_OPS_FL_DELETED; Index: linux-trace.git/kernel/trace/trace_selftest.c =================================================================== --- linux-trace.git.orig/kernel/trace/trace_selftest.c +++ linux-trace.git/kernel/trace/trace_selftest.c @@ -257,6 +257,7 @@ static int trace_selftest_ops(struct tra dyn_ops->func = trace_selftest_test_dyn_func; + printk("reg dynamic ops\n"); register_ftrace_function(dyn_ops); trace_selftest_test_global_cnt = 0; @@ -291,6 +292,7 @@ static int trace_selftest_ops(struct tra ret = 0; out_free: + printk("free dyn_ops\n"); unregister_ftrace_function(dyn_ops); kfree(dyn_ops); ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: kmemleak not always catching stuff 2017-09-05 14:15 ` Steven Rostedt @ 2017-09-08 17:45 ` Catalin Marinas 2017-09-08 18:34 ` Steven Rostedt 0 siblings, 1 reply; 8+ messages in thread From: Catalin Marinas @ 2017-09-08 17:45 UTC (permalink / raw) To: Steven Rostedt; +Cc: LKML, Andrey Ryabinin, kasan-dev On Tue, Sep 05, 2017 at 10:15:45AM -0400, Steven Rostedt wrote: > On Mon, 4 Sep 2017 11:09:05 +0100 > Catalin Marinas <catalin.marinas@arm.com> wrote: > > On Fri, Sep 01, 2017 at 06:33:11PM -0400, Steven Rostedt wrote: > > > Now I was thinking that it may be due to the fact that the trampoline > > > is allocated with module_alloc(), and that has some magic kasan goo in > > > it. But when forcing the issue with adding the following code: > > > > > > void **pblah; > > > void *blah; > > > > > > pblah = kmalloc(sizeof(*pblah), GFP_KERNEL); > > > blah = module_alloc(PAGE_SIZE); > > > *pblah = blah; > > > printk("allocated blah %p\n", blah); > > > kfree(pblah); > > > > > > in a path that I could control, it would catch it only after doing it > > > several times. I was never able to have kmemleak catch the actual bug > > > from user space no matter how many times I triggered it. > > > > module_alloc() uses vmalloc_exec(), so it is tracked by kmemleak but you > > probably hit a false negative with the blah pointer lingering somewhere > > on some stack. > > Hmm, could this also be what causes the miss of catching the lingering > ftrace trampoline? Not sure (not without some additional support in kmemleak to help track down the source of false negatives; I'm on a long flight to LA next week, maybe I manage to hack something up ;)). BTW, I had a quick look at the trace_selftest_ops() function (without pretending I understand the ftrace code) and there is one case where this function can exit without freeing dyn_ops. Is this intentional? diff --git a/kernel/trace/trace_selftest.c b/kernel/trace/trace_selftest.c index cb917cebae29..b17ec642793b 100644 --- a/kernel/trace/trace_selftest.c +++ b/kernel/trace/trace_selftest.c @@ -273,7 +273,7 @@ static int trace_selftest_ops(struct trace_array *tr, int cnt) goto out_free; if (cnt > 1) { if (trace_selftest_test_global_cnt == 0) - goto out; + goto out_free; } if (trace_selftest_test_dyn_cnt == 0) goto out_free; > > > [do the test] > > > > > > # echo scan > /sys/kernel/debug/kmemleak > > > > Some heuristics in kmemleak cause the first leak of an object not to be > > reported (too many false positives). You'd need to do "echo scan" at > > least twice after an allocation. > > OK, is this documented somewhere? Apparently not. I'll add some notes to the kmemleak documentation. > > > Most of the times it found nothing. Even when I switched the above from > > > module_alloc() to kmalloc(). > > > > > > Is this normal? > > > > In general, a leak would eventually appear after a few scans or in time > > when some memory location is overridden. > > > > Yet another heuristics in kmemleak is to treat pointers at some offset > > inside an object as valid references (because of the container_of > > tricks). However, the downside is that the bigger the object, the > > greater chances of finding some random data that looks like a pointer. > > We could change this logic to require precise pointers above a certain > > size (e.g. PAGE_SIZE) where the use of container_of() is less likely. > > > > Kmemleak doesn't have a way to inspect false negatives but if you are > > interested in digging further, I could add a "find=0x..." command to > > print all references to an object during scanning. I also need to find > > some time to implement a "stopscan" command which uses stop_machine() > > and skips the heuristics for reducing false positives. > > Without the patch in the link above, there's a memory leak with the > ftrace trampoline with the following commands: > > Requires: CONFIG_FUNCTION_TRACER and CONFIG_SCHED_TRACER > > # cd /sys/kernel/debug/tracing > # mkdir instances/foo > # echo wakeup > instances/foo/current_tracer > # echo 0 > /proc/sys/kernel/ftrace_enabled > # echo nop > instances/foo/current_tracer > # rmdir instances/foo > > What the above does, is creates a new (allocated/non-static) buffer in > the instance directory. Then we enable the wakeup tracer which > enables function tracing and also creates a dynamic ftrace trampoline > for it. We disable function tracing for all tracers with the proc > sysctl ftrace_enabled set to zero. The nop removes the wakeup tracer > and unregisters its function tracing handler. This is where the leak > happens. The unregister path sees that function tracing is disabled and > exits out early, without releasing the trampoline. Are the ftrace_ops allocated dynamically in this case (and freed when unregistered)? Otherwise, you may have an ops->trampoline still around that kmemleak finds. -- Catalin ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: kmemleak not always catching stuff 2017-09-08 17:45 ` Catalin Marinas @ 2017-09-08 18:34 ` Steven Rostedt 0 siblings, 0 replies; 8+ messages in thread From: Steven Rostedt @ 2017-09-08 18:34 UTC (permalink / raw) To: Catalin Marinas; +Cc: LKML, Andrey Ryabinin, kasan-dev On Fri, 8 Sep 2017 18:45:20 +0100 Catalin Marinas <catalin.marinas@arm.com> wrote: > > Hmm, could this also be what causes the miss of catching the lingering > > ftrace trampoline? > > Not sure (not without some additional support in kmemleak to help track > down the source of false negatives; I'm on a long flight to LA next > week, maybe I manage to hack something up ;)). I'll be there too. We can talk more about this in person ;-) I'll probably be at the VMware booth a bit, as one of our team had a family emergency and had to cancel. > > BTW, I had a quick look at the trace_selftest_ops() function (without > pretending I understand the ftrace code) and there is one case where I personally just pretend I understand it. > this function can exit without freeing dyn_ops. Is this intentional? > > diff --git a/kernel/trace/trace_selftest.c b/kernel/trace/trace_selftest.c > index cb917cebae29..b17ec642793b 100644 > --- a/kernel/trace/trace_selftest.c > +++ b/kernel/trace/trace_selftest.c > @@ -273,7 +273,7 @@ static int trace_selftest_ops(struct trace_array *tr, int cnt) > goto out_free; > if (cnt > 1) { > if (trace_selftest_test_global_cnt == 0) > - goto out; > + goto out_free; > } > if (trace_selftest_test_dyn_cnt == 0) > goto out_free; > You mean something like this: http://lkml.kernel.org/r/20170905133217.770389331@goodmis.org :-) > > > > Without the patch in the link above, there's a memory leak with the > > ftrace trampoline with the following commands: > > > > Requires: CONFIG_FUNCTION_TRACER and CONFIG_SCHED_TRACER > > > > # cd /sys/kernel/debug/tracing > > # mkdir instances/foo > > # echo wakeup > instances/foo/current_tracer > > # echo 0 > /proc/sys/kernel/ftrace_enabled > > # echo nop > instances/foo/current_tracer > > # rmdir instances/foo > > > > What the above does, is creates a new (allocated/non-static) buffer in > > the instance directory. Then we enable the wakeup tracer which > > enables function tracing and also creates a dynamic ftrace trampoline > > for it. We disable function tracing for all tracers with the proc > > sysctl ftrace_enabled set to zero. The nop removes the wakeup tracer > > and unregisters its function tracing handler. This is where the leak > > happens. The unregister path sees that function tracing is disabled and > > exits out early, without releasing the trampoline. > > Are the ftrace_ops allocated dynamically in this case (and freed when > unregistered)? Otherwise, you may have an ops->trampoline still around > that kmemleak finds. > The ftrace_ops is allocated when the instance is created, and freed when the instance is removed: instance_mkdir() { init_tracer_fs() { ftrace_create_function_files() { allocate_ftrace_ops() { ops = kzalloc(); tr->ops = ops; } } } } instance_rmdir() { ftrace_destroy_function_files() { kfree(tr->ops); } } -- Steve ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: kmemleak not always catching stuff 2017-09-04 10:09 ` Catalin Marinas 2017-09-05 14:15 ` Steven Rostedt @ 2017-09-05 14:23 ` Dmitry Vyukov 2017-09-05 14:39 ` Catalin Marinas 1 sibling, 1 reply; 8+ messages in thread From: Dmitry Vyukov @ 2017-09-05 14:23 UTC (permalink / raw) To: Catalin Marinas; +Cc: Steven Rostedt, LKML, Andrey Ryabinin, kasan-dev On Mon, Sep 4, 2017 at 12:09 PM, Catalin Marinas <catalin.marinas@arm.com> wrote: > Hi Steve, > > On Fri, Sep 01, 2017 at 06:33:11PM -0400, Steven Rostedt wrote: >> Recently kmemleak discovered a bug in my code where an allocated >> trampoline for a ftrace function tracer wasn't freed due to an exit >> path. The thing is, kmemleak was able to catch this 100% when it was >> triggered by one of my ftrace selftests that happen at bootup. But when >> I trigger the issue from user space after bootup finished, it would not >> catch it. > > Is this the create_filter() fix that went in recently? > >> Now I was thinking that it may be due to the fact that the trampoline >> is allocated with module_alloc(), and that has some magic kasan goo in >> it. But when forcing the issue with adding the following code: >> >> void **pblah; >> void *blah; >> >> pblah = kmalloc(sizeof(*pblah), GFP_KERNEL); >> blah = module_alloc(PAGE_SIZE); >> *pblah = blah; >> printk("allocated blah %p\n", blah); >> kfree(pblah); >> >> in a path that I could control, it would catch it only after doing it >> several times. I was never able to have kmemleak catch the actual bug >> from user space no matter how many times I triggered it. > > module_alloc() uses vmalloc_exec(), so it is tracked by kmemleak but you > probably hit a false negative with the blah pointer lingering somewhere > on some stack. > >> # dmesg |grep kmemleak >> [ 16.746832] kmemleak: Kernel memory leak detector initialized >> [ 16.746888] kmemleak: Automatic memory scanning thread started >> >> And then I would do: >> >> # echo scan=on > /sys/kernel/debug/kmemleak > > scan=on is not necessary since this just enables the scanning thread > (already started as per dmesg). > >> [do the test] >> >> # echo scan > /sys/kernel/debug/kmemleak > > Some heuristics in kmemleak cause the first leak of an object not to be > reported (too many false positives). You'd need to do "echo scan" at > least twice after an allocation. > > I tried the same test code you have above triggered with an echo ... > > /sys from user space. After the second scan it shows the leak, both with > and without KASan. > >> Most of the times it found nothing. Even when I switched the above from >> module_alloc() to kmalloc(). >> >> Is this normal? > > In general, a leak would eventually appear after a few scans or in time > when some memory location is overridden. > > Yet another heuristics in kmemleak is to treat pointers at some offset > inside an object as valid references (because of the container_of > tricks). However, the downside is that the bigger the object, the > greater chances of finding some random data that looks like a pointer. > We could change this logic to require precise pointers above a certain > size (e.g. PAGE_SIZE) where the use of container_of() is less likely. > > Kmemleak doesn't have a way to inspect false negatives but if you are > interested in digging further, I could add a "find=0x..." command to > print all references to an object during scanning. I also need to find > some time to implement a "stopscan" command which uses stop_machine() > and skips the heuristics for reducing false positives. "stopscan" would be great. We would like to deploy continuous testing with kmemleak, but while it has systematic false positives, it is not possible. Performance kinda matters, but only when a tool does not have false positives. We probably will just not enable it on all machines (it introduces significant slowdown even today). ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: kmemleak not always catching stuff 2017-09-05 14:23 ` Dmitry Vyukov @ 2017-09-05 14:39 ` Catalin Marinas 0 siblings, 0 replies; 8+ messages in thread From: Catalin Marinas @ 2017-09-05 14:39 UTC (permalink / raw) To: Dmitry Vyukov; +Cc: Steven Rostedt, LKML, Andrey Ryabinin, kasan-dev On Tue, Sep 05, 2017 at 04:23:47PM +0200, Dmitry Vyukov wrote: > On Mon, Sep 4, 2017 at 12:09 PM, Catalin Marinas > <catalin.marinas@arm.com> wrote: > > I also need to find > > some time to implement a "stopscan" command which uses stop_machine() > > and skips the heuristics for reducing false positives. > > "stopscan" would be great. We would like to deploy continuous testing > with kmemleak, but while it has systematic false positives, it is not > possible. Performance kinda matters, but only when a tool does not > have false positives. We probably will just not enable it on all > machines (it introduces significant slowdown even today). The downside is that scanning may take minutes. I would expect some RCU warnings once scanning is done. -- Catalin ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-09-08 18:34 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-09-01 22:33 kmemleak not always catching stuff Steven Rostedt 2017-09-02 10:35 ` Dmitry Vyukov 2017-09-04 10:09 ` Catalin Marinas 2017-09-05 14:15 ` Steven Rostedt 2017-09-08 17:45 ` Catalin Marinas 2017-09-08 18:34 ` Steven Rostedt 2017-09-05 14:23 ` Dmitry Vyukov 2017-09-05 14:39 ` Catalin Marinas
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.