* [PATCH 01/13] perf annotate: Properly interpret indirect call
2018-09-03 14:52 [GIT PULL 00/13] perf/urgent fixes Arnaldo Carvalho de Melo
@ 2018-09-03 14:52 ` Arnaldo Carvalho de Melo
2018-09-03 14:52 ` [PATCH 02/13] perf tests: Add breakpoint modify tests Arnaldo Carvalho de Melo
` (12 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: Arnaldo Carvalho de Melo @ 2018-09-03 14:52 UTC (permalink / raw)
To: Ingo Molnar
Cc: Clark Williams, linux-kernel, linux-perf-users, Martin Liška,
Jiri Olsa, Arnaldo Carvalho de Melo
From: Martin Liška <mliska@suse.cz>
The patch changes the parsing of:
callq *0x8(%rbx)
from:
0.26 │ → callq *8
to:
0.26 │ → callq *0x8(%rbx)
in this case an address is followed by a register, thus one can't parse
only the address.
Committer testing:
1) run 'perf record sleep 10'
2) before applying the patch, run:
perf annotate --stdio2 > /tmp/before
3) after applying the patch, run:
perf annotate --stdio2 > /tmp/after
4) diff /tmp/before /tmp/after:
--- /tmp/before 2018-08-28 11:16:03.238384143 -0300
+++ /tmp/after 2018-08-28 11:15:39.335341042 -0300
@@ -13274,7 +13274,7 @@
↓ jle 128
hash_value = hash_table->hash_func (key);
mov 0x8(%rsp),%rdi
- 0.91 → callq *30
+ 0.91 → callq *0x30(%r12)
mov $0x2,%r8d
cmp $0x2,%eax
node_hash = hash_table->hashes[node_index];
@@ -13848,7 +13848,7 @@
mov %r14,%rdi
sub %rbx,%r13
mov %r13,%rdx
- → callq *38
+ → callq *0x38(%r15)
cmp %rax,%r13
1.91 ↓ je 240
1b4: mov $0xffffffff,%r13d
@@ -14026,7 +14026,7 @@
mov %rcx,-0x500(%rbp)
mov %r15,%rsi
mov %r14,%rdi
- → callq *38
+ → callq *0x38(%rax)
mov -0x500(%rbp),%rcx
cmp %rax,%rcx
↓ jne 9b0
<SNIP tons of other such cases>
Signed-off-by: Martin Liška <mliska@suse.cz>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Tested-by: Kim Phillips <kim.phillips@arm.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Link: http://lkml.kernel.org/r/bd1f3932-be2b-85f9-7582-111ee0a43b07@suse.cz
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
tools/perf/util/annotate.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index 20061cf42288..e62b69ea87cd 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -246,8 +246,14 @@ static int call__parse(struct arch *arch, struct ins_operands *ops, struct map_s
indirect_call:
tok = strchr(endptr, '*');
- if (tok != NULL)
- ops->target.addr = strtoull(tok + 1, NULL, 16);
+ if (tok != NULL) {
+ endptr++;
+
+ /* Indirect call can use a non-rip register and offset: callq *0x8(%rbx).
+ * Do not parse such instruction. */
+ if (strstr(endptr, "(%r") == NULL)
+ ops->target.addr = strtoull(endptr, NULL, 16);
+ }
goto find_target;
}
--
2.14.4
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH 02/13] perf tests: Add breakpoint modify tests
2018-09-03 14:52 [GIT PULL 00/13] perf/urgent fixes Arnaldo Carvalho de Melo
2018-09-03 14:52 ` [PATCH 01/13] perf annotate: Properly interpret indirect call Arnaldo Carvalho de Melo
@ 2018-09-03 14:52 ` Arnaldo Carvalho de Melo
2018-09-03 14:52 ` [PATCH 03/13] perf/hw_breakpoint: Modify breakpoint even if the new attr has disabled set Arnaldo Carvalho de Melo
` (11 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: Arnaldo Carvalho de Melo @ 2018-09-03 14:52 UTC (permalink / raw)
To: Ingo Molnar
Cc: Clark Williams, linux-kernel, linux-perf-users, Jiri Olsa,
Alexander Shishkin, David Ahern, Milind Chabbi, Namhyung Kim,
Oleg Nesterov, Peter Zijlstra, Arnaldo Carvalho de Melo
From: Jiri Olsa <jolsa@kernel.org>
Adding to tests that aims on kernel breakpoint modification bugs.
First test creates HW breakpoint, tries to change it and checks it was
properly changed. It aims on kernel issue that prevents HW breakpoint to
be changed via ptrace interface.
The first test forks, the child sets itself as ptrace tracee and waits
in signal for parent to trace it, then it calls bp_1 and quits.
The parent does following steps:
- creates a new breakpoint (id 0) for bp_2 function
- changes that breakpoint to bp_1 function
- waits for the breakpoint to hit and checks
it has proper rip of bp_1 function
This test aims on an issue in kernel preventing to change disabled
breakpoints
Second test mimics the first one except for few steps
in the parent:
- creates a new breakpoint (id 0) for bp_1 function
- changes that breakpoint to bogus (-1) address
- waits for the breakpoint to hit and checks
it has proper rip of bp_1 function
This test aims on an issue in kernel disabling enabled
breakpoint after unsuccesful change.
Committer testing:
# uname -a
Linux jouet 4.18.0-rc8-00002-g1236568ee3cb #12 SMP Tue Aug 7 14:08:26 -03 2018 x86_64 x86_64 x86_64 GNU/Linux
# perf test -v "bp modify"
62: x86 bp modify :
--- start ---
test child forked, pid 25671
in bp_1
tracee exited prematurely 2
FAILED arch/x86/tests/bp-modify.c:209 modify test 1 failed
test child finished with -1
---- end ----
x86 bp modify: FAILED!
#
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Milind Chabbi <chabbi.milind@gmail.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/20180827091228.2878-2-jolsa@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
tools/perf/arch/x86/include/arch-tests.h | 1 +
tools/perf/arch/x86/tests/Build | 1 +
tools/perf/arch/x86/tests/arch-tests.c | 6 +
tools/perf/arch/x86/tests/bp-modify.c | 213 +++++++++++++++++++++++++++++++
4 files changed, 221 insertions(+)
create mode 100644 tools/perf/arch/x86/tests/bp-modify.c
diff --git a/tools/perf/arch/x86/include/arch-tests.h b/tools/perf/arch/x86/include/arch-tests.h
index c1bd979b957b..613709cfbbd0 100644
--- a/tools/perf/arch/x86/include/arch-tests.h
+++ b/tools/perf/arch/x86/include/arch-tests.h
@@ -9,6 +9,7 @@ struct test;
int test__rdpmc(struct test *test __maybe_unused, int subtest);
int test__perf_time_to_tsc(struct test *test __maybe_unused, int subtest);
int test__insn_x86(struct test *test __maybe_unused, int subtest);
+int test__bp_modify(struct test *test, int subtest);
#ifdef HAVE_DWARF_UNWIND_SUPPORT
struct thread;
diff --git a/tools/perf/arch/x86/tests/Build b/tools/perf/arch/x86/tests/Build
index 8e2c5a38c3b9..586849ff83a0 100644
--- a/tools/perf/arch/x86/tests/Build
+++ b/tools/perf/arch/x86/tests/Build
@@ -5,3 +5,4 @@ libperf-y += arch-tests.o
libperf-y += rdpmc.o
libperf-y += perf-time-to-tsc.o
libperf-$(CONFIG_AUXTRACE) += insn-x86.o
+libperf-$(CONFIG_X86_64) += bp-modify.o
diff --git a/tools/perf/arch/x86/tests/arch-tests.c b/tools/perf/arch/x86/tests/arch-tests.c
index cc1802ff5410..d47d3f8e3c8e 100644
--- a/tools/perf/arch/x86/tests/arch-tests.c
+++ b/tools/perf/arch/x86/tests/arch-tests.c
@@ -23,6 +23,12 @@ struct test arch_tests[] = {
.desc = "x86 instruction decoder - new instructions",
.func = test__insn_x86,
},
+#endif
+#if defined(__x86_64__)
+ {
+ .desc = "x86 bp modify",
+ .func = test__bp_modify,
+ },
#endif
{
.func = NULL,
diff --git a/tools/perf/arch/x86/tests/bp-modify.c b/tools/perf/arch/x86/tests/bp-modify.c
new file mode 100644
index 000000000000..f53e4406709f
--- /dev/null
+++ b/tools/perf/arch/x86/tests/bp-modify.c
@@ -0,0 +1,213 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/compiler.h>
+#include <sys/types.h>
+#include <sys/wait.h>
+#include <sys/user.h>
+#include <syscall.h>
+#include <unistd.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <sys/ptrace.h>
+#include <asm/ptrace.h>
+#include <errno.h>
+#include "debug.h"
+#include "tests/tests.h"
+#include "arch-tests.h"
+
+static noinline int bp_1(void)
+{
+ pr_debug("in %s\n", __func__);
+ return 0;
+}
+
+static noinline int bp_2(void)
+{
+ pr_debug("in %s\n", __func__);
+ return 0;
+}
+
+static int spawn_child(void)
+{
+ int child = fork();
+
+ if (child == 0) {
+ /*
+ * The child sets itself for as tracee and
+ * waits in signal for parent to trace it,
+ * then it calls bp_1 and quits.
+ */
+ int err = ptrace(PTRACE_TRACEME, 0, NULL, NULL);
+
+ if (err) {
+ pr_debug("failed to PTRACE_TRACEME\n");
+ exit(1);
+ }
+
+ raise(SIGCONT);
+ bp_1();
+ exit(0);
+ }
+
+ return child;
+}
+
+/*
+ * This tests creates HW breakpoint, tries to
+ * change it and checks it was properly changed.
+ */
+static int bp_modify1(void)
+{
+ pid_t child;
+ int status;
+ unsigned long rip = 0, dr7 = 1;
+
+ child = spawn_child();
+
+ waitpid(child, &status, 0);
+ if (WIFEXITED(status)) {
+ pr_debug("tracee exited prematurely 1\n");
+ return TEST_FAIL;
+ }
+
+ /*
+ * The parent does following steps:
+ * - creates a new breakpoint (id 0) for bp_2 function
+ * - changes that breakponit to bp_1 function
+ * - waits for the breakpoint to hit and checks
+ * it has proper rip of bp_1 function
+ * - detaches the child
+ */
+ if (ptrace(PTRACE_POKEUSER, child,
+ offsetof(struct user, u_debugreg[0]), bp_2)) {
+ pr_debug("failed to set breakpoint, 1st time: %s\n",
+ strerror(errno));
+ goto out;
+ }
+
+ if (ptrace(PTRACE_POKEUSER, child,
+ offsetof(struct user, u_debugreg[0]), bp_1)) {
+ pr_debug("failed to set breakpoint, 2nd time: %s\n",
+ strerror(errno));
+ goto out;
+ }
+
+ if (ptrace(PTRACE_POKEUSER, child,
+ offsetof(struct user, u_debugreg[7]), dr7)) {
+ pr_debug("failed to set dr7: %s\n", strerror(errno));
+ goto out;
+ }
+
+ if (ptrace(PTRACE_CONT, child, NULL, NULL)) {
+ pr_debug("failed to PTRACE_CONT: %s\n", strerror(errno));
+ goto out;
+ }
+
+ waitpid(child, &status, 0);
+ if (WIFEXITED(status)) {
+ pr_debug("tracee exited prematurely 2\n");
+ return TEST_FAIL;
+ }
+
+ rip = ptrace(PTRACE_PEEKUSER, child,
+ offsetof(struct user_regs_struct, rip), NULL);
+ if (rip == (unsigned long) -1) {
+ pr_debug("failed to PTRACE_PEEKUSER: %s\n",
+ strerror(errno));
+ goto out;
+ }
+
+ pr_debug("rip %lx, bp_1 %p\n", rip, bp_1);
+
+out:
+ if (ptrace(PTRACE_DETACH, child, NULL, NULL)) {
+ pr_debug("failed to PTRACE_DETACH: %s", strerror(errno));
+ return TEST_FAIL;
+ }
+
+ return rip == (unsigned long) bp_1 ? TEST_OK : TEST_FAIL;
+}
+
+/*
+ * This tests creates HW breakpoint, tries to
+ * change it to bogus value and checks the original
+ * breakpoint is hit.
+ */
+static int bp_modify2(void)
+{
+ pid_t child;
+ int status;
+ unsigned long rip = 0, dr7 = 1;
+
+ child = spawn_child();
+
+ waitpid(child, &status, 0);
+ if (WIFEXITED(status)) {
+ pr_debug("tracee exited prematurely 1\n");
+ return TEST_FAIL;
+ }
+
+ /*
+ * The parent does following steps:
+ * - creates a new breakpoint (id 0) for bp_1 function
+ * - tries to change that breakpoint to (-1) address
+ * - waits for the breakpoint to hit and checks
+ * it has proper rip of bp_1 function
+ * - detaches the child
+ */
+ if (ptrace(PTRACE_POKEUSER, child,
+ offsetof(struct user, u_debugreg[0]), bp_1)) {
+ pr_debug("failed to set breakpoint: %s\n",
+ strerror(errno));
+ goto out;
+ }
+
+ if (ptrace(PTRACE_POKEUSER, child,
+ offsetof(struct user, u_debugreg[7]), dr7)) {
+ pr_debug("failed to set dr7: %s\n", strerror(errno));
+ goto out;
+ }
+
+ if (!ptrace(PTRACE_POKEUSER, child,
+ offsetof(struct user, u_debugreg[0]), (unsigned long) (-1))) {
+ pr_debug("failed, breakpoint set to bogus address\n");
+ goto out;
+ }
+
+ if (ptrace(PTRACE_CONT, child, NULL, NULL)) {
+ pr_debug("failed to PTRACE_CONT: %s\n", strerror(errno));
+ goto out;
+ }
+
+ waitpid(child, &status, 0);
+ if (WIFEXITED(status)) {
+ pr_debug("tracee exited prematurely 2\n");
+ return TEST_FAIL;
+ }
+
+ rip = ptrace(PTRACE_PEEKUSER, child,
+ offsetof(struct user_regs_struct, rip), NULL);
+ if (rip == (unsigned long) -1) {
+ pr_debug("failed to PTRACE_PEEKUSER: %s\n",
+ strerror(errno));
+ goto out;
+ }
+
+ pr_debug("rip %lx, bp_1 %p\n", rip, bp_1);
+
+out:
+ if (ptrace(PTRACE_DETACH, child, NULL, NULL)) {
+ pr_debug("failed to PTRACE_DETACH: %s", strerror(errno));
+ return TEST_FAIL;
+ }
+
+ return rip == (unsigned long) bp_1 ? TEST_OK : TEST_FAIL;
+}
+
+int test__bp_modify(struct test *test __maybe_unused,
+ int subtest __maybe_unused)
+{
+ TEST_ASSERT_VAL("modify test 1 failed\n", !bp_modify1());
+ TEST_ASSERT_VAL("modify test 2 failed\n", !bp_modify2());
+
+ return 0;
+}
--
2.14.4
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH 03/13] perf/hw_breakpoint: Modify breakpoint even if the new attr has disabled set
2018-09-03 14:52 [GIT PULL 00/13] perf/urgent fixes Arnaldo Carvalho de Melo
2018-09-03 14:52 ` [PATCH 01/13] perf annotate: Properly interpret indirect call Arnaldo Carvalho de Melo
2018-09-03 14:52 ` [PATCH 02/13] perf tests: Add breakpoint modify tests Arnaldo Carvalho de Melo
@ 2018-09-03 14:52 ` Arnaldo Carvalho de Melo
2018-09-03 14:52 ` [PATCH 04/13] perf/hw_breakpoint: Remove superfluous bp->attr.disabled = 0 Arnaldo Carvalho de Melo
` (10 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: Arnaldo Carvalho de Melo @ 2018-09-03 14:52 UTC (permalink / raw)
To: Ingo Molnar
Cc: Clark Williams, linux-kernel, linux-perf-users, Jiri Olsa,
Alexander Shishkin, David Ahern, Namhyung Kim, Peter Zijlstra,
Arnaldo Carvalho de Melo
From: Jiri Olsa <jolsa@kernel.org>
We need to change the breakpoint even if the attr with new fields has
disabled set to true.
Current code prevents following user code to change the breakpoint
address:
ptrace(PTRACE_POKEUSER, child, offsetof(struct user, u_debugreg[0]), addr_1)
ptrace(PTRACE_POKEUSER, child, offsetof(struct user, u_debugreg[0]), addr_2)
ptrace(PTRACE_POKEUSER, child, offsetof(struct user, u_debugreg[7]), dr7)
The first PTRACE_POKEUSER creates the breakpoint with attr.disabled set
to true:
ptrace_set_breakpoint_addr(nr = 0)
struct perf_event *bp = t->ptrace_bps[nr];
ptrace_register_breakpoint(..., disabled = true)
ptrace_fill_bp_fields(..., disabled)
register_user_hw_breakpoint
So the second PTRACE_POKEUSER will be omitted:
ptrace_set_breakpoint_addr(nr = 0)
struct perf_event *bp = t->ptrace_bps[nr];
struct perf_event_attr attr = bp->attr;
modify_user_hw_breakpoint(bp, &attr)
if (!attr->disabled)
modify_user_hw_breakpoint_check
Reported-by: Milind Chabbi <chabbi.milind@gmail.com>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Acked-by: Frederic Weisbecker <frederic@kernel.org>
Acked-by: Oleg Nesterov <oleg@redhat.com>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/20180827091228.2878-3-jolsa@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
kernel/events/hw_breakpoint.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
index b3814fce5ecb..fb229d9c7f3c 100644
--- a/kernel/events/hw_breakpoint.c
+++ b/kernel/events/hw_breakpoint.c
@@ -509,6 +509,8 @@ modify_user_hw_breakpoint_check(struct perf_event *bp, struct perf_event_attr *a
*/
int modify_user_hw_breakpoint(struct perf_event *bp, struct perf_event_attr *attr)
{
+ int err;
+
/*
* modify_user_hw_breakpoint can be invoked with IRQs disabled and hence it
* will not be possible to raise IPIs that invoke __perf_event_disable.
@@ -520,11 +522,11 @@ int modify_user_hw_breakpoint(struct perf_event *bp, struct perf_event_attr *att
else
perf_event_disable(bp);
- if (!attr->disabled) {
- int err = modify_user_hw_breakpoint_check(bp, attr, false);
+ err = modify_user_hw_breakpoint_check(bp, attr, false);
+ if (err)
+ return err;
- if (err)
- return err;
+ if (!attr->disabled) {
perf_event_enable(bp);
bp->attr.disabled = 0;
}
--
2.14.4
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH 04/13] perf/hw_breakpoint: Remove superfluous bp->attr.disabled = 0
2018-09-03 14:52 [GIT PULL 00/13] perf/urgent fixes Arnaldo Carvalho de Melo
` (2 preceding siblings ...)
2018-09-03 14:52 ` [PATCH 03/13] perf/hw_breakpoint: Modify breakpoint even if the new attr has disabled set Arnaldo Carvalho de Melo
@ 2018-09-03 14:52 ` Arnaldo Carvalho de Melo
2018-09-03 14:52 ` [PATCH 05/13] perf/hw_breakpoint: Enable breakpoint in modify_user_hw_breakpoint Arnaldo Carvalho de Melo
` (9 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: Arnaldo Carvalho de Melo @ 2018-09-03 14:52 UTC (permalink / raw)
To: Ingo Molnar
Cc: Clark Williams, linux-kernel, linux-perf-users, Jiri Olsa,
Alexander Shishkin, David Ahern, Milind Chabbi, Namhyung Kim,
Peter Zijlstra, Arnaldo Carvalho de Melo
From: Jiri Olsa <jolsa@kernel.org>
Once the breakpoint was succesfully modified, the attr->disabled value
is in bp->attr.disabled. So there's no reason to set it again, removing
that.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Acked-by: Frederic Weisbecker <frederic@kernel.org>
Acked-by: Oleg Nesterov <oleg@redhat.com>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Milind Chabbi <chabbi.milind@gmail.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/20180827091228.2878-4-jolsa@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
kernel/events/hw_breakpoint.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
index fb229d9c7f3c..3e560d7609fd 100644
--- a/kernel/events/hw_breakpoint.c
+++ b/kernel/events/hw_breakpoint.c
@@ -526,10 +526,9 @@ int modify_user_hw_breakpoint(struct perf_event *bp, struct perf_event_attr *att
if (err)
return err;
- if (!attr->disabled) {
+ if (!attr->disabled)
perf_event_enable(bp);
- bp->attr.disabled = 0;
- }
+
return 0;
}
EXPORT_SYMBOL_GPL(modify_user_hw_breakpoint);
--
2.14.4
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH 05/13] perf/hw_breakpoint: Enable breakpoint in modify_user_hw_breakpoint
2018-09-03 14:52 [GIT PULL 00/13] perf/urgent fixes Arnaldo Carvalho de Melo
` (3 preceding siblings ...)
2018-09-03 14:52 ` [PATCH 04/13] perf/hw_breakpoint: Remove superfluous bp->attr.disabled = 0 Arnaldo Carvalho de Melo
@ 2018-09-03 14:52 ` Arnaldo Carvalho de Melo
2018-09-03 14:52 ` [PATCH 06/13] perf/hw_breakpoint: Simplify breakpoint enable in perf_event_modify_breakpoint Arnaldo Carvalho de Melo
` (8 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: Arnaldo Carvalho de Melo @ 2018-09-03 14:52 UTC (permalink / raw)
To: Ingo Molnar
Cc: Clark Williams, linux-kernel, linux-perf-users, Jiri Olsa,
Alexander Shishkin, David Ahern, Milind Chabbi, Namhyung Kim,
Peter Zijlstra, Arnaldo Carvalho de Melo
From: Jiri Olsa <jolsa@kernel.org>
Currently we enable the breakpoint back only if the breakpoint
modification was successful. If it fails we can leave the breakpoint in
disabled state with attr->disabled == 0.
We can safely enable the breakpoint back for both the fail and success
paths by checking the bp->attr.disabled, which either holds the new
'requested' disabled state or the original breakpoint state.
Suggested-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Acked-by: Frederic Weisbecker <frederic@kernel.org>
Acked-by: Oleg Nesterov <oleg@redhat.com>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Milind Chabbi <chabbi.milind@gmail.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/20180827091228.2878-5-jolsa@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
kernel/events/hw_breakpoint.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
index 3e560d7609fd..d6b56180827c 100644
--- a/kernel/events/hw_breakpoint.c
+++ b/kernel/events/hw_breakpoint.c
@@ -523,13 +523,11 @@ int modify_user_hw_breakpoint(struct perf_event *bp, struct perf_event_attr *att
perf_event_disable(bp);
err = modify_user_hw_breakpoint_check(bp, attr, false);
- if (err)
- return err;
- if (!attr->disabled)
+ if (!bp->attr.disabled)
perf_event_enable(bp);
- return 0;
+ return err;
}
EXPORT_SYMBOL_GPL(modify_user_hw_breakpoint);
--
2.14.4
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH 06/13] perf/hw_breakpoint: Simplify breakpoint enable in perf_event_modify_breakpoint
2018-09-03 14:52 [GIT PULL 00/13] perf/urgent fixes Arnaldo Carvalho de Melo
` (4 preceding siblings ...)
2018-09-03 14:52 ` [PATCH 05/13] perf/hw_breakpoint: Enable breakpoint in modify_user_hw_breakpoint Arnaldo Carvalho de Melo
@ 2018-09-03 14:52 ` Arnaldo Carvalho de Melo
2018-09-03 14:52 ` [PATCH 07/13] perf arm64: Fix include path for asm-generic/unistd.h Arnaldo Carvalho de Melo
` (7 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: Arnaldo Carvalho de Melo @ 2018-09-03 14:52 UTC (permalink / raw)
To: Ingo Molnar
Cc: Clark Williams, linux-kernel, linux-perf-users, Jiri Olsa,
Alexander Shishkin, David Ahern, Milind Chabbi, Namhyung Kim,
Peter Zijlstra, Arnaldo Carvalho de Melo
From: Jiri Olsa <jolsa@kernel.org>
We can safely enable the breakpoint back for both the fail and success
paths by checking only the bp->attr.disabled, which either holds the new
'requested' disabled state or the original breakpoint state.
Committer testing:
At the end of the series, the 'perf test' entry introduced as the first
patch now runs to completion without finding the fixed issues:
# perf test "bp modify"
62: x86 bp modify : Ok
#
In verbose mode:
# perf test -v "bp modify"
62: x86 bp modify :
--- start ---
test child forked, pid 5161
rip 5950a0, bp_1 0x5950a0
in bp_1
rip 5950a0, bp_1 0x5950a0
in bp_1
test child finished with 0
---- end ----
x86 bp modify: Ok
Suggested-by: Oleg Nesterov <oleg@redhat.com>
Acked-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Milind Chabbi <chabbi.milind@gmail.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/20180827091228.2878-6-jolsa@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
kernel/events/core.c | 11 +++--------
1 file changed, 3 insertions(+), 8 deletions(-)
diff --git a/kernel/events/core.c b/kernel/events/core.c
index f6ea33a9f904..22ede28ec07d 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -2867,16 +2867,11 @@ static int perf_event_modify_breakpoint(struct perf_event *bp,
_perf_event_disable(bp);
err = modify_user_hw_breakpoint_check(bp, attr, true);
- if (err) {
- if (!bp->attr.disabled)
- _perf_event_enable(bp);
- return err;
- }
-
- if (!attr->disabled)
+ if (!bp->attr.disabled)
_perf_event_enable(bp);
- return 0;
+
+ return err;
}
static int perf_event_modify_attr(struct perf_event *event,
--
2.14.4
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH 07/13] perf arm64: Fix include path for asm-generic/unistd.h
2018-09-03 14:52 [GIT PULL 00/13] perf/urgent fixes Arnaldo Carvalho de Melo
` (5 preceding siblings ...)
2018-09-03 14:52 ` [PATCH 06/13] perf/hw_breakpoint: Simplify breakpoint enable in perf_event_modify_breakpoint Arnaldo Carvalho de Melo
@ 2018-09-03 14:52 ` Arnaldo Carvalho de Melo
2018-09-03 14:52 ` [PATCH 08/13] perf evsel: Fix potential null pointer dereference in perf_evsel__new_idx() Arnaldo Carvalho de Melo
` (6 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: Arnaldo Carvalho de Melo @ 2018-09-03 14:52 UTC (permalink / raw)
To: Ingo Molnar
Cc: Clark Williams, linux-kernel, linux-perf-users, Kim Phillips,
Alexander Shishkin, Hendrik Brueckner, Jiri Olsa,
Michael Ellerman, Namhyung Kim, Peter Zijlstra, Ravi Bangoria,
Thomas Richter, Arnaldo Carvalho de Melo
From: Kim Phillips <kim.phillips@arm.com>
The new syscall table support for arm64 mistakenly used the system's
asm-generic/unistd.h file when processing the
tools/arch/arm64/include/uapi/asm/unistd.h file's include directive:
#include <asm-generic/unistd.h>
See "Committer notes" section of commit 2b5882435606 "perf arm64:
Generate system call table from asm/unistd.h" for more details.
This patch removes the committer's temporary workaround, and instructs
the host compiler to search the build tree's include path for the right
copy of the unistd.h file, instead of the one on the system's
/usr/include path.
It thus fixes the committer's test that cross-builds an arm64 perf on an
x86 platform running Ubuntu 14.04.5 LTS with an old toolchain:
$ tools/perf/arch/arm64/entry/syscalls/mksyscalltbl /gcc-linaro-5.4.1-2017.05-x86_64_aarch64-linux-gnu/bin/aarch64-linux-gnu-gcc gcc `pwd`/tools tools/arch/arm64/include/uapi/asm/unistd.h | grep bpf
[280] = "bpf",
Signed-off-by: Kim Phillips <kim.phillips@arm.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Hendrik Brueckner <brueckner@linux.vnet.ibm.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
Cc: Thomas Richter <tmricht@linux.vnet.ibm.com>
Fixes: 2b5882435606 ("perf arm64: Generate system call table from asm/unistd.h")
Link: http://lkml.kernel.org/r/20180806172800.bbcec3cfcc51e2facc978bf2@arm.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
tools/perf/arch/arm64/Makefile | 5 +++--
tools/perf/arch/arm64/entry/syscalls/mksyscalltbl | 6 +++---
2 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/tools/perf/arch/arm64/Makefile b/tools/perf/arch/arm64/Makefile
index f013b115dc86..dbef716a1913 100644
--- a/tools/perf/arch/arm64/Makefile
+++ b/tools/perf/arch/arm64/Makefile
@@ -11,7 +11,8 @@ PERF_HAVE_ARCH_REGS_QUERY_REGISTER_OFFSET := 1
out := $(OUTPUT)arch/arm64/include/generated/asm
header := $(out)/syscalls.c
-sysdef := $(srctree)/tools/include/uapi/asm-generic/unistd.h
+incpath := $(srctree)/tools
+sysdef := $(srctree)/tools/arch/arm64/include/uapi/asm/unistd.h
sysprf := $(srctree)/tools/perf/arch/arm64/entry/syscalls/
systbl := $(sysprf)/mksyscalltbl
@@ -19,7 +20,7 @@ systbl := $(sysprf)/mksyscalltbl
_dummy := $(shell [ -d '$(out)' ] || mkdir -p '$(out)')
$(header): $(sysdef) $(systbl)
- $(Q)$(SHELL) '$(systbl)' '$(CC)' '$(HOSTCC)' $(sysdef) > $@
+ $(Q)$(SHELL) '$(systbl)' '$(CC)' '$(HOSTCC)' $(incpath) $(sysdef) > $@
clean::
$(call QUIET_CLEAN, arm64) $(RM) $(header)
diff --git a/tools/perf/arch/arm64/entry/syscalls/mksyscalltbl b/tools/perf/arch/arm64/entry/syscalls/mksyscalltbl
index 52e197317d3e..2dbb8cade048 100755
--- a/tools/perf/arch/arm64/entry/syscalls/mksyscalltbl
+++ b/tools/perf/arch/arm64/entry/syscalls/mksyscalltbl
@@ -11,7 +11,8 @@
gcc=$1
hostcc=$2
-input=$3
+incpath=$3
+input=$4
if ! test -r $input; then
echo "Could not read input file" >&2
@@ -28,7 +29,6 @@ create_table_from_c()
cat <<-_EoHEADER
#include <stdio.h>
- #define __ARCH_WANT_RENAMEAT
#include "$input"
int main(int argc, char *argv[])
{
@@ -42,7 +42,7 @@ create_table_from_c()
printf "%s\n" " printf(\"#define SYSCALLTBL_ARM64_MAX_ID %d\\n\", __NR_$last_sc);"
printf "}\n"
- } | $hostcc -o $create_table_exe -x c -
+ } | $hostcc -I $incpath/include/uapi -o $create_table_exe -x c -
$create_table_exe
--
2.14.4
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH 08/13] perf evsel: Fix potential null pointer dereference in perf_evsel__new_idx()
2018-09-03 14:52 [GIT PULL 00/13] perf/urgent fixes Arnaldo Carvalho de Melo
` (6 preceding siblings ...)
2018-09-03 14:52 ` [PATCH 07/13] perf arm64: Fix include path for asm-generic/unistd.h Arnaldo Carvalho de Melo
@ 2018-09-03 14:52 ` Arnaldo Carvalho de Melo
2018-09-03 14:52 ` [PATCH 09/13] perf tools: Streamline bpf examples and headers installation Arnaldo Carvalho de Melo
` (5 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: Arnaldo Carvalho de Melo @ 2018-09-03 14:52 UTC (permalink / raw)
To: Ingo Molnar
Cc: Clark Williams, linux-kernel, linux-perf-users, Hisao Tanabe,
Jiri Olsa, Wang Nan, Arnaldo Carvalho de Melo
From: Hisao Tanabe <xtanabe@gmail.com>
If evsel is NULL, we should return NULL to avoid a NULL pointer
dereference a bit later in the code.
Signed-off-by: Hisao Tanabe <xtanabe@gmail.com>
Acked-by: Namhyung Kim <namhyung@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Wang Nan <wangnan0@huawei.com>
Fixes: 03e0a7df3efd ("perf tools: Introduce bpf-output event")
LPU-Reference: 20180824154556.23428-1-xtanabe@gmail.com
Link: https://lkml.kernel.org/n/tip-e5plzjhx6595a5yjaf22jss3@git.kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
tools/perf/util/evsel.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index c980bbff6353..1a61628a1c12 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -251,8 +251,9 @@ struct perf_evsel *perf_evsel__new_idx(struct perf_event_attr *attr, int idx)
{
struct perf_evsel *evsel = zalloc(perf_evsel__object.size);
- if (evsel != NULL)
- perf_evsel__init(evsel, attr, idx);
+ if (!evsel)
+ return NULL;
+ perf_evsel__init(evsel, attr, idx);
if (perf_evsel__is_bpf_output(evsel)) {
evsel->attr.sample_type |= (PERF_SAMPLE_RAW | PERF_SAMPLE_TIME |
--
2.14.4
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH 09/13] perf tools: Streamline bpf examples and headers installation
2018-09-03 14:52 [GIT PULL 00/13] perf/urgent fixes Arnaldo Carvalho de Melo
` (7 preceding siblings ...)
2018-09-03 14:52 ` [PATCH 08/13] perf evsel: Fix potential null pointer dereference in perf_evsel__new_idx() Arnaldo Carvalho de Melo
@ 2018-09-03 14:52 ` Arnaldo Carvalho de Melo
2018-09-03 14:52 ` [PATCH 10/13] perf util: Fix bad memory access in trace info Arnaldo Carvalho de Melo
` (4 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: Arnaldo Carvalho de Melo @ 2018-09-03 14:52 UTC (permalink / raw)
To: Ingo Molnar
Cc: Clark Williams, linux-kernel, linux-perf-users,
Arnaldo Carvalho de Melo, Adrian Hunter, David Ahern, Jiri Olsa,
Namhyung Kim, Wang Nan
From: Arnaldo Carvalho de Melo <acme@redhat.com>
We were emitting 4 lines, two of them misleading:
make: Entering directory '/home/acme/git/perf/tools/perf'
<SNIP>
INSTALL lib
INSTALL include/bpf
INSTALL lib
INSTALL examples/bpf
<SNIP>
make: Leaving directory '/home/acme/git/perf/tools/perf'
Make it more compact by showing just two lines:
make: Entering directory '/home/acme/git/perf/tools/perf'
INSTALL bpf-headers
INSTALL bpf-examples
make: Leaving directory '/home/acme/git/perf/tools/perf'
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Wang Nan <wangnan0@huawei.com>
Link: https://lkml.kernel.org/n/tip-0nvkyciqdkrgy829lony5925@git.kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
tools/perf/Makefile.perf | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/tools/perf/Makefile.perf b/tools/perf/Makefile.perf
index b3d1b12a5081..5224ade3d5af 100644
--- a/tools/perf/Makefile.perf
+++ b/tools/perf/Makefile.perf
@@ -777,14 +777,12 @@ endif
$(call QUIET_INSTALL, libexec) \
$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(perfexec_instdir_SQ)'
ifndef NO_LIBBPF
- $(call QUIET_INSTALL, lib) \
- $(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(perf_include_instdir_SQ)/bpf'
- $(call QUIET_INSTALL, include/bpf) \
- $(INSTALL) include/bpf/*.h '$(DESTDIR_SQ)$(perf_include_instdir_SQ)/bpf'
- $(call QUIET_INSTALL, lib) \
- $(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(perf_examples_instdir_SQ)/bpf'
- $(call QUIET_INSTALL, examples/bpf) \
- $(INSTALL) examples/bpf/*.c '$(DESTDIR_SQ)$(perf_examples_instdir_SQ)/bpf'
+ $(call QUIET_INSTALL, bpf-headers) \
+ $(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(perf_include_instdir_SQ)/bpf'; \
+ $(INSTALL) include/bpf/*.h -t '$(DESTDIR_SQ)$(perf_include_instdir_SQ)/bpf'
+ $(call QUIET_INSTALL, bpf-examples) \
+ $(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(perf_examples_instdir_SQ)/bpf'; \
+ $(INSTALL) examples/bpf/*.c -t '$(DESTDIR_SQ)$(perf_examples_instdir_SQ)/bpf'
endif
$(call QUIET_INSTALL, perf-archive) \
$(INSTALL) $(OUTPUT)perf-archive -t '$(DESTDIR_SQ)$(perfexec_instdir_SQ)'
--
2.14.4
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH 10/13] perf util: Fix bad memory access in trace info.
2018-09-03 14:52 [GIT PULL 00/13] perf/urgent fixes Arnaldo Carvalho de Melo
` (8 preceding siblings ...)
2018-09-03 14:52 ` [PATCH 09/13] perf tools: Streamline bpf examples and headers installation Arnaldo Carvalho de Melo
@ 2018-09-03 14:52 ` Arnaldo Carvalho de Melo
2018-09-03 14:52 ` [PATCH 11/13] perf event-parse: Use fixed size string for comms Arnaldo Carvalho de Melo
` (3 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: Arnaldo Carvalho de Melo @ 2018-09-03 14:52 UTC (permalink / raw)
To: Ingo Molnar
Cc: Clark Williams, linux-kernel, linux-perf-users, Chris Phlipot,
Namhyung Kim, Peter Zijlstra, Arnaldo Carvalho de Melo
From: Chris Phlipot <cphlipot0@gmail.com>
In the write to the output_fd in the error condition of
record_saved_cmdline(), we are writing 8 bytes from a memory location on
the stack that contains a primitive that is only 4 bytes in size.
Change the primitive to 8 bytes in size to match the size of the write
in order to avoid reading unknown memory from the stack.
Signed-off-by: Chris Phlipot <cphlipot0@gmail.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/20180829061954.18871-1-cphlipot0@gmail.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
tools/perf/util/trace-event-info.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/perf/util/trace-event-info.c b/tools/perf/util/trace-event-info.c
index c85d0d1a65ed..7b0ca7cbb7de 100644
--- a/tools/perf/util/trace-event-info.c
+++ b/tools/perf/util/trace-event-info.c
@@ -377,7 +377,7 @@ static int record_ftrace_printk(void)
static int record_saved_cmdline(void)
{
- unsigned int size;
+ unsigned long long size;
char *path;
struct stat st;
int ret, err = 0;
--
2.14.4
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH 11/13] perf event-parse: Use fixed size string for comms
2018-09-03 14:52 [GIT PULL 00/13] perf/urgent fixes Arnaldo Carvalho de Melo
` (9 preceding siblings ...)
2018-09-03 14:52 ` [PATCH 10/13] perf util: Fix bad memory access in trace info Arnaldo Carvalho de Melo
@ 2018-09-03 14:52 ` Arnaldo Carvalho de Melo
2018-09-03 14:52 ` [PATCH 12/13] perf probe powerpc: Ignore SyS symbols irrespective of endianness Arnaldo Carvalho de Melo
` (2 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: Arnaldo Carvalho de Melo @ 2018-09-03 14:52 UTC (permalink / raw)
To: Ingo Molnar
Cc: Clark Williams, linux-kernel, linux-perf-users, Chris Phlipot,
Namhyung Kim, Peter Zijlstra, Arnaldo Carvalho de Melo
From: Chris Phlipot <cphlipot0@gmail.com>
Some implementations of libc do not support the 'm' width modifier as
part of the scanf string format specifier. This can cause the parsing to
fail. Since the parser never checks if the scanf parsing was
successesful, this can result in a crash.
Change the comm string to be allocated as a fixed size instead of
dynamically using 'm' scanf width modifier. This can be safely done
since comm size is limited to 16 bytes by TASK_COMM_LEN within the
kernel.
This change prevents perf from crashing when linked against bionic as
well as reduces the total number of heap allocations and frees invoked
while accomplishing the same task.
Signed-off-by: Chris Phlipot <cphlipot0@gmail.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/20180830021950.15563-1-cphlipot0@gmail.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
tools/perf/util/trace-event-parse.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/tools/perf/util/trace-event-parse.c b/tools/perf/util/trace-event-parse.c
index 920b1d58a068..e76214f8d596 100644
--- a/tools/perf/util/trace-event-parse.c
+++ b/tools/perf/util/trace-event-parse.c
@@ -164,16 +164,15 @@ void parse_ftrace_printk(struct tep_handle *pevent,
void parse_saved_cmdline(struct tep_handle *pevent,
char *file, unsigned int size __maybe_unused)
{
- char *comm;
+ char comm[17]; /* Max comm length in the kernel is 16. */
char *line;
char *next = NULL;
int pid;
line = strtok_r(file, "\n", &next);
while (line) {
- sscanf(line, "%d %ms", &pid, &comm);
- tep_register_comm(pevent, comm, pid);
- free(comm);
+ if (sscanf(line, "%d %16s", &pid, comm) == 2)
+ tep_register_comm(pevent, comm, pid);
line = strtok_r(NULL, "\n", &next);
}
}
--
2.14.4
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH 12/13] perf probe powerpc: Ignore SyS symbols irrespective of endianness
2018-09-03 14:52 [GIT PULL 00/13] perf/urgent fixes Arnaldo Carvalho de Melo
` (10 preceding siblings ...)
2018-09-03 14:52 ` [PATCH 11/13] perf event-parse: Use fixed size string for comms Arnaldo Carvalho de Melo
@ 2018-09-03 14:52 ` Arnaldo Carvalho de Melo
2018-09-03 14:52 ` [PATCH 13/13] perf annotate: Fix parsing aarch64 branch instructions after objdump update Arnaldo Carvalho de Melo
2018-09-09 19:39 ` [GIT PULL 00/13] perf/urgent fixes Ingo Molnar
13 siblings, 0 replies; 15+ messages in thread
From: Arnaldo Carvalho de Melo @ 2018-09-03 14:52 UTC (permalink / raw)
To: Ingo Molnar
Cc: Clark Williams, linux-kernel, linux-perf-users, Sandipan Das,
Jiri Olsa, Ravi Bangoria, Arnaldo Carvalho de Melo
From: Sandipan Das <sandipan@linux.ibm.com>
This makes sure that the SyS symbols are ignored for any powerpc system,
not just the big endian ones.
Reported-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
Signed-off-by: Sandipan Das <sandipan@linux.ibm.com>
Reviewed-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com>
Acked-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Fixes: fb6d59423115 ("perf probe ppc: Use the right prefix when ignoring SyS symbols on ppc")
Link: http://lkml.kernel.org/r/20180828090848.1914-1-sandipan@linux.ibm.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
tools/perf/arch/powerpc/util/sym-handling.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/tools/perf/arch/powerpc/util/sym-handling.c b/tools/perf/arch/powerpc/util/sym-handling.c
index 20e7d74d86cd..10a44e946f77 100644
--- a/tools/perf/arch/powerpc/util/sym-handling.c
+++ b/tools/perf/arch/powerpc/util/sym-handling.c
@@ -22,15 +22,16 @@ bool elf__needs_adjust_symbols(GElf_Ehdr ehdr)
#endif
-#if !defined(_CALL_ELF) || _CALL_ELF != 2
int arch__choose_best_symbol(struct symbol *syma,
struct symbol *symb __maybe_unused)
{
char *sym = syma->name;
+#if !defined(_CALL_ELF) || _CALL_ELF != 2
/* Skip over any initial dot */
if (*sym == '.')
sym++;
+#endif
/* Avoid "SyS" kernel syscall aliases */
if (strlen(sym) >= 3 && !strncmp(sym, "SyS", 3))
@@ -41,6 +42,7 @@ int arch__choose_best_symbol(struct symbol *syma,
return SYMBOL_A;
}
+#if !defined(_CALL_ELF) || _CALL_ELF != 2
/* Allow matching against dot variants */
int arch__compare_symbol_names(const char *namea, const char *nameb)
{
--
2.14.4
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH 13/13] perf annotate: Fix parsing aarch64 branch instructions after objdump update
2018-09-03 14:52 [GIT PULL 00/13] perf/urgent fixes Arnaldo Carvalho de Melo
` (11 preceding siblings ...)
2018-09-03 14:52 ` [PATCH 12/13] perf probe powerpc: Ignore SyS symbols irrespective of endianness Arnaldo Carvalho de Melo
@ 2018-09-03 14:52 ` Arnaldo Carvalho de Melo
2018-09-09 19:39 ` [GIT PULL 00/13] perf/urgent fixes Ingo Molnar
13 siblings, 0 replies; 15+ messages in thread
From: Arnaldo Carvalho de Melo @ 2018-09-03 14:52 UTC (permalink / raw)
To: Ingo Molnar
Cc: Clark Williams, linux-kernel, linux-perf-users, Kim Phillips,
Alexander Shishkin, Anton Blanchard, Christian Borntraeger,
Jiri Olsa, Mark Rutland, Namhyung Kim, Peter Zijlstra,
Ravi Bangoria, Robin Murphy, Taeung Song, linux-arm-kernel,
Arnaldo Carvalho de Melo
From: Kim Phillips <kim.phillips@arm.com>
Starting with binutils 2.28, aarch64 objdump adds comments to the
disassembly output to show the alternative names of a condition code
[1].
It is assumed that commas in objdump comments could occur in other
arches now or in the future, so this fix is arch-independent.
The fix could have been done with arm64 specific jump__parse and
jump__scnprintf functions, but the jump__scnprintf instruction would
have to have its comment character be a literal, since the scnprintf
functions cannot receive a struct arch easily.
This inconvenience also applies to the generic jump__scnprintf, which is
why we add a raw_comment pointer to struct ins_operands, so the __parse
function assigns it to be re-used by its corresponding __scnprintf
function.
Example differences in 'perf annotate --stdio2' output on an aarch64
perf.data file:
BEFORE: → b.cs ffff200008133d1c <unwind_frame+0x18c> // b.hs, dffff7ecc47b
AFTER : ↓ b.cs 18c
BEFORE: → b.cc ffff200008d8d9cc <get_alloc_profile+0x31c> // b.lo, b.ul, dffff727295b
AFTER : ↓ b.cc 31c
The branch target labels 18c and 31c also now appear in the output:
BEFORE: add x26, x29, #0x80
AFTER : 18c: add x26, x29, #0x80
BEFORE: add x21, x21, #0x8
AFTER : 31c: add x21, x21, #0x8
The Fixes: tag below is added so stable branches will get the update; it
doesn't necessarily mean that commit was broken at the time, rather it
didn't withstand the aarch64 objdump update.
Tested no difference in output for sample x86_64, power arch perf.data files.
[1] https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=bb7eff5206e4795ac79c177a80fe9f4630aaf730
Signed-off-by: Kim Phillips <kim.phillips@arm.com>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Anton Blanchard <anton@samba.org>
Cc: Christian Borntraeger <borntraeger@de.ibm.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
Cc: Robin Murphy <robin.murphy@arm.com>
Cc: Taeung Song <treeze.taeung@gmail.com>
Cc: linux-arm-kernel@lists.infradead.org
Fixes: b13bbeee5ee6 ("perf annotate: Fix branch instruction with multiple operands")
Link: http://lkml.kernel.org/r/20180827125340.a2f7e291901d17cea05daba4@arm.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
tools/perf/util/annotate.c | 22 +++++++++++++++++++++-
tools/perf/util/annotate.h | 1 +
2 files changed, 22 insertions(+), 1 deletion(-)
diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index e62b69ea87cd..28cd6a17491b 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -282,7 +282,19 @@ bool ins__is_call(const struct ins *ins)
return ins->ops == &call_ops || ins->ops == &s390_call_ops;
}
-static int jump__parse(struct arch *arch __maybe_unused, struct ins_operands *ops, struct map_symbol *ms)
+/*
+ * Prevents from matching commas in the comment section, e.g.:
+ * ffff200008446e70: b.cs ffff2000084470f4 <generic_exec_single+0x314> // b.hs, b.nlast
+ */
+static inline const char *validate_comma(const char *c, struct ins_operands *ops)
+{
+ if (ops->raw_comment && c > ops->raw_comment)
+ return NULL;
+
+ return c;
+}
+
+static int jump__parse(struct arch *arch, struct ins_operands *ops, struct map_symbol *ms)
{
struct map *map = ms->map;
struct symbol *sym = ms->sym;
@@ -291,6 +303,10 @@ static int jump__parse(struct arch *arch __maybe_unused, struct ins_operands *op
};
const char *c = strchr(ops->raw, ',');
u64 start, end;
+
+ ops->raw_comment = strchr(ops->raw, arch->objdump.comment_char);
+ c = validate_comma(c, ops);
+
/*
* Examples of lines to parse for the _cpp_lex_token@@Base
* function:
@@ -310,6 +326,7 @@ static int jump__parse(struct arch *arch __maybe_unused, struct ins_operands *op
ops->target.addr = strtoull(c, NULL, 16);
if (!ops->target.addr) {
c = strchr(c, ',');
+ c = validate_comma(c, ops);
if (c++ != NULL)
ops->target.addr = strtoull(c, NULL, 16);
}
@@ -367,9 +384,12 @@ static int jump__scnprintf(struct ins *ins, char *bf, size_t size,
return scnprintf(bf, size, "%-6s %s", ins->name, ops->target.sym->name);
c = strchr(ops->raw, ',');
+ c = validate_comma(c, ops);
+
if (c != NULL) {
const char *c2 = strchr(c + 1, ',');
+ c2 = validate_comma(c2, ops);
/* check for 3-op insn */
if (c2 != NULL)
c = c2;
diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
index 005a5fe8a8c6..5399ba2321bb 100644
--- a/tools/perf/util/annotate.h
+++ b/tools/perf/util/annotate.h
@@ -22,6 +22,7 @@ struct ins {
struct ins_operands {
char *raw;
+ char *raw_comment;
struct {
char *raw;
char *name;
--
2.14.4
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [GIT PULL 00/13] perf/urgent fixes
2018-09-03 14:52 [GIT PULL 00/13] perf/urgent fixes Arnaldo Carvalho de Melo
` (12 preceding siblings ...)
2018-09-03 14:52 ` [PATCH 13/13] perf annotate: Fix parsing aarch64 branch instructions after objdump update Arnaldo Carvalho de Melo
@ 2018-09-09 19:39 ` Ingo Molnar
13 siblings, 0 replies; 15+ messages in thread
From: Ingo Molnar @ 2018-09-09 19:39 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Clark Williams, linux-kernel, linux-perf-users, Adrian Hunter,
Alexander Shishkin, Anton Blanchard, Chris Phlipot,
Christian Borntraeger, David Ahern, Frederic Weisbecker,
Hendrik Brueckner, Hisao Tanabe, Jiri Olsa, Kamalesh Babulal,
Kim Phillips, linux-arm-kernel, Mark Rutland, Martin Liška,
Michael Ellerman
* Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> Hi Ingo,
>
> Please consider pulling,
>
> - Arnaldo
>
> Test results at the end of this message, as usual.
>
> The following changes since commit 66e5db4a1ccc64f278653bc69dc406d184dc750a:
>
> Merge tag 'perf-core-for-mingo-4.19-20180820' of git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux into perf/urgent (2018-08-23 10:29:19 +0200)
>
> are available in the Git repository at:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git tags/perf-urgent-for-mingo-4.19-20180903
>
> for you to fetch changes up to 4e67b2a5df5d3f341776d12ee575e00ca3ef92de:
>
> perf annotate: Fix parsing aarch64 branch instructions after objdump update (2018-08-30 15:51:54 -0300)
>
> ----------------------------------------------------------------
> perf/urgent fixes:
>
> Kernel:
>
> - Modify breakpoint fixes (Jiri Olsa)
>
> perf annotate:
>
> - Fix parsing aarch64 branch instructions after objdump update (Kim Phillips)
>
> - Fix parsing indirect calls in 'perf annotate' (Martin Liška)
>
> perf probe:
>
> - Ignore SyS symbols irrespective of endianness on PowerPC (Sandipan Das)
>
> perf trace:
>
> - Fix include path for asm-generic/unistd.h on arm64 (Kim Phillips)
>
> Core libraries:
>
> - Fix potential null pointer dereference in perf_evsel__new_idx() (Hisao Tanabe)
>
> - Use fixed size string for comms instead of scanf("%m"), that is
> not present in the bionic libc and leads to a crash (Chris Phlipot)
>
> - Fix bad memory access in trace info on 32-bit systems, we were reading
> 8 bytes from a 4-byte long variable when saving the command line in the
> perf.data file. (Chris Phlipot)
>
> Build system:
>
> - Streamline bpf examples and headers installation, clarifying
> some install messages. (Arnaldo Carvalho de Melo)
>
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
>
> ----------------------------------------------------------------
> Arnaldo Carvalho de Melo (1):
> perf tools: Streamline bpf examples and headers installation
>
> Chris Phlipot (2):
> perf util: Fix bad memory access in trace info.
> perf event-parse: Use fixed size string for comms
>
> Hisao Tanabe (1):
> perf evsel: Fix potential null pointer dereference in perf_evsel__new_idx()
>
> Jiri Olsa (5):
> perf tests: Add breakpoint modify tests
> perf/hw_breakpoint: Modify breakpoint even if the new attr has disabled set
> perf/hw_breakpoint: Remove superfluous bp->attr.disabled = 0
> perf/hw_breakpoint: Enable breakpoint in modify_user_hw_breakpoint
> perf/hw_breakpoint: Simplify breakpoint enable in perf_event_modify_breakpoint
>
> Kim Phillips (2):
> perf arm64: Fix include path for asm-generic/unistd.h
> perf annotate: Fix parsing aarch64 branch instructions after objdump update
>
> Martin Liška (1):
> perf annotate: Properly interpret indirect call
>
> Sandipan Das (1):
> perf probe powerpc: Ignore SyS symbols irrespective of endianness
>
> kernel/events/core.c | 11 +-
> kernel/events/hw_breakpoint.c | 13 +-
> tools/perf/Makefile.perf | 14 +-
> tools/perf/arch/arm64/Makefile | 5 +-
> tools/perf/arch/arm64/entry/syscalls/mksyscalltbl | 6 +-
> tools/perf/arch/powerpc/util/sym-handling.c | 4 +-
> tools/perf/arch/x86/include/arch-tests.h | 1 +
> tools/perf/arch/x86/tests/Build | 1 +
> tools/perf/arch/x86/tests/arch-tests.c | 6 +
> tools/perf/arch/x86/tests/bp-modify.c | 213 ++++++++++++++++++++++
> tools/perf/util/annotate.c | 32 +++-
> tools/perf/util/annotate.h | 1 +
> tools/perf/util/evsel.c | 5 +-
> tools/perf/util/trace-event-info.c | 2 +-
> tools/perf/util/trace-event-parse.c | 7 +-
> 15 files changed, 282 insertions(+), 39 deletions(-)
> create mode 100644 tools/perf/arch/x86/tests/bp-modify.c
Pulled into tip:perf/urgent, thanks a lot Arnaldo!
Ingo
^ permalink raw reply [flat|nested] 15+ messages in thread