Hi.  Can you help me understand why arch/powerpc/kernel/traps.c's
single_step_exception and emulate_single_step functions clear the trace
flags (MSR_SE et al) on every trace trap?

The users of this bit that I know in any detail are ptrace and kprobes.
It looks to me like kprobes does not expect MSR_SE to be cleared
automatically, though I am not entirely certain.  

ptrace does not depend on MSR_SE being cleared to run correctly.  That
is, it always explicitly clears it before resuming for non-stepping
anyway.  It may be relevant that ptrace users do not see the bits set
when in msr they fetch the registers.  But, though a user ptrace can set
the bits while the target is stopped, at least MSR_SE is always cleared
on resuming it.  I'm not aware of what userland debuggers change msr and
what they are doing, if any do.

On x86, the processor single-step flag can also be set by unprivileged
instructions.  User programs do this, and catch their own SIGTRAPs, for
strange purposes.  Is it possible to set MSR_SE/MSR_BE with unprivileged
instructions on powerpc?

Dave Nomura discovered this issue when using my utrace patches on powerpc.
This is why I'm asking.  There is no problem with the current behavior for
ptrace as implemented under utrace, either.  Like vanilla ptrace, in the
utrace implementation, whenever a thread has been stopped for tracing and
then resumes, it will explicitly set or clear single-stepping every time.
However, utrace also allows uses where the single-step event is handled
(recorded, whatever) without stopping; Dave is experimenting with exactly
that.  In this case, the thread never goes through the "stop for tracing
and wake up again" logic.  On other machines, the flag just stays set in
the user registers until explicitly cleared by debugger action.  So that's
what the existing generic utrace logic expects.

I'm tentatively using this short patch (more text below):

diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index d8502e3..0000000 100644  
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -525,7 +525,8 @@ void RunModeException(struct pt_regs *re
 
 void __kprobes single_step_exception(struct pt_regs *regs)
 {
-       regs->msr &= ~(MSR_SE | MSR_BE);  /* Turn off 'trace' bits */
+       if (!user_mode(regs) || !test_thread_flag(TIF_SINGLESTEP))
+               regs->msr &= ~(MSR_SE | MSR_BE);  /* Turn off 'trace' bits */
 
        if (notify_die(DIE_SSTEP, "single_step", regs, 5,
                                        5, SIGTRAP) == NOTIFY_STOP)
@@ -545,7 +546,8 @@ void __kprobes single_step_exception(str
 static void emulate_single_step(struct pt_regs *regs)
 {
        if (single_stepping(regs)) {
-               clear_single_step(regs);
+               if (!user_mode(regs) || !test_thread_flag(TIF_SINGLESTEP))
+                       clear_single_step(regs);
                _exception(SIGTRAP, regs, TRAP_TRACE, 0);
        }
 }


That is a conservative change that won't affect things like kprobes if
they care.  It will affect the msr seen by ptrace after PTRACE_SINGLESTEP,
though not that seen when the PTRACE_SINGLESTEP'd instruction hits another
signal, for example.  

But, I wonder if there is really a need to clear the bits here at all.
Why not let whoever set them be responsible for clearing them?  That's how
we handle the analogous bit on x86 and some others.  It seems good for
overall comprehensibility to make the details of how we use similar
hardware features on different hardware similar when possible.  Unless
there is a reason for it I don't understand, maybe these functions can
change not to touch msr at all?

Another alternative is that I can change utrace so that it always
explicitly enables single-step anew after there has been a trap and
single-step mode was left set.  (In the utrace interface, it's a
persistent thread state, not a one-shot command like PTRACE_SINGLESTEP.)
But if we can (continue to) avoid this extra code path when not stopping,
that seems preferable.

Looking at this made me notice MSR_BE.  The powerpc manual I have says
that implementing branch tracing is optional.  Do common chips in fact
implement it?  How can you tell if it is supported (at compile time or via
runtime chip feature bits)?


Thanks,
Roland

Reply via email to