x86: do_debug && PTRACE_SINGLESTEP broken by 08d68323d1f0c34452e614263b212ca556dae47f [Kernel]

Prev: [PATCH 2/3] Convert arm to arch_gettimeoffset()
Next: [GIT -maybe- PULL] notification - fanotify

From: Frederic Weisbecker on 17 Dec 2009 22:10

From: Roland McGrath on 17 Dec 2009 23:00

> > + dr6 = tsk->thread.debugreg6;
>
> why? we have "tsk->thread.debugreg6 = dr6" above

Yeah, it's a little superfluous. Except that the existing code uses
tsk->thread.debugreg6 and dr6 inconsistently. It only matters either way
if some notifier function might change thread.debugreg6, which I wasn't
sure that none might. i.e., does/should hw_breakpoint hide/remap the
watchpoint-fired bits when they are not for the same-numbered,
ptrace-installed virtual debugreg? And does/should kprobes, kgdb, and
whatnot, hide DR_STEP from thread.debugreg6 for a step that's not from
user_enable_single_step?

> > if ((dr6 & DR_STEP) && !user_mode(regs)) {
> > tsk->thread.debugreg6 &= ~DR_STEP;
> > set_tsk_thread_flag(tsk, TIF_SINGLESTEP);
> > regs->flags &= ~X86_EFLAGS_TF;
>
> this looks strange... we set TIF_SINGLESTEP but clear X86_EFLAGS_TF

This was the original purpose of TIF_SINGLESTEP from long, long ago. This
happens when TF was set in user mode and then it did syscall/sysenter so TF
is still set at the first instruction in kernel mode. TF is cleared from
the interrupted kernel registers so the kernel can resume normally. In the
original logic, TIF_SINGLESTEP served just to make it turn TF back on when
going to user mode. Since then we grew the complicated step.c stuff and
it all fits together slightly differently than it did when the original
traps.c path was written.

> can't understand how this change can fix the problem. We should always
> send SIGTRAP if the task returns to user-mode with X86_EFLAGS_TF?

If the debug exception happened in user mode, then we should send SIGTRAP.

In the old (2.6.32) code with its goto-heavy logic the !user_mode(regs)
was goto clear_TF_reenable; and that is:

clear_TF_reenable:
set_tsk_thread_flag(tsk, TIF_SINGLESTEP);
regs->flags &= ~X86_EFLAGS_TF;
preempt_conditional_cli(regs);
return;

I thought the new logic was falling through to the send_sigtrap case after
"if ((dr6 & DR_STEP) && !user_mode(regs))". But now I see that the subtle
use of dr6 vs tsk->thread.debugreg6 (without comments about it!) meant
that DR_STEP is cleared from tsk->thread.debugreg6 before we test it.

So I guess the idea there is that the !user_mode case would swallow the
step indication but still leave some DR_TRAP_BITS set and so you'd generate
a user SIGTRAP in honor of those (i.e. watchpoint hits). But I thought the
hardware behavior was that a step will set DR_STEP in DR6 but not clear any
DR_TRAP_BITS set from before, so I'm not sure this can't sometimes send a
SIGTRAP twice for a combination of a watchpoint hit and a delayed step.

> OK. I blindly applied this patch, step-simple still fails.

Yeah, it was a quick reaction to the funny-looking control flow.
But I didn't really investigate what is actually happening.

Thanks,
Roland
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

From: Oleg Nesterov on 18 Dec 2009 12:40

On 12/18, Frederic Weisbecker wrote:
>
> On Fri, Dec 18, 2009 at 01:56:50AM +0100, Oleg Nesterov wrote:
> > Hi.
> >
> > do_debug() is obviously wrong wrt PTRACE_SINGLESTEP/TIF_SINGLESTEP, no?
> >
> > Afaics this was broken by
> >
> > hw-breakpoints: modifying generic debug exception to use thread-specific debug registers
> > commit 08d68323d1f0c34452e614263b212ca556dae47f
> >
> > To verify, the "patch" below fixes the stepping for me, not sure what
> > is the proper fix...
> >
> > Oleg.
> >
> > --- arch/x86/kernel/traps.c~ 2009-12-18 00:20:49.000000000 +0100
> > +++ arch/x86/kernel/traps.c 2009-12-18 01:44:05.000000000 +0100
> > @@ -575,7 +575,7 @@ dotraplinkage void __kprobes do_debug(st
> > regs->flags &= ~X86_EFLAGS_TF;
> > }
> > si_code = get_si_code(tsk->thread.debugreg6);
> > - if (tsk->thread.debugreg6 & (DR_STEP | DR_TRAP_BITS))
> > +// if (tsk->thread.debugreg6 & (DR_STEP | DR_TRAP_BITS))
> > send_sigtrap(tsk, regs, error_code, si_code);
>
>
>
> But I don't understand why it is broken with the check.
> If we are in a singlestep exception, dr6 should have its
> DR_STEP bit set...
>
> Single stepping works well for me, after a quick check on
> gdb. How did you trigger the bug?

Please find the trivial test-case below. It hangs, because
PTRACE_SINGLESTEP doesn't trigger the trap.

(not sure this matters, but I did the testing under kvm)

Oleg.

#include <stdio.h>
#include <unistd.h>
#include <signal.h>
#include <sys/ptrace.h>
#include <sys/wait.h>
#include <assert.h>

int main(void)
{
int pid, status, i;

pid = fork();
if (!pid)
for (;;);

sleep(1);
assert(ptrace(PTRACE_ATTACH, pid, 0,0) == 0);

assert(pid == wait(&status));
assert(WIFSTOPPED(status));

for (i = 0; i < 10; ++i) {
assert(ptrace(PTRACE_SINGLESTEP, pid, 0,0) == 0);

printf("wait %d ...\n", i);
assert(pid == wait(&status));

assert(WIFSTOPPED(status) && WSTOPSIG(status) == SIGTRAP);
}

kill(pid, SIGKILL);
return 0;
}

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

From: K.Prasad on 18 Dec 2009 12:40

On Fri, Dec 18, 2009 at 01:56:50AM +0100, Oleg Nesterov wrote:
> Hi.
>
> do_debug() is obviously wrong wrt PTRACE_SINGLESTEP/TIF_SINGLESTEP, no?
>
> Afaics this was broken by
>
> hw-breakpoints: modifying generic debug exception to use thread-specific debug registers
> commit 08d68323d1f0c34452e614263b212ca556dae47f
>
> To verify, the "patch" below fixes the stepping for me, not sure what
> is the proper fix...
>
> Oleg.
>
> --- arch/x86/kernel/traps.c~ 2009-12-18 00:20:49.000000000 +0100
> +++ arch/x86/kernel/traps.c 2009-12-18 01:44:05.000000000 +0100
> @@ -575,7 +575,7 @@ dotraplinkage void __kprobes do_debug(st
> regs->flags &= ~X86_EFLAGS_TF;
> }
> si_code = get_si_code(tsk->thread.debugreg6);
> - if (tsk->thread.debugreg6 & (DR_STEP | DR_TRAP_BITS))
> +// if (tsk->thread.debugreg6 & (DR_STEP | DR_TRAP_BITS))
> send_sigtrap(tsk, regs, error_code, si_code);
> preempt_conditional_cli(regs);
>

The cause for such a behaviour isn't apparent to me and like others, I'm
unable to recreate it (Single-stepping using gdb over a tiny program
running on x86, latest -tip works fine).

Did you try to narrow down the causative piece of code, among the
several hooks in do_debug()?

A separate 'dr6' and 'thread.debugreg6' was desired by the community (refer:
Pine.LNX.4.44L0.0904011216460.3736-100000(a)iolanthe.rowland.org and
Pine.LNX.4.44L0.0904091634150.4094-100000(a)iolanthe.rowland.org) then.
'dr6' and 'thread.deebugreg6' would contain the value of the DR6 status
register and every exception handler would reset the bits in them
corresponding to which action has been taken. The difference in them being
that 'thread.debugreg6' would be eventually processed by code interested
in user-space while 'dr6' was restricted to those hooks in do_debug().

Thanks,
K.Prasad

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

From: K.Prasad on 18 Dec 2009 13:00

On Fri, Dec 18, 2009 at 06:27:47PM +0100, Oleg Nesterov wrote:
> On 12/18, Frederic Weisbecker wrote:
> >
> > On Fri, Dec 18, 2009 at 01:56:50AM +0100, Oleg Nesterov wrote:
> > > Hi.

<snipped>

> > Single stepping works well for me, after a quick check on
> > gdb. How did you trigger the bug?
>
> Please find the trivial test-case below. It hangs, because
> PTRACE_SINGLESTEP doesn't trigger the trap.
>

aah...my other mail just criss-crossed yours.

I quickly ran on the said x86 box, loaded with -tip (commit
7818b3d0fc68f5c2a85fed86d9fa37131c5a3068) and it runs fine.

[root(a)llm05 prasadkr]# cat oleg.c
#include <stdio.h>
#include <unistd.h>
#include <signal.h>
#include <sys/ptrace.h>
#include <sys/wait.h>
#include <assert.h>

int main(void)
{
int pid, status, i;

pid = fork();
if (!pid)
for (;;);

sleep(1);
assert(ptrace(PTRACE_ATTACH, pid, 0,0) == 0);

assert(pid == wait(&status));
assert(WIFSTOPPED(status));

for (i = 0; i < 10; ++i) {
assert(ptrace(PTRACE_SINGLESTEP, pid, 0,0) == 0);

printf("wait %d ...\n", i);
assert(pid == wait(&status));

assert(WIFSTOPPED(status) && WSTOPSIG(status) == SIGTRAP);
}

kill(pid, SIGKILL);
return 0;
}

[root(a)llm05 prasadkr]# gcc -o oleg oleg.c -g -Wall
[root(a)llm05 prasadkr]# ./oleg
wait 0 ...
wait 1 ...
wait 2 ...
wait 3 ...
wait 4 ...
wait 5 ...
wait 6 ...
wait 7 ...
wait 8 ...
wait 9 ...
[root(a)llm05 prasadkr]#

Am I missing something here?

Thanks,
K.Prasad

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

First | Prev | Next | Last
Pages: 1 2 3
Prev: [PATCH 2/3] Convert arm to arch_gettimeoffset()
Next: [GIT -maybe- PULL] notification - fanotify