> It now fails on non-utrace 2.6.25-rc5-git4 with IMO a kernel ptrace bug:
> Trap flag found clear after it was set by the inferior.
I have a fix for this one. This case is that TF was just set
by using debugger stepping, which sets a kernel flag saying
this is a "forced" TF rather than a "natural" one (so ptrace
masks off TF when you peek). Then we step again into a "popf"
instruction, which will explicitly set or clear it to a new
"natural" state. In this case, the kernel failed to reset its
flag, so after the popf it still thinks TF is "forced" and
hides it. (I think this bug ought to manifest in some case or
other with either single-step or block-step.)
What the kernel did is just skip other bookkeeping if TF was
already set before the step. My fix is to clear the "forced"
flag when about to step into a "popf" or similar instruction,
even if TF was already set.
Btw, I'd find it clearer to read the asm block if you just used
"jnz instr3" and the like instead of 1b with multiple 1: labels
at the same places you already have named labels.
Many of the asserts would be nicer as messages telling you what
the offending value of ip or stopsig or whatever was. Whenever
I actually debug the kernel, I have to tweak the test programs
by hand to display enough info to figure out what I'm doing.
> Also it fails later on:
> PTRACE_SINGLEBLOCK should stop us at instr8 (not instr5).
There was a related bug due to the same control flow in the
kernel bookkeeping, i.e. when TF was already set it skipped it
all and suppressed block-step. I fixed it so that if TF was
already set, it only suppresses block-step only if TF was
"natural". (When user-mode has set TF with popf, we "suppress"
enabling the block-step hardware for a PTRACE_SINGLEBLOCK, so
it degenerates to a plain instruction step as the user wanted
to see.)
With the kernel fix I'm using, the test case now barfs because
of hitting "instr12: hlt" (and so SIGSEGV instead of SIGTRAP).
That's because of the known limitation that stepping "pushf"
puts a false TF in the flags the inferior sees, so the instr10
branch is not taken.
I think this adds up to all expected behavior. The limitations
explained in the comments are probably never going to change.
(At least, it's not worth addressing them for a synthetic test
case before any real-world need arises.)
It's especially mind-bending because this is testing everything
at once. I think there are permutations it's not testing that
were in fact wrong as well and should be fixed by my patch. I
would be more confident of the fixes if we were to test some of
the cases with few variables as well as the ones we test now.
e.g. I think that even if no pushf/popf were involved recently,
if we just single-stepped before single-step into popf, we'll
fail to clear the "forced"; also, if we just single-stepped we
will always fail to suppress block-step. I've been thinking
about it too long now and can't quite keep it all straight.
I'd like to make sure this all seems coherent and there aren't
holes in my logic you can think of before I send this upstream.
Thanks,
Roland
diff --git a/arch/x86/kernel/step.c b/arch/x86/kernel/step.c
index 071ff47..0000000 100644
--- a/arch/x86/kernel/step.c
+++ b/arch/x86/kernel/step.c
@@ -105,6 +105,7 @@ static int is_setting_trap_flag(struct t
static int enable_single_step(struct task_struct *child)
{
struct pt_regs *regs = task_pt_regs(child);
+ unsigned long oflags;
/*
* Always set TIF_SINGLESTEP - this guarantees that
@@ -113,11 +114,7 @@ static int enable_single_step(struct tas
*/
set_tsk_thread_flag(child, TIF_SINGLESTEP);
- /*
- * If TF was already set, don't do anything else
- */
- if (regs->flags & X86_EFLAGS_TF)
- return 0;
+ oflags = regs->flags;
/* Set TF on the kernel stack.. */
regs->flags |= X86_EFLAGS_TF;
@@ -126,9 +123,22 @@ static int enable_single_step(struct tas
* ..but if TF is changed by the instruction we will trace,
* don't mark it as being "us" that set it, so that we
* won't clear it by hand later.
+ *
+ * Note that if we don't actually execute the popf because
+ * of a signal arriving right now or suchlike, we will lose
+ * track of the fact that it really was "us" that set it.
*/
- if (is_setting_trap_flag(child, regs))
+ if (is_setting_trap_flag(child, regs)) {
+ clear_tsk_thread_flag(child, TIF_FORCED_TF);
return 0;
+ }
+
+ /*
+ * If TF was already set, check whether it was us who set it.
+ * If not, we should never attempt a block step.
+ */
+ if (oflags & X86_EFLAGS_TF)
+ return test_tsk_thread_flag(child, TIF_FORCED_TF);
set_tsk_thread_flag(child, TIF_FORCED_TF);
