[PATCH 0/2] [GIT PULL] tracing: Two small fixes

[PATCH 0/2] [GIT PULL] tracing: Two small fixes

[Steven Rostedt]

Linus,

Two fixes for the tracing system.

 o When trace_printk() is used with a non constant format descriptor,
   it adds a NULL pointer into the trace format section, and the code
   isn't prepared to deal with it. This bug appeared by a change that
   was added in v3.5.

 o The ftracetest (selftests section) can't handle testing histograms
   when histograms are not configured. Currently it shows that they
   fail the test, when they should state that they are unsupported.
   This bug was added in the 4.7 merge window with the addition of
   the historgram code.

Please pull the latest trace-v4.7-rc3 tree, which can be found at:


  git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
trace-v4.7-rc3

Tag SHA1: ae75242b4ca18af8d75769d4885c7584ae5a5d4c
Head SHA1: 0ded5174e976e2b2a354fe38abf1ebf4492c6dc3


Steven Rostedt (Red Hat) (2):
      tracing: Handle NULL formats in hold_module_trace_bprintk_format()
      ftracetest: Fix hist unsupported result in hist selftests

----
 kernel/trace/trace_printk.c                                      | 7 ++++++-
 .../testing/selftests/ftrace/test.d/trigger/trigger-hist-mod.tc  | 9 ++++-----
 tools/testing/selftests/ftrace/test.d/trigger/trigger-hist.tc    | 9 ++++-----
 .../testing/selftests/ftrace/test.d/trigger/trigger-multihist.tc | 9 ++++-----
 4 files changed, 18 insertions(+), 16 deletions(-)

[PATCH 1/2] tracing: Handle NULL formats in hold_module_trace_bprintk_format()

[Steven Rostedt]

[-- Attachment #1: 0001-tracing-Handle-NULL-formats-in-hold_module_trace_bpr.patch --]
[-- Type: text/plain, Size: 2156 bytes --]

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

If a task uses a non constant string for the format parameter in
trace_printk(), then the trace_printk_fmt variable is set to NULL. This
variable is then saved in the __trace_printk_fmt section.

The function hold_module_trace_bprintk_format() checks to see if duplicate
formats are used by modules, and reuses them if so (saves them to the list
if it is new). But this function calls lookup_format() that does a strcmp()
to the value (which is now NULL) and can cause a kernel oops.

This wasn't an issue till 3debb0a9ddb ("tracing: Fix trace_printk() to print
when not using bprintk()") which added "__used" to the trace_printk_fmt
variable, and before that, the kernel simply optimized it out (no NULL value
was saved).

The fix is simply to handle the NULL pointer in lookup_format() and have the
caller ignore the value if it was NULL.

Link: http://lkml.kernel.org/r/1464769870-18344-1-git-send-email-zhengjun.xing@intel.com

Reported-by: xingzhen <zhengjun.xing@intel.com>
Acked-by: Namhyung Kim <namhyung@kernel.org>
Fixes: 3debb0a9ddb ("tracing: Fix trace_printk() to print when not using bprintk()")
Cc: stable@vger.kernel.org # v3.5+
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/trace/trace_printk.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/kernel/trace/trace_printk.c b/kernel/trace/trace_printk.c
index f96f0383f6c6..ad1d6164e946 100644
--- a/kernel/trace/trace_printk.c
+++ b/kernel/trace/trace_printk.c
@@ -36,6 +36,10 @@ struct trace_bprintk_fmt {
 static inline struct trace_bprintk_fmt *lookup_format(const char *fmt)
 {
 	struct trace_bprintk_fmt *pos;
+
+	if (!fmt)
+		return ERR_PTR(-EINVAL);
+
 	list_for_each_entry(pos, &trace_bprintk_fmt_list, list) {
 		if (!strcmp(pos->fmt, fmt))
 			return pos;
@@ -57,7 +61,8 @@ void hold_module_trace_bprintk_format(const char **start, const char **end)
 	for (iter = start; iter < end; iter++) {
 		struct trace_bprintk_fmt *tb_fmt = lookup_format(*iter);
 		if (tb_fmt) {
-			*iter = tb_fmt->fmt;
+			if (!IS_ERR(tb_fmt))
+				*iter = tb_fmt->fmt;
 			continue;
 		}
 
-- 
2.8.0.rc3

RE: [PATCH 1/2] tracing: Handle NULL formats in hold_module_trace_bprintk_format()

[Xing, Zhengjun]

I agree with you. You can also add me to the "Signed-off-by".

Best Regards,
Zhengjun

-----Original Message-----
From: Steven Rostedt [mailto:rostedt@goodmis.org] 
Sent: Monday, June 20, 2016 9:53 PM
To: linux-kernel@vger.kernel.org
Cc: Linus Torvalds <torvalds@linux-foundation.org>; Ingo Molnar <mingo@kernel.org>; Andrew Morton <akpm@linux-foundation.org>; Xing, Zhengjun <zhengjun.xing@intel.com>; Namhyung Kim <namhyung@kernel.org>; stable@vger.kernel.org
Subject: [PATCH 1/2] tracing: Handle NULL formats in hold_module_trace_bprintk_format()

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

If a task uses a non constant string for the format parameter in trace_printk(), then the trace_printk_fmt variable is set to NULL. This variable is then saved in the __trace_printk_fmt section.

The function hold_module_trace_bprintk_format() checks to see if duplicate formats are used by modules, and reuses them if so (saves them to the list if it is new). But this function calls lookup_format() that does a strcmp() to the value (which is now NULL) and can cause a kernel oops.

This wasn't an issue till 3debb0a9ddb ("tracing: Fix trace_printk() to print when not using bprintk()") which added "__used" to the trace_printk_fmt variable, and before that, the kernel simply optimized it out (no NULL value was saved).

The fix is simply to handle the NULL pointer in lookup_format() and have the caller ignore the value if it was NULL.

Link: http://lkml.kernel.org/r/1464769870-18344-1-git-send-email-zhengjun.xing@intel.com

Reported-by: xingzhen <zhengjun.xing@intel.com>
Acked-by: Namhyung Kim <namhyung@kernel.org>
Fixes: 3debb0a9ddb ("tracing: Fix trace_printk() to print when not using bprintk()")
Cc: stable@vger.kernel.org # v3.5+
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/trace/trace_printk.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/kernel/trace/trace_printk.c b/kernel/trace/trace_printk.c index f96f0383f6c6..ad1d6164e946 100644
--- a/kernel/trace/trace_printk.c
+++ b/kernel/trace/trace_printk.c
@@ -36,6 +36,10 @@ struct trace_bprintk_fmt {  static inline struct trace_bprintk_fmt *lookup_format(const char *fmt)  {
 	struct trace_bprintk_fmt *pos;
+
+	if (!fmt)
+		return ERR_PTR(-EINVAL);
+
 	list_for_each_entry(pos, &trace_bprintk_fmt_list, list) {
 		if (!strcmp(pos->fmt, fmt))
 			return pos;
@@ -57,7 +61,8 @@ void hold_module_trace_bprintk_format(const char **start, const char **end)
 	for (iter = start; iter < end; iter++) {
 		struct trace_bprintk_fmt *tb_fmt = lookup_format(*iter);
 		if (tb_fmt) {
-			*iter = tb_fmt->fmt;
+			if (!IS_ERR(tb_fmt))
+				*iter = tb_fmt->fmt;
 			continue;
 		}
 
--
2.8.0.rc3

[PATCH 2/2] ftracetest: Fix hist unsupported result in hist selftests

[Steven Rostedt]

[-- Attachment #1: 0002-ftracetest-Fix-hist-unsupported-result-in-hist-selft.patch --]
[-- Type: text/plain, Size: 3996 bytes --]

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

When histograms are not configured in the kernel, the ftracetest histogram
selftests should return "unsupported" and not "Failed". To detect this, the
test scripts have:

 FEATURE=`grep hist events/sched/sched_process_fork/trigger`
 if [ -z "$FEATURE" ]; then
     echo "hist trigger is not supported"
     exit_unsupported
 fi

The problem is that '-e' is in effect and any error will cause the program
to terminate. The grep for 'hist' fails, because it is not compiled it (thus
unsupported), but because grep has an error code for failing to find the
string, it causes the program to terminate, and is marked as a failed test.

Namhyung Kim recommended to test for the "hist" file located in
events/sched/sched_process_fork/hist instead, as it is more inline with the
other checks. As the hist file is only created if the histogram feature is
enabled, that is a valid check.

Link: http://lkml.kernel.org/r/20160523151538.4ea9ce0c@gandalf.local.home

Suggested-by: Namhyung Kim <namhyung@kernel.org>
Acked-by: Namhyung Kim <namhyung@kernel.org>
Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
Fixes: 76929ab51f0ee ("kselftests/ftrace: Add hist trigger testcases")
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 .../testing/selftests/ftrace/test.d/trigger/trigger-hist-mod.tc  | 9 ++++-----
 tools/testing/selftests/ftrace/test.d/trigger/trigger-hist.tc    | 9 ++++-----
 .../testing/selftests/ftrace/test.d/trigger/trigger-multihist.tc | 9 ++++-----
 3 files changed, 12 insertions(+), 15 deletions(-)

diff --git a/tools/testing/selftests/ftrace/test.d/trigger/trigger-hist-mod.tc b/tools/testing/selftests/ftrace/test.d/trigger/trigger-hist-mod.tc
index c2b61c4fda11..0bf5085281f3 100644
--- a/tools/testing/selftests/ftrace/test.d/trigger/trigger-hist-mod.tc
+++ b/tools/testing/selftests/ftrace/test.d/trigger/trigger-hist-mod.tc
@@ -23,15 +23,14 @@ if [ ! -f events/sched/sched_process_fork/trigger ]; then
     exit_unsupported
 fi
 
-reset_tracer
-do_reset
-
-FEATURE=`grep hist events/sched/sched_process_fork/trigger`
-if [ -z "$FEATURE" ]; then
+if [ ! -f events/sched/sched_process_fork/hist ]; then
     echo "hist trigger is not supported"
     exit_unsupported
 fi
 
+reset_tracer
+do_reset
+
 echo "Test histogram with execname modifier"
 
 echo 'hist:keys=common_pid.execname' > events/sched/sched_process_fork/trigger
diff --git a/tools/testing/selftests/ftrace/test.d/trigger/trigger-hist.tc b/tools/testing/selftests/ftrace/test.d/trigger/trigger-hist.tc
index b2902d42a537..a00184cd9c95 100644
--- a/tools/testing/selftests/ftrace/test.d/trigger/trigger-hist.tc
+++ b/tools/testing/selftests/ftrace/test.d/trigger/trigger-hist.tc
@@ -23,15 +23,14 @@ if [ ! -f events/sched/sched_process_fork/trigger ]; then
     exit_unsupported
 fi
 
-reset_tracer
-do_reset
-
-FEATURE=`grep hist events/sched/sched_process_fork/trigger`
-if [ -z "$FEATURE" ]; then
+if [ ! -f events/sched/sched_process_fork/hist ]; then
     echo "hist trigger is not supported"
     exit_unsupported
 fi
 
+reset_tracer
+do_reset
+
 echo "Test histogram basic tigger"
 
 echo 'hist:keys=parent_pid:vals=child_pid' > events/sched/sched_process_fork/trigger
diff --git a/tools/testing/selftests/ftrace/test.d/trigger/trigger-multihist.tc b/tools/testing/selftests/ftrace/test.d/trigger/trigger-multihist.tc
index 03c4a46561fc..3478b00ead57 100644
--- a/tools/testing/selftests/ftrace/test.d/trigger/trigger-multihist.tc
+++ b/tools/testing/selftests/ftrace/test.d/trigger/trigger-multihist.tc
@@ -23,15 +23,14 @@ if [ ! -f events/sched/sched_process_fork/trigger ]; then
     exit_unsupported
 fi
 
-reset_tracer
-do_reset
-
-FEATURE=`grep hist events/sched/sched_process_fork/trigger`
-if [ -z "$FEATURE" ]; then
+if [ ! -f events/sched/sched_process_fork/hist ]; then
     echo "hist trigger is not supported"
     exit_unsupported
 fi
 
+reset_tracer
+do_reset
+
 reset_trigger
 
 echo "Test histogram multiple tiggers"
-- 
2.8.0.rc3