perf/hw_breakpoint: Fix breakpoint modify

PATCH Superseded Jiri Olsa 3 participants 15 messages Aug 6, 2018 – Aug 7, 2018 perf events

Share & tools

Jiri Olsa

author reviewer

2018-08-06 10:12 UTC #0 lore raw

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

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

Jiri Olsa

author reviewer

2018-08-06 10:12 UTC #1 lore raw

PATCH 1/2 perf tests: Add breakpoint modify test

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/[email protected] open here

Signed-off-by Jiri Olsa <[email protected]>

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

tools/perf/arch/x86/include/arch-tests.h +1 -0

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;

tools/perf/arch/x86/tests/Build +1 -0

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

tools/perf/arch/x86/tests/arch-tests.c +6 -0

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,

tools/perf/arch/x86/tests/bp-modify.c new file +119 -0

119 lines not on this page · raw message

Signature · 1 line

2.17.1

Jiri Olsa

author reviewer

2018-08-06 10:12 UTC #2 lore raw

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

In reply to 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 <[email protected]>

Link http://lkml.kernel.org/n/[email protected] open here

Signed-off-by Jiri Olsa <[email protected]>

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

kernel/events/hw_breakpoint.c +6 -4

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

Signature · 1 line

2.17.1

Oleg Nesterov

2018-08-06 12:48 UTC #3 lore raw

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

Quoted 3 lines · Jiri Olsa

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

Quoted 12 lines

int modify_user_hw_breakpoint(struct perf_event *bp, struct perf_event_attr *attr) {

kernel/events/hw_breakpoint.c +2 -1

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

kernel/events/hw_breakpoint.c int modify_user_hw_breakpoint(struct perf_event *bp, struct perf_event_attr *att · line 531

Quoted 10 lines

kernel/events/hw_breakpoint.c +4 -3

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

Oleg Nesterov

2018-08-06 13:07 UTC #4 lore raw

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

Quoted 4 lines · Oleg Nesterov

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.

Jiri Olsa

2018-08-06 13:37 UTC #5 lore raw

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

Quoted 6 lines · Oleg Nesterov

Quoted 4 lines · Oleg Nesterov
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.

Quoted 6 lines

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

Oleg Nesterov

2018-08-06 13:49 UTC #6 lore raw

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

Quoted 8 lines · Jiri Olsa

Quoted 6 lines · Oleg Nesterov
Quoted 4 lines · Oleg Nesterov
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.

Quoted 8 lines

Quoted 6 lines
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.

Jiri Olsa

2018-08-06 14:21 UTC #7 lore raw

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

Quoted 16 lines · Oleg Nesterov

Quoted 15 lines · Jiri Olsa
Quoted 12 lines · Oleg Nesterov
Quoted 4 lines · Oleg Nesterov
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

Quoted 6 lines

Quoted 1 line
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

Jiri Olsa

2018-08-06 13:23 UTC #8 lore raw

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

In reply to Oleg Nesterov

Quoted 36 lines · Oleg Nesterov

Quoted 3 lines · Jiri Olsa
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
Quoted 12 lines
int modify_user_hw_breakpoint(struct perf_event *bp, struct perf_event_attr *attr) {
kernel/events/hw_breakpoint.c +2 -1
+	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) {
kernel/events/hw_breakpoint.c int modify_user_hw_breakpoint(struct perf_event *bp, struct perf_event_attr *att · line 531
Quoted 10 lines
kernel/events/hw_breakpoint.c +4 -3
-		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);

Quoted 7 lines

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

Oleg Nesterov

Acked-by

2018-08-06 13:47 UTC #9 lore raw

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

Quoted 10 lines · Jiri Olsa

Quoted 10 lines
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.

Quoted 7 lines

Quoted 4 lines
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 <[email protected]>

Jiri Olsa

2018-08-06 15:08 UTC #10 lore raw

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

In reply to Jiri Olsa

Quoted 45 lines · Jiri Olsa

Quoted 36 lines · Oleg Nesterov
Quoted 3 lines · Jiri Olsa
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
Quoted 12 lines
int modify_user_hw_breakpoint(struct perf_event *bp, struct perf_event_attr *attr) {
(quoted diff) +2 -1
+	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) {
Review int modify_user_hw_breakpoint(struct perf_event *bp, struct perf_event_attr *att · line 531
Quoted 10 lines
(quoted diff) +4 -3
-		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);
Quoted 7 lines
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.

Quoted 4 lines

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/[email protected] open here

Signed-off-by Jiri Olsa <[email protected]>

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

kernel/events/hw_breakpoint.c +2 -3

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

Signature · 1 line

2.17.1

Oleg Nesterov

2018-08-06 16:34 UTC #11 lore raw

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

Quoted 16 lines · 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/[email protected] open here
Signed-off-by Jiri Olsa <[email protected]>
---
 kernel/events/hw_breakpoint.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)
kernel/events/hw_breakpoint.c +0 -0
index fb229d9c7f3c..3e560d7609fd 100644

kernel/events/hw_breakpoint.c int modify_user_hw_breakpoint(struct perf_event *bp, struct perf_event_attr *att · line 531

Quoted 9 lines

kernel/events/hw_breakpoint.c +2 -3

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

Jiri Olsa

2018-08-07 08:16 UTC #12 lore raw

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

Quoted 29 lines · Oleg Nesterov

Quoted 16 lines · 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/[email protected] open here
Signed-off-by Jiri Olsa <[email protected]>
---
 kernel/events/hw_breakpoint.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)
kernel/events/hw_breakpoint.c +0 -0
index fb229d9c7f3c..3e560d7609fd 100644
kernel/events/hw_breakpoint.c int modify_user_hw_breakpoint(struct perf_event *bp, struct perf_event_attr *att · line 531
Quoted 9 lines
kernel/events/hw_breakpoint.c +2 -3
@@ -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.

Quoted 8 lines

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

Oleg Nesterov

2018-08-07 09:10 UTC #13 lore raw

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

Quoted 22 lines · Jiri Olsa

Quoted 20 lines · Oleg Nesterov
kernel/events/hw_breakpoint.c int modify_user_hw_breakpoint(struct perf_event *bp, struct perf_event_attr *att · line 531
Quoted 12 lines
kernel/events/hw_breakpoint.c +2 -3
@@ -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.

Quoted 6 lines

Quoted 3 lines
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.

Jiri Olsa

2018-08-07 14:39 UTC #14 lore raw

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

Quoted 27 lines · Oleg Nesterov

Quoted 26 lines · Jiri Olsa
Quoted 23 lines · Oleg Nesterov
kernel/events/hw_breakpoint.c int modify_user_hw_breakpoint(struct perf_event *bp, struct perf_event_attr *att · line 531
Quoted 12 lines
kernel/events/hw_breakpoint.c +2 -3
@@ -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?

Quoted 8 lines

Quoted 2 lines
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

---

kernel/events/hw_breakpoint.c +5 -4

index fb229d9c7f3c..eff7ab716139 100644
@@ -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);

Signature · 1 line

2.17.1