Hello.
On top of "[PATCH 0/5] kill ptrace_{get,put}_breakpoints()".
Cleanup and preparation for the potential fix, see below.
------------------------------------------------------------------------------
Now the question. Initially I was going to make more patches
and fix the regression introduced by 24f1e32c (although I am
not 100% sure which exactly patch should be blamed).
See https://bugzilla.redhat.com/show_bug.cgi?id=660204 for
details.
ptrace_write_dr7() does not create bp if it is zero, the comment
says:
/*
* We should have at least an inactive breakpoint at
* this slot. It means the user is writing dr7 without
* having written the address register first.
*/
and this looks logical. However, at least until 72f674d2
ptrace_set_debugreg(n => 7) worked even if addr wasn't set
by ptrace_set_debugreg(n => 0|1|2|3) before.
And note that ptrace_get_debugreg() does not fail if !ptrace_bps[n],
it just returns zero as if the address register was written. And
there is no way to know if address was actually set, not good and
not consistent.
Jan, Frederic, et all. What do you think we should do?
1. Change ptrace_write_dr7() to do register_user_hw_breakpoint()
if necessary.
This is what I was going to do, but I am no longer sure
we want this. For what? Unlikely it is very useful to use
the "default" addr == 0 for debugging.
2. Change ptrace_get_debugreg(0-4) to return -ESOMETHING if
ptrace_bps[n] == NULL.
This will match ptrace_set_debugreg(), but this can break
something else...
3. Do nothing.
I am inclined to do "1", but please comment.
Oleg.
ptrace_write_dr7() looks unnecessarily overcomplicated. We can
factor out ptrace_modify_breakpoint() and do not do "continue"
twice, just we need to pass the proper "disabled" argument to
ptrace_modify_breakpoint().
Signed-off-by: Oleg Nesterov <[email protected]>
---
arch/x86/kernel/ptrace.c | 40 +++++++++++++++-------------------------
1 files changed, 15 insertions(+), 25 deletions(-)
diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
index 7a98b21..0649f16 100644
--- a/arch/x86/kernel/ptrace.c
+++ b/arch/x86/kernel/ptrace.c
@@ -637,9 +637,7 @@ static int ptrace_write_dr7(struct task_struct *tsk, unsigned long data)
struct thread_struct *thread = &(tsk->thread);
unsigned long old_dr7;
int i, orig_ret = 0, rc = 0;
- int enabled, second_pass = 0;
- unsigned len, type;
- struct perf_event *bp;
+ int second_pass = 0;
data &= ~DR_CONTROL_RESERVED;
old_dr7 = ptrace_get_dr7(thread->ptrace_bps);
@@ -649,30 +647,22 @@ restore:
* appropriate changes to each.
*/
for (i = 0; i < HBP_NUM; i++) {
- enabled = decode_dr7(data, i, &len, &type);
- bp = thread->ptrace_bps[i];
-
- if (!enabled) {
- if (bp) {
- /*
- * Don't unregister the breakpoints right-away,
- * unless all register_user_hw_breakpoint()
- * requests have succeeded. This prevents
- * any window of opportunity for debug
- * register grabbing by other users.
- */
- if (!second_pass)
- continue;
-
- rc = ptrace_modify_breakpoint(bp, len, type,
- tsk, 1);
- if (rc)
- break;
- }
- continue;
+ unsigned len, type;
+ bool disabled = !decode_dr7(data, i, &len, &type);
+ struct perf_event *bp = thread->ptrace_bps[i];
+
+ if (disabled) {
+ /*
+ * Don't unregister the breakpoints right-away, unless
+ * all register_user_hw_breakpoint() requests have
+ * succeeded. This prevents any window of opportunity
+ * for debug register grabbing by other users.
+ */
+ if (!bp || !second_pass)
+ continue;
}
- rc = ptrace_modify_breakpoint(bp, len, type, tsk, 0);
+ rc = ptrace_modify_breakpoint(bp, len, type, tsk, disabled);
if (rc)
break;
}
--
1.5.5.1
ptrace_write_dr7() skips ptrace_modify_breakpoint(disabled => true)
unless second_pass, this buys nothing but complicates the code and
means that we always do the main loop twice even if "disabled" was
never true.
The comment says:
Don't unregister the breakpoints right-away,
unless all register_user_hw_breakpoint()
requests have succeeded.
I think this logic was always wrong, hw_breakpoint_del() does not
free the slot so perf_event_disable() can't hurt.
But in any case this looks unneeded nowadays, and contrary to what
the comment says we do not do register_user_hw_breakpoint(), this
was removed by 24f1e32c "hw-breakpoints: Rewrite the hw-breakpoints
layer on top of perf events".
Remove the "second_pass" check from the main loop and simplify the
code. Since we have to check "bp != NULL" anyway, the patch also
removes the same check in ptrace_modify_breakpoint() and moves the
comment into ptrace_write_dr7().
Signed-off-by: Oleg Nesterov <[email protected]>
---
arch/x86/kernel/ptrace.c | 46 +++++++++++++++++-----------------------------
1 files changed, 17 insertions(+), 29 deletions(-)
diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
index 0649f16..6814f27 100644
--- a/arch/x86/kernel/ptrace.c
+++ b/arch/x86/kernel/ptrace.c
@@ -609,14 +609,6 @@ ptrace_modify_breakpoint(struct perf_event *bp, int len, int type,
int gen_len, gen_type;
struct perf_event_attr attr;
- /*
- * We should have at least an inactive breakpoint at this
- * slot. It means the user is writing dr7 without having
- * written the address register first
- */
- if (!bp)
- return -EINVAL;
-
err = arch_bp_generic_fields(len, type, &gen_len, &gen_type);
if (err)
return err;
@@ -634,10 +626,10 @@ ptrace_modify_breakpoint(struct perf_event *bp, int len, int type,
*/
static int ptrace_write_dr7(struct task_struct *tsk, unsigned long data)
{
- struct thread_struct *thread = &(tsk->thread);
+ struct thread_struct *thread = &tsk->thread;
unsigned long old_dr7;
- int i, orig_ret = 0, rc = 0;
- int second_pass = 0;
+ int i, ret = 0, rc = 0;
+ bool second_pass = false;
data &= ~DR_CONTROL_RESERVED;
old_dr7 = ptrace_get_dr7(thread->ptrace_bps);
@@ -651,35 +643,31 @@ restore:
bool disabled = !decode_dr7(data, i, &len, &type);
struct perf_event *bp = thread->ptrace_bps[i];
- if (disabled) {
+ if (!bp) {
+ if (disabled)
+ continue;
/*
- * Don't unregister the breakpoints right-away, unless
- * all register_user_hw_breakpoint() requests have
- * succeeded. This prevents any window of opportunity
- * for debug register grabbing by other users.
+ * We should have at least an inactive breakpoint at
+ * this slot. It means the user is writing dr7 without
+ * having written the address register first.
*/
- if (!bp || !second_pass)
- continue;
+ rc = -EINVAL;
+ break;
}
rc = ptrace_modify_breakpoint(bp, len, type, tsk, disabled);
if (rc)
break;
}
- /*
- * Make a second pass to free the remaining unused breakpoints
- * or to restore the original breakpoints if an error occurred.
- */
- if (!second_pass) {
- second_pass = 1;
- if (rc < 0) {
- orig_ret = rc;
- data = old_dr7;
- }
+ /* Make a second pass to restore the original breakpoints if failed */
+ if (!second_pass && rc) {
+ second_pass = true;
+ ret = rc;
+ data = old_dr7;
goto restore;
}
- return orig_ret < 0 ? orig_ret : rc;
+ return ret;
}
/*
--
1.5.5.1
On Sun, 14 Apr 2013 21:12:05 +0200, Oleg Nesterov wrote:
> Jan, Frederic, et all. What do you think we should do?
>
> 1. Change ptrace_write_dr7() to do register_user_hw_breakpoint()
> if necessary.
>
> This is what I was going to do, but I am no longer sure
> we want this. For what? Unlikely it is very useful to use
> the "default" addr == 0 for debugging.
I do not understand how these functions map to the PTRACE_* syscall.
But this was a regression from the application point of view as some
application did/do:
* waitpid - get the process to: t (tracing stop)
* PTRACE_POKEUSER DR7, enableDR0
* PTRACE_POKEUSER DR0, address
* PTRACE_CONT
This was perfectly valid before, there is no "default" addr == 0 used for any
debugging. Just the applications did not care about PTRACE_POKEUSER ordering.
This is also how the bug was found.
Thanks,
Jan
On 04/14, Jan Kratochvil wrote:
>
> On Sun, 14 Apr 2013 21:12:05 +0200, Oleg Nesterov wrote:
> > Jan, Frederic, et all. What do you think we should do?
> >
> > 1. Change ptrace_write_dr7() to do register_user_hw_breakpoint()
> > if necessary.
> >
> > This is what I was going to do, but I am no longer sure
> > we want this. For what? Unlikely it is very useful to use
> > the "default" addr == 0 for debugging.
>
> I do not understand how these functions map to the PTRACE_* syscall.
>
> But this was a regression from the application point of view as some
> application did/do:
> * waitpid - get the process to: t (tracing stop)
> * PTRACE_POKEUSER DR7, enableDR0
> * PTRACE_POKEUSER DR0, address
> * PTRACE_CONT
>
> This was perfectly valid before, there is no "default" addr == 0 used for any
> debugging. Just the applications did not care about PTRACE_POKEUSER ordering.
> This is also how the bug was found.
Yes, exactly.
Except 'there is no "default" addr == 0', the first
"PTRACE_POKEUSER DR7, enableDR0" used addr == 0 and then it was
changed by "PTRACE_POKEUSER DR0".
And once again, I am ready to make the patch, it should be simple.
Just I am not sure it worth the trouble, so I decided to ask first.
Nobody noticed this problem(?) except you, and this was broken a
long ago.
PTRACE_POKEUSER DR0, address
PTRACE_POKEUSER DR7, enableDR0
should work and this looks better, we do not enable bp until it
has the correct address set. Of course this doesn't really matter
if the tracee doesn't not run in between, but still...
Oleg.
On Sun, Apr 14, 2013 at 09:12:05PM +0200, Oleg Nesterov wrote:
> Hello.
>
> On top of "[PATCH 0/5] kill ptrace_{get,put}_breakpoints()".
> Cleanup and preparation for the potential fix, see below.
>
> ------------------------------------------------------------------------------
> Now the question. Initially I was going to make more patches
> and fix the regression introduced by 24f1e32c (although I am
> not 100% sure which exactly patch should be blamed).
>
> See https://bugzilla.redhat.com/show_bug.cgi?id=660204 for
> details.
Oh I missed that.
>
> ptrace_write_dr7() does not create bp if it is zero, the comment
> says:
>
> /*
> * We should have at least an inactive breakpoint at
> * this slot. It means the user is writing dr7 without
> * having written the address register first.
> */
>
> and this looks logical. However, at least until 72f674d2
> ptrace_set_debugreg(n => 7) worked even if addr wasn't set
> by ptrace_set_debugreg(n => 0|1|2|3) before.
>
> And note that ptrace_get_debugreg() does not fail if !ptrace_bps[n],
> it just returns zero as if the address register was written. And
> there is no way to know if address was actually set, not good and
> not consistent.
Indeed.
Looking at the bug report, it seems they only reproduced with a homemade
test. No real app has reported that issue?
Now I guess this is irrelevant. It indeed seems to me saner to be
consistent with regs read like you are pointing out. And that ABI
breakage makes me uncomfortable, even though we haven't heard about
real breakage yet.
>
> Jan, Frederic, et all. What do you think we should do?
>
> 1. Change ptrace_write_dr7() to do register_user_hw_breakpoint()
> if necessary.
>
> This is what I was going to do, but I am no longer sure
> we want this. For what? Unlikely it is very useful to use
> the "default" addr == 0 for debugging.
So you mean assume that the addr is 0 in dr[0-3] if we write dr7 before writing
the addr register?
Yes, I'm convinced that's the right direction!
>
> 2. Change ptrace_get_debugreg(0-4) to return -ESOMETHING if
> ptrace_bps[n] == NULL.
>
> This will match ptrace_set_debugreg(), but this can break
> something else...
Yeah that would be worse, and I'm sure that breaks existing apps :)
Thanks!
On Sun, Apr 14, 2013 at 09:12:28PM +0200, Oleg Nesterov wrote:
> ptrace_write_dr7() looks unnecessarily overcomplicated. We can
> factor out ptrace_modify_breakpoint() and do not do "continue"
> twice, just we need to pass the proper "disabled" argument to
> ptrace_modify_breakpoint().
>
> Signed-off-by: Oleg Nesterov <[email protected]>
Acked-by: Frederic Weisbecker <[email protected]>
On Sun, Apr 14, 2013 at 09:12:32PM +0200, Oleg Nesterov wrote:
> ptrace_write_dr7() skips ptrace_modify_breakpoint(disabled => true)
> unless second_pass, this buys nothing but complicates the code and
> means that we always do the main loop twice even if "disabled" was
> never true.
>
> The comment says:
>
> Don't unregister the breakpoints right-away,
> unless all register_user_hw_breakpoint()
> requests have succeeded.
>
> I think this logic was always wrong, hw_breakpoint_del() does not
> free the slot so perf_event_disable() can't hurt.
For the record, I think it was necessary before
44234adcdce38f83c56e05f808ce656175b4beeb
("hw-breakpoints: Modify breakpoints without unregistering them") because
modifying a breakpoint implied that the old bp was released and a new one
was created, opening a little race window in between against concurrent
breakpoint users.
>
> But in any case this looks unneeded nowadays, and contrary to what
> the comment says we do not do register_user_hw_breakpoint(), this
> was removed by 24f1e32c "hw-breakpoints: Rewrite the hw-breakpoints
> layer on top of perf events".
>
> Remove the "second_pass" check from the main loop and simplify the
> code. Since we have to check "bp != NULL" anyway, the patch also
> removes the same check in ptrace_modify_breakpoint() and moves the
> comment into ptrace_write_dr7().
>
> Signed-off-by: Oleg Nesterov <[email protected]>
Yeah I can't find a reason to keep that deferred disabling.
Acked-by: Frederic Weisbecker <[email protected]>
The patch looks good, I just have a small suggestion below:
> ---
> arch/x86/kernel/ptrace.c | 46 +++++++++++++++++-----------------------------
> 1 files changed, 17 insertions(+), 29 deletions(-)
>
> diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
> index 0649f16..6814f27 100644
> --- a/arch/x86/kernel/ptrace.c
> +++ b/arch/x86/kernel/ptrace.c
> @@ -609,14 +609,6 @@ ptrace_modify_breakpoint(struct perf_event *bp, int len, int type,
> int gen_len, gen_type;
> struct perf_event_attr attr;
>
> - /*
> - * We should have at least an inactive breakpoint at this
> - * slot. It means the user is writing dr7 without having
> - * written the address register first
> - */
> - if (!bp)
> - return -EINVAL;
> -
> err = arch_bp_generic_fields(len, type, &gen_len, &gen_type);
> if (err)
> return err;
> @@ -634,10 +626,10 @@ ptrace_modify_breakpoint(struct perf_event *bp, int len, int type,
> */
> static int ptrace_write_dr7(struct task_struct *tsk, unsigned long data)
> {
> - struct thread_struct *thread = &(tsk->thread);
> + struct thread_struct *thread = &tsk->thread;
> unsigned long old_dr7;
> - int i, orig_ret = 0, rc = 0;
> - int second_pass = 0;
> + int i, ret = 0, rc = 0;
> + bool second_pass = false;
>
> data &= ~DR_CONTROL_RESERVED;
> old_dr7 = ptrace_get_dr7(thread->ptrace_bps);
> @@ -651,35 +643,31 @@ restore:
> bool disabled = !decode_dr7(data, i, &len, &type);
> struct perf_event *bp = thread->ptrace_bps[i];
>
> - if (disabled) {
> + if (!bp) {
> + if (disabled)
> + continue;
> /*
> - * Don't unregister the breakpoints right-away, unless
> - * all register_user_hw_breakpoint() requests have
> - * succeeded. This prevents any window of opportunity
> - * for debug register grabbing by other users.
> + * We should have at least an inactive breakpoint at
> + * this slot. It means the user is writing dr7 without
> + * having written the address register first.
> */
> - if (!bp || !second_pass)
> - continue;
> + rc = -EINVAL;
> + break;
> }
>
> rc = ptrace_modify_breakpoint(bp, len, type, tsk, disabled);
> if (rc)
> break;
It would be nice to warn here:
WARN_ON_ONCE(rc && second_pass);
Because we rely on the second pass operations to always succeed. And these are indeed supposed
to. If not then we have a bug hiding somewhere that we really want to know about. And given
the complicated code path we can have with breakpoints and perf, it would be nice to
have that check.
Thanks.
> }
> - /*
> - * Make a second pass to free the remaining unused breakpoints
> - * or to restore the original breakpoints if an error occurred.
> - */
> - if (!second_pass) {
> - second_pass = 1;
> - if (rc < 0) {
> - orig_ret = rc;
> - data = old_dr7;
> - }
> + /* Make a second pass to restore the original breakpoints if failed */
> + if (!second_pass && rc) {
> + second_pass = true;
> + ret = rc;
> + data = old_dr7;
> goto restore;
> }
>
> - return orig_ret < 0 ? orig_ret : rc;
> + return ret;
> }
>
> /*
> --
> 1.5.5.1
>
On 04/16, Frederic Weisbecker wrote:
>
> On Sun, Apr 14, 2013 at 09:12:05PM +0200, Oleg Nesterov wrote:
>
> Looking at the bug report, it seems they only reproduced with a homemade
> test. No real app has reported that issue?
iirc (Jan can correct me) gdb hit this problem, but it was already
changed to change DR0 first.
> > Jan, Frederic, et all. What do you think we should do?
> >
> > 1. Change ptrace_write_dr7() to do register_user_hw_breakpoint()
> > if necessary.
> >
> > This is what I was going to do, but I am no longer sure
> > we want this. For what? Unlikely it is very useful to use
> > the "default" addr == 0 for debugging.
>
> So you mean assume that the addr is 0 in dr[0-3] if we write dr7 before writing
> the addr register?
Yes,
> Yes, I'm convinced that's the right direction!
OK. Thanks Jan and Frederic.
I'll send v2 tomorrow with the 3rd patch which adds _register into
write_dr7. Plus another minor/offtopic fix I forgot to send.
Oleg.
On 04/16, Frederic Weisbecker wrote:
>
> On Sun, Apr 14, 2013 at 09:12:32PM +0200, Oleg Nesterov wrote:
> > ptrace_write_dr7() skips ptrace_modify_breakpoint(disabled => true)
> > unless second_pass, this buys nothing but complicates the code and
> > means that we always do the main loop twice even if "disabled" was
> > never true.
> >
> > The comment says:
> >
> > Don't unregister the breakpoints right-away,
> > unless all register_user_hw_breakpoint()
> > requests have succeeded.
> >
> > I think this logic was always wrong, hw_breakpoint_del() does not
> > free the slot so perf_event_disable() can't hurt.
>
> For the record, I think it was necessary before
> 44234adcdce38f83c56e05f808ce656175b4beeb
> ("hw-breakpoints: Modify breakpoints without unregistering them") because
> modifying a breakpoint implied that the old bp was released and a new one
> was created, opening a little race window in between against concurrent
> breakpoint users.
Aah, thank, I'll update the changelog.
> Acked-by: Frederic Weisbecker <[email protected]>
Thanks!
> > old_dr7 = ptrace_get_dr7(thread->ptrace_bps);
> > @@ -651,35 +643,31 @@ restore:
> > bool disabled = !decode_dr7(data, i, &len, &type);
> > struct perf_event *bp = thread->ptrace_bps[i];
> >
> > - if (disabled) {
> > + if (!bp) {
> > + if (disabled)
> > + continue;
> > /*
> > - * Don't unregister the breakpoints right-away, unless
> > - * all register_user_hw_breakpoint() requests have
> > - * succeeded. This prevents any window of opportunity
> > - * for debug register grabbing by other users.
> > + * We should have at least an inactive breakpoint at
> > + * this slot. It means the user is writing dr7 without
> > + * having written the address register first.
> > */
> > - if (!bp || !second_pass)
> > - continue;
> > + rc = -EINVAL;
> > + break;
> > }
> >
> > rc = ptrace_modify_breakpoint(bp, len, type, tsk, disabled);
> > if (rc)
> > break;
>
> It would be nice to warn here:
>
> WARN_ON_ONCE(rc && second_pass);
Well, I disagree.
To clarify, I agree with WARN_ON_ONCE(), but afaics it has nothing to
do with "second_pass",
> And these are indeed supposed
> to.
Indeed, but this is because ptrace_modify_breakpoint() should not fail.
So, what do you think if I change the main loop above
rc = ptrace_modify_breakpoint(...)
- if (rc)
+ if (WARN_ON_ONCE(rc))
break;
instead?
Oleg.
2013/4/16 Oleg Nesterov <[email protected]>:
> On 04/16, Frederic Weisbecker wrote:
>> > rc = ptrace_modify_breakpoint(bp, len, type, tsk, disabled);
>> > if (rc)
>> > break;
>>
>> It would be nice to warn here:
>>
>> WARN_ON_ONCE(rc && second_pass);
>
> Well, I disagree.
>
> To clarify, I agree with WARN_ON_ONCE(), but afaics it has nothing to
> do with "second_pass",
>
>> And these are indeed supposed
>> to.
>
> Indeed, but this is because ptrace_modify_breakpoint() should not fail.
>
> So, what do you think if I change the main loop above
>
> rc = ptrace_modify_breakpoint(...)
> - if (rc)
> + if (WARN_ON_ONCE(rc))
> break;
It can fail in the first pass if dr7 is incorrect. For example passing
a length of 8 in x86-32 is rejected. The type can be wrong too.
But the second pass shouldn't fail. If it was validated once, then it
should be valid a second time.
On Tue, 16 Apr 2013 15:25:45 +0200, Oleg Nesterov wrote:
> On 04/16, Frederic Weisbecker wrote:
> > On Sun, Apr 14, 2013 at 09:12:05PM +0200, Oleg Nesterov wrote:
> > Looking at the bug report, it seems they only reproduced with a homemade
> > test. No real app has reported that issue?
>
> iirc (Jan can correct me) gdb hit this problem, but it was already
> changed to change DR0 first.
Both old GDB and recent GDB handle it correctly (DR0 first, then DR7).
I do not remember how but I have hit this issue, probably during development
of the GDB watchpoint code, there were many variants of it in the past.
So it was not found just by an artificial testing.
My concern was not so much about GDB but rather about possible other existing
debugging/tracing software using DR.
Jan
On 04/17, Frederic Weisbecker wrote:
>
> 2013/4/16 Oleg Nesterov <[email protected]>:
> >
> > Well, I disagree.
> >
> > To clarify, I agree with WARN_ON_ONCE(), but afaics it has nothing to
> > do with "second_pass",
> >
> >> And these are indeed supposed
> >> to.
> >
> > Indeed, but this is because ptrace_modify_breakpoint() should not fail.
> >
> > So, what do you think if I change the main loop above
> >
> > rc = ptrace_modify_breakpoint(...)
> > - if (rc)
> > + if (WARN_ON_ONCE(rc))
> > break;
>
> It can fail in the first pass if dr7 is incorrect. For example passing
> a length
^^^^^^
Oops. Indeed, I forgot that ptrace_modify_breakpoint() has other arguments,
not only "disabled" ;)
Thanks for correcting me.
Oleg.