[PATCH 0/2] ptrace: DEBUGCTLMSR_BTF fixes

[PATCH 0/2] ptrace: DEBUGCTLMSR_BTF fixes

[Oleg Nesterov]

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.

[PATCH 1/2] ptrace: introduce set_task_blockstep() helper

[Oleg Nesterov]

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 <oleg@redhat.com>
---
 arch/x86/kernel/step.c |   43 +++++++++++++++++++++++--------------------
 1 files changed, 23 insertions(+), 20 deletions(-)

diff --git a/arch/x86/kernel/step.c b/arch/x86/kernel/step.c
index c346d11..afa60db 100644
--- a/arch/x86/kernel/step.c
+++ b/arch/x86/kernel/step.c
@@ -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);
-- 
1.5.5.1

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

[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 <oleg@redhat.com>
---
 arch/x86/kernel/step.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kernel/step.c b/arch/x86/kernel/step.c
index afa60db..636402e 100644
--- a/arch/x86/kernel/step.c
+++ b/arch/x86/kernel/step.c
@@ -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();
 }
 
 /*
-- 
1.5.5.1

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

[Sebastian Andrzej Siewior]

On 08/03/2012 06:29 PM, Oleg Nesterov wrote:
> diff --git a/arch/x86/kernel/step.c b/arch/x86/kernel/step.c
> index afa60db..636402e 100644
> --- a/arch/x86/kernel/step.c
> +++ b/arch/x86/kernel/step.c
> @@ -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();

wouldn't preempt_disable() be enough?

>   }
>
>   /*

Sebastian

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

[Oleg Nesterov]

On 08/03, Sebastian Andrzej Siewior wrote:
>
> On 08/03/2012 06:29 PM, Oleg Nesterov wrote:
>> diff --git a/arch/x86/kernel/step.c b/arch/x86/kernel/step.c
>> index afa60db..636402e 100644
>> --- a/arch/x86/kernel/step.c
>> +++ b/arch/x86/kernel/step.c
>> @@ -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();
>
> wouldn't preempt_disable() be enough?

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

Oleg.

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

[Sebastian Andrzej Siewior]

On 08/03/2012 07:38 PM, Oleg Nesterov wrote:
>>>    	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?

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.

>
> Oleg.

Sebastian

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

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


Peter, Ingo, could you help?

See the question about nmi at the end.

On 08/03, Sebastian Andrzej Siewior wrote:
>
> On 08/03/2012 07:38 PM, Oleg Nesterov wrote:
>>>>    	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?
>
> 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.

> shouldn't you do the same?

raw_local_irq_disable? I don't think so.

> 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

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

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

[Sebastian Andrzej Siewior]

On 08/03/2012 06:29 PM, Oleg Nesterov wrote:
> --- a/arch/x86/kernel/step.c
> +++ b/arch/x86/kernel/step.c
> @@ -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();
>   }

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

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

[Sebastian Andrzej Siewior]

On 08/07/2012 11:41 AM, Sebastian Andrzej Siewior wrote:
> 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

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

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

On 08/07, Sebastian Andrzej Siewior wrote:
>
> On 08/03/2012 06:29 PM, Oleg Nesterov wrote:
>> --- a/arch/x86/kernel/step.c
>> +++ b/arch/x86/kernel/step.c
>> @@ -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();
>>   }
>
> 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.

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

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

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

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

[Sebastian Andrzej Siewior]

On 08/07/2012 05:15 PM, Oleg Nesterov wrote:
> 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.

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

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

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.

>
> Oleg.

Sebastian

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

[Oleg Nesterov]

On 08/07, Sebastian Andrzej Siewior wrote:
>
> On 08/07/2012 05:15 PM, Oleg Nesterov wrote:
>> 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.

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

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


On 08/03, Oleg Nesterov wrote:
>
> 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.

Re: [PATCH 0/2] ptrace: DEBUGCTLMSR_BTF fixes

[Sebastian Andrzej Siewior]

On 08/03/2012 06:29 PM, Oleg Nesterov wrote:

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

> Oleg.
>

Sebastian

Re: [PATCH 0/2] ptrace: DEBUGCTLMSR_BTF fixes

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

On 08/06, Sebastian Andrzej Siewior wrote:
>
> On 08/03/2012 06:29 PM, Oleg Nesterov wrote:
>
>> 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.

OK, thanks,

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

Why?

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

Re: [PATCH 0/2] ptrace: DEBUGCTLMSR_BTF fixes

[Sebastian Andrzej Siewior]

On 08/07/2012 05:15 PM, Oleg Nesterov wrote:
>> 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.

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

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

> Oleg.

Sebastian

Re: [PATCH 0/2] ptrace: DEBUGCTLMSR_BTF fixes

[Oleg Nesterov]

On 08/07, Sebastian Andrzej Siewior wrote:
>
> On 08/07/2012 05:15 PM, Oleg Nesterov wrote:
>>> 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.

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

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