perf/hw_breakpoint: Fix breakpoint modify

Commit-ID:  9b3579fc6c6ac45502de1fa9a1fdf873805c2157
Gitweb:     https://git.kernel.org/tip/9b3579fc6c6ac45502de1fa9a1fdf873805c2157
Author:     Jiri Olsa <[email protected]>
AuthorDate: Mon, 27 Aug 2018 11:12:24 +0200
Committer:  Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Thu, 30 Aug 2018 14:49:22 -0300

perf tests: Add breakpoint modify tests

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.

Committer testing:

  # uname -a
  Linux jouet 4.18.0-rc8-00002-g1236568ee3cb #12 SMP Tue Aug 7 14:08:26 -03 2018 x86_64 x86_64 x86_64 GNU/Linux
  # perf test -v "bp modify"
  62: x86 bp modify                                         :
  --- start ---
  test child forked, pid 25671
  in bp_1
  tracee exited prematurely 2
  FAILED arch/x86/tests/bp-modify.c:209 modify test 1 failed

test child finished with -1
---- end ----
x86 bp modify: FAILED!
#

---
 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(+)

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,

Commit-ID:  bd14406b78e6daa1ea3c1673bda1ffc9efdeead0
Gitweb:     https://git.kernel.org/tip/bd14406b78e6daa1ea3c1673bda1ffc9efdeead0
Author:     Jiri Olsa <[email protected]>
AuthorDate: Mon, 27 Aug 2018 11:12:25 +0200
Committer:  Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Thu, 30 Aug 2018 14:49:23 -0300

perf/hw_breakpoint: Modify breakpoint even if the new attr has disabled set

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

Commit-ID:  cb45302d7c5e20f0c0598cdbd7753fa44daceb2a
Gitweb:     https://git.kernel.org/tip/cb45302d7c5e20f0c0598cdbd7753fa44daceb2a
Author:     Jiri Olsa <[email protected]>
AuthorDate: Mon, 27 Aug 2018 11:12:26 +0200
Committer:  Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Thu, 30 Aug 2018 14:49:23 -0300

perf/hw_breakpoint: Remove superfluous bp->attr.disabled = 0

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

Commit-ID:  969558371bf926258241727ebb994f516f2e6f61
Gitweb:     https://git.kernel.org/tip/969558371bf926258241727ebb994f516f2e6f61
Author:     Jiri Olsa <[email protected]>
AuthorDate: Mon, 27 Aug 2018 11:12:27 +0200
Committer:  Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Thu, 30 Aug 2018 14:49:23 -0300

perf/hw_breakpoint: Enable breakpoint in modify_user_hw_breakpoint

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

Commit-ID:  bf06278c3fdf8909c3a9283e2c270b0fc170fa90
Gitweb:     https://git.kernel.org/tip/bf06278c3fdf8909c3a9283e2c270b0fc170fa90
Author:     Jiri Olsa <[email protected]>
AuthorDate: Mon, 27 Aug 2018 11:12:28 +0200
Committer:  Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Thu, 30 Aug 2018 14:49:24 -0300

perf/hw_breakpoint: Simplify breakpoint enable in perf_event_modify_breakpoint

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.

Committer testing:

At the end of the series, the 'perf test' entry introduced as the first patch now runs to completion without finding the fixed issues:

  # perf test "bp modify"
  62: x86 bp modify                                         : Ok
  #

In verbose mode:

  # perf test -v "bp modify"
  62: x86 bp modify                                         :
  --- start ---
  test child forked, pid 5161
  rip 5950a0, bp_1 0x5950a0
  in bp_1
  rip 5950a0, bp_1 0x5950a0
  in bp_1
  test child finished with 0
  ---- end ----
  x86 bp modify: Ok

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