perf/hw_breakpoint: Fix breakpoint modify

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

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.

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

index c1bd979b957b..613709cfbbd0 100644
@@ -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;

index 8e2c5a38c3b9..586849ff83a0 100644
@@ -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

index cc1802ff5410..d47d3f8e3c8e 100644
@@ -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,

2.17.1

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

---
 kernel/events/hw_breakpoint.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

index b3814fce5ecb..fb229d9c7f3c 100644
@@ -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

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.

---
 kernel/events/hw_breakpoint.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

index fb229d9c7f3c..3e560d7609fd 100644
@@ -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

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.

---
 kernel/events/hw_breakpoint.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

index 3e560d7609fd..eff7ab716139 100644
@@ -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

yep, seems good.. will send new version

thanks,
jirka

Em Thu, Aug 09, 2018 at 06:30:15PM +0200, Jiri Olsa escreveu:

<SNIP>

<SNIP>

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

- Arnaldo

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

Em Tue, Aug 28, 2018 at 04:38:23PM +0200, Jiri Olsa escreveu:

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

- Arnaldo

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.

---
 arch/x86/kernel/ptrace.c | 5 +++++
 1 file changed, 5 insertions(+)

index e2ee403865eb..22c06d0a38d1 100644
@@ -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

Em Thu, Aug 09, 2018 at 02:03:00PM +0200, Jiri Olsa escreveu:

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

- Arnaldo