[PATCH] uprobes: don't enable/disable signle step if the user did it

[PATCH] uprobes: don't enable/disable signle step if the user did it

[Sebastian Andrzej Siewior]

If someone is using single stepping over uprobe brackpoint then after
we pass the uprobe single step, single stepping is disabled and the user
who enebaled them in the first place does not know anything about this.

This patch avoids enabling / disabling the single step mode if it is
already enabled.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 include/linux/uprobes.h |    2 ++
 kernel/events/uprobes.c |   10 ++++++++--
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index efe4b33..1be2d44 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -44,6 +44,8 @@ struct inode;
 #define UPROBE_RUN_HANDLER	0x2
 /* Can skip singlestep */
 #define UPROBE_SKIP_SSTEP	0x4
+/* user does also singlestep */
+#define UPROBE_USER_SSTEP	0x8
 
 struct uprobe_consumer {
 	int (*handler)(struct uprobe_consumer *self, struct pt_regs *regs);
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index f935327..772eb3a 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1528,7 +1528,10 @@ static void handle_swbp(struct pt_regs *regs)
 
 	utask->state = UTASK_SSTEP;
 	if (!pre_ssout(uprobe, regs, bp_vaddr)) {
-		user_enable_single_step(current);
+		if (test_tsk_thread_flag(current, TIF_SINGLESTEP))
+			uprobe->flags |= UPROBE_USER_SSTEP;
+		else
+			user_enable_single_step(current);
 		return;
 	}
 
@@ -1569,7 +1572,10 @@ static void handle_singlestep(struct uprobe_task *utask, struct pt_regs *regs)
 	put_uprobe(uprobe);
 	utask->active_uprobe = NULL;
 	utask->state = UTASK_RUNNING;
-	user_disable_single_step(current);
+	if (uprobe->flags & UPROBE_USER_SSTEP)
+		uprobe->flags &= ~UPROBE_USER_SSTEP;
+	else
+		user_disable_single_step(current);
 	xol_free_insn_slot(current);
 
 	spin_lock_irq(&current->sighand->siglock);
-- 
1.7.10.4

Re: [PATCH] uprobes: don't enable/disable signle step if the user did it

[Oleg Nesterov]

Well. I agree, this needs changes. To begin with, uprobe should avoid
user_enable_single_step() which does access_process_vm(). And I suspect
uprobes have the problems with TIF_FORCED_TF logic.

But I am not sure about this patch...

On 07/26, Sebastian Andrzej Siewior wrote:
>
> @@ -1528,7 +1528,10 @@ static void handle_swbp(struct pt_regs *regs)
>
>  	utask->state = UTASK_SSTEP;
>  	if (!pre_ssout(uprobe, regs, bp_vaddr)) {
> -		user_enable_single_step(current);
> +		if (test_tsk_thread_flag(current, TIF_SINGLESTEP))
> +			uprobe->flags |= UPROBE_USER_SSTEP;
> +		else
> +			user_enable_single_step(current);

This is x86 specific, TIF_SINGLESTEP is not defined on every arch.

> @@ -1569,7 +1572,10 @@ static void handle_singlestep(struct uprobe_task *utask, struct pt_regs *regs)
>  	put_uprobe(uprobe);
>  	utask->active_uprobe = NULL;
>  	utask->state = UTASK_RUNNING;
> -	user_disable_single_step(current);
> +	if (uprobe->flags & UPROBE_USER_SSTEP)
> +		uprobe->flags &= ~UPROBE_USER_SSTEP;
> +	else
> +		user_disable_single_step(current);

This is not enough (and I am not sure this is portable).

If SINGLESTEP was set, we should send SIGTRAP here. With this patch
we return with X86_EFLAGS_TF set, gdb will be notified only after the
next insn. And if we notify gdb, there is no need to keep X86_EFLAGS_TF.

I'm afraid this needs more thinking and new arch-dependant helpers.

Oleg.

Re: [PATCH] uprobes: don't enable/disable signle step if the user did it

[Sebastian Andrzej Siewior]

On 07/26/2012 07:31 PM, Oleg Nesterov wrote:
> Well. I agree, this needs changes. To begin with, uprobe should avoid
> user_enable_single_step() which does access_process_vm(). And I suspect
> uprobes have the problems with TIF_FORCED_TF logic.

Why? Shouldn't wee keep the trap flag if the instruction on which we
placed the uprobe activates it?

>
> But I am not sure about this patch...
>
> On 07/26, Sebastian Andrzej Siewior wrote:
>>
>> @@ -1528,7 +1528,10 @@ static void handle_swbp(struct pt_regs *regs)
>>
>>   	utask->state = UTASK_SSTEP;
>>   	if (!pre_ssout(uprobe, regs, bp_vaddr)) {
>> -		user_enable_single_step(current);
>> +		if (test_tsk_thread_flag(current, TIF_SINGLESTEP))
>> +			uprobe->flags |= UPROBE_USER_SSTEP;
>> +		else
>> +			user_enable_single_step(current);
>
> This is x86 specific, TIF_SINGLESTEP is not defined on every arch.

It is not defined on every arch but I wouldn't say it is 86 specific.
 From the architectures which have user_enable_single_step() defined I
see

  avr32	TIF_SINGLE_STEP
  m68k	TIF_DELAYED_TRACE
  s390	TIF_SINGLE_STEP

which means those three could rename their flag so things are
consistent. The remaining architectures are

  alpha
  cris
  h8300
  score

and they don't set a flag and it seems they change the register
directly.

>
>> @@ -1569,7 +1572,10 @@ static void handle_singlestep(struct uprobe_task *utask, struct pt_regs *regs)
>>   	put_uprobe(uprobe);
>>   	utask->active_uprobe = NULL;
>>   	utask->state = UTASK_RUNNING;
>> -	user_disable_single_step(current);
>> +	if (uprobe->flags&  UPROBE_USER_SSTEP)
>> +		uprobe->flags&= ~UPROBE_USER_SSTEP;
>> +	else
>> +		user_disable_single_step(current);
>
> This is not enough (and I am not sure this is portable).
>
> If SINGLESTEP was set, we should send SIGTRAP here. With this patch
> we return with X86_EFLAGS_TF set, gdb will be notified only after the
> next insn. And if we notify gdb, there is no need to keep X86_EFLAGS_TF.

Sending SIGTRAP is, yes.

> I'm afraid this needs more thinking and new arch-dependant helpers.
>
> Oleg.
>

Sebastian

Re: [PATCH] uprobes: don't enable/disable signle step if the user did it

[Oleg Nesterov]

On 07/27, Sebastian Andrzej Siewior wrote:
>
> On 07/26/2012 07:31 PM, Oleg Nesterov wrote:
>> Well. I agree, this needs changes. To begin with, uprobe should avoid
>> user_enable_single_step() which does access_process_vm(). And I suspect
>> uprobes have the problems with TIF_FORCED_TF logic.
>
> Why? Shouldn't wee keep the trap flag if the instruction on which we
> placed the uprobe activates it?

Yes. But user_enable_single_step() is not the right interface.

>> But I am not sure about this patch...
>>
>> On 07/26, Sebastian Andrzej Siewior wrote:
>>>
>>> @@ -1528,7 +1528,10 @@ static void handle_swbp(struct pt_regs *regs)
>>>
>>>   	utask->state = UTASK_SSTEP;
>>>   	if (!pre_ssout(uprobe, regs, bp_vaddr)) {
>>> -		user_enable_single_step(current);
>>> +		if (test_tsk_thread_flag(current, TIF_SINGLESTEP))
>>> +			uprobe->flags |= UPROBE_USER_SSTEP;
>>> +		else
>>> +			user_enable_single_step(current);
>>
>> This is x86 specific, TIF_SINGLESTEP is not defined on every arch.
>
> It is not defined on every arch but I wouldn't say it is 86 specific.
> From the architectures which have user_enable_single_step() defined I
> see

But we do not need TIF_SINGLESTEP. At all. Again, this is ptrace thing
connected to user_enable_single_step().

Sebastian, I am sorry for being terse, I'll write another email later.

Oleg.

Re: [PATCH] uprobes: don't enable/disable signle step if the user did it

[Sebastian Andrzej Siewior]

On 07/26/2012 05:20 PM, Sebastian Andrzej Siewior wrote:
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index f935327..772eb3a 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1528,7 +1528,10 @@ static void handle_swbp(struct pt_regs *regs)
>
>   	utask->state = UTASK_SSTEP;
>   	if (!pre_ssout(uprobe, regs, bp_vaddr)) {
> -		user_enable_single_step(current);
> +		if (test_tsk_thread_flag(current, TIF_SINGLESTEP))
> +			uprobe->flags |= UPROBE_USER_SSTEP;
> +		else
> +			user_enable_single_step(current);

After looking at it for a bit I noticed that the state should be saved
in utask intead of uprobe because uprobe might be shared with another
task.
I would resend the fixed patch unless someone comes up with something
else..

Sebastian

Re: [PATCH] uprobes: don't enable/disable signle step if the user did it

[Ananth N Mavinakayanahalli]

On Thu, Jul 26, 2012 at 05:20:43PM +0200, Sebastian Andrzej Siewior wrote:
> If someone is using single stepping over uprobe brackpoint then after
> we pass the uprobe single step, single stepping is disabled and the user
> who enebaled them in the first place does not know anything about this.
> 
> This patch avoids enabling / disabling the single step mode if it is
> already enabled.

This could happen any time 2 different entities call the
user_(en/dis)able_single_step() helpers on the same thread.

Wouldn't the right way to fix it be to teach these helpers
to honor what the TIF_SINGLESTEP flag setting was in the first place?
This way you'd get rid of the portability concerns too, since these
helpers are available on most architectures.

Ananth

Re: [PATCH] uprobes: don't enable/disable signle step if the user did it

[Oleg Nesterov]

On 07/30, Ananth N Mavinakayanahalli wrote:
>
> On Thu, Jul 26, 2012 at 05:20:43PM +0200, Sebastian Andrzej Siewior wrote:
> > If someone is using single stepping over uprobe brackpoint then after
> > we pass the uprobe single step, single stepping is disabled and the user
> > who enebaled them in the first place does not know anything about this.
> >
> > This patch avoids enabling / disabling the single step mode if it is
> > already enabled.
>
> This could happen any time 2 different entities call the
> user_(en/dis)able_single_step() helpers on the same thread.

Yes. But nobody except ptrace should do use these helpers, I think.

> Wouldn't the right way to fix it be to teach these helpers
> to honor what the TIF_SINGLESTEP

Well, I think uprobes should not use TIF_SINGLESTEP at all. This
bit is (mostly) needed to handle the stepping over syscall. But
I guess you didn't actually mean TIF_SINGLESTEP...

> flag setting was in the first place?

Perhaps, but I don't think so. If nothing else, we do not want
to add the new counter/whatever in task_struct, while uprobes
already has uprobe_task which can "remember" the state of _TF
bit and more.

And this can't solve other problems. Suppose that gdb does
PTRACE_SINGLESTEP but the original "popf" insn was already replaced
by "int3", this will obviously confuse is_setting_trap_flag().

And we need the additional SIGTRAP from handle_singlestep().
And we have more problems with DEBUGCTLMSR_BTF. And we do
not want access_process_vm() from uprobes code.

So I think we need arch_uprobe_*able_step(struct uprobe_task *utask).
Ignoring all problems except the one this patch tries to fix, x86
can simply do:

	arch_uprobe_enble_step(utask, struct arch_uprobe *auprobe)
	{
		utask->clear_tf =
			!(regs->flags & X86_EFLAGS_TF) &&
			(auprobe->insn != "popf");
		regs->flags |= X86_EFLAGS_TF;
	}

	arch_uprobe_disable_step(utask)
	{
		if (utask->clear_tf)
			regs->flags &= ~X86_EFLAGS_TF;
	}

Fortunately, we can never race with gdb/enable_step(), and we do
not care why X86_EFLAGS_TF was set, and we do not care about
TIF_SINGLESTEP/TIF_FORCED_TF.




However. This all needs more discussion (and help from Roland I guess).

Sebastian, I think your patch is simple and certainly makes the things
better, just it is not correct (you already realized you can't use
uprobe->flags) and it is not arch-friendly.

I'd suggest you to make 2 patches:

	- 1/2 creates arch_uprobe_*_step(...) __weak helpers in
	      kernel/events/uprobes.c which simply call
	      user_*_single_step() and updates the callers

	      Not strictly necessary, but imho makes sense...

	- 2/2 adds the x86 implementation in arch/x86/kernel/uprobes.c
	      which still uses user_*_single_step() but checks
	      TIF_SINGLESTEP. As your patch does, but you should use
	      utask, not uprobe.

IOW, I simply suggest to make your patch x86-specific. Then we
will try to do more fixes/improvements.


Sebastian, Ananth, what do you think?

Oleg.

Re: [PATCH] uprobes: don't enable/disable signle step if the user did it

[Sebastian Andrzej Siewior]

On 07/30/2012 04:16 PM, Oleg Nesterov wrote:

> I'd suggest you to make 2 patches:
>
> 	- 1/2 creates arch_uprobe_*_step(...) __weak helpers in
> 	      kernel/events/uprobes.c which simply call
> 	      user_*_single_step() and updates the callers
>
> 	      Not strictly necessary, but imho makes sense...
>
> 	- 2/2 adds the x86 implementation in arch/x86/kernel/uprobes.c
> 	      which still uses user_*_single_step() but checks
> 	      TIF_SINGLESTEP. As your patch does, but you should use
> 	      utask, not uprobe.
>
> IOW, I simply suggest to make your patch x86-specific. Then we
> will try to do more fixes/improvements.
>
>
> Sebastian, Ananth, what do you think?

Yup, let me try…

>
> Oleg.
>

Sebastian

Re: [PATCH] uprobes: don't enable/disable signle step if the user did it

[Ananth N Mavinakayanahalli]

On Mon, Jul 30, 2012 at 04:16:38PM +0200, Oleg Nesterov wrote:
> On 07/30, Ananth N Mavinakayanahalli wrote:
> >
> > On Thu, Jul 26, 2012 at 05:20:43PM +0200, Sebastian Andrzej Siewior wrote:
> > > If someone is using single stepping over uprobe brackpoint then after
> > > we pass the uprobe single step, single stepping is disabled and the user
> > > who enebaled them in the first place does not know anything about this.
> > >
> > > This patch avoids enabling / disabling the single step mode if it is
> > > already enabled.
> >
> > This could happen any time 2 different entities call the
> > user_(en/dis)able_single_step() helpers on the same thread.
> 
> Yes. But nobody except ptrace should do use these helpers, I think.

Right now, yes.

> > Wouldn't the right way to fix it be to teach these helpers
> > to honor what the TIF_SINGLESTEP
> 
> Well, I think uprobes should not use TIF_SINGLESTEP at all. This
> bit is (mostly) needed to handle the stepping over syscall. But
> I guess you didn't actually mean TIF_SINGLESTEP...
> 
> > flag setting was in the first place?
> 
> Perhaps, but I don't think so. If nothing else, we do not want
> to add the new counter/whatever in task_struct, while uprobes
> already has uprobe_task which can "remember" the state of _TF
> bit and more.
> 
> And this can't solve other problems. Suppose that gdb does
> PTRACE_SINGLESTEP but the original "popf" insn was already replaced
> by "int3", this will obviously confuse is_setting_trap_flag().
> 
> And we need the additional SIGTRAP from handle_singlestep().
> And we have more problems with DEBUGCTLMSR_BTF. And we do
> not want access_process_vm() from uprobes code.

IIUC you'd want uprobes to do similar to what kprobes does today (see
prepare_singlestep() in arch/xxx/kernel/kprobes.c).

> So I think we need arch_uprobe_*able_step(struct uprobe_task *utask).
> Ignoring all problems except the one this patch tries to fix, x86
> can simply do:
> 
> 	arch_uprobe_enble_step(utask, struct arch_uprobe *auprobe)
> 	{
> 		utask->clear_tf =
> 			!(regs->flags & X86_EFLAGS_TF) &&
> 			(auprobe->insn != "popf");
> 		regs->flags |= X86_EFLAGS_TF;
> 	}
> 
> 	arch_uprobe_disable_step(utask)
> 	{
> 		if (utask->clear_tf)
> 			regs->flags &= ~X86_EFLAGS_TF;
> 	}

Right.

> However. This all needs more discussion (and help from Roland I guess).
> 
> Sebastian, I think your patch is simple and certainly makes the things
> better, just it is not correct (you already realized you can't use
> uprobe->flags) and it is not arch-friendly.
> 
> I'd suggest you to make 2 patches:
> 
> 	- 1/2 creates arch_uprobe_*_step(...) __weak helpers in
> 	      kernel/events/uprobes.c which simply call
> 	      user_*_single_step() and updates the callers
> 
> 	      Not strictly necessary, but imho makes sense...
> 
> 	- 2/2 adds the x86 implementation in arch/x86/kernel/uprobes.c
> 	      which still uses user_*_single_step() but checks
> 	      TIF_SINGLESTEP. As your patch does, but you should use
> 	      utask, not uprobe.
> 
> IOW, I simply suggest to make your patch x86-specific. Then we
> will try to do more fixes/improvements.
> 
> 
> Sebastian, Ananth, what do you think?

Agreed.

Ananth

Re: [PATCH] uprobes: don't enable/disable signle step if the user did it

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-07-30 16:16:38]:

> On 07/30, Ananth N Mavinakayanahalli wrote:
> >
> > On Thu, Jul 26, 2012 at 05:20:43PM +0200, Sebastian Andrzej Siewior wrote:
> > > If someone is using single stepping over uprobe brackpoint then after
> > > we pass the uprobe single step, single stepping is disabled and the user
> > > who enebaled them in the first place does not know anything about this.
> > >
> > > This patch avoids enabling / disabling the single step mode if it is
> > > already enabled.
> >
> > This could happen any time 2 different entities call the
> > user_(en/dis)able_single_step() helpers on the same thread.
> 
> Yes. But nobody except ptrace should do use these helpers, I think.
> 
> > Wouldn't the right way to fix it be to teach these helpers
> > to honor what the TIF_SINGLESTEP
> 
> Well, I think uprobes should not use TIF_SINGLESTEP at all. This
> bit is (mostly) needed to handle the stepping over syscall. But
> I guess you didn't actually mean TIF_SINGLESTEP...
> 
> > flag setting was in the first place?
> 
> Perhaps, but I don't think so. If nothing else, we do not want
> to add the new counter/whatever in task_struct, while uprobes
> already has uprobe_task which can "remember" the state of _TF
> bit and more.
> 
> And this can't solve other problems. Suppose that gdb does
> PTRACE_SINGLESTEP but the original "popf" insn was already replaced
> by "int3", this will obviously confuse is_setting_trap_flag().
> 
> And we need the additional SIGTRAP from handle_singlestep().
> And we have more problems with DEBUGCTLMSR_BTF. And we do
> not want access_process_vm() from uprobes code.
> 
> So I think we need arch_uprobe_*able_step(struct uprobe_task *utask).
> Ignoring all problems except the one this patch tries to fix, x86
> can simply do:
> 
> 	arch_uprobe_enble_step(utask, struct arch_uprobe *auprobe)
> 	{
> 		utask->clear_tf =
> 			!(regs->flags & X86_EFLAGS_TF) &&
> 			(auprobe->insn != "popf");
> 		regs->flags |= X86_EFLAGS_TF;
> 	}
> 
> 	arch_uprobe_disable_step(utask)
> 	{
> 		if (utask->clear_tf)
> 			regs->flags &= ~X86_EFLAGS_TF;
> 	}
> 

We were using something similar to this approach. [though we were still
using TIF_SINGLESTEP flag]. However this was all changed based on
feedback from Roland and Peter.

Here is the pointer to the discussion.

https://lkml.org/lkml/2011/1/27/283

> Fortunately, we can never race with gdb/enable_step(), and we do
> not care why X86_EFLAGS_TF was set, and we do not care about
> TIF_SINGLESTEP/TIF_FORCED_TF.
> 
> However. This all needs more discussion (and help from Roland I guess).
> 
> Sebastian, I think your patch is simple and certainly makes the things
> better, just it is not correct (you already realized you can't use
> uprobe->flags) and it is not arch-friendly.
> 
> I'd suggest you to make 2 patches:
> 
> 	- 1/2 creates arch_uprobe_*_step(...) __weak helpers in
> 	      kernel/events/uprobes.c which simply call
> 	      user_*_single_step() and updates the callers
> 
> 	      Not strictly necessary, but imho makes sense...
> 
> 	- 2/2 adds the x86 implementation in arch/x86/kernel/uprobes.c
> 	      which still uses user_*_single_step() but checks
> 	      TIF_SINGLESTEP. As your patch does, but you should use
> 	      utask, not uprobe.
> 
> IOW, I simply suggest to make your patch x86-specific. Then we
> will try to do more fixes/improvements.
> 
> Sebastian, Ananth, what do you think?
> 
> Oleg.
> 

Re: [PATCH] uprobes: don't enable/disable signle step if the user did it

[Oleg Nesterov]

On 07/31, Srikar Dronamraju wrote:
>
> * Oleg Nesterov <oleg@redhat.com> [2012-07-30 16:16:38]:
>
> > So I think we need arch_uprobe_*able_step(struct uprobe_task *utask).
> > Ignoring all problems except the one this patch tries to fix, x86
> > can simply do:
> >
> > 	arch_uprobe_enble_step(utask, struct arch_uprobe *auprobe)
> > 	{
> > 		utask->clear_tf =
> > 			!(regs->flags & X86_EFLAGS_TF) &&
> > 			(auprobe->insn != "popf");
> > 		regs->flags |= X86_EFLAGS_TF;
> > 	}
> >
> > 	arch_uprobe_disable_step(utask)
> > 	{
> > 		if (utask->clear_tf)
> > 			regs->flags &= ~X86_EFLAGS_TF;
> > 	}
> >
>
> We were using something similar to this approach. [though we were still
> using TIF_SINGLESTEP flag].

(and afaics the code was wrong)

> However this was all changed based on
> feedback from Roland and Peter.
>
> Here is the pointer to the discussion.
>
> https://lkml.org/lkml/2011/1/27/283

Looking at this discussion now, I am not sure that Roland was against
the per-arch uprobe_enable_step() implementation.

And when I read you message I do not understand your opinion ;)

And just in case, the pseudo code above is only for illustration,
note also "Ignoring all problems except the one".

In any case I agree, this needs more discussion. Personally I think
it doesn't make sense to try to teach user_enable_single_step() to
work correctly with ptrace and uprobes at the same time, I can be
wrong of-course.

Oleg.

[PATCH 1/2] uprobes: Use a helper instead of ptrace's single step enable

[Sebastian Andrzej Siewior]

As Oleg pointed out in [0] utrace should not use the ptrace interface
for enabling/disabling single stepping.

[0] http://lkml.kernel.org/20120730141638.GA5306@redhat.com

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 include/linux/uprobes.h |    4 ++++
 kernel/events/uprobes.c |   16 ++++++++++++++--
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index efe4b33..ea6603b 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -111,6 +111,10 @@ extern void uprobe_munmap(struct vm_area_struct *vma, unsigned long start, unsig
 extern void uprobe_free_utask(struct task_struct *t);
 extern void uprobe_copy_process(struct task_struct *t);
 extern unsigned long __weak uprobe_get_swbp_addr(struct pt_regs *regs);
+extern void __weak arch_uprobe_enable_step(struct task_struct *child,
+		struct arch_uprobe *arch);
+extern void __weak arch_uprobe_disable_step(struct task_struct *child,
+		struct arch_uprobe *arch);
 extern int uprobe_post_sstep_notifier(struct pt_regs *regs);
 extern int uprobe_pre_sstep_notifier(struct pt_regs *regs);
 extern void uprobe_notify_resume(struct pt_regs *regs);
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index f935327..3a61f16 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1482,6 +1482,18 @@ static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp)
 	return uprobe;
 }
 
+void __weak arch_uprobe_enable_step(struct task_struct *child,
+		struct arch_uprobe *arch)
+{
+	user_enable_single_step(current);
+}
+
+void __weak arch_uprobe_disable_step(struct task_struct *child,
+		struct arch_uprobe *arch)
+{
+	user_disable_single_step(child);
+}
+
 /*
  * Run handler and ask thread to singlestep.
  * Ensure all non-fatal signals cannot interrupt thread while it singlesteps.
@@ -1528,7 +1540,7 @@ static void handle_swbp(struct pt_regs *regs)
 
 	utask->state = UTASK_SSTEP;
 	if (!pre_ssout(uprobe, regs, bp_vaddr)) {
-		user_enable_single_step(current);
+		arch_uprobe_enable_step(current, &uprobe->arch);
 		return;
 	}
 
@@ -1569,7 +1581,7 @@ static void handle_singlestep(struct uprobe_task *utask, struct pt_regs *regs)
 	put_uprobe(uprobe);
 	utask->active_uprobe = NULL;
 	utask->state = UTASK_RUNNING;
-	user_disable_single_step(current);
+	arch_uprobe_disable_step(current, &uprobe->arch);
 	xol_free_insn_slot(current);
 
 	spin_lock_irq(&current->sighand->siglock);
-- 
1.7.10.4

[PATCH 2/2] x86/uprobes: implement x86 specific arch_uprobe_*_step

[Sebastian Andrzej Siewior]

The arch specific implementation enables single stepping directly by
setting the trap flag. "Single-Step on branches" is always disabled
since only one opcode has to be executed.

The disable call removes the trap flag unless it was there before. It
does not touch the flags register if the executed instruction was
"popf". It does not take into account various prefixes like "lock popf"
or "repz popf".
SIGTRAP is sent to the process in case it was traced so the debugger
knows once we advanced by one opcode. This isn't done in case we have to
restore the BTF flag. In case the BTF flag is set, we should look at the
opcode and send SIGTRAP depending on the jump/flag status. For now we
wait for the next exception/jump to be taken.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 arch/x86/include/asm/uprobes.h |    3 ++
 arch/x86/kernel/uprobes.c      |   60 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 63 insertions(+)

diff --git a/arch/x86/include/asm/uprobes.h b/arch/x86/include/asm/uprobes.h
index f3971bb..47f4cf1 100644
--- a/arch/x86/include/asm/uprobes.h
+++ b/arch/x86/include/asm/uprobes.h
@@ -46,6 +46,9 @@ struct arch_uprobe_task {
 #ifdef CONFIG_X86_64
 	unsigned long			saved_scratch_register;
 #endif
+#define UPROBE_CLEAR_TF			(1 << 0)
+#define UPROBE_SET_BTF			(1 << 1)
+	unsigned int			restore_flags;
 };
 
 extern int  arch_uprobe_analyze_insn(struct arch_uprobe *aup, struct mm_struct *mm, unsigned long addr);
diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
index 36fd420..6eec3e4 100644
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -673,3 +673,63 @@ bool arch_uprobe_skip_sstep(struct arch_uprobe *auprobe, struct pt_regs *regs)
 	}
 	return false;
 }
+
+static int insn_is_popf(const u8 *insn)
+{
+	/* popf */
+	if (insn[0] == 0x9d)
+		return 1;
+	return 0;
+}
+
+void arch_uprobe_enable_step(struct task_struct *child,
+		struct arch_uprobe *auprobe)
+{
+	struct uprobe_task	*utask		= child->utask;
+	struct arch_uprobe_task	*autask		= &utask->autask;
+	struct pt_regs		*regs		= task_pt_regs(child);
+	unsigned long		debugctl;
+
+	autask->restore_flags = 0;
+	if (!(regs->flags & X86_EFLAGS_TF) &&
+			!insn_is_popf(auprobe->insn)) {
+		autask->restore_flags |= UPROBE_CLEAR_TF;
+
+		debugctl = get_debugctlmsr();
+		if (debugctl & DEBUGCTLMSR_BTF) {
+			autask->restore_flags |= UPROBE_SET_BTF;
+			debugctl &= ~DEBUGCTLMSR_BTF;
+			update_debugctlmsr(debugctl);
+		}
+	}
+	regs->flags |= X86_EFLAGS_TF;
+}
+
+void arch_uprobe_disable_step(struct task_struct *child,
+		struct arch_uprobe *auprobe)
+{
+	struct uprobe_task *utask	= child->utask;
+	struct arch_uprobe_task	*autask		= &utask->autask;
+	struct pt_regs *regs		= task_pt_regs(child);
+
+	/*
+	 * Disable the single step flag if it was set by us. Notify the debugger
+	 * via SIGTRAP in case it was already there so it learns that we
+	 * advanced by an opcode unless the debugger is waiting for the next
+	 * jump to be taken. This logic gets it wrong if the uprobe was set
+	 * on jump instruction that would raise an exception.
+	 */
+	if (autask->restore_flags & UPROBE_CLEAR_TF) {
+		regs->flags &= ~X86_EFLAGS_TF;
+	} else {
+		if (autask->restore_flags & UPROBE_SET_BTF) {
+			unsigned long	debugctl;
+
+			debugctl = get_debugctlmsr();
+			debugctl |= DEBUGCTLMSR_BTF;
+			update_debugctlmsr(debugctl);
+		} else {
+			send_sig(SIGTRAP, current, 0);
+		}
+	}
+}
-- 
1.7.10.4

Re: [PATCH 2/2] x86/uprobes: implement x86 specific arch_uprobe_*_step

[Oleg Nesterov]

Oh, Sebastian, I'll try to read this patch tomorrow, but I am not
expert anyway.

However, honestly I do not like it. I think we should change this
step-by-step, that is why I suggested to use TIF_SINGLESTEP and
user_enable_single_step() like your initial patch did. With this
patch at least the debugger doesn't lose the control over the tracee
if it steps over the probed insn, and this is the main (and known ;)
problem to me.

Every change needs the discussion. For example, _enable should
clear DEBUGCTLMSR_BTF, this is obvious. But it is not clear to
me if _disable should restore it. What if the probed insn was
"jmp"? We need the additional complications to handle this case
really correctly, and for what? OK, gdb can get the extra SIGTRAP
from the tracee, but this is fine. And uprobes can confuse gdb
in many ways.

Even the "send_sig(SIGTRAP)" from _disable should be weighted.
Yes, yes, it was me who pointed we miss the trap. But is it really
that important? I dunno, and the problem is that send_sig(SIGTRAP)
is not the right interface (yes, handle_swbp() does this too).


But I am not maintainer.

Srikar, what do you think?


On 07/31, Sebastian Andrzej Siewior wrote:
> The arch specific implementation enables single stepping directly by
> setting the trap flag. "Single-Step on branches" is always disabled
> since only one opcode has to be executed.
> 
> The disable call removes the trap flag unless it was there before. It
> does not touch the flags register if the executed instruction was
> "popf". It does not take into account various prefixes like "lock popf"
> or "repz popf".
> SIGTRAP is sent to the process in case it was traced so the debugger
> knows once we advanced by one opcode. This isn't done in case we have to
> restore the BTF flag. In case the BTF flag is set, we should look at the
> opcode and send SIGTRAP depending on the jump/flag status. For now we
> wait for the next exception/jump to be taken.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  arch/x86/include/asm/uprobes.h |    3 ++
>  arch/x86/kernel/uprobes.c      |   60 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 63 insertions(+)
> 
> diff --git a/arch/x86/include/asm/uprobes.h b/arch/x86/include/asm/uprobes.h
> index f3971bb..47f4cf1 100644
> --- a/arch/x86/include/asm/uprobes.h
> +++ b/arch/x86/include/asm/uprobes.h
> @@ -46,6 +46,9 @@ struct arch_uprobe_task {
>  #ifdef CONFIG_X86_64
>  	unsigned long			saved_scratch_register;
>  #endif
> +#define UPROBE_CLEAR_TF			(1 << 0)
> +#define UPROBE_SET_BTF			(1 << 1)
> +	unsigned int			restore_flags;
>  };
>  
>  extern int  arch_uprobe_analyze_insn(struct arch_uprobe *aup, struct mm_struct *mm, unsigned long addr);
> diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
> index 36fd420..6eec3e4 100644
> --- a/arch/x86/kernel/uprobes.c
> +++ b/arch/x86/kernel/uprobes.c
> @@ -673,3 +673,63 @@ bool arch_uprobe_skip_sstep(struct arch_uprobe *auprobe, struct pt_regs *regs)
>  	}
>  	return false;
>  }
> +
> +static int insn_is_popf(const u8 *insn)
> +{
> +	/* popf */
> +	if (insn[0] == 0x9d)
> +		return 1;
> +	return 0;
> +}
> +
> +void arch_uprobe_enable_step(struct task_struct *child,
> +		struct arch_uprobe *auprobe)
> +{
> +	struct uprobe_task	*utask		= child->utask;
> +	struct arch_uprobe_task	*autask		= &utask->autask;
> +	struct pt_regs		*regs		= task_pt_regs(child);
> +	unsigned long		debugctl;
> +
> +	autask->restore_flags = 0;
> +	if (!(regs->flags & X86_EFLAGS_TF) &&
> +			!insn_is_popf(auprobe->insn)) {
> +		autask->restore_flags |= UPROBE_CLEAR_TF;
> +
> +		debugctl = get_debugctlmsr();
> +		if (debugctl & DEBUGCTLMSR_BTF) {
> +			autask->restore_flags |= UPROBE_SET_BTF;
> +			debugctl &= ~DEBUGCTLMSR_BTF;
> +			update_debugctlmsr(debugctl);
> +		}
> +	}
> +	regs->flags |= X86_EFLAGS_TF;
> +}
> +
> +void arch_uprobe_disable_step(struct task_struct *child,
> +		struct arch_uprobe *auprobe)
> +{
> +	struct uprobe_task *utask	= child->utask;
> +	struct arch_uprobe_task	*autask		= &utask->autask;
> +	struct pt_regs *regs		= task_pt_regs(child);
> +
> +	/*
> +	 * Disable the single step flag if it was set by us. Notify the debugger
> +	 * via SIGTRAP in case it was already there so it learns that we
> +	 * advanced by an opcode unless the debugger is waiting for the next
> +	 * jump to be taken. This logic gets it wrong if the uprobe was set
> +	 * on jump instruction that would raise an exception.
> +	 */
> +	if (autask->restore_flags & UPROBE_CLEAR_TF) {
> +		regs->flags &= ~X86_EFLAGS_TF;
> +	} else {
> +		if (autask->restore_flags & UPROBE_SET_BTF) {
> +			unsigned long	debugctl;
> +
> +			debugctl = get_debugctlmsr();
> +			debugctl |= DEBUGCTLMSR_BTF;
> +			update_debugctlmsr(debugctl);
> +		} else {
> +			send_sig(SIGTRAP, current, 0);
> +		}
> +	}
> +}
> -- 
> 1.7.10.4
> 

Re: [PATCH 2/2] x86/uprobes: implement x86 specific arch_uprobe_*_step

[Sebastian Andrzej Siewior]

On 07/31/2012 07:51 PM, Oleg Nesterov wrote:
> However, honestly I do not like it. I think we should change this
> step-by-step, that is why I suggested to use TIF_SINGLESTEP and
> user_enable_single_step() like your initial patch did. With this
> patch at least the debugger doesn't lose the control over the tracee
> if it steps over the probed insn, and this is the main (and known ;)
> problem to me.

I thought you did not like the nesting with TIF_SIGNLESTEP and the
_FORCE and suggested to handle the complete state within uprobe.

> Every change needs the discussion. For example, _enable should
> clear DEBUGCTLMSR_BTF, this is obvious. But it is not clear to
> me if _disable should restore it. What if the probed insn was
> "jmp"? We need the additional complications to handle this case
> really correctly, and for what? OK, gdb can get the extra SIGTRAP
> from the tracee, but this is fine. And uprobes can confuse gdb
> in many ways.

I don't know if it is worth to have correct behavior here or rather go
for the easy way which is either always do the wakeup or delay until
the next jump.

Sebastian

Re: [PATCH 2/2] x86/uprobes: implement x86 specific arch_uprobe_*_step

[Oleg Nesterov]

On 07/31, Sebastian Andrzej Siewior wrote:
>
> On 07/31/2012 07:51 PM, Oleg Nesterov wrote:
>> However, honestly I do not like it. I think we should change this
>> step-by-step, that is why I suggested to use TIF_SINGLESTEP and
>> user_enable_single_step() like your initial patch did. With this
>> patch at least the debugger doesn't lose the control over the tracee
>> if it steps over the probed insn, and this is the main (and known ;)
>> problem to me.
>
> I thought you did not like the nesting with TIF_SIGNLESTEP and the
> _FORCE and suggested to handle the complete state within uprobe.

Yes, but I suggested to do this in a separate patch.

In particular, because even if we do not care about DEBUGCTLMSR_BTF
after _disable, _enable has to clear it. See below.

>> Every change needs the discussion. For example, _enable should
>> clear DEBUGCTLMSR_BTF, this is obvious. But it is not clear to
>> me if _disable should restore it. What if the probed insn was
>> "jmp"? We need the additional complications to handle this case
>> really correctly, and for what? OK, gdb can get the extra SIGTRAP
>> from the tracee, but this is fine. And uprobes can confuse gdb
>> in many ways.
>
> I don't know if it is worth to have correct behavior here or rather go
> for the easy way which is either always do the wakeup or delay until
> the next jump.

This is debatable, personally I think it is fine to lose DEBUGCTLMSR_BTF.
Otherwise we need much more complications, not sure if it worth a trouble.
And an extra SIGTRAP for gdb is certainly better than the lost SIGTRAP.

However, once again, at least we should clear DEBUGCTLMSR_BTF
before the step. And I strongly believe we should not copy-and-paste
this code from step.c. This means we need something like the patch
below, before we teach uprobes to play with _TF directly, imho.

And btw, this is offtopic, but the usage of update_debugctlmsr()
doesn't look right to me (I can be easily wrong though). I'll write
another email.

Oleg.

--- x/arch/x86/kernel/step.c
+++ x/arch/x86/kernel/step.c
@@ -157,6 +157,21 @@ static int enable_single_step(struct tas
 	return 1;
 }
 
+void enable_block_step(struct task_struct *child, bool on)
+{
+	unsigned long debugctl = get_debugctlmsr();
+
+	if (on) {
+		set_tsk_thread_flag(child, TIF_BLOCKSTEP);
+		debugctl |= DEBUGCTLMSR_BTF;
+	} else {
+		clear_tsk_thread_flag(child, TIF_BLOCKSTEP);
+		debugctl &= ~DEBUGCTLMSR_BTF;
+	}
+
+	update_debugctlmsr(debugctl);
+}
+
 /*
  * Enable single or block step.
  */
@@ -169,19 +184,10 @@ static void enable_step(struct task_stru
 	 * 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)
+		enable_block_step(true);
+	else if (test_tsk_thread_flag(child, TIF_BLOCKSTEP))
+		enable_block_step(false);
 }
 
 void user_enable_single_step(struct task_struct *child)
@@ -199,13 +205,8 @@ void user_disable_single_step(struct tas
 	/*
 	 * 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))
+		enable_block_step(false);
 
 	/* Always clear TIF_SINGLESTEP... */
 	clear_tsk_thread_flag(child, TIF_SINGLESTEP);

Q: user_enable_single_step() && update_debugctlmsr()

[Oleg Nesterov]

On 08/01, Oleg Nesterov wrote:
>
> And btw, this is offtopic, but the usage of update_debugctlmsr()
> doesn't look right to me (I can be easily wrong though). I'll write
> another email.

user_enable_single_step() does

	if (enable_single_step(child) && block) {
		unsigned long debugctl = get_debugctlmsr();

		debugctl |= DEBUGCTLMSR_BTF;
		update_debugctlmsr(debugctl);
		set_tsk_thread_flag(child, TIF_BLOCKSTEP);
	}

and I do not understand update_debugctlmsr() above (and other
callsites).

Lets ignore uprobes which needs the changes anyway. This is
only used by ptrace and the task is stopped. So, unless I missed
something obvious, this update_debugctlmsr() is simply unneeded,
__switch_to/__switch_to_xtra should notice _TIF_BLOCKSTEP and do
update_debugctlmsr(DEBUGCTLMSR_BTF).

But, worse, isn't it wrong? Suppose that debugger switches to
another TIF_SINGLESTEP && !TIF_BLOCKSTEP task, in this case
we "leak" DEBUGCTLMSR_BTF, no?

IOW, it seems to me we could safely remove update_debugctlmsr()
arch/x86/kernel/step.c. However, if we want to re-use this code
in uprobes, then we probably need to add "if (child == current)".

Or I am totally confused. Help!

Oleg.

Re: Q: user_enable_single_step() && update_debugctlmsr()

[Sebastian Andrzej Siewior]

On 08/01/2012 03:01 PM, Oleg Nesterov wrote:
> Lets ignore uprobes which needs the changes anyway. This is
> only used by ptrace and the task is stopped. So, unless I missed
> something obvious, this update_debugctlmsr() is simply unneeded,
> __switch_to/__switch_to_xtra should notice _TIF_BLOCKSTEP and do
> update_debugctlmsr(DEBUGCTLMSR_BTF).

It looks like it unless a processes ptraces itself (which does not make
much sense anyway).

> But, worse, isn't it wrong? Suppose that debugger switches to
> another TIF_SINGLESTEP&&  !TIF_BLOCKSTEP task, in this case
> we "leak" DEBUGCTLMSR_BTF, no?

__switch_to_xtra() should notice the difference in the TIF_BLOCKSTEP
flag and disable it.

> IOW, it seems to me we could safely remove update_debugctlmsr()
> arch/x86/kernel/step.c. However, if we want to re-use this code
> in uprobes, then we probably need to add "if (child == current)".
It looks that way.

>
> Or I am totally confused. Help!
>
> Oleg.

Sebastian

Re: Q: user_enable_single_step() && update_debugctlmsr()

[Oleg Nesterov]

On 08/01, Sebastian Andrzej Siewior wrote:
>
> On 08/01/2012 03:01 PM, Oleg Nesterov wrote:
>> Lets ignore uprobes which needs the changes anyway. This is
>> only used by ptrace and the task is stopped. So, unless I missed
>> something obvious, this update_debugctlmsr() is simply unneeded,
>> __switch_to/__switch_to_xtra should notice _TIF_BLOCKSTEP and do
>> update_debugctlmsr(DEBUGCTLMSR_BTF).
>
> It looks like it unless a processes ptraces itself (which does not make
> much sense anyway).

and forbidden ;) See ptrace_attach()->same_thread_group().

>> But, worse, isn't it wrong? Suppose that debugger switches to
>> another TIF_SINGLESTEP&&  !TIF_BLOCKSTEP task, in this case
>> we "leak" DEBUGCTLMSR_BTF, no?
>
> __switch_to_xtra() should notice the difference in the TIF_BLOCKSTEP
> flag and disable it.

And how it can notice the difference if there is no difference?

(unless, of course debugger is TIF_BLOCKSTEP'ed).

Oleg.

Re: Q: user_enable_single_step() && update_debugctlmsr()

[Sebastian Andrzej Siewior]

On 08/01/2012 03:46 PM, Oleg Nesterov wrote:

>>> But, worse, isn't it wrong? Suppose that debugger switches to
>>> another TIF_SINGLESTEP&&   !TIF_BLOCKSTEP task, in this case
>>> we "leak" DEBUGCTLMSR_BTF, no?
>>
>> __switch_to_xtra() should notice the difference in the TIF_BLOCKSTEP
>> flag and disable it.
>
> And how it can notice the difference if there is no difference?
>
> (unless, of course debugger is TIF_BLOCKSTEP'ed).

Yes. enable_step() sets DEBUGCTLMSR_BTF along with TIF_BLOCKSTEP. 
kprobes checks the same flag before touching DEBUGCTLMSR_BTF.

>
> Oleg.
>
Sebastian

Re: Q: user_enable_single_step() && update_debugctlmsr()

[Oleg Nesterov]

On 08/01, Sebastian Andrzej Siewior wrote:
>
> On 08/01/2012 03:46 PM, Oleg Nesterov wrote:
>
>>>> But, worse, isn't it wrong? Suppose that debugger switches to
>>>> another TIF_SINGLESTEP&&   !TIF_BLOCKSTEP task, in this case
>>>> we "leak" DEBUGCTLMSR_BTF, no?
>>>
>>> __switch_to_xtra() should notice the difference in the TIF_BLOCKSTEP
>>> flag and disable it.
>>
>> And how it can notice the difference if there is no difference?
>>
>> (unless, of course debugger is TIF_BLOCKSTEP'ed).
>
> Yes. enable_step() sets DEBUGCTLMSR_BTF along with TIF_BLOCKSTEP.
> kprobes checks the same flag before touching DEBUGCTLMSR_BTF.

It seems that you replied to the wrong email or I am confused ;)

Let's ignore kprobes here.

Oleg.

Re: Q: user_enable_single_step() && update_debugctlmsr()

[Sebastian Andrzej Siewior]

On 08/01/2012 04:01 PM, Oleg Nesterov wrote:
> On 08/01, Sebastian Andrzej Siewior wrote:
>>
>> On 08/01/2012 03:46 PM, Oleg Nesterov wrote:
>>
>>>>> But, worse, isn't it wrong? Suppose that debugger switches to
>>>>> another TIF_SINGLESTEP&&    !TIF_BLOCKSTEP task, in this case
>>>>> we "leak" DEBUGCTLMSR_BTF, no?
>>>>
>>>> __switch_to_xtra() should notice the difference in the TIF_BLOCKSTEP
>>>> flag and disable it.
>>>
>>> And how it can notice the difference if there is no difference?
>>>
>>> (unless, of course debugger is TIF_BLOCKSTEP'ed).
>>
>> Yes. enable_step() sets DEBUGCTLMSR_BTF along with TIF_BLOCKSTEP.
>> kprobes checks the same flag before touching DEBUGCTLMSR_BTF.
>
> It seems that you replied to the wrong email or I am confused ;)

No I think I replied to the correct one :)
enable_step() is the only place for ptrace/debugger which is touching
DEBUGCTLMSR_BTF. It always sets DEBUGCTLMSR_BTF and TIF_BLOCKSTEP in
sync so why should they both end up different? And once 
__switch_to_extra() notices that TIF_BLOCKSTEP from the previous task
is different from the next task is different, then the CPU flag has
to be changed.

> Let's ignore kprobes here.

done.

>
> Oleg.
>


Sebastian

Re: Q: user_enable_single_step() && update_debugctlmsr()

[Oleg Nesterov]

On 08/01, Sebastian Andrzej Siewior wrote:
>
> On 08/01/2012 04:01 PM, Oleg Nesterov wrote:
>> On 08/01, Sebastian Andrzej Siewior wrote:
>>>
>>> On 08/01/2012 03:46 PM, Oleg Nesterov wrote:
>>>
>>>>>> But, worse, isn't it wrong? Suppose that debugger switches to
>>>>>> another TIF_SINGLESTEP&&    !TIF_BLOCKSTEP task, in this case
>>>>>> we "leak" DEBUGCTLMSR_BTF, no?
>>>>>
>>>>> __switch_to_xtra() should notice the difference in the TIF_BLOCKSTEP
>>>>> flag and disable it.
>>>>
>>>> And how it can notice the difference if there is no difference?
>>>>
>>>> (unless, of course debugger is TIF_BLOCKSTEP'ed).
>>>
>>> Yes. enable_step() sets DEBUGCTLMSR_BTF along with TIF_BLOCKSTEP.
>>> kprobes checks the same flag before touching DEBUGCTLMSR_BTF.
>>
>> It seems that you replied to the wrong email or I am confused ;)
>
> No I think I replied to the correct one :)
> enable_step() is the only place for ptrace/debugger which is touching
> DEBUGCTLMSR_BTF. It always sets DEBUGCTLMSR_BTF and TIF_BLOCKSTEP in
> sync so why should they both end up different? And once
> __switch_to_extra() notices that TIF_BLOCKSTEP from the previous task
> is different from the next task is different, then the CPU flag has
> to be changed.

OK, I was confuse by "kprobes" above.

And I think you missed my point. I'll try again.

We have the GDB process and the (stopped) tracee T. And we have
another task X which have TIF_SINGLESTEP but not TIF_BLOCKSTEP.
To simplify, suppose that X is already TASK_RUNNING but not on rq.

GDB does ptrace(PTRACE_SINGLEBLOCK, T). This sets X->TIF_BLOCKSTEP.
Now suppose that GDB is preempted right after it does
update_debugctlmsr(), and the scheduler choses X as the next task.

Both GDB and X do not have TIF_BLOCKSTEP, so __switch_to_extra()
does not update DEBUGCTLMSR_BTF.

X returns to the user-mode with TIF_SINGLESTEP and TIF_BLOCKSTEP,
the latter is wrong.

No?

Oleg.

Re: Q: user_enable_single_step() && update_debugctlmsr()

[Oleg Nesterov]

On 08/01, Oleg Nesterov wrote:
>
> GDB does ptrace(PTRACE_SINGLEBLOCK, T). This sets X->TIF_BLOCKSTEP.
                                                   ^^^
this sets T->TIF_BLOCKSTEP, I meant

Oleg.

Re: Q: user_enable_single_step() && update_debugctlmsr()

[Sebastian Andrzej Siewior]

On 08/01/2012 04:31 PM, Oleg Nesterov wrote:
> And I think you missed my point. I'll try again.
Okay.

> We have the GDB process and the (stopped) tracee T. And we have
> another task X which have TIF_SINGLESTEP but not TIF_BLOCKSTEP.
> To simplify, suppose that X is already TASK_RUNNING but not on rq.
>
> GDB does ptrace(PTRACE_SINGLEBLOCK, T). This sets X->TIF_BLOCKSTEP.
> Now suppose that GDB is preempted right after it does
> update_debugctlmsr(), and the scheduler choses X as the next task.
>
> Both GDB and X do not have TIF_BLOCKSTEP, so __switch_to_extra()
> does not update DEBUGCTLMSR_BTF.
>
> X returns to the user-mode with TIF_SINGLESTEP and TIF_BLOCKSTEP,
> the latter is wrong.
>
> No?

Yup, correct, you are right sir. Thank you for trying so hard to make
me see this :)

So a patch like
--- a/arch/x86/kernel/step.c
+++ b/arch/x86/kernel/step.c
@@ -173,8 +173,8 @@ static void enable_step(struct task_struct *child, 
bool block)
                 unsigned long debugctl = get_debugctlmsr();

                 debugctl |= DEBUGCTLMSR_BTF;
-               update_debugctlmsr(debugctl);
                 set_tsk_thread_flag(child, TIF_BLOCKSTEP);
+               update_debugctlmsr(debugctl);
         } else if (test_tsk_thread_flag(child, TIF_BLOCKSTEP)) {
                 unsigned long debugctl = get_debugctlmsr();

should fix the race and _yes_ I also would follow your suggestion to
remove this update_debugctlmsr() here since switch_to() should do this.

>
> Oleg.
>

Sebastian

Re: Q: user_enable_single_step() && update_debugctlmsr()

[Oleg Nesterov]

On 08/01, Sebastian Andrzej Siewior wrote:
>
> So a patch like
> --- a/arch/x86/kernel/step.c
> +++ b/arch/x86/kernel/step.c
> @@ -173,8 +173,8 @@ static void enable_step(struct task_struct *child,  
> bool block)
>                 unsigned long debugctl = get_debugctlmsr();
>
>                 debugctl |= DEBUGCTLMSR_BTF;
> -               update_debugctlmsr(debugctl);
>                 set_tsk_thread_flag(child, TIF_BLOCKSTEP);
> +               update_debugctlmsr(debugctl);
>         } else if (test_tsk_thread_flag(child, TIF_BLOCKSTEP)) {
>                 unsigned long debugctl = get_debugctlmsr();
>
> should fix the race

No, I don't think it can fix something ;) or make any difference.

> and _yes_ I also would follow your suggestion to
> remove this update_debugctlmsr() here since switch_to() should do this.

Agreed, but once again, uprobes needs it if child == current (but we should
move this code into the trivial helper). If we change (I hope) uprobes to
avoid user_enable_single_step() we will export the helper.

Oleg.

Re: Q: user_enable_single_step() && update_debugctlmsr()

[Sebastian Andrzej Siewior]

On 08/01/2012 05:01 PM, Oleg Nesterov wrote:
> On 08/01, Sebastian Andrzej Siewior wrote:
>> So a patch like
>> --- a/arch/x86/kernel/step.c
>> +++ b/arch/x86/kernel/step.c
>> @@ -173,8 +173,8 @@ static void enable_step(struct task_struct *child,
>> bool block)
>>                  unsigned long debugctl = get_debugctlmsr();
>>
>>                  debugctl |= DEBUGCTLMSR_BTF;
>> -               update_debugctlmsr(debugctl);
>>                  set_tsk_thread_flag(child, TIF_BLOCKSTEP);
>> +               update_debugctlmsr(debugctl);
>>          } else if (test_tsk_thread_flag(child, TIF_BLOCKSTEP)) {
>>                  unsigned long debugctl = get_debugctlmsr();
>>
>> should fix the race
>
> No, I don't think it can fix something ;) or make any difference.

Why? You _first_ set the task flag followed by the CPU register. Now 
switch_to() would see the bit set and act.

>> and _yes_ I also would follow your suggestion to
>> remove this update_debugctlmsr() here since switch_to() should do this.
>
> Agreed, but once again, uprobes needs it if child == current (but we should
> move this code into the trivial helper). If we change (I hope) uprobes to
> avoid user_enable_single_step() we will export the helper.

Okay. Looking at TIF_NOTSC I see that it does a preempt_disable() while
playing with the bit. So this would be probably more obvious than
switching the order :)

>
> Oleg.

Sebastian

Re: Q: user_enable_single_step() && update_debugctlmsr()

[Oleg Nesterov]

On 08/01, Sebastian Andrzej Siewior wrote:
>
> On 08/01/2012 05:01 PM, Oleg Nesterov wrote:
>> On 08/01, Sebastian Andrzej Siewior wrote:
>>> So a patch like
>>> --- a/arch/x86/kernel/step.c
>>> +++ b/arch/x86/kernel/step.c
>>> @@ -173,8 +173,8 @@ static void enable_step(struct task_struct *child,
>>> bool block)
>>>                  unsigned long debugctl = get_debugctlmsr();
>>>
>>>                  debugctl |= DEBUGCTLMSR_BTF;
>>> -               update_debugctlmsr(debugctl);
>>>                  set_tsk_thread_flag(child, TIF_BLOCKSTEP);
>>> +               update_debugctlmsr(debugctl);
>>>          } else if (test_tsk_thread_flag(child, TIF_BLOCKSTEP)) {
>>>                  unsigned long debugctl = get_debugctlmsr();
>>>
>>> should fix the race
>>
>> No, I don't think it can fix something ;) or make any difference.
>
> Why? You _first_ set the task flag

Yes, and this task is "child".

> followed by the CPU register. Now
> switch_to() would see the bit set and act.

child sleeps and doesn't participate in switch_to(). Debugger and another
(unrelated) task do.

Oleg.

Re: Q: user_enable_single_step() && update_debugctlmsr()

[Sebastian Andrzej Siewior]

On 08/01/2012 05:14 PM, Oleg Nesterov wrote:
> On 08/01, Sebastian Andrzej Siewior wrote:
>>
>> On 08/01/2012 05:01 PM, Oleg Nesterov wrote:
>>> On 08/01, Sebastian Andrzej Siewior wrote:
>>>> So a patch like
>>>> --- a/arch/x86/kernel/step.c
>>>> +++ b/arch/x86/kernel/step.c
>>>> @@ -173,8 +173,8 @@ static void enable_step(struct task_struct *child,
>>>> bool block)
>>>>                   unsigned long debugctl = get_debugctlmsr();
>>>>
>>>>                   debugctl |= DEBUGCTLMSR_BTF;
>>>> -               update_debugctlmsr(debugctl);
>>>>                   set_tsk_thread_flag(child, TIF_BLOCKSTEP);
>>>> +               update_debugctlmsr(debugctl);
>>>>           } else if (test_tsk_thread_flag(child, TIF_BLOCKSTEP)) {
>>>>                   unsigned long debugctl = get_debugctlmsr();
>>>>
>>>> should fix the race
>>>
>>> No, I don't think it can fix something ;) or make any difference.
>>
>> Why? You _first_ set the task flag
>
> Yes, and this task is "child".
>
>> followed by the CPU register. Now
>> switch_to() would see the bit set and act.
>
> child sleeps and doesn't participate in switch_to(). Debugger and another
> (unrelated) task do.

This is confusing.

In order to allow the debugger to ptrace()->enable_blockstep() the
child has to be stopped/traced. We switch X86_EFLAGS_TF in child's regs
and enable DEBUGCTLMSR_BTF for the debugger which is wrong. If we quit
to userspace then the CPU on which the debugger runs has
DEBUGCTLMSR_BTF. If the tracee task runs on the same then nothing 
happens, the bit remains set.
If the tracee happens to run on a different CPU then switch_to() will
enable the DEBUGCTLMSR_BTF bit for the debugger's CPU and switch_to() 
will enable it also on the other CPU.
I added a few printks in the source and I see output that
__switch_to_xtra() enables the bit as well as in enable_single() for
debugger's CPU. I didn't find where the single step is disabled for the
tracee. I haven't notice this in __switch_to_xtra() nor or in
disable_single_step().

> Oleg.
>

Sebastian

Re: Q: user_enable_single_step() && update_debugctlmsr()

[Oleg Nesterov]

On 08/01, Sebastian Andrzej Siewior wrote:
>
> On 08/01/2012 05:14 PM, Oleg Nesterov wrote:
>> On 08/01, Sebastian Andrzej Siewior wrote:
>>>
>>> On 08/01/2012 05:01 PM, Oleg Nesterov wrote:
>>>> On 08/01, Sebastian Andrzej Siewior wrote:
>>>>> So a patch like
>>>>> --- a/arch/x86/kernel/step.c
>>>>> +++ b/arch/x86/kernel/step.c
>>>>> @@ -173,8 +173,8 @@ static void enable_step(struct task_struct *child,
>>>>> bool block)
>>>>>                   unsigned long debugctl = get_debugctlmsr();
>>>>>
>>>>>                   debugctl |= DEBUGCTLMSR_BTF;
>>>>> -               update_debugctlmsr(debugctl);
>>>>>                   set_tsk_thread_flag(child, TIF_BLOCKSTEP);
>>>>> +               update_debugctlmsr(debugctl);
>>>>>           } else if (test_tsk_thread_flag(child, TIF_BLOCKSTEP)) {
>>>>>                   unsigned long debugctl = get_debugctlmsr();
>>>>>
>>>>> should fix the race
>>>>
>>>> No, I don't think it can fix something ;) or make any difference.
>>>
>>> Why? You _first_ set the task flag
>>
>> Yes, and this task is "child".
>>
>>> followed by the CPU register. Now
>>> switch_to() would see the bit set and act.
>>
>> child sleeps and doesn't participate in switch_to(). Debugger and another
>> (unrelated) task do.
>
> This is confusing.

Yes, I guess you misread http://marc.info/?l=linux-kernel&m=134383196411020

> In order to allow the debugger to ptrace()->enable_blockstep() the
> child has to be stopped/traced.

Yes,

> We switch X86_EFLAGS_TF in child's regs

Yes,

> and enable DEBUGCTLMSR_BTF for the debugger which is wrong.

Yes, DEBUGCTLMSR_BTF is "global" (ok, per-cpu)

> If we quit
> to userspace then the CPU on which the debugger runs has
> DEBUGCTLMSR_BTF.

Yes, this doesn't look right too, but I meant another race.

I have no idea what DEBUGCTLMSR_BTF means without X86_EFLAGS_TF
though. And if gdb itself is TIF_SINGLESTEP'ed, it won't return
to userspace without report/schedule.

But, yes sure! this doesn't look right and this is the source of
other problems, and this is why I started this thread.

> If the tracee task runs

In the scenario I tried to describe above, the tracee does _not_ run.

gdb switches to _another_ X86_EFLAGS_TF task before the tracee is resumed.

>From the link above,

	We have the GDB process and the (stopped) tracee T. And we have
	another task X
        ^^^^^^^^^^^^^^

Oleg.

Re: Q: user_enable_single_step() && update_debugctlmsr()

[Sebastian Andrzej Siewior]

On 08/02/2012 03:05 PM, Oleg Nesterov wrote:
> I have no idea what DEBUGCTLMSR_BTF means without X86_EFLAGS_TF
> though. And if gdb itself is TIF_SINGLESTEP'ed, it won't return
> to userspace without report/schedule.

In the Manuel it is only described what happens together with the TF
bit. So one might think nothing. Userland can not read the bit back
however leaking it does not look right.

> But, yes sure! this doesn't look right and this is the source of
> other problems, and this is why I started this thread.
>
>> If the tracee task runs
>
> In the scenario I tried to describe above, the tracee does _not_ run.
>
> gdb switches to _another_ X86_EFLAGS_TF task before the tracee is resumed.
>
>> From the link above,
>
> 	We have the GDB process and the (stopped) tracee T. And we have
> 	another task X
>          ^^^^^^^^^^^^^^

Yes that is correct. Let me try to figure out how to plumb that leak
and fix this before playing with it further.

>
> Oleg.

Sebastian

Re: Q: user_enable_single_step() && update_debugctlmsr()

[Oleg Nesterov]

On 08/02, Sebastian Andrzej Siewior wrote:
>
>>> From the link above,
>>
>> 	We have the GDB process and the (stopped) tracee T. And we have
>> 	another task X
>>          ^^^^^^^^^^^^^^
>
> Yes that is correct. Let me try to figure out how to plumb that leak
> and fix this before playing with it further.

I'll send 2 patches today. the 1st one adds the helper, the 2nd patch
fixes the usage of update_debugctlmsr().

Oleg.

Re: [PATCH 2/2] x86/uprobes: implement x86 specific arch_uprobe_*_step

[Oleg Nesterov]

See my previous emails... and a couple of other nits.

On 07/31, Sebastian Andrzej Siewior wrote:
>
> +static int insn_is_popf(const u8 *insn)
> +{
> +	/* popf */
> +	if (insn[0] == 0x9d)
> +		return 1;
> +	return 0;
> +}

I can't believe I am going to blame the naming ;)

But "insn_is_popf" looks confusing, imho. Yes, currently "iret" can't
be probed, so (afaics) we only need to check "popf". Still I think the
name should be generic, and the comment should explain that only "popf"
can be probed. And I think it would be better to pass auprobe, not
->insn. But this all is cosmetic.

> +void arch_uprobe_enable_step(struct task_struct *child,
> +		struct arch_uprobe *auprobe)
> +{
> +	struct uprobe_task	*utask		= child->utask;
> +	struct arch_uprobe_task	*autask		= &utask->autask;
> +	struct pt_regs		*regs		= task_pt_regs(child);
> +	unsigned long		debugctl;
> +
> +	autask->restore_flags = 0;
> +	if (!(regs->flags & X86_EFLAGS_TF) &&
> +			!insn_is_popf(auprobe->insn)) {
> +		autask->restore_flags |= UPROBE_CLEAR_TF;

This looks correct, but

> +		debugctl = get_debugctlmsr();
> +		if (debugctl & DEBUGCTLMSR_BTF) {

No, I don't think "X86_EFLAGS_TF && !insn_is_popf" is right. I guess
this mimics "enable_single_step(child) && block" in enable_step(), but
we can't trust insn_is_popf(), we should check/clear DEBUGCTLMSR_BTF
unconditionally.

And get_debugctlmsr() is another reason why arch_uprobe_enable_step()
should not have "struct task_struct *child" argument, otherwise the
code looks confusing.

However, I am not sure we can trust it. We are in kernel mode,
DEBUGCTLMSR_BTF can be cleared by kprobes (Ananth, please correct me).
I think we need to check TIF_BLOCKSTEP.

Oleg.

Re: [PATCH 2/2] x86/uprobes: implement x86 specific arch_uprobe_*_step

[Ananth N Mavinakayanahalli]

On Wed, Aug 01, 2012 at 03:43:37PM +0200, Oleg Nesterov wrote:

...

> However, I am not sure we can trust it. We are in kernel mode,
> DEBUGCTLMSR_BTF can be cleared by kprobes (Ananth, please correct me).
> I think we need to check TIF_BLOCKSTEP.

Kprobes resets DEBUGCTLMSR_BTF only if we have to single-step in
hardware. It puts the flag back if TIF_BLOCKSTEP is set, after the
single-step is complete; so yes, a better check is TIF_BLOCKSTEP.

Ananth

Re: [PATCH 1/2] uprobes: Use a helper instead of ptrace's single step enable

[Oleg Nesterov]

On 07/31, Sebastian Andrzej Siewior wrote:
>
> +void __weak arch_uprobe_enable_step(struct task_struct *child,
> +		struct arch_uprobe *arch)
> +{
> +	user_enable_single_step(current);
> +}
> +
> +void __weak arch_uprobe_disable_step(struct task_struct *child,
> +		struct arch_uprobe *arch)
> +{
> +	user_disable_single_step(child);
> +}

I don't think this needs "struct task_struct *child", it is always
current "by definition". Only current can play with ->utask.

Oleg.