[RFC][PATCH 1/6] ftrace: Remove incorrect setting of glob search field

[RFC][PATCH 1/6] ftrace: Remove incorrect setting of glob search field

[Steven Rostedt]

[-- 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

[RFC][PATCH 2/6] tracing: Fix parsing of globs with a wildcard at the beginning

[Steven Rostedt]

[-- 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

Re: [RFC][PATCH 1/6] ftrace: Remove incorrect setting of glob search field

[Dmitry Safonov]

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

Re: [RFC][PATCH 1/6] ftrace: Remove incorrect setting of glob search field

[Masami Hiramatsu]

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>

Re: [RFC][PATCH 2/6] tracing: Fix parsing of globs with a wildcard at the beginning

[Masami Hiramatsu]

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>