[PATCHv2 0/5] perf/hw_breakpoint: Fix breakpoint modify

[PATCHv2 0/5] perf/hw_breakpoint: Fix breakpoint modify

[Jiri Olsa]

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..

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: Set breakpoint as disabled in modify_user_hw_breakpoint error path
      perf/hw_breakpoint: Add fallback code for ptrace_set_breakpoint_addr

 arch/x86/kernel/ptrace.c                 |   5 +++
 kernel/events/hw_breakpoint.c            |  15 +++++----
 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, 235 insertions(+), 6 deletions(-)
 create mode 100644 tools/perf/arch/x86/tests/bp-modify.c

[PATCH 1/5] perf tests: Add breakpoint modify tests

[Jiri Olsa]

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

[PATCH 2/5] perf/hw_breakpoint: Modify breakpoint even if the new attr has disabled set

[Jiri Olsa]

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

[PATCH 3/5] perf/hw_breakpoint: Remove superfluous bp->attr.disabled = 0

[Jiri Olsa]

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

[PATCH 4/5] perf/hw_breakpoint: Set breakpoint as disabled in modify_user_hw_breakpoint error path

[Jiri Olsa]

When we failed to modify breakpoint, we ended up with
disabled event. Mirror that in its 'attr.disabled',
because some parts of the ptrace code depends on it.

Link: http://lkml.kernel.org/n/tip-rp81jgq4rx19ozxm9decxq98@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 kernel/events/hw_breakpoint.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
index 3e560d7609fd..eff7ab716139 100644
--- a/kernel/events/hw_breakpoint.c
+++ b/kernel/events/hw_breakpoint.c
@@ -523,8 +523,10 @@ 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)
 		perf_event_enable(bp);
-- 
2.17.1

[PATCH 5/5] perf/hw_breakpoint: Add fallback code for ptrace_set_breakpoint_addr

[Jiri Olsa]

Restoring the breakpoint after unsuccesfull address change,
so following user code no longer produces disabled breakpoint.

  ptrace(PTRACE_POKEUSER, child, offsetof(struct user, u_debugreg[0]), addr_1)
  ptrace(PTRACE_POKEUSER, child, offsetof(struct user, u_debugreg[7]), dr7)
  ptrace(PTRACE_POKEUSER, child, offsetof(struct user, u_debugreg[0]), -1)

The first 2 ptrace calls set breakpoint on addr_1. The 3rd ptrace
call tries to set it to bogus address (-1). This would normaly
end up with disabled breakpoint. This patch adds the code that
restores the breakpoint to its original state.

Link: http://lkml.kernel.org/n/tip-h9ut835vl297roen0v163zg6@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 arch/x86/kernel/ptrace.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
index e2ee403865eb..22c06d0a38d1 100644
--- a/arch/x86/kernel/ptrace.c
+++ b/arch/x86/kernel/ptrace.c
@@ -693,9 +693,14 @@ static int ptrace_set_breakpoint_addr(struct task_struct *tsk, int nr,
 			t->ptrace_bps[nr] = bp;
 	} else {
 		struct perf_event_attr attr = bp->attr;
+		bool disabled = attr.disabled;
 
 		attr.bp_addr = addr;
 		err = modify_user_hw_breakpoint(bp, &attr);
+		if (err && !disabled) {
+			bp->attr.disabled = false;
+			WARN_ON(modify_user_hw_breakpoint(bp, &bp->attr));
+		}
 	}
 
 	return err;
-- 
2.17.1

Re: [PATCH 3/5] perf/hw_breakpoint: Remove superfluous bp->attr.disabled = 0

[Oleg Nesterov]

On 08/09, Jiri Olsa wrote:
>
> @@ -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;
> -	}

Acked-by: Oleg Nesterov <oleg@redhat.com>

Re: [PATCH 4/5] perf/hw_breakpoint: Set breakpoint as disabled in modify_user_hw_breakpoint error path

[Oleg Nesterov]

On 08/09, Jiri Olsa wrote:
>
> @@ -523,8 +523,10 @@ 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;

Yes, but on the second thought... Can't we simply do

	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.
		 * So call the function directly after making sure we are targeting the
		 * current task.
		 */
		if (irqs_disabled() && bp->ctx && bp->ctx->task == current)
			perf_event_disable_local(bp);
		else
			perf_event_disable(bp);

		err = modify_user_hw_breakpoint_check(bp, attr, false);

		if (!bp.attr->disabled)
			perf_event_enable(bp);

		return err;
	}

instead of this and the next patch?

We can do this because (as you pointed out) validate_hw_breakpoint() has already
gone in -tip tree, and (afaics) modify_user_hw_breakpoint_check() never changes
perf_event's state on failure, so we can safely "restart" this bp if it was enabled
before.

1. This is what we had before the recent f67b15037a7a50c57f72e69a6d59941ad90a0f0f
   "perf/hwbp: Simplify the perf-hwbp code, fix documentation".

   Note that this commit was actually the bug fix which introduced another problem
   fixed by your 2/5.

   But see above, perf_event_enable() is no longer unsafe after
   modify_user_hw_breakpoint_check(), we can restore the previous semantics.

2. This matches perf_event_modify_breakpoint(). Which btw can be simplified a bit,
   it too can simply do

		if (!bp->attr.disabled)
			_perf_event_enable(bp);

		return err;

Oleg.

Re: [PATCH 4/5] perf/hw_breakpoint: Set breakpoint as disabled in modify_user_hw_breakpoint error path

[Jiri Olsa]

On Thu, Aug 09, 2018 at 04:17:13PM +0200, Oleg Nesterov wrote:
> On 08/09, Jiri Olsa wrote:
> >
> > @@ -523,8 +523,10 @@ 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;
> 
> Yes, but on the second thought... Can't we simply do
> 
> 	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.
> 		 * So call the function directly after making sure we are targeting the
> 		 * current task.
> 		 */
> 		if (irqs_disabled() && bp->ctx && bp->ctx->task == current)
> 			perf_event_disable_local(bp);
> 		else
> 			perf_event_disable(bp);
> 
> 		err = modify_user_hw_breakpoint_check(bp, attr, false);
> 
> 		if (!bp.attr->disabled)
> 			perf_event_enable(bp);
> 
> 		return err;
> 	}
> 
> instead of this and the next patch?
> 
> We can do this because (as you pointed out) validate_hw_breakpoint() has already
> gone in -tip tree, and (afaics) modify_user_hw_breakpoint_check() never changes
> perf_event's state on failure, so we can safely "restart" this bp if it was enabled
> before.
> 
> 1. This is what we had before the recent f67b15037a7a50c57f72e69a6d59941ad90a0f0f
>    "perf/hwbp: Simplify the perf-hwbp code, fix documentation".
> 
>    Note that this commit was actually the bug fix which introduced another problem
>    fixed by your 2/5.
> 
>    But see above, perf_event_enable() is no longer unsafe after
>    modify_user_hw_breakpoint_check(), we can restore the previous semantics.
> 
> 2. This matches perf_event_modify_breakpoint(). Which btw can be simplified a bit,
>    it too can simply do
> 
> 		if (!bp->attr.disabled)
> 			_perf_event_enable(bp);
> 
> 		return err;

yep, seems good.. will send new version

thanks,
jirka

Re: [PATCHv2 0/5] perf/hw_breakpoint: Fix breakpoint modify

[Arnaldo Carvalho de Melo]

Em Thu, Aug 09, 2018 at 02:03:00PM +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.

Going over these patches now, applying the test, running it, then the
kernel bits, reboot, retest, etc.

- Arnaldo

Re: [PATCH 4/5] perf/hw_breakpoint: Set breakpoint as disabled in modify_user_hw_breakpoint error path

[Arnaldo Carvalho de Melo]

Em Thu, Aug 09, 2018 at 06:30:15PM +0200, Jiri Olsa escreveu:
> On Thu, Aug 09, 2018 at 04:17:13PM +0200, Oleg Nesterov wrote:
> > On 08/09, Jiri Olsa wrote:
> > > -	if (err)
> > > +	if (err) {
> > > +		bp->attr.disabled = 1;
> > >  		return err;
> > 
> > Yes, but on the second thought... Can't we simply do

<SNIP>

> > instead of this and the next patch?

<SNIP>
 
> yep, seems good.. will send new version

Ok, but this is just for 4/5 and 5/5, right?

- Arnaldo

Re: [PATCH 4/5] perf/hw_breakpoint: Set breakpoint as disabled in modify_user_hw_breakpoint error path

[Jiri Olsa]

On Tue, Aug 28, 2018 at 11:29:14AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Thu, Aug 09, 2018 at 06:30:15PM +0200, Jiri Olsa escreveu:
> > On Thu, Aug 09, 2018 at 04:17:13PM +0200, Oleg Nesterov wrote:
> > > On 08/09, Jiri Olsa wrote:
> > > > -	if (err)
> > > > +	if (err) {
> > > > +		bp->attr.disabled = 1;
> > > >  		return err;
> > > 
> > > Yes, but on the second thought... Can't we simply do
> 
> <SNIP>
> 
> > > instead of this and the next patch?
> 
> <SNIP>
>  
> > yep, seems good.. will send new version
> 
> Ok, but this is just for 4/5 and 5/5, right?

I resent v4 yesterday with final changes/acks, in here:

7186 r   Aug 27 Jiri Olsa       (1.6K) [PATCHv4 0/5] perf/hw_breakpoint: Fix breakpoint modify

thanks,
jirka

Re: [PATCH 4/5] perf/hw_breakpoint: Set breakpoint as disabled in modify_user_hw_breakpoint error path

[Arnaldo Carvalho de Melo]

Em Tue, Aug 28, 2018 at 04:38:23PM +0200, Jiri Olsa escreveu:
> On Tue, Aug 28, 2018 at 11:29:14AM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Thu, Aug 09, 2018 at 06:30:15PM +0200, Jiri Olsa escreveu:
> > > On Thu, Aug 09, 2018 at 04:17:13PM +0200, Oleg Nesterov wrote:
> > > > On 08/09, Jiri Olsa wrote:
> > > > > -	if (err)
> > > > > +	if (err) {
> > > > > +		bp->attr.disabled = 1;
> > > > >  		return err;
> > > > 
> > > > Yes, but on the second thought... Can't we simply do
> > 
> > <SNIP>
> > 
> > > > instead of this and the next patch?
> > 
> > <SNIP>
> >  
> > > yep, seems good.. will send new version
> > 
> > Ok, but this is just for 4/5 and 5/5, right?
> 
> I resent v4 yesterday with final changes/acks, in here:
> 
> 7186 r   Aug 27 Jiri Olsa       (1.6K) [PATCHv4 0/5] perf/hw_breakpoint: Fix breakpoint modify

Ok, will search that, sorry for the noise :-)

- Arnaldo