linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2] Make _FORTIFY_SOURCE defines dependent on the feature
@ 2019-09-25 19:59 Ian Rogers
  2019-09-25 19:59 ` [PATCH 2/2] Avoid raising segv using an obvious null dereference Ian Rogers
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Ian Rogers @ 2019-09-25 19:59 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Alexander Shishkin, Jiri Olsa, Namhyung Kim, Andi Kleen,
	linux-kernel
  Cc: Stephane Eranian, Ian Rogers

Unconditionally defining _FORTIFY_SOURCE can break tools that don't work
with it, such as memory sanitizers:
https://github.com/google/sanitizers/wiki/AddressSanitizer#faq

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/lib/subcmd/Makefile | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/tools/lib/subcmd/Makefile b/tools/lib/subcmd/Makefile
index ed61fb3a46c0..5b2cd5e58df0 100644
--- a/tools/lib/subcmd/Makefile
+++ b/tools/lib/subcmd/Makefile
@@ -20,7 +20,13 @@ MAKEFLAGS += --no-print-directory
 LIBFILE = $(OUTPUT)libsubcmd.a
 
 CFLAGS := $(EXTRA_WARNINGS) $(EXTRA_CFLAGS)
-CFLAGS += -ggdb3 -Wall -Wextra -std=gnu99 -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fPIC
+CFLAGS += -ggdb3 -Wall -Wextra -std=gnu99 -fPIC
+
+ifeq ($(DEBUG),0)
+  ifeq ($(feature-fortify-source), 1)
+    CFLAGS += -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2
+  endif
+endif
 
 ifeq ($(CC_NO_CLANG), 0)
   CFLAGS += -O3
-- 
2.23.0.351.gc4317032e6-goog


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

* [PATCH 2/2] Avoid raising segv using an obvious null dereference
  2019-09-25 19:59 [PATCH 1/2] Make _FORTIFY_SOURCE defines dependent on the feature Ian Rogers
@ 2019-09-25 19:59 ` Ian Rogers
  2019-09-26 15:24   ` Arnaldo Carvalho de Melo
  2019-10-07 14:49   ` [tip: perf/urgent] perf tests: Avoid raising SEGV using an obvious NULL dereference tip-bot2 for Ian Rogers
  2019-09-26 15:18 ` [PATCH 1/2] Make _FORTIFY_SOURCE defines dependent on the feature Arnaldo Carvalho de Melo
  2019-10-07 14:49 ` [tip: perf/urgent] libsubcmd: " tip-bot2 for Ian Rogers
  2 siblings, 2 replies; 6+ messages in thread
From: Ian Rogers @ 2019-09-25 19:59 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Alexander Shishkin, Jiri Olsa, Namhyung Kim, Andi Kleen,
	linux-kernel
  Cc: Stephane Eranian, Ian Rogers

An optimized build such as:
make -C tools/perf CLANG=1 CC=clang EXTRA_CFLAGS="-O3
will turn the dereference operation into a ud2 instruction, raising a SIGILL
rather than a SIGSEGV. Use raise(..) for correctness and clarity.

Similar issues were addressed in Numfor Mbiziwo-Tiapo's patch:
https://lkml.org/lkml/2019/7/8/1234

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/tests/perf-hooks.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tools/perf/tests/perf-hooks.c b/tools/perf/tests/perf-hooks.c
index dbc27199c65e..dd865e0bea12 100644
--- a/tools/perf/tests/perf-hooks.c
+++ b/tools/perf/tests/perf-hooks.c
@@ -19,12 +19,11 @@ static void sigsegv_handler(int sig __maybe_unused)
 static void the_hook(void *_hook_flags)
 {
 	int *hook_flags = _hook_flags;
-	int *p = NULL;
 
 	*hook_flags = 1234;
 
 	/* Generate a segfault, test perf_hooks__recover */
-	*p = 0;
+	raise(SIGSEGV);
 }
 
 int test__perf_hooks(struct test *test __maybe_unused, int subtest __maybe_unused)
-- 
2.23.0.351.gc4317032e6-goog


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

* Re: [PATCH 1/2] Make _FORTIFY_SOURCE defines dependent on the feature
  2019-09-25 19:59 [PATCH 1/2] Make _FORTIFY_SOURCE defines dependent on the feature Ian Rogers
  2019-09-25 19:59 ` [PATCH 2/2] Avoid raising segv using an obvious null dereference Ian Rogers
@ 2019-09-26 15:18 ` Arnaldo Carvalho de Melo
  2019-10-07 14:49 ` [tip: perf/urgent] libsubcmd: " tip-bot2 for Ian Rogers
  2 siblings, 0 replies; 6+ messages in thread
From: Arnaldo Carvalho de Melo @ 2019-09-26 15:18 UTC (permalink / raw)
  To: Ian Rogers
  Cc: Peter Zijlstra, Ingo Molnar, Alexander Shishkin, Jiri Olsa,
	Namhyung Kim, Andi Kleen, linux-kernel, Stephane Eranian,
	Josh Poimboeuf

Em Wed, Sep 25, 2019 at 12:59:23PM -0700, Ian Rogers escreveu:
> Unconditionally defining _FORTIFY_SOURCE can break tools that don't work
> with it, such as memory sanitizers:
> https://github.com/google/sanitizers/wiki/AddressSanitizer#faq

Thanks, and added this:

Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Fixes: 4b6ab94eabe4 ("perf subcmd: Create subcmd library")
 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/lib/subcmd/Makefile | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/lib/subcmd/Makefile b/tools/lib/subcmd/Makefile
> index ed61fb3a46c0..5b2cd5e58df0 100644
> --- a/tools/lib/subcmd/Makefile
> +++ b/tools/lib/subcmd/Makefile
> @@ -20,7 +20,13 @@ MAKEFLAGS += --no-print-directory
>  LIBFILE = $(OUTPUT)libsubcmd.a
>  
>  CFLAGS := $(EXTRA_WARNINGS) $(EXTRA_CFLAGS)
> -CFLAGS += -ggdb3 -Wall -Wextra -std=gnu99 -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fPIC
> +CFLAGS += -ggdb3 -Wall -Wextra -std=gnu99 -fPIC
> +
> +ifeq ($(DEBUG),0)
> +  ifeq ($(feature-fortify-source), 1)
> +    CFLAGS += -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2
> +  endif
> +endif
>  
>  ifeq ($(CC_NO_CLANG), 0)
>    CFLAGS += -O3
> -- 
> 2.23.0.351.gc4317032e6-goog

-- 

- Arnaldo

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

* Re: [PATCH 2/2] Avoid raising segv using an obvious null dereference
  2019-09-25 19:59 ` [PATCH 2/2] Avoid raising segv using an obvious null dereference Ian Rogers
@ 2019-09-26 15:24   ` Arnaldo Carvalho de Melo
  2019-10-07 14:49   ` [tip: perf/urgent] perf tests: Avoid raising SEGV using an obvious NULL dereference tip-bot2 for Ian Rogers
  1 sibling, 0 replies; 6+ messages in thread
From: Arnaldo Carvalho de Melo @ 2019-09-26 15:24 UTC (permalink / raw)
  To: Ian Rogers
  Cc: Peter Zijlstra, Ingo Molnar, Alexander Shishkin, Jiri Olsa,
	Namhyung Kim, Andi Kleen, linux-kernel, Stephane Eranian

Em Wed, Sep 25, 2019 at 12:59:24PM -0700, Ian Rogers escreveu:
> An optimized build such as:
> make -C tools/perf CLANG=1 CC=clang EXTRA_CFLAGS="-O3
> will turn the dereference operation into a ud2 instruction, raising a SIGILL
> rather than a SIGSEGV. Use raise(..) for correctness and clarity.
> 
> Similar issues were addressed in Numfor Mbiziwo-Tiapo's patch:
> https://lkml.org/lkml/2019/7/8/1234

Added:

Cc: Numfor Mbiziwo-Tiapo <nums@google.com>

And:

Cc: Wang Nan <wangnan0@huawei.com>
Fixes: a074865e60ed ("perf tools: Introduce perf hooks")

And:

Committer testing:

Before:

  [root@quaco ~]# perf test hooks
  55: perf hooks                                            : Ok
  [root@quaco ~]# perf test -v hooks
  55: perf hooks                                            :
  --- start ---
  test child forked, pid 17092
  SIGSEGV is observed as expected, try to recover.
  Fatal error (SEGFAULT) in perf hook 'test'
  test child finished with 0
  ---- end ----
  perf hooks: Ok
  [root@quaco ~]# 

After:

  [root@quaco ~]# perf test hooks
  55: perf hooks                                            : Ok
  [root@quaco ~]# perf test -v hooks
  55: perf hooks                                            :
  --- start ---
  test child forked, pid 17909
  SIGSEGV is observed as expected, try to recover.
  Fatal error (SEGFAULT) in perf hook 'test'
  test child finished with 0
  ---- end ----
  perf hooks: Ok
  [root@quaco ~]#


 ----------------------------------------------------------------------------

Thanks, applied.

- Arnaldo
 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/tests/perf-hooks.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/tools/perf/tests/perf-hooks.c b/tools/perf/tests/perf-hooks.c
> index dbc27199c65e..dd865e0bea12 100644
> --- a/tools/perf/tests/perf-hooks.c
> +++ b/tools/perf/tests/perf-hooks.c
> @@ -19,12 +19,11 @@ static void sigsegv_handler(int sig __maybe_unused)
>  static void the_hook(void *_hook_flags)
>  {
>  	int *hook_flags = _hook_flags;
> -	int *p = NULL;
>  
>  	*hook_flags = 1234;
>  
>  	/* Generate a segfault, test perf_hooks__recover */
> -	*p = 0;
> +	raise(SIGSEGV);
>  }
>  
>  int test__perf_hooks(struct test *test __maybe_unused, int subtest __maybe_unused)
> -- 
> 2.23.0.351.gc4317032e6-goog

-- 

- Arnaldo

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

* [tip: perf/urgent] perf tests: Avoid raising SEGV using an obvious NULL dereference
  2019-09-25 19:59 ` [PATCH 2/2] Avoid raising segv using an obvious null dereference Ian Rogers
  2019-09-26 15:24   ` Arnaldo Carvalho de Melo
@ 2019-10-07 14:49   ` tip-bot2 for Ian Rogers
  1 sibling, 0 replies; 6+ messages in thread
From: tip-bot2 for Ian Rogers @ 2019-10-07 14:49 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: Ian Rogers, Arnaldo Carvalho de Melo, Alexander Shishkin,
	Andi Kleen, Jiri Olsa, Namhyung Kim, Peter Zijlstra,
	Stephane Eranian, Wang Nan, Ingo Molnar, Borislav Petkov,
	linux-kernel

The following commit has been merged into the perf/urgent branch of tip:

Commit-ID:     e3e2cf3d5b1fe800b032e14c0fdcd9a6fb20cf3b
Gitweb:        https://git.kernel.org/tip/e3e2cf3d5b1fe800b032e14c0fdcd9a6fb20cf3b
Author:        Ian Rogers <irogers@google.com>
AuthorDate:    Wed, 25 Sep 2019 12:59:24 -07:00
Committer:     Arnaldo Carvalho de Melo <acme@redhat.com>
CommitterDate: Fri, 27 Sep 2019 09:26:14 -03:00

perf tests: Avoid raising SEGV using an obvious NULL dereference

An optimized build such as:

  make -C tools/perf CLANG=1 CC=clang EXTRA_CFLAGS="-O3

will turn the dereference operation into a ud2 instruction, raising a
SIGILL rather than a SIGSEGV. Use raise(..) for correctness and clarity.

Similar issues were addressed in Numfor Mbiziwo-Tiapo's patch:

  https://lkml.org/lkml/2019/7/8/1234

Committer testing:

Before:

  [root@quaco ~]# perf test hooks
  55: perf hooks                                            : Ok
  [root@quaco ~]# perf test -v hooks
  55: perf hooks                                            :
  --- start ---
  test child forked, pid 17092
  SIGSEGV is observed as expected, try to recover.
  Fatal error (SEGFAULT) in perf hook 'test'
  test child finished with 0
  ---- end ----
  perf hooks: Ok
  [root@quaco ~]#

After:

  [root@quaco ~]# perf test hooks
  55: perf hooks                                            : Ok
  [root@quaco ~]# perf test -v hooks
  55: perf hooks                                            :
  --- start ---
  test child forked, pid 17909
  SIGSEGV is observed as expected, try to recover.
  Fatal error (SEGFAULT) in perf hook 'test'
  test child finished with 0
  ---- end ----
  perf hooks: Ok
  [root@quaco ~]#

Fixes: a074865e60ed ("perf tools: Introduce perf hooks")
Signed-off-by: Ian Rogers <irogers@google.com>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: Wang Nan <wangnan0@huawei.com>
Link: http://lore.kernel.org/lkml/20190925195924.152834-2-irogers@google.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/tests/perf-hooks.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tools/perf/tests/perf-hooks.c b/tools/perf/tests/perf-hooks.c
index dbc2719..dd865e0 100644
--- a/tools/perf/tests/perf-hooks.c
+++ b/tools/perf/tests/perf-hooks.c
@@ -19,12 +19,11 @@ static void sigsegv_handler(int sig __maybe_unused)
 static void the_hook(void *_hook_flags)
 {
 	int *hook_flags = _hook_flags;
-	int *p = NULL;
 
 	*hook_flags = 1234;
 
 	/* Generate a segfault, test perf_hooks__recover */
-	*p = 0;
+	raise(SIGSEGV);
 }
 
 int test__perf_hooks(struct test *test __maybe_unused, int subtest __maybe_unused)

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

* [tip: perf/urgent] libsubcmd: Make _FORTIFY_SOURCE defines dependent on the feature
  2019-09-25 19:59 [PATCH 1/2] Make _FORTIFY_SOURCE defines dependent on the feature Ian Rogers
  2019-09-25 19:59 ` [PATCH 2/2] Avoid raising segv using an obvious null dereference Ian Rogers
  2019-09-26 15:18 ` [PATCH 1/2] Make _FORTIFY_SOURCE defines dependent on the feature Arnaldo Carvalho de Melo
@ 2019-10-07 14:49 ` tip-bot2 for Ian Rogers
  2 siblings, 0 replies; 6+ messages in thread
From: tip-bot2 for Ian Rogers @ 2019-10-07 14:49 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: Ian Rogers, Alexander Shishkin, Andi Kleen, Jiri Olsa,
	Josh Poimboeuf, Namhyung Kim, Peter Zijlstra, Stephane Eranian,
	Arnaldo Carvalho de Melo, Ingo Molnar, Borislav Petkov,
	linux-kernel

The following commit has been merged into the perf/urgent branch of tip:

Commit-ID:     4b0b2b096da9d296e0e5668cdfba8613bd6f5bc8
Gitweb:        https://git.kernel.org/tip/4b0b2b096da9d296e0e5668cdfba8613bd6f5bc8
Author:        Ian Rogers <irogers@google.com>
AuthorDate:    Wed, 25 Sep 2019 12:59:23 -07:00
Committer:     Arnaldo Carvalho de Melo <acme@redhat.com>
CommitterDate: Fri, 27 Sep 2019 09:26:14 -03:00

libsubcmd: Make _FORTIFY_SOURCE defines dependent on the feature

Unconditionally defining _FORTIFY_SOURCE can break tools that don't work
with it, such as memory sanitizers:

  https://github.com/google/sanitizers/wiki/AddressSanitizer#faq

Fixes: 4b6ab94eabe4 ("perf subcmd: Create subcmd library")
Signed-off-by: Ian Rogers <irogers@google.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Link: http://lore.kernel.org/lkml/20190925195924.152834-1-irogers@google.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/lib/subcmd/Makefile | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/tools/lib/subcmd/Makefile b/tools/lib/subcmd/Makefile
index ed61fb3..5b2cd5e 100644
--- a/tools/lib/subcmd/Makefile
+++ b/tools/lib/subcmd/Makefile
@@ -20,7 +20,13 @@ MAKEFLAGS += --no-print-directory
 LIBFILE = $(OUTPUT)libsubcmd.a
 
 CFLAGS := $(EXTRA_WARNINGS) $(EXTRA_CFLAGS)
-CFLAGS += -ggdb3 -Wall -Wextra -std=gnu99 -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fPIC
+CFLAGS += -ggdb3 -Wall -Wextra -std=gnu99 -fPIC
+
+ifeq ($(DEBUG),0)
+  ifeq ($(feature-fortify-source), 1)
+    CFLAGS += -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2
+  endif
+endif
 
 ifeq ($(CC_NO_CLANG), 0)
   CFLAGS += -O3

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

end of thread, other threads:[~2019-10-07 14:50 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-25 19:59 [PATCH 1/2] Make _FORTIFY_SOURCE defines dependent on the feature Ian Rogers
2019-09-25 19:59 ` [PATCH 2/2] Avoid raising segv using an obvious null dereference Ian Rogers
2019-09-26 15:24   ` Arnaldo Carvalho de Melo
2019-10-07 14:49   ` [tip: perf/urgent] perf tests: Avoid raising SEGV using an obvious NULL dereference tip-bot2 for Ian Rogers
2019-09-26 15:18 ` [PATCH 1/2] Make _FORTIFY_SOURCE defines dependent on the feature Arnaldo Carvalho de Melo
2019-10-07 14:49 ` [tip: perf/urgent] libsubcmd: " tip-bot2 for Ian Rogers

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).