Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752960AbbFVORd (ORCPT ); Mon, 22 Jun 2015 10:17:33 -0400 Received: from smtprelay0192.hostedemail.com ([216.40.44.192]:48198 "EHLO smtprelay.hostedemail.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751656AbbFVORc convert rfc822-to-8bit (ORCPT ); Mon, 22 Jun 2015 10:17:32 -0400 X-Session-Marker: 726F737465647440676F6F646D69732E6F7267 X-Spam-Summary: 50,0,0,,d41d8cd98f00b204,rostedt@goodmis.org,:::::::::::::,RULES_HIT:2:41:69:152:355:379:541:599:800:960:967:968:973:988:989:1260:1263:1277:1311:1313:1314:1345:1359:1431:1437:1513:1515:1516:1518:1521:1535:1593:1594:1605:1730:1747:1777:1792:2110:2393:2525:2553:2560:2563:2682:2685:2859:2898:2902:2933:2937:2939:2942:2945:2947:2951:2954:3022:3138:3139:3140:3141:3142:3622:3865:3867:3868:3870:3871:3872:3873:3874:3934:3936:3938:3941:3944:3947:3950:3953:3956:3959:4049:4118:4250:4321:4401:4605:5007:6119:6261:7576:7875:7903:7904:8599:8660:8784:9010:9025:9163:9545:10004:10848:10913:10967:11026:11232:11473:11658:11914:12043:12295:12296:12438:12517:12519:12555:12740:12783:13148:13230:14093:14096:14097:21060:21080,0,RBL:none,CacheIP:none,Bayesian:0.5,0.5,0.5,Netcheck:none,DomainCache:0,MSF:not bulk,SPF:fn,MSBL:0,DNSBL:none,Custom_rules:0:0:0 X-HE-Tag: pet09_5e825c044844d X-Filterd-Recvd-Size: 7539 Date: Mon, 22 Jun 2015 10:17:28 -0400 From: Steven Rostedt To: Luis Henriques Cc: Linus Torvalds , LKML , Ingo Molnar , Peter Zijlstra , Vince Weaver , Arnaldo Carvalho de Melo Subject: Re: [GIT PULL] tracing: Have filter check for balanced ops Message-ID: <20150622101728.63190cd8@gandalf.local.home> In-Reply-To: <20150622140314.GC2036@ares> References: <20150617083638.20304e44@gandalf.local.home> <20150622135317.GB2036@ares> <20150622140314.GC2036@ares> X-Mailer: Claws Mail 3.11.1 (GTK+ 2.24.25; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6259 Lines: 171 On Mon, 22 Jun 2015 15:03:14 +0100 Luis Henriques 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 > 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: > [] dump_stack+0x4c/0x6e > [] warn_slowpath_common+0x97/0xe0 > [] ? _kstrtoull+0x2c/0x80 > [] warn_slowpath_null+0x1a/0x20 > [] replace_preds+0x3c5/0x990 > [] create_filter+0x82/0xb0 > [] apply_event_filter+0xd4/0x180 > [] event_filter_write+0x8f/0x120 > [] __vfs_write+0x28/0xe0 > [] ? __sb_start_write+0x53/0xf0 > [] ? security_file_permission+0x30/0xc0 > [] vfs_write+0xb8/0x1b0 > [] SyS_write+0x4f/0xb0 > [] 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 > Cc: Ingo Molnar > Cc: Arnaldo Carvalho de Melo > Reported-by: Vince Weaver > Tested-by: Vince Weaver > Signed-off-by: Steven Rostedt > [ 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 > --- > 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/