----- Original Message ----- > From: "Steven Rostedt" <rost...@goodmis.org> > To: "Chunyu Hu" <ch...@redhat.com> > Cc: mi...@kernel.org, linux-kernel@vger.kernel.org > Sent: Wednesday, August 23, 2017 12:52:49 PM > Subject: Re: [PATCH 2/2] tracing: Fix kmemleak in set_trigger_filter > > On Wed, 23 Aug 2017 10:41:55 -0400 > Steven Rostedt <rost...@goodmis.org> wrote: > > > * On success, returns 0 and *@filterp points to the new filter. On > > * failure, returns -errno and *@filterp may point to %NULL or to a new > > * filter. In the latter case, the returned filter contains error > > * information if @set_str is %true and the caller is responsible for > > * freeing it. > > > > So filter contains an error string when it fails. It seems that we > > should somehow propagate that up the chain to display. I'll look more > > into this. > > The bug is in create_filter(), because "set_str" is set to false, and > the filter should not be passed back allocated on error.
Thanks for all the analysis. I think you are right. I'll try to have a test on it in case we miss something. But please don't block on my test. > > The correct fix is below. > > Thanks! > > -- Steve > > > From 9975be0b2dccaea8ec3574d69a3504e11659a6ea Mon Sep 17 00:00:00 2001 > From: "Steven Rostedt (VMware)" <rost...@goodmis.org> > Date: Wed, 23 Aug 2017 12:46:27 -0400 > Subject: [PATCH] tracing: Fix freeing of filter in create_filter() when > set_str is false > > Performing the following task with kmemleak enabled: > > # cd /sys/kernel/tracing/events/irq/irq_handler_entry/ > # echo 'enable_event:kmem:kmalloc:3 if irq >' > trigger > # echo 'enable_event:kmem:kmalloc:3 if irq > 31' > trigger > # echo scan > /sys/kernel/debug/kmemleak > # cat /sys/kernel/debug/kmemleak > unreferenced object 0xffff8800b9290308 (size 32): > comm "bash", pid 1114, jiffies 4294848451 (age 141.139s) > 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: > [<ffffffff81cef5aa>] kmemleak_alloc+0x4a/0xa0 > [<ffffffff81357938>] kmem_cache_alloc_trace+0x158/0x290 > [<ffffffff81261c09>] create_filter_start.constprop.28+0x99/0x940 > [<ffffffff812639c9>] create_filter+0xa9/0x160 > [<ffffffff81263bdc>] create_event_filter+0xc/0x10 > [<ffffffff812655e5>] set_trigger_filter+0xe5/0x210 > [<ffffffff812660c4>] event_enable_trigger_func+0x324/0x490 > [<ffffffff812652e2>] event_trigger_write+0x1a2/0x260 > [<ffffffff8138cf87>] __vfs_write+0xd7/0x380 > [<ffffffff8138f421>] vfs_write+0x101/0x260 > [<ffffffff8139187b>] SyS_write+0xab/0x130 > [<ffffffff81cfd501>] entry_SYSCALL_64_fastpath+0x1f/0xbe > [<ffffffffffffffff>] 0xffffffffffffffff > > The function create_filter() is passed a 'filterp' pointer that gets > allocated, and if "set_str" is true, it is up to the caller to free it, even > on error. The problem is that the pointer is not freed by create_filter() > when set_str is false. This is a bug, and it is not up to the caller to free > the filter on error if it doesn't care about the string. > > Link: > http://lkml.kernel.org/r/1502705898-27571-2-git-send-email-ch...@redhat.com > > Reported-by: Chunyu Hu <ch...@redhat.com> > Signed-off-by: Steven Rostedt (VMware) <rost...@goodmis.org> > --- > kernel/trace/trace_events_filter.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/kernel/trace/trace_events_filter.c > b/kernel/trace/trace_events_filter.c > index 59a411f..181e139 100644 > --- a/kernel/trace/trace_events_filter.c > +++ b/kernel/trace/trace_events_filter.c > @@ -1959,6 +1959,10 @@ static int create_filter(struct trace_event_call > *call, > if (err && set_str) > append_filter_err(ps, filter); > } > + if (err && !set_str) { > + free_event_filter(filter); > + filter = NULL; > + } > create_filter_finish(ps); > > *filterp = filter; > -- > 2.9.5 > > -- Regards, Chunyu Hu
