Messages in this thread | | | Date | Mon, 22 Jun 2015 10:17:28 -0400 | From | Steven Rostedt <> | Subject | Re: [GIT PULL] tracing: Have filter check for balanced ops |
| |
On Mon, 22 Jun 2015 15:03:14 +0100 Luis Henriques <luis.henriques@canonical.com> wrote:
> On Mon, Jun 22, 2015 at 02:53:17PM +0100, Luis Henriques wrote: > > Hi Steven, > > > > On Wed, Jun 17, 2015 at 08:36:38AM -0400, Steven Rostedt wrote: > > ... > > > diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c > > > index ced69da0ff55..7f2e97ce71a7 100644 > > > --- a/kernel/trace/trace_events_filter.c > > > +++ b/kernel/trace/trace_events_filter.c > > > @@ -1369,19 +1369,26 @@ static int check_preds(struct filter_parse_state *ps) > > > { > > > int n_normal_preds = 0, n_logical_preds = 0; > > > struct postfix_elt *elt; > > > + int cnt = 0; > > > > > > list_for_each_entry(elt, &ps->postfix, list) { > > > - if (elt->op == OP_NONE) > > > + if (elt->op == OP_NONE) { > > > + cnt++; > > > continue; > > > + } > > > > > > if (elt->op == OP_AND || elt->op == OP_OR) { > > > n_logical_preds++; > > > + cnt--; > > > continue; > > > } > > > + if (elt->op != OP_NOT) > > > + cnt--; > > > > Since the OP_NOT was introduced only with e12c09cf3087 ("tracing: Add > > NOT to filtering logic"), how would stable kernels backport this fix? > > Do you think that just dropping the 'if' and do the 'cnt--' > > unconditionally would be ok? > > Something like the patch below (only compile-tested). > > Cheers, > -- > Luís > > >From db7ac73e73dd0ef0259d5d2871b7b9402b12e4d3 Mon Sep 17 00:00:00 2001 > From: Steven Rostedt <rostedt@goodmis.org> > Date: Mon, 15 Jun 2015 17:50:25 -0400 > Subject: [PATCH] tracing: Have filter check for balanced ops > > commit 2cf30dc180cea808077f003c5116388183e54f9e upstream. > > When the following filter is used it causes a warning to trigger: > > # cd /sys/kernel/debug/tracing > # echo "((dev==1)blocks==2)" > events/ext4/ext4_truncate_exit/filter > -bash: echo: write error: Invalid argument > # cat events/ext4/ext4_truncate_exit/filter > ((dev==1)blocks==2) > ^ > parse_error: No error > > ------------[ cut here ]------------ > WARNING: CPU: 2 PID: 1223 at kernel/trace/trace_events_filter.c:1640 replace_preds+0x3c5/0x990() > Modules linked in: bnep lockd grace bluetooth ... > CPU: 3 PID: 1223 Comm: bash Tainted: G W 4.1.0-rc3-test+ #450 > Hardware name: Hewlett-Packard HP Compaq Pro 6300 SFF/339A, BIOS K01 v02.05 05/07/2012 > 0000000000000668 ffff8800c106bc98 ffffffff816ed4f9 ffff88011ead0cf0 > 0000000000000000 ffff8800c106bcd8 ffffffff8107fb07 ffffffff8136b46c > ffff8800c7d81d48 ffff8800d4c2bc00 ffff8800d4d4f920 00000000ffffffea > Call Trace: > [<ffffffff816ed4f9>] dump_stack+0x4c/0x6e > [<ffffffff8107fb07>] warn_slowpath_common+0x97/0xe0 > [<ffffffff8136b46c>] ? _kstrtoull+0x2c/0x80 > [<ffffffff8107fb6a>] warn_slowpath_null+0x1a/0x20 > [<ffffffff81159065>] replace_preds+0x3c5/0x990 > [<ffffffff811596b2>] create_filter+0x82/0xb0 > [<ffffffff81159944>] apply_event_filter+0xd4/0x180 > [<ffffffff81152bbf>] event_filter_write+0x8f/0x120 > [<ffffffff811db2a8>] __vfs_write+0x28/0xe0 > [<ffffffff811dda43>] ? __sb_start_write+0x53/0xf0 > [<ffffffff812e51e0>] ? security_file_permission+0x30/0xc0 > [<ffffffff811dc408>] vfs_write+0xb8/0x1b0 > [<ffffffff811dc72f>] SyS_write+0x4f/0xb0 > [<ffffffff816f5217>] system_call_fastpath+0x12/0x6a > ---[ end trace e11028bd95818dcd ]--- > > Worse yet, reading the error message (the filter again) it says that > there was no error, when there clearly was. The issue is that the > code that checks the input does not check for balanced ops. That is, > having an op between a closed parenthesis and the next token. > > This would only cause a warning, and fail out before doing any real > harm, but it should still not caues a warning, and the error reported > should work: > > # cd /sys/kernel/debug/tracing > # echo "((dev==1)blocks==2)" > events/ext4/ext4_truncate_exit/filter > -bash: echo: write error: Invalid argument > # cat events/ext4/ext4_truncate_exit/filter > ((dev==1)blocks==2) > ^ > parse_error: Meaningless filter expression > > And give no kernel warning. > > Link: http://lkml.kernel.org/r/20150615175025.7e809215@gandalf.local.home > > Cc: Peter Zijlstra <a.p.zijlstra@chello.nl> > Cc: Ingo Molnar <mingo@redhat.com> > Cc: Arnaldo Carvalho de Melo <acme@kernel.org> > Reported-by: Vince Weaver <vincent.weaver@maine.edu> > Tested-by: Vince Weaver <vincent.weaver@maine.edu> > Signed-off-by: Steven Rostedt <rostedt@goodmis.org> > [ luis: backported to 3.16: > - unconditionally decrement cnt as the OP_NOT logic was introduced only > by e12c09cf3087 ("tracing: Add NOT to filtering logic") ] > Signed-off-by: Luis Henriques <luis.henriques@canonical.com> > --- > kernel/trace/trace_events_filter.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c > index 8a8631926a07..cc1cdaff9e9f 100644 > --- a/kernel/trace/trace_events_filter.c > +++ b/kernel/trace/trace_events_filter.c > @@ -1399,19 +1399,25 @@ static int check_preds(struct filter_parse_state *ps) > { > int n_normal_preds = 0, n_logical_preds = 0; > struct postfix_elt *elt; > + int cnt = 0; > > list_for_each_entry(elt, &ps->postfix, list) { > - if (elt->op == OP_NONE) > + if (elt->op == OP_NONE) { > + cnt++; > continue; > + } > > if (elt->op == OP_AND || elt->op == OP_OR) { > n_logical_preds++; > + cnt--; > continue; > } > + cnt--;
To make this patch simpler, just move the "cnt--;" above the OP_AND and OP_OR check and remove the "cnt--;" from that if statement.
But yeah, this looks fine. You can test it with the example in the change log.
echo "((dev==1)blocks==2)" > events/ext4/ext4_truncate_exit/filter
And see if it gives an error other than "parse_error: No error".
-- Steve
> n_normal_preds++; > + WARN_ON_ONCE(cnt < 0); > } > > - if (!n_normal_preds || n_logical_preds >= n_normal_preds) { > + if (cnt != 1 || !n_normal_preds || n_logical_preds >= n_normal_preds) { > parse_error(ps, FILT_ERR_INVALID_FILTER, 0); > return -EINVAL; > }
-- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in Please read the FAQ at http://www.tux.org/lkml/
| |