On Mon, 22 Jun 2015 15:03:14 +0100
Luis Henriques <luis.henriq...@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 <rost...@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.7e809...@gandalf.local.home
>
> Cc: Peter Zijlstra <a.p.zijls...@chello.nl>
> Cc: Ingo Molnar <mi...@redhat.com>
> Cc: Arnaldo Carvalho de Melo <a...@kernel.org>
> Reported-by: Vince Weaver <vincent.wea...@maine.edu>
> Tested-by: Vince Weaver <vincent.wea...@maine.edu>
> Signed-off-by: Steven Rostedt <rost...@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.henriq...@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/
