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

v3 changes:
- added Oleg's ack for patch 3
- new patches 4,5 based on Oleg's suggestions replacing the v2 fallback approach by enabling the event directly after failed modification

v2 changes:
- added Oleg's ack for patch 2
- added new changes based on Oleg's questions
plus new test code

thanks,
jirka

---
Jiri Olsa (5):
perf tests: Add breakpoint modify tests
perf/hw_breakpoint: Modify breakpoint even if the new attr has disabled set perf/hw_breakpoint: Remove superfluous bp->attr.disabled = 0 perf/hw_breakpoint: Enable breakpoint in modify_user_hw_breakpoint perf/hw_breakpoint: Simplify breakpoint enable in perf_event_modify_breakpoint

 kernel/events/core.c                     |  11 ++-----
 kernel/events/hw_breakpoint.c            |  13 ++++----
 tools/perf/arch/x86/include/arch-tests.h |   1 +
 tools/perf/arch/x86/tests/Build          |   1 +
 tools/perf/arch/x86/tests/arch-tests.c   |   6 ++++
 tools/perf/arch/x86/tests/bp-modify.c    | 213 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 230 insertions(+), 15 deletions(-)
 create mode 100644 tools/perf/arch/x86/tests/bp-modify.c

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

Right, now that we fixed the breakpoint modification arch code, it should be safe to do that.

Thanks!

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

Currently we enable the breakpoint back only if the breakpoint modification was successful. If it fails we can leave the breakpoint in disabled state with attr->disabled == 0.

We can safely enable the breakpoint back for both the fail and success paths by checking the bp->attr.disabled, which either holds the new 'requested' disabled state or the original breakpoint state.

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

index 3e560d7609fd..d6b56180827c 100644
@@ -523,13 +523,11 @@ int modify_user_hw_breakpoint(struct perf_event *bp, struct perf_event_attr *att
 		perf_event_disable(bp);
 
 	err = modify_user_hw_breakpoint_check(bp, attr, false);
-	if (err)
-		return err;
 
-	if (!attr->disabled)
+	if (!bp->attr.disabled)
 		perf_event_enable(bp);
 
-	return 0;
+	return err;
 }
 EXPORT_SYMBOL_GPL(modify_user_hw_breakpoint);
 

2.17.1

Thanks a lot!

We can safely enable the breakpoint back for both the fail and success paths by checking only the bp->attr.disabled, which either holds the new 'requested' disabled state or the original breakpoint state.

---
 kernel/events/core.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

index f6ea33a9f904..22ede28ec07d 100644
@@ -2867,16 +2867,11 @@ static int perf_event_modify_breakpoint(struct perf_event *bp,
 	_perf_event_disable(bp);
 
 	err = modify_user_hw_breakpoint_check(bp, attr, true);
-	if (err) {
-		if (!bp->attr.disabled)
-			_perf_event_enable(bp);
 
-		return err;
-	}
-
-	if (!attr->disabled)
+	if (!bp->attr.disabled)
 		_perf_event_enable(bp);
-	return 0;
+
+	return err;
 }
 
 static int perf_event_modify_attr(struct perf_event *event,

2.17.1

Ingo, Arnaldo,
would you please pick up those, or should I pushed them through somebody else?

thanks,
jirka