* [PATCH 0/2] perf/hw_breakpoint: Fix breakpoint modify @ 2018-08-06 10:12 Jiri Olsa 2018-08-06 10:12 ` [PATCH 1/2] perf tests: Add breakpoint modify test Jiri Olsa 2018-08-06 10:12 ` [PATCH 2/2] perf/hw_breakpoint: Modify breakpoint even if the new attr has disabled set Jiri Olsa 0 siblings, 2 replies; 15+ messages in thread From: Jiri Olsa @ 2018-08-06 10:12 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.. thanks, jirka --- Jiri Olsa (2): perf tests: Add breakpoint modify test perf/hw_breakpoint: Modify breakpoint even if the new attr has disabled set kernel/events/hw_breakpoint.c | 10 ++++++---- 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 | 119 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 133 insertions(+), 4 deletions(-) create mode 100644 tools/perf/arch/x86/tests/bp-modify.c ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/2] perf tests: Add breakpoint modify test 2018-08-06 10:12 [PATCH 0/2] perf/hw_breakpoint: Fix breakpoint modify Jiri Olsa @ 2018-08-06 10:12 ` Jiri Olsa 2018-08-06 10:12 ` [PATCH 2/2] perf/hw_breakpoint: Modify breakpoint even if the new attr has disabled set Jiri Olsa 1 sibling, 0 replies; 15+ messages in thread From: Jiri Olsa @ 2018-08-06 10:12 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 This 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 test forks and 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 breakponit to bp_1 function - waits for the breakpoint to hit and checks it has proper rip of bp_1 function - detaches the child 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 | 119 +++++++++++++++++++++++ 4 files changed, 127 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..a7aa6d39021c --- /dev/null +++ b/tools/perf/arch/x86/tests/bp-modify.c @@ -0,0 +1,119 @@ +// 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) +{ + return 0; +} + +static noinline int bp_2(void) +{ + return 0; +} + +/* + * This tests creates HW breakpoint, tries to + * change it and checks it was properly changed. + */ +int test__bp_modify(struct test *test __maybe_unused, + int subtest __maybe_unused) +{ + pid_t child; + int status; + unsigned long rip, dr7 = 1; + + 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); + } + + waitpid(child, &status, 0); + if (WIFEXITED(status)) { + pr_debug("tracee exited prematurely\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\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; +} -- 2.17.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 2/2] perf/hw_breakpoint: Modify breakpoint even if the new attr has disabled set 2018-08-06 10:12 [PATCH 0/2] perf/hw_breakpoint: Fix breakpoint modify Jiri Olsa 2018-08-06 10:12 ` [PATCH 1/2] perf tests: Add breakpoint modify test Jiri Olsa @ 2018-08-06 10:12 ` Jiri Olsa 2018-08-06 12:48 ` Oleg Nesterov 1 sibling, 1 reply; 15+ messages in thread From: Jiri Olsa @ 2018-08-06 10:12 UTC (permalink / raw) To: Arnaldo Carvalho de Melo Cc: Milind Chabbi, lkml, Ingo Molnar, Namhyung Kim, David Ahern, Alexander Shishkin, Peter Zijlstra, 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 Cc: 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] 15+ messages in thread
* Re: [PATCH 2/2] perf/hw_breakpoint: Modify breakpoint even if the new attr has disabled set 2018-08-06 10:12 ` [PATCH 2/2] perf/hw_breakpoint: Modify breakpoint even if the new attr has disabled set Jiri Olsa @ 2018-08-06 12:48 ` Oleg Nesterov 2018-08-06 13:07 ` Oleg Nesterov 2018-08-06 13:23 ` Jiri Olsa 0 siblings, 2 replies; 15+ messages in thread From: Oleg Nesterov @ 2018-08-06 12:48 UTC (permalink / raw) To: Jiri Olsa Cc: Arnaldo Carvalho de Melo, Milind Chabbi, lkml, Ingo Molnar, Namhyung Kim, David Ahern, Alexander Shishkin, Peter Zijlstra, Frederic Weisbecker On 08/06, Jiri Olsa wrote: > > We need to change the breakpoint even if the attr with > new fields has disabled set to true. Agreed... The patch looks fine to me, but I have a question > 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; Afaics you do not need to clear attr.disabled, modify_user_hw_breakpoint_check() updates it if err = 0. So I think if (!bp->attr.disabled) perf_event_enable(bp); will look a bit better. But, with or without this fix, shouldn't we set .disabled = 1 if modify_() fails? IIUC this doesn't matter, bp->attr.disabled is not really used anyway, but looks a bit confusing. Oleg. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] perf/hw_breakpoint: Modify breakpoint even if the new attr has disabled set 2018-08-06 12:48 ` Oleg Nesterov @ 2018-08-06 13:07 ` Oleg Nesterov 2018-08-06 13:37 ` Jiri Olsa 2018-08-06 13:23 ` Jiri Olsa 1 sibling, 1 reply; 15+ messages in thread From: Oleg Nesterov @ 2018-08-06 13:07 UTC (permalink / raw) To: Jiri Olsa Cc: Arnaldo Carvalho de Melo, Milind Chabbi, lkml, Ingo Molnar, Namhyung Kim, David Ahern, Alexander Shishkin, Peter Zijlstra, Frederic Weisbecker On 08/06, Oleg Nesterov wrote: > > But, with or without this fix, shouldn't we set .disabled = 1 if modify_() fails? > IIUC this doesn't matter, bp->attr.disabled is not really used anyway, but looks a > bit confusing. I am looking at another caller perf_event_modify_breakpoint(). It too doesn't set attr.disabled = 1 on failure, it does _perf_event_enable() instead so attr.disabled should be correct. But this looks wrong. If modify_user_hw_breakpoint_check() paths fails after arch_validate_hwbkpt_settings() was called, then we can not simply restore bp_addr/bp_type/bp_len and do _perf_event_enable(). We need another modify_user_hw_breakpoint_check() or validate_hw_breakpoint(). Note that arch_validate_hwbkpt_settings() updates arch_hw_breakpoint according to bp.attr, the restored bp->attr.bp_addr/bp_typebp_len have no effect if we call _perf_event_enable() after the failure. Or I am totally confused? Oleg. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] perf/hw_breakpoint: Modify breakpoint even if the new attr has disabled set 2018-08-06 13:07 ` Oleg Nesterov @ 2018-08-06 13:37 ` Jiri Olsa 2018-08-06 13:49 ` Oleg Nesterov 0 siblings, 1 reply; 15+ messages in thread From: Jiri Olsa @ 2018-08-06 13:37 UTC (permalink / raw) To: Oleg Nesterov Cc: Jiri Olsa, Arnaldo Carvalho de Melo, Milind Chabbi, lkml, Ingo Molnar, Namhyung Kim, David Ahern, Alexander Shishkin, Peter Zijlstra, Frederic Weisbecker On Mon, Aug 06, 2018 at 03:07:31PM +0200, Oleg Nesterov wrote: > On 08/06, Oleg Nesterov wrote: > > > > But, with or without this fix, shouldn't we set .disabled = 1 if modify_() fails? > > IIUC this doesn't matter, bp->attr.disabled is not really used anyway, but looks a > > bit confusing. > > I am looking at another caller perf_event_modify_breakpoint(). It too doesn't set > attr.disabled = 1 on failure, it does _perf_event_enable() instead so attr.disabled > should be correct. > > But this looks wrong. If modify_user_hw_breakpoint_check() paths fails after > arch_validate_hwbkpt_settings() was called, then we can not simply restore hum, arch_validate_hwbkpt_settings was removed recently.. r u checking the last sources? jirka ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] perf/hw_breakpoint: Modify breakpoint even if the new attr has disabled set 2018-08-06 13:37 ` Jiri Olsa @ 2018-08-06 13:49 ` Oleg Nesterov 2018-08-06 14:21 ` Jiri Olsa 0 siblings, 1 reply; 15+ messages in thread From: Oleg Nesterov @ 2018-08-06 13:49 UTC (permalink / raw) To: Jiri Olsa Cc: Jiri Olsa, Arnaldo Carvalho de Melo, Milind Chabbi, lkml, Ingo Molnar, Namhyung Kim, David Ahern, Alexander Shishkin, Peter Zijlstra, Frederic Weisbecker On 08/06, Jiri Olsa wrote: > > On Mon, Aug 06, 2018 at 03:07:31PM +0200, Oleg Nesterov wrote: > > On 08/06, Oleg Nesterov wrote: > > > > > > But, with or without this fix, shouldn't we set .disabled = 1 if modify_() fails? > > > IIUC this doesn't matter, bp->attr.disabled is not really used anyway, but looks a > > > bit confusing. > > > > I am looking at another caller perf_event_modify_breakpoint(). It too doesn't set > > attr.disabled = 1 on failure, it does _perf_event_enable() instead so attr.disabled > > should be correct. > > > > But this looks wrong. If modify_user_hw_breakpoint_check() paths fails after > > arch_validate_hwbkpt_settings() was called, then we can not simply restore > > hum, arch_validate_hwbkpt_settings was removed recently.. r u checking the last sources? I do not see this change in Linus's tree... Nevermind, if this was already changed/fixed somewhere, please forget. Oleg. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] perf/hw_breakpoint: Modify breakpoint even if the new attr has disabled set 2018-08-06 13:49 ` Oleg Nesterov @ 2018-08-06 14:21 ` Jiri Olsa 0 siblings, 0 replies; 15+ messages in thread From: Jiri Olsa @ 2018-08-06 14:21 UTC (permalink / raw) To: Oleg Nesterov Cc: Jiri Olsa, Arnaldo Carvalho de Melo, Milind Chabbi, lkml, Ingo Molnar, Namhyung Kim, David Ahern, Alexander Shishkin, Peter Zijlstra, Frederic Weisbecker On Mon, Aug 06, 2018 at 03:49:47PM +0200, Oleg Nesterov wrote: > On 08/06, Jiri Olsa wrote: > > > > On Mon, Aug 06, 2018 at 03:07:31PM +0200, Oleg Nesterov wrote: > > > On 08/06, Oleg Nesterov wrote: > > > > > > > > But, with or without this fix, shouldn't we set .disabled = 1 if modify_() fails? > > > > IIUC this doesn't matter, bp->attr.disabled is not really used anyway, but looks a > > > > bit confusing. > > > > > > I am looking at another caller perf_event_modify_breakpoint(). It too doesn't set > > > attr.disabled = 1 on failure, it does _perf_event_enable() instead so attr.disabled > > > should be correct. > > > > > > But this looks wrong. If modify_user_hw_breakpoint_check() paths fails after > > > arch_validate_hwbkpt_settings() was called, then we can not simply restore > > > > hum, arch_validate_hwbkpt_settings was removed recently.. r u checking the last sources? > > I do not see this change in Linus's tree... > > > Nevermind, if this was already changed/fixed somewhere, please forget. should be in tip tree, all around this commit: cffbb3bd444b perf/hw_breakpoint: Remove default hw_breakpoint_arch_parse() jirka ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] perf/hw_breakpoint: Modify breakpoint even if the new attr has disabled set 2018-08-06 12:48 ` Oleg Nesterov 2018-08-06 13:07 ` Oleg Nesterov @ 2018-08-06 13:23 ` Jiri Olsa 2018-08-06 13:46 ` Oleg Nesterov 2018-08-06 15:08 ` [PATCH 3/2] perf/hw_breakpoint: Remove superfluous bp->attr.disabled = 0 " Jiri Olsa 1 sibling, 2 replies; 15+ messages in thread From: Jiri Olsa @ 2018-08-06 13:23 UTC (permalink / raw) To: Oleg Nesterov Cc: Jiri Olsa, Arnaldo Carvalho de Melo, Milind Chabbi, lkml, Ingo Molnar, Namhyung Kim, David Ahern, Alexander Shishkin, Peter Zijlstra, Frederic Weisbecker On Mon, Aug 06, 2018 at 02:48:40PM +0200, Oleg Nesterov wrote: > On 08/06, Jiri Olsa wrote: > > > > We need to change the breakpoint even if the attr with > > new fields has disabled set to true. > > Agreed... The patch looks fine to me, but I have a question > > > 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; > > Afaics you do not need to clear attr.disabled, modify_user_hw_breakpoint_check() > updates it if err = 0. So I think > > if (!bp->attr.disabled) > perf_event_enable(bp); > > will look a bit better. > > > But, with or without this fix, shouldn't we set .disabled = 1 if modify_() fails? > IIUC this doesn't matter, bp->attr.disabled is not really used anyway, but looks a > bit confusing. > yea, I was looking on that, but as u said it makes no difference and I wanted to keep the patch as simple as possible ;-) I'll send something on top of this patch jirka ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] perf/hw_breakpoint: Modify breakpoint even if the new attr has disabled set 2018-08-06 13:23 ` Jiri Olsa @ 2018-08-06 13:46 ` Oleg Nesterov 2018-08-06 15:08 ` [PATCH 3/2] perf/hw_breakpoint: Remove superfluous bp->attr.disabled = 0 " Jiri Olsa 1 sibling, 0 replies; 15+ messages in thread From: Oleg Nesterov @ 2018-08-06 13:46 UTC (permalink / raw) To: Jiri Olsa Cc: Jiri Olsa, Arnaldo Carvalho de Melo, Milind Chabbi, lkml, Ingo Molnar, Namhyung Kim, David Ahern, Alexander Shishkin, Peter Zijlstra, Frederic Weisbecker On 08/06, Jiri Olsa wrote: > > > Afaics you do not need to clear attr.disabled, modify_user_hw_breakpoint_check() > > updates it if err = 0. So I think > > > > if (!bp->attr.disabled) > > perf_event_enable(bp); > > > > will look a bit better. > > > > > > But, with or without this fix, shouldn't we set .disabled = 1 if modify_() fails? > > IIUC this doesn't matter, bp->attr.disabled is not really used anyway, but looks a > > bit confusing. > > > > yea, I was looking on that, but as u said it makes no difference > and I wanted to keep the patch as simple as possible ;-) OK. So both patches look good to me. Acked-by: Oleg Nesterov <oleg@redhat.com> ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 3/2] perf/hw_breakpoint: Remove superfluous bp->attr.disabled = 0 new attr has disabled set 2018-08-06 13:23 ` Jiri Olsa 2018-08-06 13:46 ` Oleg Nesterov @ 2018-08-06 15:08 ` Jiri Olsa 2018-08-06 16:34 ` Oleg Nesterov 1 sibling, 1 reply; 15+ messages in thread From: Jiri Olsa @ 2018-08-06 15:08 UTC (permalink / raw) To: Oleg Nesterov Cc: Jiri Olsa, Arnaldo Carvalho de Melo, Milind Chabbi, lkml, Ingo Molnar, Namhyung Kim, David Ahern, Alexander Shishkin, Peter Zijlstra, Frederic Weisbecker On Mon, Aug 06, 2018 at 03:23:53PM +0200, Jiri Olsa wrote: > On Mon, Aug 06, 2018 at 02:48:40PM +0200, Oleg Nesterov wrote: > > On 08/06, Jiri Olsa wrote: > > > > > > We need to change the breakpoint even if the attr with > > > new fields has disabled set to true. > > > > Agreed... The patch looks fine to me, but I have a question > > > > > 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; > > > > Afaics you do not need to clear attr.disabled, modify_user_hw_breakpoint_check() > > updates it if err = 0. So I think > > > > if (!bp->attr.disabled) > > perf_event_enable(bp); > > > > will look a bit better. > > > > > > But, with or without this fix, shouldn't we set .disabled = 1 if modify_() fails? > > IIUC this doesn't matter, bp->attr.disabled is not really used anyway, but looks a > > bit confusing. > > > > yea, I was looking on that, but as u said it makes no difference > and I wanted to keep the patch as simple as possible ;-) > > I'll send something on top of this patch like this ;-) jirka --- 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. 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] 15+ messages in thread
* Re: [PATCH 3/2] perf/hw_breakpoint: Remove superfluous bp->attr.disabled = 0 new attr has disabled set 2018-08-06 15:08 ` [PATCH 3/2] perf/hw_breakpoint: Remove superfluous bp->attr.disabled = 0 " Jiri Olsa @ 2018-08-06 16:34 ` Oleg Nesterov 2018-08-07 8:16 ` Jiri Olsa 0 siblings, 1 reply; 15+ messages in thread From: Oleg Nesterov @ 2018-08-06 16:34 UTC (permalink / raw) To: Jiri Olsa Cc: Jiri Olsa, Arnaldo Carvalho de Melo, Milind Chabbi, lkml, Ingo Molnar, Namhyung Kim, David Ahern, Alexander Shishkin, Peter Zijlstra, Frederic Weisbecker On 08/06, 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. > > 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; > - } > + Yes, but again, this still looks confusing. IMO, we should either remove "bp->attr.disabled = attr->disabled" in modify_user_hw_breakpoint_check() because bp->attr.disabled is not really used, or we should set bp->attr.disabled = 1 on failure just for consistency. Hmm... actually ptrace_get_dr7() checks ->attr.disabled, so we can hit WARN_ON(second_pass) in ptrace_write_dr7() in case when attr.disabled is falsely 0 because modify_user_hw_breakpoint_check() failed before? It seems I am totally confused and need to sleep ;) Oleg. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/2] perf/hw_breakpoint: Remove superfluous bp->attr.disabled = 0 new attr has disabled set 2018-08-06 16:34 ` Oleg Nesterov @ 2018-08-07 8:16 ` Jiri Olsa 2018-08-07 9:10 ` Oleg Nesterov 0 siblings, 1 reply; 15+ messages in thread From: Jiri Olsa @ 2018-08-07 8:16 UTC (permalink / raw) To: Oleg Nesterov Cc: Jiri Olsa, Arnaldo Carvalho de Melo, Milind Chabbi, lkml, Ingo Molnar, Namhyung Kim, David Ahern, Alexander Shishkin, Peter Zijlstra, Frederic Weisbecker On Mon, Aug 06, 2018 at 06:34:36PM +0200, Oleg Nesterov wrote: > On 08/06, 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. > > > > 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; > > - } > > + > > Yes, but again, this still looks confusing. > > IMO, we should either remove "bp->attr.disabled = attr->disabled" in > modify_user_hw_breakpoint_check() because bp->attr.disabled is not really > used, or we should set bp->attr.disabled = 1 on failure just for consistency. > > > Hmm... actually ptrace_get_dr7() checks ->attr.disabled, so we can hit > WARN_ON(second_pass) in ptrace_write_dr7() in case when attr.disabled is > falsely 0 because modify_user_hw_breakpoint_check() failed before? hum, I can't see how modify_user_hw_breakpoint_check could falsely set disabled new attr stuff is copied once all checks passed jirka ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/2] perf/hw_breakpoint: Remove superfluous bp->attr.disabled = 0 new attr has disabled set 2018-08-07 8:16 ` Jiri Olsa @ 2018-08-07 9:10 ` Oleg Nesterov 2018-08-07 14:39 ` Jiri Olsa 0 siblings, 1 reply; 15+ messages in thread From: Oleg Nesterov @ 2018-08-07 9:10 UTC (permalink / raw) To: Jiri Olsa Cc: Jiri Olsa, Arnaldo Carvalho de Melo, Milind Chabbi, lkml, Ingo Molnar, Namhyung Kim, David Ahern, Alexander Shishkin, Peter Zijlstra, Frederic Weisbecker On 08/07, Jiri Olsa wrote: > > On Mon, Aug 06, 2018 at 06:34:36PM +0200, Oleg Nesterov wrote: > > > --- 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; > > > - } > > > + > > > > Yes, but again, this still looks confusing. > > > > IMO, we should either remove "bp->attr.disabled = attr->disabled" in > > modify_user_hw_breakpoint_check() because bp->attr.disabled is not really > > used, or we should set bp->attr.disabled = 1 on failure just for consistency. > > > > > > Hmm... actually ptrace_get_dr7() checks ->attr.disabled, so we can hit > > WARN_ON(second_pass) in ptrace_write_dr7() in case when attr.disabled is > > falsely 0 because modify_user_hw_breakpoint_check() failed before? > > hum, I can't see how modify_user_hw_breakpoint_check could falsely set disabled > new attr stuff is copied once all checks passed Hmm. So modify_user_hw_breakpoint() does perf_event_disable(bp) first and afaics this doesn't set bp->attr.disabled = 1. If modify_user_hw_breakpoint_check() fails after that we do not update bp->attr, so bp->attr.disabled is still zero while this bp is actually disabled. Again, afaics the core perf code doesn't actually use bp->attr.disabled after perf_event__state_init(). But this can confuse ptrace_write_dr7/ptrace_get_dr7. No? I am on Linus's tree, but I see the same logic in https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/tree/kernel/events/hw_breakpoint.c?h=perf/core Oleg. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/2] perf/hw_breakpoint: Remove superfluous bp->attr.disabled = 0 new attr has disabled set 2018-08-07 9:10 ` Oleg Nesterov @ 2018-08-07 14:39 ` Jiri Olsa 0 siblings, 0 replies; 15+ messages in thread From: Jiri Olsa @ 2018-08-07 14:39 UTC (permalink / raw) To: Oleg Nesterov Cc: Jiri Olsa, Arnaldo Carvalho de Melo, Milind Chabbi, lkml, Ingo Molnar, Namhyung Kim, David Ahern, Alexander Shishkin, Peter Zijlstra, Frederic Weisbecker On Tue, Aug 07, 2018 at 11:10:08AM +0200, Oleg Nesterov wrote: > On 08/07, Jiri Olsa wrote: > > > > On Mon, Aug 06, 2018 at 06:34:36PM +0200, Oleg Nesterov wrote: > > > > --- 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; > > > > - } > > > > + > > > > > > Yes, but again, this still looks confusing. > > > > > > IMO, we should either remove "bp->attr.disabled = attr->disabled" in > > > modify_user_hw_breakpoint_check() because bp->attr.disabled is not really > > > used, or we should set bp->attr.disabled = 1 on failure just for consistency. > > > > > > > > > Hmm... actually ptrace_get_dr7() checks ->attr.disabled, so we can hit > > > WARN_ON(second_pass) in ptrace_write_dr7() in case when attr.disabled is > > > falsely 0 because modify_user_hw_breakpoint_check() failed before? > > > > hum, I can't see how modify_user_hw_breakpoint_check could falsely set disabled > > new attr stuff is copied once all checks passed > > Hmm. So modify_user_hw_breakpoint() does perf_event_disable(bp) first and afaics > this doesn't set bp->attr.disabled = 1. > > If modify_user_hw_breakpoint_check() fails after that we do not update bp->attr, > so bp->attr.disabled is still zero while this bp is actually disabled. this seems to be cared of the 2nd pass restore logic, where the old dr7 value will make the breakpoint to be enabled again: bp0.attr.disabled = 0 (enabled) ptrace_write_dr7(newdr7) dr7 = newdr7 ptrace_modify_breakpoint(bp0) perf_event_disable -> bp0 disabled, but bp0.attr.disabled = 0 modify_user_hw_breakpoint_check fails return -1 -> leaving with bp0 disabled, bp0.attr.disabled = 0 dr7 = olddr7 -> bp0 bit enabled second_pass = true ptrace_modify_breakpoint -> attr->disabled = 0 perf_event_disable modify_user_hw_breakpoint_check hw_breakpoint_copy_attr perf_event_enable -> bp0 enabled, bp0.attr.disabled = 0 but this could be problem for ptrace_set_breakpoint_addr, because the new address modification in modify_user_hw_breakpoint_check, can fail and leave enabled breakpoint with disabled = 0 might be enough to manualy disable it in case of error like below (squashed with my previou change) need to test this.. jirka --- diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c index fb229d9c7f3c..eff7ab716139 100644 --- a/kernel/events/hw_breakpoint.c +++ b/kernel/events/hw_breakpoint.c @@ -523,13 +523,14 @@ 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) + if (err) { + bp->attr.disabled = 1; 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] 15+ messages in thread
end of thread, other threads:[~2018-08-07 14:39 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-08-06 10:12 [PATCH 0/2] perf/hw_breakpoint: Fix breakpoint modify Jiri Olsa 2018-08-06 10:12 ` [PATCH 1/2] perf tests: Add breakpoint modify test Jiri Olsa 2018-08-06 10:12 ` [PATCH 2/2] perf/hw_breakpoint: Modify breakpoint even if the new attr has disabled set Jiri Olsa 2018-08-06 12:48 ` Oleg Nesterov 2018-08-06 13:07 ` Oleg Nesterov 2018-08-06 13:37 ` Jiri Olsa 2018-08-06 13:49 ` Oleg Nesterov 2018-08-06 14:21 ` Jiri Olsa 2018-08-06 13:23 ` Jiri Olsa 2018-08-06 13:46 ` Oleg Nesterov 2018-08-06 15:08 ` [PATCH 3/2] perf/hw_breakpoint: Remove superfluous bp->attr.disabled = 0 " Jiri Olsa 2018-08-06 16:34 ` Oleg Nesterov 2018-08-07 8:16 ` Jiri Olsa 2018-08-07 9:10 ` Oleg Nesterov 2018-08-07 14:39 ` Jiri Olsa
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).