* [PATCHv4 0/5] perf/hw_breakpoint: Fix breakpoint modify @ 2018-08-27 9:12 Jiri Olsa 2018-08-27 9:12 ` [PATCH 1/5] perf tests: Add breakpoint modify tests Jiri Olsa ` (5 more replies) 0 siblings, 6 replies; 14+ messages in thread From: Jiri Olsa @ 2018-08-27 9:12 UTC (permalink / raw) To: Ingo Molnar, Peter Zijlstra, Arnaldo Carvalho de Melo Cc: lkml, Namhyung Kim, David Ahern, Alexander Shishkin, Milind Chabbi, Oleg Nesterov hi, Milind reported that modify_user_hw_breakpoint wouldn't allow the breakpoint changing if the new attr had 'disabled' set to true. I found a case where it actualy prevents ptrace user interface to change the breakpoint. It's described in patch 1 as perf test, patch 2 is the breakpoint code fix. I ran strace tests, nothing (new) broken there.. v4 changes: - added Oleg's and Frederic's acks v3 changes: - added Oleg's ack for patch 3 - new patches 4,5 based on Oleg's suggestions replacing the v2 fallback approach by enabling the event directly after failed modification v2 changes: - added Oleg's ack for patch 2 - added new changes based on Oleg's questions plus new test code thanks, jirka --- 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 kernel/events/core.c | 11 ++----- kernel/events/hw_breakpoint.c | 13 ++++---- 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 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 230 insertions(+), 15 deletions(-) create mode 100644 tools/perf/arch/x86/tests/bp-modify.c ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/5] perf tests: Add breakpoint modify tests 2018-08-27 9:12 [PATCHv4 0/5] perf/hw_breakpoint: Fix breakpoint modify Jiri Olsa @ 2018-08-27 9:12 ` Jiri Olsa 2018-09-06 13:01 ` [tip:perf/core] " tip-bot for Jiri Olsa 2018-08-27 9:12 ` [PATCH 2/5] perf/hw_breakpoint: Modify breakpoint even if the new attr has disabled set Jiri Olsa ` (4 subsequent siblings) 5 siblings, 1 reply; 14+ messages in thread From: Jiri Olsa @ 2018-08-27 9:12 UTC (permalink / raw) To: Ingo Molnar, Peter Zijlstra, Arnaldo Carvalho de Melo Cc: lkml, Namhyung Kim, David Ahern, Alexander Shishkin, Milind Chabbi, Oleg Nesterov 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. Link: http://lkml.kernel.org/n/tip-8a3x8utnljw8xtumkxkxkt18@git.kernel.org Signed-off-by: Jiri Olsa <jolsa@kernel.org> --- 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.17.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [tip:perf/core] perf tests: Add breakpoint modify tests 2018-08-27 9:12 ` [PATCH 1/5] perf tests: Add breakpoint modify tests Jiri Olsa @ 2018-09-06 13:01 ` tip-bot for Jiri Olsa 0 siblings, 0 replies; 14+ messages in thread From: tip-bot for Jiri Olsa @ 2018-09-06 13:01 UTC (permalink / raw) To: linux-tip-commits Cc: mingo, alexander.shishkin, peterz, namhyung, linux-kernel, jolsa, acme, oleg, hpa, chabbi.milind, tglx, dsahern Commit-ID: 9b3579fc6c6ac45502de1fa9a1fdf873805c2157 Gitweb: https://git.kernel.org/tip/9b3579fc6c6ac45502de1fa9a1fdf873805c2157 Author: Jiri Olsa <jolsa@kernel.org> AuthorDate: Mon, 27 Aug 2018 11:12:24 +0200 Committer: Arnaldo Carvalho de Melo <acme@redhat.com> CommitDate: Thu, 30 Aug 2018 14:49:22 -0300 perf tests: Add breakpoint modify tests 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(+) 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; +} ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 2/5] perf/hw_breakpoint: Modify breakpoint even if the new attr has disabled set 2018-08-27 9:12 [PATCHv4 0/5] perf/hw_breakpoint: Fix breakpoint modify Jiri Olsa 2018-08-27 9:12 ` [PATCH 1/5] perf tests: Add breakpoint modify tests Jiri Olsa @ 2018-08-27 9:12 ` Jiri Olsa 2018-09-06 13:02 ` [tip:perf/core] " tip-bot for Jiri Olsa 2018-08-27 9:12 ` [PATCH 3/5] perf/hw_breakpoint: Remove superfluous bp->attr.disabled = 0 Jiri Olsa ` (3 subsequent siblings) 5 siblings, 1 reply; 14+ messages in thread From: Jiri Olsa @ 2018-08-27 9:12 UTC (permalink / raw) To: Ingo Molnar, Peter Zijlstra, Arnaldo Carvalho de Melo Cc: lkml, Namhyung Kim, David Ahern, Alexander Shishkin, Milind Chabbi, Oleg Nesterov 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 Acked-by: Oleg Nesterov <oleg@redhat.com> Acked-by: Frederic Weisbecker <frederic@kernel.org> Reported-by: Milind Chabbi <chabbi.milind@gmail.com> Link: http://lkml.kernel.org/n/tip-yjhgplc28gk5gfzt7ceooe6z@git.kernel.org Signed-off-by: Jiri Olsa <jolsa@kernel.org> --- 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.17.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [tip:perf/core] perf/hw_breakpoint: Modify breakpoint even if the new attr has disabled set 2018-08-27 9:12 ` [PATCH 2/5] perf/hw_breakpoint: Modify breakpoint even if the new attr has disabled set Jiri Olsa @ 2018-09-06 13:02 ` tip-bot for Jiri Olsa 0 siblings, 0 replies; 14+ messages in thread From: tip-bot for Jiri Olsa @ 2018-09-06 13:02 UTC (permalink / raw) To: linux-tip-commits Cc: alexander.shishkin, frederic, oleg, linux-kernel, hpa, mingo, peterz, namhyung, tglx, dsahern, jolsa, acme, chabbi.milind Commit-ID: bd14406b78e6daa1ea3c1673bda1ffc9efdeead0 Gitweb: https://git.kernel.org/tip/bd14406b78e6daa1ea3c1673bda1ffc9efdeead0 Author: Jiri Olsa <jolsa@kernel.org> AuthorDate: Mon, 27 Aug 2018 11:12:25 +0200 Committer: Arnaldo Carvalho de Melo <acme@redhat.com> CommitDate: Thu, 30 Aug 2018 14:49:23 -0300 perf/hw_breakpoint: Modify breakpoint even if the new attr has disabled set 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; } ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 3/5] perf/hw_breakpoint: Remove superfluous bp->attr.disabled = 0 2018-08-27 9:12 [PATCHv4 0/5] perf/hw_breakpoint: Fix breakpoint modify Jiri Olsa 2018-08-27 9:12 ` [PATCH 1/5] perf tests: Add breakpoint modify tests Jiri Olsa 2018-08-27 9:12 ` [PATCH 2/5] perf/hw_breakpoint: Modify breakpoint even if the new attr has disabled set Jiri Olsa @ 2018-08-27 9:12 ` Jiri Olsa 2018-09-06 13:02 ` [tip:perf/core] " tip-bot for Jiri Olsa 2018-08-27 9:12 ` [PATCH 4/5] perf/hw_breakpoint: Enable breakpoint in modify_user_hw_breakpoint Jiri Olsa ` (2 subsequent siblings) 5 siblings, 1 reply; 14+ messages in thread From: Jiri Olsa @ 2018-08-27 9:12 UTC (permalink / raw) To: Ingo Molnar, Peter Zijlstra, Arnaldo Carvalho de Melo Cc: lkml, Namhyung Kim, David Ahern, Alexander Shishkin, Milind Chabbi, Oleg Nesterov 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. Acked-by: Oleg Nesterov <oleg@redhat.com> Acked-by: Frederic Weisbecker <frederic@kernel.org> Link: http://lkml.kernel.org/n/tip-v5oaellzsmyszv3rfucuxkp0@git.kernel.org Signed-off-by: Jiri Olsa <jolsa@kernel.org> --- 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.17.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [tip:perf/core] perf/hw_breakpoint: Remove superfluous bp->attr.disabled = 0 2018-08-27 9:12 ` [PATCH 3/5] perf/hw_breakpoint: Remove superfluous bp->attr.disabled = 0 Jiri Olsa @ 2018-09-06 13:02 ` tip-bot for Jiri Olsa 0 siblings, 0 replies; 14+ messages in thread From: tip-bot for Jiri Olsa @ 2018-09-06 13:02 UTC (permalink / raw) To: linux-tip-commits Cc: mingo, peterz, jolsa, tglx, linux-kernel, namhyung, dsahern, alexander.shishkin, hpa, acme, oleg, chabbi.milind, frederic Commit-ID: cb45302d7c5e20f0c0598cdbd7753fa44daceb2a Gitweb: https://git.kernel.org/tip/cb45302d7c5e20f0c0598cdbd7753fa44daceb2a Author: Jiri Olsa <jolsa@kernel.org> AuthorDate: Mon, 27 Aug 2018 11:12:26 +0200 Committer: Arnaldo Carvalho de Melo <acme@redhat.com> CommitDate: Thu, 30 Aug 2018 14:49:23 -0300 perf/hw_breakpoint: Remove superfluous bp->attr.disabled = 0 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); ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 4/5] perf/hw_breakpoint: Enable breakpoint in modify_user_hw_breakpoint 2018-08-27 9:12 [PATCHv4 0/5] perf/hw_breakpoint: Fix breakpoint modify Jiri Olsa ` (2 preceding siblings ...) 2018-08-27 9:12 ` [PATCH 3/5] perf/hw_breakpoint: Remove superfluous bp->attr.disabled = 0 Jiri Olsa @ 2018-08-27 9:12 ` Jiri Olsa 2018-09-06 13:03 ` [tip:perf/core] " tip-bot for Jiri Olsa 2018-08-27 9:12 ` [PATCH 5/5] perf/hw_breakpoint: Simplify breakpoint enable in perf_event_modify_breakpoint Jiri Olsa 2018-08-30 18:39 ` [PATCHv4 0/5] perf/hw_breakpoint: Fix breakpoint modify Arnaldo Carvalho de Melo 5 siblings, 1 reply; 14+ messages in thread From: Jiri Olsa @ 2018-08-27 9:12 UTC (permalink / raw) To: Ingo Molnar, Peter Zijlstra, Arnaldo Carvalho de Melo Cc: lkml, Namhyung Kim, David Ahern, Alexander Shishkin, Milind Chabbi, Oleg Nesterov 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> Acked-by: Oleg Nesterov <oleg@redhat.com> Acked-by: Frederic Weisbecker <frederic@kernel.org> Link: http://lkml.kernel.org/n/tip-79p9ttocwy2ju2s6qh25dvre@git.kernel.org Signed-off-by: Jiri Olsa <jolsa@kernel.org> --- 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.17.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [tip:perf/core] perf/hw_breakpoint: Enable breakpoint in modify_user_hw_breakpoint 2018-08-27 9:12 ` [PATCH 4/5] perf/hw_breakpoint: Enable breakpoint in modify_user_hw_breakpoint Jiri Olsa @ 2018-09-06 13:03 ` tip-bot for Jiri Olsa 0 siblings, 0 replies; 14+ messages in thread From: tip-bot for Jiri Olsa @ 2018-09-06 13:03 UTC (permalink / raw) To: linux-tip-commits Cc: acme, jolsa, oleg, tglx, peterz, mingo, dsahern, hpa, namhyung, frederic, linux-kernel, alexander.shishkin, chabbi.milind Commit-ID: 969558371bf926258241727ebb994f516f2e6f61 Gitweb: https://git.kernel.org/tip/969558371bf926258241727ebb994f516f2e6f61 Author: Jiri Olsa <jolsa@kernel.org> AuthorDate: Mon, 27 Aug 2018 11:12:27 +0200 Committer: Arnaldo Carvalho de Melo <acme@redhat.com> CommitDate: Thu, 30 Aug 2018 14:49:23 -0300 perf/hw_breakpoint: Enable breakpoint in modify_user_hw_breakpoint 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); ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 5/5] perf/hw_breakpoint: Simplify breakpoint enable in perf_event_modify_breakpoint 2018-08-27 9:12 [PATCHv4 0/5] perf/hw_breakpoint: Fix breakpoint modify Jiri Olsa ` (3 preceding siblings ...) 2018-08-27 9:12 ` [PATCH 4/5] perf/hw_breakpoint: Enable breakpoint in modify_user_hw_breakpoint Jiri Olsa @ 2018-08-27 9:12 ` Jiri Olsa 2018-09-06 13:03 ` [tip:perf/core] " tip-bot for Jiri Olsa 2018-08-30 18:39 ` [PATCHv4 0/5] perf/hw_breakpoint: Fix breakpoint modify Arnaldo Carvalho de Melo 5 siblings, 1 reply; 14+ messages in thread From: Jiri Olsa @ 2018-08-27 9:12 UTC (permalink / raw) To: Ingo Molnar, Peter Zijlstra, Arnaldo Carvalho de Melo Cc: lkml, Namhyung Kim, David Ahern, Alexander Shishkin, Milind Chabbi, Oleg Nesterov 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. Suggested-by: Oleg Nesterov <oleg@redhat.com> Acked-by: Oleg Nesterov <oleg@redhat.com> Link: http://lkml.kernel.org/n/tip-vo1jm1u2nar6lj6hnd9g73ug@git.kernel.org Signed-off-by: Jiri Olsa <jolsa@kernel.org> --- 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.17.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [tip:perf/core] perf/hw_breakpoint: Simplify breakpoint enable in perf_event_modify_breakpoint 2018-08-27 9:12 ` [PATCH 5/5] perf/hw_breakpoint: Simplify breakpoint enable in perf_event_modify_breakpoint Jiri Olsa @ 2018-09-06 13:03 ` tip-bot for Jiri Olsa 0 siblings, 0 replies; 14+ messages in thread From: tip-bot for Jiri Olsa @ 2018-09-06 13:03 UTC (permalink / raw) To: linux-tip-commits Cc: hpa, namhyung, jolsa, chabbi.milind, peterz, mingo, alexander.shishkin, dsahern, oleg, acme, linux-kernel, tglx Commit-ID: bf06278c3fdf8909c3a9283e2c270b0fc170fa90 Gitweb: https://git.kernel.org/tip/bf06278c3fdf8909c3a9283e2c270b0fc170fa90 Author: Jiri Olsa <jolsa@kernel.org> AuthorDate: Mon, 27 Aug 2018 11:12:28 +0200 Committer: Arnaldo Carvalho de Melo <acme@redhat.com> CommitDate: Thu, 30 Aug 2018 14:49:24 -0300 perf/hw_breakpoint: Simplify breakpoint enable in perf_event_modify_breakpoint 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, ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCHv4 0/5] perf/hw_breakpoint: Fix breakpoint modify 2018-08-27 9:12 [PATCHv4 0/5] perf/hw_breakpoint: Fix breakpoint modify Jiri Olsa ` (4 preceding siblings ...) 2018-08-27 9:12 ` [PATCH 5/5] perf/hw_breakpoint: Simplify breakpoint enable in perf_event_modify_breakpoint Jiri Olsa @ 2018-08-30 18:39 ` Arnaldo Carvalho de Melo 5 siblings, 0 replies; 14+ messages in thread From: Arnaldo Carvalho de Melo @ 2018-08-30 18:39 UTC (permalink / raw) To: Jiri Olsa Cc: Ingo Molnar, Peter Zijlstra, lkml, Namhyung Kim, David Ahern, Alexander Shishkin, Milind Chabbi, Oleg Nesterov Em Mon, Aug 27, 2018 at 11:12:23AM +0200, Jiri Olsa escreveu: > hi, > Milind reported that modify_user_hw_breakpoint wouldn't > allow the breakpoint changing if the new attr had 'disabled' > set to true. > > I found a case where it actualy prevents ptrace user interface > to change the breakpoint. It's described in patch 1 as perf test, > patch 2 is the breakpoint code fix. > > I ran strace tests, nothing (new) broken there.. Thanks, tested and applied. - Arnaldo ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCHv3 0/5] perf/hw_breakpoint: Fix breakpoint modify @ 2018-08-10 10:47 Jiri Olsa 2018-08-10 10:47 ` [PATCH 5/5] perf/hw_breakpoint: Simplify breakpoint enable in perf_event_modify_breakpoint Jiri Olsa 0 siblings, 1 reply; 14+ messages in thread From: Jiri Olsa @ 2018-08-10 10:47 UTC (permalink / raw) To: Arnaldo Carvalho de Melo Cc: lkml, Ingo Molnar, Namhyung Kim, David Ahern, Alexander Shishkin, Peter Zijlstra, Milind Chabbi, Oleg Nesterov, Frederic Weisbecker hi, Milind reported that modify_user_hw_breakpoint wouldn't allow the breakpoint changing if the new attr had 'disabled' set to true. I found a case where it actualy prevents ptrace user interface to change the breakpoint. It's described in patch 1 as perf test, patch 2 is the breakpoint code fix. I ran strace tests, nothing (new) broken there.. v3 changes: - added Oleg's ack for patch 3 - new patches 4,5 based on Oleg's suggestions replacing the v2 fallback approach by enabling the event directly after failed modification v2 changes: - added Oleg's ack for patch 2 - added new changes based on Oleg's questions plus new test code thanks, jirka --- 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 kernel/events/core.c | 11 ++----- kernel/events/hw_breakpoint.c | 13 ++++---- 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 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 230 insertions(+), 15 deletions(-) create mode 100644 tools/perf/arch/x86/tests/bp-modify.c ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 5/5] perf/hw_breakpoint: Simplify breakpoint enable in perf_event_modify_breakpoint 2018-08-10 10:47 [PATCHv3 " Jiri Olsa @ 2018-08-10 10:47 ` Jiri Olsa 2018-08-14 10:32 ` Oleg Nesterov 0 siblings, 1 reply; 14+ messages in thread From: Jiri Olsa @ 2018-08-10 10:47 UTC (permalink / raw) To: Arnaldo Carvalho de Melo Cc: lkml, Ingo Molnar, Namhyung Kim, David Ahern, Alexander Shishkin, Peter Zijlstra, Milind Chabbi, Oleg Nesterov, Frederic Weisbecker 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. Suggested-by: Oleg Nesterov <oleg@redhat.com> Link: http://lkml.kernel.org/n/tip-vo1jm1u2nar6lj6hnd9g73ug@git.kernel.org Signed-off-by: Jiri Olsa <jolsa@kernel.org> --- 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.17.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 5/5] perf/hw_breakpoint: Simplify breakpoint enable in perf_event_modify_breakpoint 2018-08-10 10:47 ` [PATCH 5/5] perf/hw_breakpoint: Simplify breakpoint enable in perf_event_modify_breakpoint Jiri Olsa @ 2018-08-14 10:32 ` Oleg Nesterov 0 siblings, 0 replies; 14+ messages in thread From: Oleg Nesterov @ 2018-08-14 10:32 UTC (permalink / raw) To: Jiri Olsa Cc: Arnaldo Carvalho de Melo, lkml, Ingo Molnar, Namhyung Kim, David Ahern, Alexander Shishkin, Peter Zijlstra, Milind Chabbi, Frederic Weisbecker On 08/10, Jiri Olsa wrote: > > --- 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; > } Acked-by: Oleg Nesterov <oleg@redhat.com> ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2018-09-06 13:04 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-08-27 9:12 [PATCHv4 0/5] perf/hw_breakpoint: Fix breakpoint modify Jiri Olsa 2018-08-27 9:12 ` [PATCH 1/5] perf tests: Add breakpoint modify tests Jiri Olsa 2018-09-06 13:01 ` [tip:perf/core] " tip-bot for Jiri Olsa 2018-08-27 9:12 ` [PATCH 2/5] perf/hw_breakpoint: Modify breakpoint even if the new attr has disabled set Jiri Olsa 2018-09-06 13:02 ` [tip:perf/core] " tip-bot for Jiri Olsa 2018-08-27 9:12 ` [PATCH 3/5] perf/hw_breakpoint: Remove superfluous bp->attr.disabled = 0 Jiri Olsa 2018-09-06 13:02 ` [tip:perf/core] " tip-bot for Jiri Olsa 2018-08-27 9:12 ` [PATCH 4/5] perf/hw_breakpoint: Enable breakpoint in modify_user_hw_breakpoint Jiri Olsa 2018-09-06 13:03 ` [tip:perf/core] " tip-bot for Jiri Olsa 2018-08-27 9:12 ` [PATCH 5/5] perf/hw_breakpoint: Simplify breakpoint enable in perf_event_modify_breakpoint Jiri Olsa 2018-09-06 13:03 ` [tip:perf/core] " tip-bot for Jiri Olsa 2018-08-30 18:39 ` [PATCHv4 0/5] perf/hw_breakpoint: Fix breakpoint modify Arnaldo Carvalho de Melo -- strict thread matches above, loose matches on Subject: below -- 2018-08-10 10:47 [PATCHv3 " Jiri Olsa 2018-08-10 10:47 ` [PATCH 5/5] perf/hw_breakpoint: Simplify breakpoint enable in perf_event_modify_breakpoint Jiri Olsa 2018-08-14 10:32 ` Oleg Nesterov
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).