Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933242AbbFVOD1 (ORCPT ); Mon, 22 Jun 2015 10:03:27 -0400 Received: from youngberry.canonical.com ([91.189.89.112]:33400 "EHLO youngberry.canonical.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752606AbbFVODR (ORCPT ); Mon, 22 Jun 2015 10:03:17 -0400 Date: Mon, 22 Jun 2015 15:03:14 +0100 From: Luis Henriques To: Steven Rostedt 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: <20150622140314.GC2036@ares> References: <20150617083638.20304e44@gandalf.local.home> <20150622135317.GB2036@ares> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20150622135317.GB2036@ares> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5507 Lines: 153 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--; 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/