Re: [GIT PULL] tracing: Have filter check for balanced ops

[Steven Rostedt <rostedt@goodmis.org>]

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/