* [PATCHv3 0/5] perf/hw_breakpoint: Fix breakpoint modify @ 2018-08-10 10:47 Jiri Olsa 2018-08-10 10:47 ` [PATCH 1/5] perf tests: Add breakpoint modify tests Jiri Olsa ` (5 more replies) 0 siblings, 6 replies; 13+ 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] 13+ messages in thread
* [PATCH 1/5] perf tests: Add breakpoint modify tests 2018-08-10 10:47 [PATCHv3 0/5] perf/hw_breakpoint: Fix breakpoint modify Jiri Olsa @ 2018-08-10 10:47 ` Jiri Olsa 2018-08-10 10:47 ` [PATCH 2/5] perf/hw_breakpoint: Modify breakpoint even if the new attr has disabled set Jiri Olsa ` (4 subsequent siblings) 5 siblings, 0 replies; 13+ 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 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] 13+ messages in thread
* [PATCH 2/5] perf/hw_breakpoint: Modify breakpoint even if the new attr has disabled set 2018-08-10 10:47 [PATCHv3 0/5] perf/hw_breakpoint: Fix breakpoint modify Jiri Olsa 2018-08-10 10:47 ` [PATCH 1/5] perf tests: Add breakpoint modify tests Jiri Olsa @ 2018-08-10 10:47 ` Jiri Olsa 2018-08-17 14:27 ` Frederic Weisbecker 2018-08-10 10:47 ` [PATCH 3/5] perf/hw_breakpoint: Remove superfluous bp->attr.disabled = 0 Jiri Olsa ` (3 subsequent siblings) 5 siblings, 1 reply; 13+ 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 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> 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] 13+ messages in thread
* Re: [PATCH 2/5] perf/hw_breakpoint: Modify breakpoint even if the new attr has disabled set 2018-08-10 10:47 ` [PATCH 2/5] perf/hw_breakpoint: Modify breakpoint even if the new attr has disabled set Jiri Olsa @ 2018-08-17 14:27 ` Frederic Weisbecker 0 siblings, 0 replies; 13+ messages in thread From: Frederic Weisbecker @ 2018-08-17 14:27 UTC (permalink / raw) To: Jiri Olsa Cc: Arnaldo Carvalho de Melo, lkml, Ingo Molnar, Namhyung Kim, David Ahern, Alexander Shishkin, Peter Zijlstra, Milind Chabbi, Oleg Nesterov On Fri, Aug 10, 2018 at 12:47:27PM +0200, Jiri Olsa wrote: > 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> > 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> Right, now that we fixed the breakpoint modification arch code, it should be safe to do that. Thanks! Acked-by: Frederic Weisbecker <frederic@kernel.org> ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 3/5] perf/hw_breakpoint: Remove superfluous bp->attr.disabled = 0 2018-08-10 10:47 [PATCHv3 0/5] perf/hw_breakpoint: Fix breakpoint modify Jiri Olsa 2018-08-10 10:47 ` [PATCH 1/5] perf tests: Add breakpoint modify tests Jiri Olsa 2018-08-10 10:47 ` [PATCH 2/5] perf/hw_breakpoint: Modify breakpoint even if the new attr has disabled set Jiri Olsa @ 2018-08-10 10:47 ` Jiri Olsa 2018-08-17 14:47 ` Frederic Weisbecker 2018-08-10 10:47 ` [PATCH 4/5] perf/hw_breakpoint: Enable breakpoint in modify_user_hw_breakpoint Jiri Olsa ` (2 subsequent siblings) 5 siblings, 1 reply; 13+ 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 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> 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] 13+ messages in thread
* Re: [PATCH 3/5] perf/hw_breakpoint: Remove superfluous bp->attr.disabled = 0 2018-08-10 10:47 ` [PATCH 3/5] perf/hw_breakpoint: Remove superfluous bp->attr.disabled = 0 Jiri Olsa @ 2018-08-17 14:47 ` Frederic Weisbecker 0 siblings, 0 replies; 13+ messages in thread From: Frederic Weisbecker @ 2018-08-17 14:47 UTC (permalink / raw) To: Jiri Olsa Cc: Arnaldo Carvalho de Melo, lkml, Ingo Molnar, Namhyung Kim, David Ahern, Alexander Shishkin, Peter Zijlstra, Milind Chabbi, Oleg Nesterov On Fri, Aug 10, 2018 at 12:47:28PM +0200, Jiri Olsa wrote: > 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> > Link: http://lkml.kernel.org/n/tip-v5oaellzsmyszv3rfucuxkp0@git.kernel.org > Signed-off-by: Jiri Olsa <jolsa@kernel.org> Acked-by: Frederic Weisbecker <frederic@kernel.org> ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 4/5] perf/hw_breakpoint: Enable breakpoint in modify_user_hw_breakpoint 2018-08-10 10:47 [PATCHv3 0/5] perf/hw_breakpoint: Fix breakpoint modify Jiri Olsa ` (2 preceding siblings ...) 2018-08-10 10:47 ` [PATCH 3/5] perf/hw_breakpoint: Remove superfluous bp->attr.disabled = 0 Jiri Olsa @ 2018-08-10 10:47 ` Jiri Olsa 2018-08-14 10:32 ` Oleg Nesterov 2018-08-17 14:50 ` Frederic Weisbecker 2018-08-10 10:47 ` [PATCH 5/5] perf/hw_breakpoint: Simplify breakpoint enable in perf_event_modify_breakpoint Jiri Olsa 2018-08-17 11:48 ` [PATCHv3 0/5] perf/hw_breakpoint: Fix breakpoint modify Jiri Olsa 5 siblings, 2 replies; 13+ 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 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> 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] 13+ messages in thread
* Re: [PATCH 4/5] perf/hw_breakpoint: Enable breakpoint in modify_user_hw_breakpoint 2018-08-10 10:47 ` [PATCH 4/5] perf/hw_breakpoint: Enable breakpoint in modify_user_hw_breakpoint Jiri Olsa @ 2018-08-14 10:32 ` Oleg Nesterov 2018-08-17 14:50 ` Frederic Weisbecker 1 sibling, 0 replies; 13+ 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/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; > } Acked-by: Oleg Nesterov <oleg@redhat.com> ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 4/5] perf/hw_breakpoint: Enable breakpoint in modify_user_hw_breakpoint 2018-08-10 10:47 ` [PATCH 4/5] perf/hw_breakpoint: Enable breakpoint in modify_user_hw_breakpoint Jiri Olsa 2018-08-14 10:32 ` Oleg Nesterov @ 2018-08-17 14:50 ` Frederic Weisbecker 1 sibling, 0 replies; 13+ messages in thread From: Frederic Weisbecker @ 2018-08-17 14:50 UTC (permalink / raw) To: Jiri Olsa Cc: Arnaldo Carvalho de Melo, lkml, Ingo Molnar, Namhyung Kim, David Ahern, Alexander Shishkin, Peter Zijlstra, Milind Chabbi, Oleg Nesterov On Fri, Aug 10, 2018 at 12:47:29PM +0200, Jiri Olsa wrote: > 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> > Link: http://lkml.kernel.org/n/tip-79p9ttocwy2ju2s6qh25dvre@git.kernel.org > Signed-off-by: Jiri Olsa <jolsa@kernel.org> Acked-by: Frederic Weisbecker <frederic@kernel.org> Thanks a lot! ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 5/5] perf/hw_breakpoint: Simplify breakpoint enable in perf_event_modify_breakpoint 2018-08-10 10:47 [PATCHv3 0/5] perf/hw_breakpoint: Fix breakpoint modify Jiri Olsa ` (3 preceding siblings ...) 2018-08-10 10:47 ` [PATCH 4/5] perf/hw_breakpoint: Enable breakpoint in modify_user_hw_breakpoint Jiri Olsa @ 2018-08-10 10:47 ` Jiri Olsa 2018-08-14 10:32 ` Oleg Nesterov 2018-08-17 11:48 ` [PATCHv3 0/5] perf/hw_breakpoint: Fix breakpoint modify Jiri Olsa 5 siblings, 1 reply; 13+ 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] 13+ 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; 13+ 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] 13+ messages in thread
* Re: [PATCHv3 0/5] perf/hw_breakpoint: Fix breakpoint modify 2018-08-10 10:47 [PATCHv3 0/5] perf/hw_breakpoint: Fix breakpoint modify Jiri Olsa ` (4 preceding siblings ...) 2018-08-10 10:47 ` [PATCH 5/5] perf/hw_breakpoint: Simplify breakpoint enable in perf_event_modify_breakpoint Jiri Olsa @ 2018-08-17 11:48 ` Jiri Olsa 5 siblings, 0 replies; 13+ messages in thread From: Jiri Olsa @ 2018-08-17 11:48 UTC (permalink / raw) To: Ingo Molnar Cc: Arnaldo Carvalho de Melo, lkml, Jiri Olsa, Namhyung Kim, David Ahern, Alexander Shishkin, Peter Zijlstra, Milind Chabbi, Oleg Nesterov, Frederic Weisbecker On Fri, Aug 10, 2018 at 12:47:25PM +0200, Jiri Olsa wrote: > 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 Ingo, Arnaldo, would you please pick up those, or should I pushed them through somebody else? thanks, jirka > > 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] 13+ messages in thread
* [PATCHv4 0/5] perf/hw_breakpoint: Fix breakpoint modify @ 2018-08-27 9:12 Jiri Olsa 2018-08-27 9:12 ` [PATCH 5/5] perf/hw_breakpoint: Simplify breakpoint enable in perf_event_modify_breakpoint Jiri Olsa 0 siblings, 1 reply; 13+ 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] 13+ messages in thread
* [PATCH 5/5] perf/hw_breakpoint: Simplify breakpoint enable in perf_event_modify_breakpoint 2018-08-27 9:12 [PATCHv4 " Jiri Olsa @ 2018-08-27 9:12 ` Jiri Olsa 0 siblings, 0 replies; 13+ 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] 13+ messages in thread
end of thread, other threads:[~2018-08-27 9:12 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-08-10 10:47 [PATCHv3 0/5] perf/hw_breakpoint: Fix breakpoint modify Jiri Olsa 2018-08-10 10:47 ` [PATCH 1/5] perf tests: Add breakpoint modify tests Jiri Olsa 2018-08-10 10:47 ` [PATCH 2/5] perf/hw_breakpoint: Modify breakpoint even if the new attr has disabled set Jiri Olsa 2018-08-17 14:27 ` Frederic Weisbecker 2018-08-10 10:47 ` [PATCH 3/5] perf/hw_breakpoint: Remove superfluous bp->attr.disabled = 0 Jiri Olsa 2018-08-17 14:47 ` Frederic Weisbecker 2018-08-10 10:47 ` [PATCH 4/5] perf/hw_breakpoint: Enable breakpoint in modify_user_hw_breakpoint Jiri Olsa 2018-08-14 10:32 ` Oleg Nesterov 2018-08-17 14:50 ` Frederic Weisbecker 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 2018-08-17 11:48 ` [PATCHv3 0/5] perf/hw_breakpoint: Fix breakpoint modify Jiri Olsa 2018-08-27 9:12 [PATCHv4 " Jiri Olsa 2018-08-27 9:12 ` [PATCH 5/5] perf/hw_breakpoint: Simplify breakpoint enable in perf_event_modify_breakpoint Jiri Olsa
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.