[PATCH 0/4] [GIT PULL] tracing: Some fixes for 4.8 (and earlier)

[PATCH 0/4] [GIT PULL] tracing: Some fixes for 4.8 (and earlier)

[Steven Rostedt]

Linus,

A few updates and fixes:

 . Move the suppressing of the __builtin_return_address >0 warning to the
   tracing directory only.

 . metag recordmcount fix for newer glibc's

 . Two tracing histogram fixes that were reported by KASAN

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


  git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
trace-v4.8-1

Tag SHA1: 2b56101983196edf7c1b2cfcec5fcfeb87a166c6
Head SHA1: 7522c03ae307e657114ff909aec650304371a134


Laura Abbott (1):
      ftrace/recordmcount: Work around for addition of metag magic but not relocations

Steven Rostedt (2):
      Makefile: Mute warning for __builtin_return_address(>0) for tracing only
      tracing: Fix use-after-free in hist_unreg_all/hist_enable_unreg_all

Tom Zanussi (1):
      tracing: Fix use-after-free in hist_register_trigger()

----
 Makefile                         |  1 -
 kernel/trace/Makefile            |  4 ++++
 kernel/trace/trace_events_hist.c | 14 +++++++-------
 scripts/recordmcount.c           |  9 ++++++++-
 4 files changed, 19 insertions(+), 9 deletions(-)

[PATCH 1/4] ftrace/recordmcount: Work around for addition of metag magic but not relocations

[Steven Rostedt]

[-- Attachment #1: 0001-ftrace-recordmcount-Work-around-for-addition-of-meta.patch --]
[-- Type: text/plain, Size: 1952 bytes --]

From: Laura Abbott <labbott@redhat.com>

glibc recently did a sync up (94e73c95d9b5 "elf.h: Sync with the gabi
webpage") that added a #define for EM_METAG but did not add relocations

This triggers build errors:

scripts/recordmcount.c: In function 'do_file':
scripts/recordmcount.c:466:28: error: 'R_METAG_ADDR32' undeclared (first use in this function)
  case EM_METAG:  reltype = R_METAG_ADDR32;
                            ^~~~~~~~~~~~~~
scripts/recordmcount.c:466:28: note: each undeclared identifier is reported only once for each function it appears in
scripts/recordmcount.c:468:20: error: 'R_METAG_NONE' undeclared (first use in this function)
     rel_type_nop = R_METAG_NONE;
                    ^~~~~~~~~~~~

Work around this change with some more #ifdefery for the relocations.

Fedora Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1354034

Link: http://lkml.kernel.org/r/1468005530-14757-1-git-send-email-labbott@redhat.com

Cc: stable@vger.kernel.org # v3.9+
Cc: James Hogan <james.hogan@imgtec.com>
Fixes: 00512bdd4573 ("metag: ftrace support")
Reported-by: Ross Burton <ross.burton@intel.com>
Signed-off-by: Laura Abbott <labbott@redhat.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 scripts/recordmcount.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/scripts/recordmcount.c b/scripts/recordmcount.c
index e167592793a7..42396a74405d 100644
--- a/scripts/recordmcount.c
+++ b/scripts/recordmcount.c
@@ -33,10 +33,17 @@
 #include <string.h>
 #include <unistd.h>
 
+/*
+ * glibc synced up and added the metag number but didn't add the relocations.
+ * Work around this in a crude manner for now.
+ */
 #ifndef EM_METAG
-/* Remove this when these make it to the standard system elf.h. */
 #define EM_METAG      174
+#endif
+#ifndef R_METAG_ADDR32
 #define R_METAG_ADDR32                   2
+#endif
+#ifndef R_METAG_NONE
 #define R_METAG_NONE                     3
 #endif
 
-- 
2.8.1

Re: [PATCH 1/4] ftrace/recordmcount: Work around for addition of metag magic but not relocations

[James Hogan]

[-- Attachment #1: Type: text/plain, Size: 2246 bytes --]

On Wed, Aug 03, 2016 at 09:54:17AM -0400, Steven Rostedt wrote:
> From: Laura Abbott <labbott@redhat.com>
> 
> glibc recently did a sync up (94e73c95d9b5 "elf.h: Sync with the gabi
> webpage") that added a #define for EM_METAG but did not add relocations
> 
> This triggers build errors:
> 
> scripts/recordmcount.c: In function 'do_file':
> scripts/recordmcount.c:466:28: error: 'R_METAG_ADDR32' undeclared (first use in this function)
>   case EM_METAG:  reltype = R_METAG_ADDR32;
>                             ^~~~~~~~~~~~~~
> scripts/recordmcount.c:466:28: note: each undeclared identifier is reported only once for each function it appears in
> scripts/recordmcount.c:468:20: error: 'R_METAG_NONE' undeclared (first use in this function)
>      rel_type_nop = R_METAG_NONE;
>                     ^~~~~~~~~~~~
> 
> Work around this change with some more #ifdefery for the relocations.
> 
> Fedora Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1354034
> 
> Link: http://lkml.kernel.org/r/1468005530-14757-1-git-send-email-labbott@redhat.com
> 
> Cc: stable@vger.kernel.org # v3.9+
> Cc: James Hogan <james.hogan@imgtec.com>
> Fixes: 00512bdd4573 ("metag: ftrace support")
> Reported-by: Ross Burton <ross.burton@intel.com>
> Signed-off-by: Laura Abbott <labbott@redhat.com>
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> ---
>  scripts/recordmcount.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/scripts/recordmcount.c b/scripts/recordmcount.c
> index e167592793a7..42396a74405d 100644
> --- a/scripts/recordmcount.c
> +++ b/scripts/recordmcount.c
> @@ -33,10 +33,17 @@
>  #include <string.h>
>  #include <unistd.h>
>  
> +/*
> + * glibc synced up and added the metag number but didn't add the relocations.
> + * Work around this in a crude manner for now.
> + */
>  #ifndef EM_METAG
> -/* Remove this when these make it to the standard system elf.h. */
>  #define EM_METAG      174
> +#endif
> +#ifndef R_METAG_ADDR32
>  #define R_METAG_ADDR32                   2
> +#endif
> +#ifndef R_METAG_NONE
>  #define R_METAG_NONE                     3
>  #endif

FWIW:

Reviewed-by: James Hogan <james.hogan@imgtec.com>

Thanks
James

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 2/4] Makefile: Mute warning for __builtin_return_address(>0) for tracing only

[Steven Rostedt]

[-- Attachment #1: 0002-Makefile-Mute-warning-for-__builtin_return_address-0.patch --]
[-- Type: text/plain, Size: 2113 bytes --]

From: Steven Rostedt <rostedt@goodmis.org>

With the latest gcc compilers, they give a warning if
__builtin_return_address() parameter is greater than 0. That is because if
it is used by a function called by a top level function (or in the case of
the kernel, by assembly), it can try to access stack frames outside the
stack and crash the system.

The tracing system uses __builtin_return_address() of up to 2! But it is
well aware of the dangers that it may have, and has even added precautions
to protect against it (see the thunk code in arch/x86/entry/thunk*.S)

Linus originally added KBUILD_CFLAGS that would suppress the warning for the
entire kernel, as simply adding KBUILD_CFLAGS to the tracing directory
wouldn't work. The tracing directory plays a bit with the CFLAGS and
requires a little more logic.

This adds that special logic to only suppress the warning for the tracing
directory. If it is used anywhere else outside of tracing, the warning will
still be triggered.

Link: http://lkml.kernel.org/r/20160728223043.51996267@grimm.local.home

Tested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 Makefile              | 1 -
 kernel/trace/Makefile | 4 ++++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 393b6159ae92..d384848478b9 100644
--- a/Makefile
+++ b/Makefile
@@ -620,7 +620,6 @@ include arch/$(SRCARCH)/Makefile
 
 KBUILD_CFLAGS	+= $(call cc-option,-fno-delete-null-pointer-checks,)
 KBUILD_CFLAGS	+= $(call cc-disable-warning,maybe-uninitialized,)
-KBUILD_CFLAGS	+= $(call cc-disable-warning,frame-address,)
 
 ifdef CONFIG_CC_OPTIMIZE_FOR_SIZE
 KBUILD_CFLAGS	+= -Os
diff --git a/kernel/trace/Makefile b/kernel/trace/Makefile
index 979e7bfbde7a..d0a1617b52b4 100644
--- a/kernel/trace/Makefile
+++ b/kernel/trace/Makefile
@@ -1,4 +1,8 @@
 
+# We are fully aware of the dangers of __builtin_return_address()
+FRAME_CFLAGS := $(call cc-disable-warning,frame-address)
+KBUILD_CFLAGS += $(FRAME_CFLAGS)
+
 # Do not instrument the tracer itself:
 
 ifdef CONFIG_FUNCTION_TRACER
-- 
2.8.1

[PATCH 3/4] tracing: Fix use-after-free in hist_unreg_all/hist_enable_unreg_all

[Steven Rostedt]

[-- Attachment #1: 0003-tracing-Fix-use-after-free-in-hist_unreg_all-hist_en.patch --]
[-- Type: text/plain, Size: 2226 bytes --]

From: Steven Rostedt <rostedt@goodmis.org>

While running tools/testing/selftests test suite with KASAN, Dmitry
Vyukov hit the following use-after-free report:

  ==================================================================
  BUG: KASAN: use-after-free in hist_unreg_all+0x1a1/0x1d0 at addr
  ffff880031632cc0
  Read of size 8 by task ftracetest/7413
  ==================================================================
  BUG kmalloc-128 (Not tainted): kasan: bad access detected
  ------------------------------------------------------------------

This fixes the problem, along with the same problem in
hist_enable_unreg_all().

Link: http://lkml.kernel.org/r/c3d05b79e42555b6e36a3a99aae0e37315ee5304.1467247517.git.tom.zanussi@linux.intel.com

Cc: Dmitry Vyukov <dvyukov@google.com>
[Copied Steve's hist_enable_unreg_all() fix to hist_unreg_all()]
Signed-off-by: Tom Zanussi <tom.zanussi@linux.intel.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/trace/trace_events_hist.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 0c05b8a99806..19ae135120a3 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -1500,9 +1500,9 @@ static void hist_unregister_trigger(char *glob, struct event_trigger_ops *ops,
 
 static void hist_unreg_all(struct trace_event_file *file)
 {
-	struct event_trigger_data *test;
+	struct event_trigger_data *test, *n;
 
-	list_for_each_entry_rcu(test, &file->triggers, list) {
+	list_for_each_entry_safe(test, n, &file->triggers, list) {
 		if (test->cmd_ops->trigger_type == ETT_EVENT_HIST) {
 			list_del_rcu(&test->list);
 			trace_event_trigger_enable_disable(file, 0);
@@ -1699,9 +1699,9 @@ hist_enable_get_trigger_ops(char *cmd, char *param)
 
 static void hist_enable_unreg_all(struct trace_event_file *file)
 {
-	struct event_trigger_data *test;
+	struct event_trigger_data *test, *n;
 
-	list_for_each_entry_rcu(test, &file->triggers, list) {
+	list_for_each_entry_safe(test, n, &file->triggers, list) {
 		if (test->cmd_ops->trigger_type == ETT_HIST_ENABLE) {
 			list_del_rcu(&test->list);
 			update_cond_flag(file);
-- 
2.8.1

Re: [PATCH 3/4] tracing: Fix use-after-free in hist_unreg_all/hist_enable_unreg_all

[Dmitry Vyukov]

On Wed, Aug 3, 2016 at 3:54 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> From: Steven Rostedt <rostedt@goodmis.org>
>
> While running tools/testing/selftests test suite with KASAN, Dmitry
> Vyukov hit the following use-after-free report:
>
>   ==================================================================
>   BUG: KASAN: use-after-free in hist_unreg_all+0x1a1/0x1d0 at addr
>   ffff880031632cc0
>   Read of size 8 by task ftracetest/7413
>   ==================================================================
>   BUG kmalloc-128 (Not tainted): kasan: bad access detected
>   ------------------------------------------------------------------
>
> This fixes the problem, along with the same problem in
> hist_enable_unreg_all().
>
> Link: http://lkml.kernel.org/r/c3d05b79e42555b6e36a3a99aae0e37315ee5304.1467247517.git.tom.zanussi@linux.intel.com
>
> Cc: Dmitry Vyukov <dvyukov@google.com>

s/Cc/Reported-by/ please

> [Copied Steve's hist_enable_unreg_all() fix to hist_unreg_all()]
> Signed-off-by: Tom Zanussi <tom.zanussi@linux.intel.com>
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> ---
>  kernel/trace/trace_events_hist.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> index 0c05b8a99806..19ae135120a3 100644
> --- a/kernel/trace/trace_events_hist.c
> +++ b/kernel/trace/trace_events_hist.c
> @@ -1500,9 +1500,9 @@ static void hist_unregister_trigger(char *glob, struct event_trigger_ops *ops,
>
>  static void hist_unreg_all(struct trace_event_file *file)
>  {
> -       struct event_trigger_data *test;
> +       struct event_trigger_data *test, *n;
>
> -       list_for_each_entry_rcu(test, &file->triggers, list) {
> +       list_for_each_entry_safe(test, n, &file->triggers, list) {
>                 if (test->cmd_ops->trigger_type == ETT_EVENT_HIST) {
>                         list_del_rcu(&test->list);
>                         trace_event_trigger_enable_disable(file, 0);
> @@ -1699,9 +1699,9 @@ hist_enable_get_trigger_ops(char *cmd, char *param)
>
>  static void hist_enable_unreg_all(struct trace_event_file *file)
>  {
> -       struct event_trigger_data *test;
> +       struct event_trigger_data *test, *n;
>
> -       list_for_each_entry_rcu(test, &file->triggers, list) {
> +       list_for_each_entry_safe(test, n, &file->triggers, list) {
>                 if (test->cmd_ops->trigger_type == ETT_HIST_ENABLE) {
>                         list_del_rcu(&test->list);
>                         update_cond_flag(file);
> --
> 2.8.1
>
>

Re: [PATCH 3/4] tracing: Fix use-after-free in hist_unreg_all/hist_enable_unreg_all

[Steven Rostedt]

On Wed, 3 Aug 2016 16:19:53 +0200
Dmitry Vyukov <dvyukov@google.com> wrote:

> On Wed, Aug 3, 2016 at 3:54 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> > From: Steven Rostedt <rostedt@goodmis.org>
> >
> > While running tools/testing/selftests test suite with KASAN, Dmitry
> > Vyukov hit the following use-after-free report:
> >
> >   ==================================================================
> >   BUG: KASAN: use-after-free in hist_unreg_all+0x1a1/0x1d0 at addr
> >   ffff880031632cc0
> >   Read of size 8 by task ftracetest/7413
> >   ==================================================================
> >   BUG kmalloc-128 (Not tainted): kasan: bad access detected
> >   ------------------------------------------------------------------
> >
> > This fixes the problem, along with the same problem in
> > hist_enable_unreg_all().
> >
> > Link: http://lkml.kernel.org/r/c3d05b79e42555b6e36a3a99aae0e37315ee5304.1467247517.git.tom.zanussi@linux.intel.com
> >
> > Cc: Dmitry Vyukov <dvyukov@google.com>  
> 
> s/Cc/Reported-by/ please
> 

Wish you posted that back when this was posted (see Link above).
Because now it's in my git tree and tagged for Linus to pull. Too much
work to rebase for a tag change :-/


-- Steve

[PATCH 4/4] tracing: Fix use-after-free in hist_register_trigger()

[Steven Rostedt]

[-- Attachment #1: 0004-tracing-Fix-use-after-free-in-hist_register_trigger.patch --]
[-- Type: text/plain, Size: 1278 bytes --]

From: Tom Zanussi <tom.zanussi@linux.intel.com>

This fixes a use-after-free case flagged by KASAN; make sure the test
happens before the potential free in this case.

Link: http://lkml.kernel.org/r/48fd74ab61bebd7dca9714386bb47d7c5ccd6a7b.1467247517.git.tom.zanussi@linux.intel.com

Signed-off-by: Tom Zanussi <tom.zanussi@linux.intel.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/trace/trace_events_hist.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 19ae135120a3..f3a960ed75a1 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -1441,6 +1441,9 @@ static int hist_register_trigger(char *glob, struct event_trigger_ops *ops,
 		goto out;
 	}
 
+	if (hist_data->attrs->pause)
+		data->paused = true;
+
 	if (named_data) {
 		destroy_hist_data(data->private_data);
 		data->private_data = named_data->private_data;
@@ -1448,9 +1451,6 @@ static int hist_register_trigger(char *glob, struct event_trigger_ops *ops,
 		data->ops = &event_hist_trigger_named_ops;
 	}
 
-	if (hist_data->attrs->pause)
-		data->paused = true;
-
 	if (data->ops->init) {
 		ret = data->ops->init(data->ops, data);
 		if (ret < 0)
-- 
2.8.1