All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC][PATCH 0/6] tracing: Fix bogus parsing of probes and set_ftrace_filter
@ 2018-02-06 22:43 Steven Rostedt
  2018-02-06 22:43 ` [RFC][PATCH 1/6] ftrace: Remove incorrect setting of glob search field Steven Rostedt
                   ` (5 more replies)
  0 siblings, 6 replies; 11+ messages in thread
From: Steven Rostedt @ 2018-02-06 22:43 UTC (permalink / raw)
  To: linux-kernel
  Cc: Ingo Molnar, Andrew Morton, Al Viro, Masami Hiramatsu, Dmitry Safonov


Al Viro pointed out a couple of bugs with the parsing of the
removing of function probes and the parsing of set_ftrace_filter
strings.

This patch series implements his suggested fixes and adds selftests
that would have caught the bugs in the first place.


Steven Rostedt (VMware) (6):
      ftrace: Remove incorrect setting of glob search field
      tracing: Fix parsing of globs with a wildcard at the beginning
      selftests/ftrace: Have reset_ftrace_filter handle modules
      selftests/ftrace: Have reset_ftrace_filter handle multiple instances
      selftests/ftrace: Add some missing glob checks
      selftests/ftrace: Add more tests for removing of function probes

----
 kernel/trace/ftrace.c                              |  1 -
 kernel/trace/trace_events_filter.c                 |  9 +++---
 .../ftrace/test.d/ftrace/func-filter-glob.tc       |  6 ++++
 .../ftrace/test.d/ftrace/func_set_ftrace_file.tc   | 37 ++++++++++++++++++++++
 tools/testing/selftests/ftrace/test.d/functions    | 10 ++++--
 5 files changed, 54 insertions(+), 9 deletions(-)

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [RFC][PATCH 1/6] ftrace: Remove incorrect setting of glob search field
  2018-02-06 22:43 [RFC][PATCH 0/6] tracing: Fix bogus parsing of probes and set_ftrace_filter Steven Rostedt
@ 2018-02-06 22:43 ` Steven Rostedt
  2018-02-07 14:35   ` Dmitry Safonov
  2018-02-08 14:00   ` Masami Hiramatsu
  2018-02-06 22:43 ` [RFC][PATCH 2/6] tracing: Fix parsing of globs with a wildcard at the beginning Steven Rostedt
                   ` (4 subsequent siblings)
  5 siblings, 2 replies; 11+ messages in thread
From: Steven Rostedt @ 2018-02-06 22:43 UTC (permalink / raw)
  To: linux-kernel
  Cc: Ingo Molnar, Andrew Morton, Al Viro, Masami Hiramatsu,
	Dmitry Safonov, stable

[-- Attachment #1: 0001-ftrace-Remove-incorrect-setting-of-glob-search-field.patch --]
[-- Type: text/plain, Size: 1784 bytes --]

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

__unregister_ftrace_function_probe() will incorrectly parse the glob filter
because it resets the search variable that was setup by filter_parse_regex().

Al Viro reported this:

    After that call of filter_parse_regex() we could have func_g.search not
    equal to glob only if glob started with '!' or '*'.  In the former case
    we would've buggered off with -EINVAL (not = 1).  In the latter we
    would've set func_g.search equal to glob + 1, calculated the length of
    that thing in func_g.len and proceeded to reset func_g.search back to
    glob.

    Suppose the glob is e.g. *foo*.  We end up with
	    func_g.type = MATCH_MIDDLE_ONLY;
	    func_g.len = 3;
	    func_g.search = "*foo";
    Feeding that to ftrace_match_record() will not do anything sane - we
    will be looking for names containing "*foo" (->len is ignored for that
    one).

Link: http://lkml.kernel.org/r/20180127031706.GE13338@ZenIV.linux.org.uk

Cc: stable@vger.kernel.org
Cc: Dmitry Safonov <0x7f454c46@gmail.com>
Fixes: 3ba009297149f ("ftrace: Introduce ftrace_glob structure")
Reported-by: Al Viro <viro@ZenIV.linux.org.uk>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 kernel/trace/ftrace.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index dabd9d167d42..eac9ce2c57a2 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -4456,7 +4456,6 @@ unregister_ftrace_function_probe_func(char *glob, struct trace_array *tr,
 		func_g.type = filter_parse_regex(glob, strlen(glob),
 						 &func_g.search, &not);
 		func_g.len = strlen(func_g.search);
-		func_g.search = glob;
 
 		/* we do not support '!' for function probes */
 		if (WARN_ON(not))
-- 
2.15.1

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [RFC][PATCH 2/6] tracing: Fix parsing of globs with a wildcard at the beginning
  2018-02-06 22:43 [RFC][PATCH 0/6] tracing: Fix bogus parsing of probes and set_ftrace_filter Steven Rostedt
  2018-02-06 22:43 ` [RFC][PATCH 1/6] ftrace: Remove incorrect setting of glob search field Steven Rostedt
@ 2018-02-06 22:43 ` Steven Rostedt
  2018-02-08 14:01   ` Masami Hiramatsu
  2018-02-06 22:43 ` [RFC][PATCH 3/6] selftests/ftrace: Have reset_ftrace_filter handle modules Steven Rostedt
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 11+ messages in thread
From: Steven Rostedt @ 2018-02-06 22:43 UTC (permalink / raw)
  To: linux-kernel
  Cc: Ingo Molnar, Andrew Morton, Al Viro, Masami Hiramatsu,
	Dmitry Safonov, stable

[-- Attachment #1: 0002-tracing-Fix-parsing-of-globs-with-a-wildcard-at-the-.patch --]
[-- Type: text/plain, Size: 2243 bytes --]

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

Al Viro reported:

    For substring - sure, but what about something like "*a*b" and "a*b"?
    AFAICS, filter_parse_regex() ends up with identical results in both
    cases - MATCH_GLOB and *search = "a*b".  And no way for the caller
    to tell one from another.

Testing this with the following:

 # cd /sys/kernel/tracing
 # echo '*raw*lock' > set_ftrace_filter
 bash: echo: write error: Invalid argument

With this patch:

 # echo '*raw*lock' > set_ftrace_filter
 # cat set_ftrace_filter
_raw_read_trylock
_raw_write_trylock
_raw_read_unlock
_raw_spin_unlock
_raw_write_unlock
_raw_spin_trylock
_raw_spin_lock
_raw_write_lock
_raw_read_lock

Al recommended not setting the search buffer to skip the first '*' unless we
know we are not using MATCH_GLOB. This implements his suggested logic.

Link: http://lkml.kernel.org/r/20180127170748.GF13338@ZenIV.linux.org.uk

Cc: stable@vger.kernel.org
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Fixes: 60f1d5e3bac44 ("ftrace: Support full glob matching")
Reported-by: Al Viro <viro@ZenIV.linux.org.uk>
Suggsted-by: Al Viro <viro@ZenIV.linux.org.uk>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 kernel/trace/trace_events_filter.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index 61e7f0678d33..a764aec3c9a1 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c
@@ -400,7 +400,6 @@ enum regex_type filter_parse_regex(char *buff, int len, char **search, int *not)
 	for (i = 0; i < len; i++) {
 		if (buff[i] == '*') {
 			if (!i) {
-				*search = buff + 1;
 				type = MATCH_END_ONLY;
 			} else if (i == len - 1) {
 				if (type == MATCH_END_ONLY)
@@ -410,14 +409,14 @@ enum regex_type filter_parse_regex(char *buff, int len, char **search, int *not)
 				buff[i] = 0;
 				break;
 			} else {	/* pattern continues, use full glob */
-				type = MATCH_GLOB;
-				break;
+				return MATCH_GLOB;
 			}
 		} else if (strchr("[?\\", buff[i])) {
-			type = MATCH_GLOB;
-			break;
+			return MATCH_GLOB;
 		}
 	}
+	if (buff[0] == '*')
+		*search = buff + 1;
 
 	return type;
 }
-- 
2.15.1

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [RFC][PATCH 3/6] selftests/ftrace: Have reset_ftrace_filter handle modules
  2018-02-06 22:43 [RFC][PATCH 0/6] tracing: Fix bogus parsing of probes and set_ftrace_filter Steven Rostedt
  2018-02-06 22:43 ` [RFC][PATCH 1/6] ftrace: Remove incorrect setting of glob search field Steven Rostedt
  2018-02-06 22:43 ` [RFC][PATCH 2/6] tracing: Fix parsing of globs with a wildcard at the beginning Steven Rostedt
@ 2018-02-06 22:43 ` Steven Rostedt
  2018-02-06 22:43 ` [RFC][PATCH 4/6] selftests/ftrace: Have reset_ftrace_filter handle multiple instances Steven Rostedt
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 11+ messages in thread
From: Steven Rostedt @ 2018-02-06 22:43 UTC (permalink / raw)
  To: linux-kernel
  Cc: Ingo Molnar, Andrew Morton, Al Viro, Masami Hiramatsu,
	Dmitry Safonov, Shuah Khan

[-- Attachment #1: 0003-selftests-ftrace-Have-reset_ftrace_filter-handle-mod.patch --]
[-- Type: text/plain, Size: 1463 bytes --]

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

If a function probe in set_ftrace_filter belongs to a module, it will
contain the module name. Like:

 wmi_query_block [wmi]:traceoff:unlimited

But writing:

 '!wmi_query_block [wmi]:traceoff' > set_ftrace_filter

will cause an error. We still need to write:

 '!wmi_query_block:traceoff' > set_ftrace_filter

Cc: Shuah Khan <shuah@kernel.org>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 tools/testing/selftests/ftrace/test.d/functions | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/ftrace/test.d/functions b/tools/testing/selftests/ftrace/test.d/functions
index f2019b37370d..e7c4c7b752a2 100644
--- a/tools/testing/selftests/ftrace/test.d/functions
+++ b/tools/testing/selftests/ftrace/test.d/functions
@@ -37,17 +37,18 @@ reset_ftrace_filter() { # reset all triggers in set_ftrace_filter
 	if [ "$tr" = "" ]; then
 	    continue
 	fi
+	name=`echo $t | cut -d: -f1 | cut -d' ' -f1`
 	if [ $tr = "enable_event" -o $tr = "disable_event" ]; then
-	    tr=`echo $t | cut -d: -f1-4`
+	    tr=`echo $t | cut -d: -f2-4`
 	    limit=`echo $t | cut -d: -f5`
 	else
-	    tr=`echo $t | cut -d: -f1-2`
+	    tr=`echo $t | cut -d: -f2`
 	    limit=`echo $t | cut -d: -f3`
 	fi
 	if [ "$limit" != "unlimited" ]; then
 	    tr="$tr:$limit"
 	fi
-	echo "!$tr" > set_ftrace_filter
+	echo "!$name:$tr" > set_ftrace_filter
     done
 }
 
-- 
2.15.1

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [RFC][PATCH 4/6] selftests/ftrace: Have reset_ftrace_filter handle multiple instances
  2018-02-06 22:43 [RFC][PATCH 0/6] tracing: Fix bogus parsing of probes and set_ftrace_filter Steven Rostedt
                   ` (2 preceding siblings ...)
  2018-02-06 22:43 ` [RFC][PATCH 3/6] selftests/ftrace: Have reset_ftrace_filter handle modules Steven Rostedt
@ 2018-02-06 22:43 ` Steven Rostedt
  2018-02-06 22:43 ` [RFC][PATCH 5/6] selftests/ftrace: Add some missing glob checks Steven Rostedt
  2018-02-06 22:43 ` [RFC][PATCH 6/6] selftests/ftrace: Add more tests for removing of function probes Steven Rostedt
  5 siblings, 0 replies; 11+ messages in thread
From: Steven Rostedt @ 2018-02-06 22:43 UTC (permalink / raw)
  To: linux-kernel
  Cc: Ingo Molnar, Andrew Morton, Al Viro, Masami Hiramatsu,
	Dmitry Safonov, Shuah Khan

[-- Attachment #1: 0004-selftests-ftrace-Have-reset_ftrace_filter-handle-mul.patch --]
[-- Type: text/plain, Size: 1462 bytes --]

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

If a probe is attached to a static function that is in multiple files with
the same name, removing it by name will remove all instances:

 # grep jump_label_unlock set_ftrace_filter
jump_label_unlock:traceoff:unlimited
jump_label_unlock:traceoff:unlimited

 # echo '!jump_label_unlock:traceoff' >> set_ftrace_filter
 # grep jump_label_unlock set_ftrace_filter
 #

But the loop in reset_ftrace_filter will try to remove multiple instances
multiple times. If this happens the second time will error and cause the
test to fail.

At each iteration of the loop, check to see if the probe being removed still
exists.

Cc: Shuah Khan <shuah@kernel.org>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 tools/testing/selftests/ftrace/test.d/functions | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/testing/selftests/ftrace/test.d/functions b/tools/testing/selftests/ftrace/test.d/functions
index e7c4c7b752a2..df3dd7fe5f9b 100644
--- a/tools/testing/selftests/ftrace/test.d/functions
+++ b/tools/testing/selftests/ftrace/test.d/functions
@@ -37,6 +37,9 @@ reset_ftrace_filter() { # reset all triggers in set_ftrace_filter
 	if [ "$tr" = "" ]; then
 	    continue
 	fi
+	if ! grep -q "$t" set_ftrace_filter; then
+		continue;
+	fi
 	name=`echo $t | cut -d: -f1 | cut -d' ' -f1`
 	if [ $tr = "enable_event" -o $tr = "disable_event" ]; then
 	    tr=`echo $t | cut -d: -f2-4`
-- 
2.15.1

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [RFC][PATCH 5/6] selftests/ftrace: Add some missing glob checks
  2018-02-06 22:43 [RFC][PATCH 0/6] tracing: Fix bogus parsing of probes and set_ftrace_filter Steven Rostedt
                   ` (3 preceding siblings ...)
  2018-02-06 22:43 ` [RFC][PATCH 4/6] selftests/ftrace: Have reset_ftrace_filter handle multiple instances Steven Rostedt
@ 2018-02-06 22:43 ` Steven Rostedt
  2018-02-08 14:07   ` Masami Hiramatsu
  2018-02-06 22:43 ` [RFC][PATCH 6/6] selftests/ftrace: Add more tests for removing of function probes Steven Rostedt
  5 siblings, 1 reply; 11+ messages in thread
From: Steven Rostedt @ 2018-02-06 22:43 UTC (permalink / raw)
  To: linux-kernel
  Cc: Ingo Molnar, Andrew Morton, Al Viro, Masami Hiramatsu,
	Dmitry Safonov, Shuah Khan

[-- Attachment #1: 0005-selftests-ftrace-Add-some-missing-glob-checks.patch --]
[-- Type: text/plain, Size: 1415 bytes --]

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

Al Viro discovered a bug in the glob ftrace filtering code where "*a*b" is
treated the same as "a*b", and functions that would be selected by "*a*b"
but not "a*b" are not selected with "*a*b".

Add tests for patterns "*a*b" and "a*b*" to the glob selftest.

Link: http://lkml.kernel.org/r/20180127170748.GF13338@ZenIV.linux.org.uk

Cc: Shuah Khan <shuah@kernel.org>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 tools/testing/selftests/ftrace/test.d/ftrace/func-filter-glob.tc | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tools/testing/selftests/ftrace/test.d/ftrace/func-filter-glob.tc b/tools/testing/selftests/ftrace/test.d/ftrace/func-filter-glob.tc
index 589d52b211b7..27a54a17da65 100644
--- a/tools/testing/selftests/ftrace/test.d/ftrace/func-filter-glob.tc
+++ b/tools/testing/selftests/ftrace/test.d/ftrace/func-filter-glob.tc
@@ -29,6 +29,12 @@ ftrace_filter_check '*schedule*' '^.*schedule.*$'
 # filter by *, end match
 ftrace_filter_check 'schedule*' '^schedule.*$'
 
+# filter by *mid*end
+ftrace_filter_check '*aw*lock' '.*aw.*lock$'
+
+# filter by start*mid*
+ftrace_filter_check 'mutex*try*' '^mutex.*try.*'
+
 # Advanced full-glob matching feature is recently supported.
 # Skip the tests if we are sure the kernel does not support it.
 if grep -q 'accepts: .* glob-matching-pattern' README ; then
-- 
2.15.1

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [RFC][PATCH 6/6] selftests/ftrace: Add more tests for removing of function probes
  2018-02-06 22:43 [RFC][PATCH 0/6] tracing: Fix bogus parsing of probes and set_ftrace_filter Steven Rostedt
                   ` (4 preceding siblings ...)
  2018-02-06 22:43 ` [RFC][PATCH 5/6] selftests/ftrace: Add some missing glob checks Steven Rostedt
@ 2018-02-06 22:43 ` Steven Rostedt
  5 siblings, 0 replies; 11+ messages in thread
From: Steven Rostedt @ 2018-02-06 22:43 UTC (permalink / raw)
  To: linux-kernel
  Cc: Ingo Molnar, Andrew Morton, Al Viro, Masami Hiramatsu,
	Dmitry Safonov, Shuah Khan

[-- Attachment #1: 0006-selftests-ftrace-Add-more-tests-for-removing-of-func.patch --]
[-- Type: text/plain, Size: 2482 bytes --]

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

Al Viro discovered a bug in the removing of function probes where if it had
a '*' at the beginning, it would fail to find any matches. That is, because
it reset the glob search string to the the initial string with a "MATCH_END"
type, instead of skipping the wildcard "*" it included it, where it would
not match any functions because "*" was being treated as a normal character
and not a wildcard one.

Link: http://lkml.kernel.org/r/20180127031706.GE13338@ZenIV.linux.org.uk

Cc: Shuah Khan <shuah@kernel.org>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 .../ftrace/test.d/ftrace/func_set_ftrace_file.tc   | 37 ++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/tools/testing/selftests/ftrace/test.d/ftrace/func_set_ftrace_file.tc b/tools/testing/selftests/ftrace/test.d/ftrace/func_set_ftrace_file.tc
index 0f3f92622e33..68e7a48f5828 100644
--- a/tools/testing/selftests/ftrace/test.d/ftrace/func_set_ftrace_file.tc
+++ b/tools/testing/selftests/ftrace/test.d/ftrace/func_set_ftrace_file.tc
@@ -128,6 +128,43 @@ if check_set_ftrace_filter "$FUNC1" "$FUNC2" ; then
     fail "Expected $FUNC1 and $FUNC2"
 fi
 
+test_actual() { # Compares $TMPDIR/expected with set_ftrace_filter
+    cat set_ftrace_filter | grep -v '#' | cut -d' ' -f1 | cut -d':' -f1 | sort -u > $TMPDIR/actual
+    DIFF=`diff $TMPDIR/actual $TMPDIR/expected`
+    test -z "$DIFF"
+}
+
+# Set traceoff trigger for all fuctions with "lock" in their name
+cat available_filter_functions | cut -d' ' -f1 |  grep 'lock' | sort -u > $TMPDIR/expected
+echo '*lock*:traceoff' > set_ftrace_filter
+test_actual
+
+# now remove all with 'try' in it, and end with lock
+grep -v 'try.*lock$' $TMPDIR/expected > $TMPDIR/expected2
+mv $TMPDIR/expected2 $TMPDIR/expected
+echo '!*try*lock:traceoff' >> set_ftrace_filter
+test_actual
+
+# remove all that start with "m" and end with "lock"
+grep -v '^m.*lock$' $TMPDIR/expected > $TMPDIR/expected2
+mv $TMPDIR/expected2 $TMPDIR/expected
+echo '!m*lock:traceoff' >> set_ftrace_filter
+test_actual
+
+# remove all that start with "c" and have "unlock"
+grep -v '^c.*unlock' $TMPDIR/expected > $TMPDIR/expected2
+mv $TMPDIR/expected2 $TMPDIR/expected
+echo '!c*unlock*:traceoff' >> set_ftrace_filter
+test_actual
+
+# clear all the rest
+> $TMPDIR/expected
+echo '!*:traceoff' >> set_ftrace_filter
+test_actual
+
+rm $TMPDIR/expected
+rm $TMPDIR/actual
+
 do_reset
 
 exit 0
-- 
2.15.1

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [RFC][PATCH 1/6] ftrace: Remove incorrect setting of glob search field
  2018-02-06 22:43 ` [RFC][PATCH 1/6] ftrace: Remove incorrect setting of glob search field Steven Rostedt
@ 2018-02-07 14:35   ` Dmitry Safonov
  2018-02-08 14:00   ` Masami Hiramatsu
  1 sibling, 0 replies; 11+ messages in thread
From: Dmitry Safonov @ 2018-02-07 14:35 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: open list, Ingo Molnar, Andrew Morton, Al Viro, Masami Hiramatsu, stable

2018-02-06 22:43 GMT+00:00 Steven Rostedt <rostedt@goodmis.org>:
> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
>
> __unregister_ftrace_function_probe() will incorrectly parse the glob filter
> because it resets the search variable that was setup by filter_parse_regex().
>
> Al Viro reported this:
>
>     After that call of filter_parse_regex() we could have func_g.search not
>     equal to glob only if glob started with '!' or '*'.  In the former case
>     we would've buggered off with -EINVAL (not = 1).  In the latter we
>     would've set func_g.search equal to glob + 1, calculated the length of
>     that thing in func_g.len and proceeded to reset func_g.search back to
>     glob.
>
>     Suppose the glob is e.g. *foo*.  We end up with
>             func_g.type = MATCH_MIDDLE_ONLY;
>             func_g.len = 3;
>             func_g.search = "*foo";
>     Feeding that to ftrace_match_record() will not do anything sane - we
>     will be looking for names containing "*foo" (->len is ignored for that
>     one).
>
> Link: http://lkml.kernel.org/r/20180127031706.GE13338@ZenIV.linux.org.uk
>
> Cc: stable@vger.kernel.org
> Cc: Dmitry Safonov <0x7f454c46@gmail.com>
> Fixes: 3ba009297149f ("ftrace: Introduce ftrace_glob structure")
> Reported-by: Al Viro <viro@ZenIV.linux.org.uk>
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

Reviewed-by: Dmitry Safonov <0x7f454c46@gmail.com>
Thank you, Steven.

-- 
             Dmitry

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFC][PATCH 1/6] ftrace: Remove incorrect setting of glob search field
  2018-02-06 22:43 ` [RFC][PATCH 1/6] ftrace: Remove incorrect setting of glob search field Steven Rostedt
  2018-02-07 14:35   ` Dmitry Safonov
@ 2018-02-08 14:00   ` Masami Hiramatsu
  1 sibling, 0 replies; 11+ messages in thread
From: Masami Hiramatsu @ 2018-02-08 14:00 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: linux-kernel, Ingo Molnar, Andrew Morton, Al Viro,
	Masami Hiramatsu, Dmitry Safonov, stable

On Tue, 06 Feb 2018 17:43:39 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> 
> __unregister_ftrace_function_probe() will incorrectly parse the glob filter
> because it resets the search variable that was setup by filter_parse_regex().
> 
> Al Viro reported this:
> 
>     After that call of filter_parse_regex() we could have func_g.search not
>     equal to glob only if glob started with '!' or '*'.  In the former case
>     we would've buggered off with -EINVAL (not = 1).  In the latter we
>     would've set func_g.search equal to glob + 1, calculated the length of
>     that thing in func_g.len and proceeded to reset func_g.search back to
>     glob.
> 
>     Suppose the glob is e.g. *foo*.  We end up with
> 	    func_g.type = MATCH_MIDDLE_ONLY;
> 	    func_g.len = 3;
> 	    func_g.search = "*foo";
>     Feeding that to ftrace_match_record() will not do anything sane - we
>     will be looking for names containing "*foo" (->len is ignored for that
>     one).
> 
> Link: http://lkml.kernel.org/r/20180127031706.GE13338@ZenIV.linux.org.uk
> 
> Cc: stable@vger.kernel.org
> Cc: Dmitry Safonov <0x7f454c46@gmail.com>
> Fixes: 3ba009297149f ("ftrace: Introduce ftrace_glob structure")
> Reported-by: Al Viro <viro@ZenIV.linux.org.uk>
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>

Thank you!

> ---
>  kernel/trace/ftrace.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index dabd9d167d42..eac9ce2c57a2 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -4456,7 +4456,6 @@ unregister_ftrace_function_probe_func(char *glob, struct trace_array *tr,
>  		func_g.type = filter_parse_regex(glob, strlen(glob),
>  						 &func_g.search, &not);
>  		func_g.len = strlen(func_g.search);
> -		func_g.search = glob;
>  
>  		/* we do not support '!' for function probes */
>  		if (WARN_ON(not))
> -- 
> 2.15.1
> 
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFC][PATCH 2/6] tracing: Fix parsing of globs with a wildcard at the beginning
  2018-02-06 22:43 ` [RFC][PATCH 2/6] tracing: Fix parsing of globs with a wildcard at the beginning Steven Rostedt
@ 2018-02-08 14:01   ` Masami Hiramatsu
  0 siblings, 0 replies; 11+ messages in thread
From: Masami Hiramatsu @ 2018-02-08 14:01 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: linux-kernel, Ingo Molnar, Andrew Morton, Al Viro,
	Masami Hiramatsu, Dmitry Safonov, stable

On Tue, 06 Feb 2018 17:43:40 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> 
> Al Viro reported:
> 
>     For substring - sure, but what about something like "*a*b" and "a*b"?
>     AFAICS, filter_parse_regex() ends up with identical results in both
>     cases - MATCH_GLOB and *search = "a*b".  And no way for the caller
>     to tell one from another.
> 
> Testing this with the following:
> 
>  # cd /sys/kernel/tracing
>  # echo '*raw*lock' > set_ftrace_filter
>  bash: echo: write error: Invalid argument
> 
> With this patch:
> 
>  # echo '*raw*lock' > set_ftrace_filter
>  # cat set_ftrace_filter
> _raw_read_trylock
> _raw_write_trylock
> _raw_read_unlock
> _raw_spin_unlock
> _raw_write_unlock
> _raw_spin_trylock
> _raw_spin_lock
> _raw_write_lock
> _raw_read_lock
> 
> Al recommended not setting the search buffer to skip the first '*' unless we
> know we are not using MATCH_GLOB. This implements his suggested logic.
> 
> Link: http://lkml.kernel.org/r/20180127170748.GF13338@ZenIV.linux.org.uk
> 
> Cc: stable@vger.kernel.org
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> Fixes: 60f1d5e3bac44 ("ftrace: Support full glob matching")
> Reported-by: Al Viro <viro@ZenIV.linux.org.uk>
> Suggsted-by: Al Viro <viro@ZenIV.linux.org.uk>
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

This looks good to me.

Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>

Thank you!

> ---
>  kernel/trace/trace_events_filter.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
> index 61e7f0678d33..a764aec3c9a1 100644
> --- a/kernel/trace/trace_events_filter.c
> +++ b/kernel/trace/trace_events_filter.c
> @@ -400,7 +400,6 @@ enum regex_type filter_parse_regex(char *buff, int len, char **search, int *not)
>  	for (i = 0; i < len; i++) {
>  		if (buff[i] == '*') {
>  			if (!i) {
> -				*search = buff + 1;
>  				type = MATCH_END_ONLY;
>  			} else if (i == len - 1) {
>  				if (type == MATCH_END_ONLY)
> @@ -410,14 +409,14 @@ enum regex_type filter_parse_regex(char *buff, int len, char **search, int *not)
>  				buff[i] = 0;
>  				break;
>  			} else {	/* pattern continues, use full glob */
> -				type = MATCH_GLOB;
> -				break;
> +				return MATCH_GLOB;
>  			}
>  		} else if (strchr("[?\\", buff[i])) {
> -			type = MATCH_GLOB;
> -			break;
> +			return MATCH_GLOB;
>  		}
>  	}
> +	if (buff[0] == '*')
> +		*search = buff + 1;
>  
>  	return type;
>  }
> -- 
> 2.15.1
> 
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFC][PATCH 5/6] selftests/ftrace: Add some missing glob checks
  2018-02-06 22:43 ` [RFC][PATCH 5/6] selftests/ftrace: Add some missing glob checks Steven Rostedt
@ 2018-02-08 14:07   ` Masami Hiramatsu
  0 siblings, 0 replies; 11+ messages in thread
From: Masami Hiramatsu @ 2018-02-08 14:07 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: linux-kernel, Ingo Molnar, Andrew Morton, Al Viro,
	Masami Hiramatsu, Dmitry Safonov, Shuah Khan

On Tue, 06 Feb 2018 17:43:43 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> 
> Al Viro discovered a bug in the glob ftrace filtering code where "*a*b" is
> treated the same as "a*b", and functions that would be selected by "*a*b"
> but not "a*b" are not selected with "*a*b".
> 
> Add tests for patterns "*a*b" and "a*b*" to the glob selftest.


Yeah, this is what we should have...

Acked-by: Masami Hiramatsu <mhiramat@kernel.org>

Thank you!

> 
> Link: http://lkml.kernel.org/r/20180127170748.GF13338@ZenIV.linux.org.uk
> 
> Cc: Shuah Khan <shuah@kernel.org>
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
>  tools/testing/selftests/ftrace/test.d/ftrace/func-filter-glob.tc | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/tools/testing/selftests/ftrace/test.d/ftrace/func-filter-glob.tc b/tools/testing/selftests/ftrace/test.d/ftrace/func-filter-glob.tc
> index 589d52b211b7..27a54a17da65 100644
> --- a/tools/testing/selftests/ftrace/test.d/ftrace/func-filter-glob.tc
> +++ b/tools/testing/selftests/ftrace/test.d/ftrace/func-filter-glob.tc
> @@ -29,6 +29,12 @@ ftrace_filter_check '*schedule*' '^.*schedule.*$'
>  # filter by *, end match
>  ftrace_filter_check 'schedule*' '^schedule.*$'
>  
> +# filter by *mid*end
> +ftrace_filter_check '*aw*lock' '.*aw.*lock$'
> +
> +# filter by start*mid*
> +ftrace_filter_check 'mutex*try*' '^mutex.*try.*'
> +
>  # Advanced full-glob matching feature is recently supported.
>  # Skip the tests if we are sure the kernel does not support it.
>  if grep -q 'accepts: .* glob-matching-pattern' README ; then
> -- 
> 2.15.1
> 
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2018-02-08 14:07 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-06 22:43 [RFC][PATCH 0/6] tracing: Fix bogus parsing of probes and set_ftrace_filter Steven Rostedt
2018-02-06 22:43 ` [RFC][PATCH 1/6] ftrace: Remove incorrect setting of glob search field Steven Rostedt
2018-02-07 14:35   ` Dmitry Safonov
2018-02-08 14:00   ` Masami Hiramatsu
2018-02-06 22:43 ` [RFC][PATCH 2/6] tracing: Fix parsing of globs with a wildcard at the beginning Steven Rostedt
2018-02-08 14:01   ` Masami Hiramatsu
2018-02-06 22:43 ` [RFC][PATCH 3/6] selftests/ftrace: Have reset_ftrace_filter handle modules Steven Rostedt
2018-02-06 22:43 ` [RFC][PATCH 4/6] selftests/ftrace: Have reset_ftrace_filter handle multiple instances Steven Rostedt
2018-02-06 22:43 ` [RFC][PATCH 5/6] selftests/ftrace: Add some missing glob checks Steven Rostedt
2018-02-08 14:07   ` Masami Hiramatsu
2018-02-06 22:43 ` [RFC][PATCH 6/6] selftests/ftrace: Add more tests for removing of function probes Steven Rostedt

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.