[PATCH 0/2] perf/hw_breakpoint: Fix breakpoint modify

[PATCH 0/2] 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..

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

[PATCH 1/2] perf tests: Add breakpoint modify test

[Jiri Olsa]

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

[PATCH 2/2] 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

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

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

[Oleg Nesterov]

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.

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

[Oleg Nesterov]

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.

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

[Jiri Olsa]

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

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

[Jiri Olsa]

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

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

[Oleg Nesterov]

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>

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

[Oleg Nesterov]

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.

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

[Jiri Olsa]

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

[PATCH 3/2] perf/hw_breakpoint: Remove superfluous bp->attr.disabled = 0 new attr has disabled set

[Jiri Olsa]

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

Re: [PATCH 3/2] perf/hw_breakpoint: Remove superfluous bp->attr.disabled = 0 new attr has disabled set

[Oleg Nesterov]

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.

Re: [PATCH 3/2] perf/hw_breakpoint: Remove superfluous bp->attr.disabled = 0 new attr has disabled set

[Jiri Olsa]

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

Re: [PATCH 3/2] perf/hw_breakpoint: Remove superfluous bp->attr.disabled = 0 new attr has disabled set

[Oleg Nesterov]

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.

Re: [PATCH 3/2] perf/hw_breakpoint: Remove superfluous bp->attr.disabled = 0 new attr has disabled set

[Jiri Olsa]

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