ptrace: DEBUGCTLMSR_BTF fixes

PATCH Superseded Oleg Nesterov 2 participants 17 messages Aug 3, 2012 – Aug 7, 2012 x86

lore

Share & tools

lore

Oleg Nesterov

author

2012-08-03 16:32 UTC #0 lore raw

cover ptrace: DEBUGCTLMSR_BTF fixes

Hello.

Ingo, could you take this into -tip as well?

The subject says "ptrace:", but we need these changes for uprobes. Although (afaics) this fix makes sense by itself.

Assuming this series passes the review of course. And it would be really nice if someone reviews 2/2, I am not confident that I fully understand this DEBUGCTLMSR_BTF magic.

The next step will change uprobes to use the new helper and avoid user_enable_single_step().

Oleg.

Oleg Nesterov

author

2012-08-03 16:33 UTC #1 lore raw

PATCH 1/2 ptrace: introduce set_task_blockstep() helper

No functional changes, preparation for the next fix and for uprobes single-step fixes.

Move the code playing with TIF_BLOCKSTEP/DEBUGCTLMSR_BTF into the new helper, set_task_blockstep().

Signed-off-by Oleg Nesterov <[email protected]>

---
 arch/x86/kernel/step.c |   43 +++++++++++++++++++++++--------------------
 1 files changed, 23 insertions(+), 20 deletions(-)

arch/x86/kernel/step.c +23 ~~-20~~

index c346d11..afa60db 100644
@@ -157,6 +157,23 @@ static int enable_single_step(struct task_struct *child)
 	return 1;
 }
 
+static void set_task_blockstep(struct task_struct *task, bool on)
+{
+	unsigned long debugctl;
+
+	if (on)
+		set_tsk_thread_flag(task, TIF_BLOCKSTEP);
+	else
+		clear_tsk_thread_flag(task, TIF_BLOCKSTEP);
+
+	debugctl = get_debugctlmsr();
+	if (on)
+		debugctl |= DEBUGCTLMSR_BTF;
+	else
+		debugctl &= ~DEBUGCTLMSR_BTF;
+	update_debugctlmsr(debugctl);
+}
+
 /*
  * Enable single or block step.
  */

@@ -169,19 +186,10 @@ static void enable_step(struct task_struct *child, bool block)
 	 * So no one should try to use debugger block stepping in a program
 	 * that uses user-mode single stepping itself.
 	 */
-	if (enable_single_step(child) && block) {
-		unsigned long debugctl = get_debugctlmsr();
-
-		debugctl |= DEBUGCTLMSR_BTF;
-		update_debugctlmsr(debugctl);
-		set_tsk_thread_flag(child, TIF_BLOCKSTEP);
-	} else if (test_tsk_thread_flag(child, TIF_BLOCKSTEP)) {
-		unsigned long debugctl = get_debugctlmsr();
-
-		debugctl &= ~DEBUGCTLMSR_BTF;
-		update_debugctlmsr(debugctl);
-		clear_tsk_thread_flag(child, TIF_BLOCKSTEP);
-	}
+	if (enable_single_step(child) && block)
+		set_task_blockstep(child, true);
+	else if (test_tsk_thread_flag(child, TIF_BLOCKSTEP))
+		set_task_blockstep(child, false);
 }
 
 void user_enable_single_step(struct task_struct *child)

@@ -199,13 +207,8 @@ void user_disable_single_step(struct task_struct *child)
 	/*
 	 * Make sure block stepping (BTF) is disabled.
 	 */
-	if (test_tsk_thread_flag(child, TIF_BLOCKSTEP)) {
-		unsigned long debugctl = get_debugctlmsr();
-
-		debugctl &= ~DEBUGCTLMSR_BTF;
-		update_debugctlmsr(debugctl);
-		clear_tsk_thread_flag(child, TIF_BLOCKSTEP);
-	}
+	if (test_tsk_thread_flag(child, TIF_BLOCKSTEP))
+		set_task_blockstep(child, false);
 
 	/* Always clear TIF_SINGLESTEP... */
 	clear_tsk_thread_flag(child, TIF_SINGLESTEP);

Signature · 1 line

1.5.5.1

Oleg Nesterov

author

2012-08-03 16:33 UTC #2 lore raw

PATCH 2/2 ptrace: fix set_task_blockstep()->update_debugctlmsr() logic

In reply to Oleg Nesterov

Afaics the usage of update_debugctlmsr() in step.c was always very wrong.

1. update_debugctlmsr() was simply unneeded. The child sleeps TASK_TRACED, __switch_to_xtra(next_p => child) should notice TIF_BLOCKSTEP and set/clear DEBUGCTLMSR_BTF after resume if needed.

2. It is wrong. The state of DEBUGCTLMSR_BTF bit in CPU register should always match the state of current's TIF_BLOCKSTEP bit.

3. Even get_debugctlmsr() + update_debugctlmsr() itself does not look right. Irq can change other bits in MSR_IA32_DEBUGCTLMSR register or the caller can be preempted in between.

However, now that uprobes uses user_enable_single_step(current) we can't simply remove update_debugctlmsr(). So this patch adds the additional "task == current" check and disables irqs to avoid the race with interrupts/preemption.

Signed-off-by Oleg Nesterov <[email protected]>

---
 arch/x86/kernel/step.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

arch/x86/kernel/step.c +6 -0

index afa60db..636402e 100644
@@ -166,12 +166,18 @@ static void set_task_blockstep(struct task_struct *task, bool on)
 	else
 		clear_tsk_thread_flag(task, TIF_BLOCKSTEP);
 
+	if (task != current)
+		return;
+
+	/* ensure irq/preemption can't change debugctl in between */
+	local_irq_disable();
 	debugctl = get_debugctlmsr();
 	if (on)
 		debugctl |= DEBUGCTLMSR_BTF;
 	else
 		debugctl &= ~DEBUGCTLMSR_BTF;
 	update_debugctlmsr(debugctl);
+	local_irq_enable();
 }
 
 /*

Signature · 1 line

1.5.5.1

Sebastian Andrzej Siewior

2012-08-03 16:43 UTC #3 lore raw

Re: [PATCH 2/2] ptrace: fix set_task_blockstep()->update_debugctlmsr() logic

Quoted 10 lines · Oleg Nesterov

arch/x86/kernel/step.c +2 -0

index afa60db..636402e 100644
@@ -166,12 +166,18 @@ static void set_task_blockstep(struct task_struct *task, bool on)
  	else
  		clear_tsk_thread_flag(task, TIF_BLOCKSTEP);
 
+	if (task != current)
+		return;

arch/x86/kernel/step.c static void set_task_blockstep(struct task_struct *task, bool on) · line 180

Quoted 10 lines

arch/x86/kernel/step.c +4 -0

+
+	/* ensure irq/preemption can't change debugctl in between */
+	local_irq_disable();
  	debugctl = get_debugctlmsr();
  	if (on)
  		debugctl |= DEBUGCTLMSR_BTF;
  	else
  		debugctl&= ~DEBUGCTLMSR_BTF;
  	update_debugctlmsr(debugctl);
+	local_irq_enable();

wouldn't preempt_disable() be enough?

arch/x86/kernel/step.c static void set_task_blockstep(struct task_struct *task, bool on) · line 183

Quoted 3 lines

arch/x86/kernel/step.c +0 -0
  }
 
  /*

Sebastian

Oleg Nesterov

author

2012-08-03 17:42 UTC #4 lore raw

Re: [PATCH 2/2] ptrace: fix set_task_blockstep()->update_debugctlmsr() logic

Quoted 10 lines · Sebastian Andrzej Siewior

Quoted 8 lines · Oleg Nesterov

arch/x86/kernel/step.c +0 -0

index afa60db..636402e 100644
@@ -166,12 +166,18 @@ static void set_task_blockstep(struct task_struct *task, bool on)
  	else
  		clear_tsk_thread_flag(task, TIF_BLOCKSTEP);

Quoted 14 lines

arch/x86/kernel/step.c static void set_task_blockstep(struct task_struct *task, bool on) · line 180
Quoted 12 lines
arch/x86/kernel/step.c +6 -0
+	if (task != current)
+		return;
+
+	/* ensure irq/preemption can't change debugctl in between */
+	local_irq_disable();
  	debugctl = get_debugctlmsr();
  	if (on)
  		debugctl |= DEBUGCTLMSR_BTF;
  	else
  		debugctl&= ~DEBUGCTLMSR_BTF;
  	update_debugctlmsr(debugctl);
+	local_irq_enable();
wouldn't preempt_disable() be enough?

preempt_disable() can't help if interrupt handler changes other bits in between?

Oleg.

Sebastian Andrzej Siewior

2012-08-03 18:28 UTC #5 lore raw

Re: [PATCH 2/2] ptrace: fix set_task_blockstep()->update_debugctlmsr() logic

Quoted 7 lines · Oleg Nesterov

Quoted 4 lines
arch/x86/kernel/step.c static void set_task_blockstep(struct task_struct *task, bool on) · line 180
Quoted 2 lines
   	update_debugctlmsr(debugctl);
arch/x86/kernel/step.c +1 -0
+	local_irq_enable();
wouldn't preempt_disable() be enough?
preempt_disable() can't help if interrupt handler changes other bits in between?

So perf() uses this register as well. Since perf() uses the raw primitives (raw_spin_lock()) shouldn't you do the same? If I recall correctly (but it is Friday and late) local_irq_enable() wouldn't disable irqs on RT and perf takes the raw lock so the irqs there should be really disabled.

Quoted 2 lines

Oleg.

Sebastian

Oleg Nesterov

author

2012-08-07 15:17 UTC #6 lore raw

Re: [PATCH 2/2] ptrace: fix set_task_blockstep()->update_debugctlmsr() logic

Hi.

Today I noticed by accident that starting from Aug 4 (at least) all my emails went to nowhere. I am resending some of them...

Peter, Ingo, could you help?

See the question about nmi at the end.

Quoted 7 lines · Sebastian Andrzej Siewior

Quoted 5 lines · Oleg Nesterov
Quoted 4 lines
arch/x86/kernel/step.c static void set_task_blockstep(struct task_struct *task, bool on) · line 180
Quoted 2 lines
   	update_debugctlmsr(debugctl);
arch/x86/kernel/step.c +1 -0
+	local_irq_enable();
wouldn't preempt_disable() be enough?

Quoted 5 lines

Quoted 2 lines
preempt_disable() can't help if interrupt handler changes other bits in between?
So perf() uses this register as well. Since perf() uses the raw primitives (raw_spin_lock())

Hmm. perf/whatever uses raw_spin_lock() if the lock is raw_spinlock_t.

But this doesn't matter? Whatever irq handler does has nothing to do with the problem, either local_irq_disable() can prevent this irq from happening, or not.

Quoted 1 line

shouldn't you do the same?

raw_local_irq_disable? I don't think so.

Quoted 3 lines

If I recall
correctly (but it is Friday and late) local_irq_enable() wouldn't disable irqs on RT

You mean it doesn't disable irqs in hardware? Yet local_irq_disable() should protect against the interrupt handler.

OK, I know nothing about RT kernel (unfortunately), perhaps it has other primitives, but

Quoted 1 line

and perf takes the raw lock

this certainly doesn't matter, afaics.

And note that __switch_to_xtra() runs under local_irq_disable() too.

However. It seems that perf (intel_pmu_handle_irq) can play with MSR_IA32_DEBUGCTLMSR bits in nmi? In this case local_irq_disable() can't help. Doesn't this mean __switch_to_xtra() has problems?

Oleg.

Sebastian Andrzej Siewior

2012-08-07 09:42 UTC #7 lore raw

Re: [PATCH 2/2] ptrace: fix set_task_blockstep()->update_debugctlmsr() logic

In reply to Oleg Nesterov

Quoted 9 lines · Oleg Nesterov

arch/x86/kernel/step.c +3 -0

@@ -166,12 +166,18 @@ static void set_task_blockstep(struct task_struct *task, bool on)
  	else
  		clear_tsk_thread_flag(task, TIF_BLOCKSTEP);
 
+	if (task != current)
+		return;
+

arch/x86/kernel/step.c static void set_task_blockstep(struct task_struct *task, bool on) · line 181

Quoted 10 lines

arch/x86/kernel/step.c +3 -0

+	/* ensure irq/preemption can't change debugctl in between */
+	local_irq_disable();
  	debugctl = get_debugctlmsr();
  	if (on)
  		debugctl |= DEBUGCTLMSR_BTF;
  	else
  		debugctl&= ~DEBUGCTLMSR_BTF;
  	update_debugctlmsr(debugctl);
+	local_irq_enable();
  }

I would say that you can remove this chunk. For task != current we leave.
For uprobes we never set the bit, we only need it cleared. We get here via int 3 and do_debug() already clears TIF_BLOCKSTEP because the CPU clears the bit in CPU. So both, TIF_BLOCKSTEP and DEBUGCTLMSR_BTF are never set.

Sebastian

Sebastian Andrzej Siewior

2012-08-07 10:53 UTC #8 lore raw

Re: [PATCH 2/2] ptrace: fix set_task_blockstep()->update_debugctlmsr() logic

Quoted 5 lines · Sebastian Andrzej Siewior

On 08/03/2012 06:29 PM, Oleg Nesterov wrote:
For uprobes we never set the bit, we only need it cleared. We get here via int 3 and do_debug() already clears TIF_BLOCKSTEP because the CPU clears the bit in CPU. So both, TIF_BLOCKSTEP and DEBUGCTLMSR_BTF are never set.

Actually I'm, wrong. Syscalls do clear the DEBUGCTLMSR_BTF bit, int3 does not. So yes, we need it after all…

Sebastian

Oleg Nesterov

author

2012-08-07 15:19 UTC #9 lore raw

Re: [PATCH 2/2] ptrace: fix set_task_blockstep()->update_debugctlmsr() logic

In reply to Sebastian Andrzej Siewior

Hi.

Today I noticed by accident that starting from Aug 4 (at least) all my emails went to nowhere. I am resending some of them...

Quoted 8 lines · Sebastian Andrzej Siewior

Quoted 6 lines · Oleg Nesterov

arch/x86/kernel/step.c +0 -0

@@ -166,12 +166,18 @@ static void set_task_blockstep(struct task_struct *task, bool on)
  	else
  		clear_tsk_thread_flag(task, TIF_BLOCKSTEP);

Quoted 16 lines

arch/x86/kernel/step.c static void set_task_blockstep(struct task_struct *task, bool on) · line 181
Quoted 13 lines
arch/x86/kernel/step.c +6 -0
+	if (task != current)
+		return;
+
+	/* ensure irq/preemption can't change debugctl in between */
+	local_irq_disable();
  	debugctl = get_debugctlmsr();
  	if (on)
  		debugctl |= DEBUGCTLMSR_BTF;
  	else
  		debugctl&= ~DEBUGCTLMSR_BTF;
  	update_debugctlmsr(debugctl);
+	local_irq_enable();
  }
I would say that you can remove this chunk. For task != current we leave.

It turns out, original code is even more buggy than I thought.

Ironically, "task != current" case is more difficult and so far I do not see how we can handle this case correctly. I'll return to this a bit later, currently I am working on other patches.

Quoted 1 line

For uprobes we never set the bit, we only need it cleared.

Yes, at least at first step, and probably we will never need more.

Quoted 2 lines

We get here
via int 3 and do_debug() already clears TIF_BLOCKSTEP

No, we get here via do_int3(), TIF_BLOCKSTEP is not cleared,

Quoted 2 lines

because the
CPU clears the bit in CPU.

I am not sure. The manual says:

If the BTF flag is set when the processor generates a debug exception, the processor clears the BTF flag along with the TF flag.

but I am not sure "debug exception" also means "breakpoint exception".

do_debug() does clear TIF_BLOCKSTEP, and "The processor cleared BTF" is true in this case. But it is called after single-step.

Oleg.

Sebastian Andrzej Siewior

2012-08-07 15:29 UTC #10 lore raw

Re: [PATCH 2/2] ptrace: fix set_task_blockstep()->update_debugctlmsr() logic

Quoted 5 lines · Oleg Nesterov

It turns out, original code is even more buggy than I thought.
Ironically, "task != current" case is more difficult and so far I do not see how we can handle this case correctly. I'll return to this a bit later, currently I am working on other patches.

maybe you could remove the autodectect mode and add helper for uprobe which disables it.

Quoted 8 lines

Quoted 1 line
For uprobes we never set the bit, we only need it cleared.
Yes, at least at first step, and probably we will never need more.
Quoted 2 lines
We get here
via int 3 and do_debug() already clears TIF_BLOCKSTEP
No, we get here via do_int3(), TIF_BLOCKSTEP is not cleared,

Yes, Sorry. my fault.

Quoted 9 lines

Quoted 2 lines
because the
CPU clears the bit in CPU.
I am not sure. The manual says:
If the BTF flag is set when the processor generates a debug exception, the processor clears the BTF flag along with the TF flag.

Quoted 6 lines

but I am not sure "debug exception" also means "breakpoint exception".
do_debug() does clear TIF_BLOCKSTEP, and "The processor cleared BTF" is true in this case. But it is called after single-step.

I was wrong here in regard to do_debug() since do_int3() is correct. Anyway, I checked it on real hardware and I saw the CPU in do_int3() with BTF set after executing int3 with TF flag set and the BTF bit.

Quoted 2 lines

Oleg.

Sebastian

Oleg Nesterov

author

2012-08-07 15:35 UTC #11 lore raw

Re: [PATCH 2/2] ptrace: fix set_task_blockstep()->update_debugctlmsr() logic

Quoted 10 lines · Sebastian Andrzej Siewior

Quoted 5 lines · Oleg Nesterov
It turns out, original code is even more buggy than I thought.
Ironically, "task != current" case is more difficult and so far I do not see how we can handle this case correctly. I'll return to this a bit later, currently I am working on other patches.
maybe you could remove the autodectect mode and add helper for uprobe which disables it.

No, this won't help.

So. Sorry for delay, I'll try to return to this tomorrow. Today I spent THE WHOLE DAY trying to understand what is wrong with my emails, and I did nothing else.

Oleg.

Oleg Nesterov

author

2012-08-07 15:16 UTC #12 lore raw

Re: [PATCH 2/2] ptrace: fix set_task_blockstep()->update_debugctlmsr() logic

In reply to Oleg Nesterov

Hi.

Today I noticed by accident that starting from Aug 4 (at least) all my emails went to nowhere. I am resending some of them...

Quoted 3 lines · Oleg Nesterov

2. It is wrong. The state of DEBUGCTLMSR_BTF bit in CPU register should always match the state of current's TIF_BLOCKSTEP bit.

Yes.

But this means we should set/clear TIF_BLOCKSTEP and update MSR_IA32_DEBUGCTLMSR "atomically" under preempt_disable().

I'll redo this patch.

Oleg.

Sebastian Andrzej Siewior

2012-08-06 16:15 UTC #13 lore raw

In reply to Oleg Nesterov

Quoted 3 lines · Oleg Nesterov

Assuming this series passes the review of course. And it would be really nice if someone reviews 2/2, I am not confident that I fully understand this DEBUGCTLMSR_BTF magic.

Here is how the processor disables the BTF:

| The processor clears the BTF flag when it generates a debug
| exception. The debugger must set the BTF flag before resuming program
| execution to continue single-stepping on branches.

This was a quote from "253668-039US, May 2011" page 16-17.

So I think __switch_to_extra() should set the bit before putting the task on the CPU. If this bit is enabled on the wrong CPU then in will remain set forever if single steeping has not been / will not be enabled.

Quoted 2 lines

Oleg.

Sebastian

Oleg Nesterov

author

2012-08-07 15:18 UTC #14 lore raw

Hi.

Today I noticed by accident that starting from Aug 4 (at least) all my emails went to nowhere. I am resending some of them...

Quoted 7 lines · Sebastian Andrzej Siewior

Quoted 3 lines · Oleg Nesterov
Assuming this series passes the review of course. And it would be really nice if someone reviews 2/2, I am not confident that I fully understand this DEBUGCTLMSR_BTF magic.

Quoted 7 lines

Here is how the processor disables the BTF:
| The processor clears the BTF flag when it generates a debug
| exception. The debugger must set the BTF flag before resuming program
| execution to continue single-stepping on branches.
This was a quote from "253668-039US, May 2011" page 16-17.

OK, thanks,

Quoted 2 lines

So I think __switch_to_extra() should set the bit before putting the task on the CPU.

Why?

Quoted 3 lines

If this bit is enabled on the wrong CPU then in will remain set forever if single steeping has not been / will not be enabled.

I don't follow, could you explain in details?

Just in case, X86_EFLAGS_TF sits in task_pt_regs(next), it has no effect until the task returns to usermode. We only need to ensure DEBUGCTLMSR_BTF was set/cleared correctly when it actually returns.

Oleg.

Sebastian Andrzej Siewior

2012-08-07 15:39 UTC #15 lore raw

Quoted 4 lines · Oleg Nesterov

Quoted 2 lines
So I think __switch_to_extra() should set the bit before putting the task on the CPU.
Why?

Pardon me? __switch_to_extra() enables BTF before putting the task on CPU. This is fine. I was trying to say that there is no need to touch the debug register in debugger's context since __switch_to_extra() does it.

Quoted 5 lines

Quoted 3 lines
If this bit is enabled on the wrong CPU then in will remain set forever if single steeping has not been / will not be enabled.
I don't follow, could you explain in details?

The SMP case where the debugger runs on CPU0 and tracee on CPU1. Without your "current != child" check the enable_block_step() enables block stepping on CPU0 and switch_to_extra() on CPU1.

Quoted 3 lines

Just in case, X86_EFLAGS_TF sits in task_pt_regs(next), it has no effect until the task returns to usermode. We only need to ensure DEBUGCTLMSR_BTF was set/cleared correctly when it actually returns.

Exactly. And __switch_to_extra() is perfect for the job (if we ignore uprobes for a moment).

Quoted 1 line

Oleg.

Sebastian

Oleg Nesterov

author

2012-08-07 15:50 UTC #16 lore raw

Quoted 11 lines · Sebastian Andrzej Siewior

Quoted 4 lines · Oleg Nesterov
Quoted 2 lines
So I think __switch_to_extra() should set the bit before putting the task on the CPU.
Why?
Pardon me? __switch_to_extra() enables BTF before putting the task on CPU. This is fine. I was trying to say that there is no need to touch the debug register in debugger's context since __switch_to_extra() does it.

And this is what the changelog says and the patch does? Confused.

Quoted 9 lines

Quoted 5 lines
Quoted 3 lines
If this bit is enabled on the wrong CPU then in will remain set forever if single steeping has not been / will not be enabled.
I don't follow, could you explain in details?
The SMP case where the debugger runs on CPU0 and tracee on CPU1. Without your "current != child" check the enable_block_step() enables block stepping on CPU0 and switch_to_extra() on CPU1.

Sure, and after the patch it doesn't touch BTF if current != child.

Quoted 6 lines

Quoted 3 lines
Just in case, X86_EFLAGS_TF sits in task_pt_regs(next), it has no effect until the task returns to usermode. We only need to ensure DEBUGCTLMSR_BTF was set/cleared correctly when it actually returns.
Exactly. And __switch_to_extra() is perfect for the job (if we ignore uprobes for a moment).

Exactly.

Ah. I guess I simply misunderstood your original email. Sorry. Somehow I thought you think that __switch_to_extra() needs fixes too.

Sorry for noise.

Oleg.