It looks like the changes to preserve eflags when single-stepping don't work
right with signals. Take this test case:
<snip>
#include <signal.h>
#include <unistd.h>
volatile int done;
void handler (int sig)
{
done = 1;
}
int main()
{
while (1)
{
done = 0;
signal (SIGALRM, handler);
alarm (1);
while (!done);
}
}
<snip>
And this GDB session:
(gdb) b 18
Breakpoint 1 at 0x804840d: file test.c, line 18.
(gdb) r
Starting program: /home/drow/eflags/test
Breakpoint 1, main () at test.c:18
18 while (!done);
(gdb) p/x $eflags
$1 = 0x200217
(gdb) c
Continuing.
Program received signal SIGTRAP, Trace/breakpoint trap.
0x08048414 in main () at test.c:18
18 while (!done);
(gdb) p/x $eflags
$2 = 0x200302
There's an implied delay before the "c" which is long enough for the signal
handler to become pending.
The reason this happens is that when the inferior hits a breakpoint, the
first thing GDB will do is remove the breakpoint, single-step past it, and
reinsert it. So GDB does a PTRACE_SINGLESTEP, and the kernel invokes the
signal handler (without single-step - good so far). When the signal handler
returns, we've lost track of the fact that ptrace set the single-step flag,
however. So the single-step completes and returns SIGTRAP to GDB. GDB is
expecting a SIGTRAP and reinserts the breakpoint. Then it resumes the
inferior, but now the trap flag is set in $eflags. So, oops, the continue
acts like a step instead.
What to do? We need to know when we restore the trap bit in sigreturn
whether it was set by ptrace or by the application (possibly including by
the signal handler).
Andrew, serious kudos for GDB's sigstep.exp, which uncovered this problem
(through a much more complicated test - I may add the smaller one).
--
Daniel Jacobowitz
CodeSourcery, LLC
On Sun, 6 Mar 2005, Daniel Jacobowitz wrote:
>
> The reason this happens is that when the inferior hits a breakpoint, the
> first thing GDB will do is remove the breakpoint, single-step past it, and
> reinsert it. So GDB does a PTRACE_SINGLESTEP, and the kernel invokes the
> signal handler (without single-step - good so far).
No, not good so far.
Yes, it cleared TF, but it saved eflags with TF set on-stack, even though
the TF was due to a TIF_SINGLESTEP (and was thus "temporary", not a real
flag). That's why you see the bogus SIGTRAP after returning from the
signal handler.
Now, we actually get this _right_ if the signal is due to a single-step,
because do_debug() will do:
if (likely(tsk->ptrace & PT_DTRACE)) {
tsk->ptrace &= ~PT_DTRACE;
regs->eflags &= ~TF_MASK;
}
but that's the only place we do that. So any _other_ signal won't do this,
and we'll enter the signal handler without checking the PT_DTRACE flag to
see if TF was a temporary thing from debugger rather than something the
program actually did on its own.
I _think_ your test-case would work right if you just moved that code from
the special-case in do_debug(), and moved it to the top of
setup_sigcontext() instead. I've not tested it, though, and haven't really
given it any "deep thought". Maybe somebody smarter can say "yeah, that's
obviously the right thing to do" or "no, that won't work because.."
Linus
On Sun, Mar 06, 2005 at 02:38:41PM -0500, Daniel Jacobowitz wrote:
> The reason this happens is that when the inferior hits a breakpoint, the
> first thing GDB will do is remove the breakpoint, single-step past it, and
> reinsert it. So GDB does a PTRACE_SINGLESTEP, and the kernel invokes the
> signal handler (without single-step - good so far). When the signal handler
> returns, we've lost track of the fact that ptrace set the single-step flag,
> however. So the single-step completes and returns SIGTRAP to GDB. GDB is
> expecting a SIGTRAP and reinserts the breakpoint. Then it resumes the
> inferior, but now the trap flag is set in $eflags. So, oops, the continue
> acts like a step instead.
Eh, I got the event sequence wrong as usual, but the basic description
is right.
- Original SIGTRAP at breakpoint
- user says "cont"
- GDB tries to singlestep past the breakpoint - PTRACE_SINGLESTEP, no
signal
- GDB receives SIGALRM at the same PC
- GDB tries to singlestep past the breakpoint - PTRACE_SINGLESTEP,
SIGALRM
- GDB receives SIGTRAP at the first instruction of the handler
- GDB reinserts the breakpoint at line 18. This is a "step-resume"
breakpoint - we were stepping, we were interrupted by a signal.
- GDB issues PTRACE_CONT, no signal
- GDB receives SIGTRAP at the sigreturn location - this is the
step-resume breakpoint.
- GDB remove that and issues PTRACE_SINGLESTEP, no signal - It
is trying again to get past the breakpoint location so that it
can honor the user's "cont" request.
- GDB receives SIGTRAP at the instruction after the breakpoint.
- GDB reinserts the original breakpoint and issues PTRACE_CONTINUE.
All of this is what's supposed to happen. The executable be running
free now until it hits the breakpoint again.
- GDB receives an unexpected SIGTRAP at the next instruction (the
second instruction after the original breakpoint).
If your compiler uses only two instructions for the loop, you might not
see this. gcc -O0 will use three by default. Just stick something
else in the loop.
> What to do? We need to know when we restore the trap bit in sigreturn
> whether it was set by ptrace or by the application (possibly including by
> the signal handler).
If I'm following this right, then the saved value of eflags in the
signal handler should not contain the trap bit at this point. It does,
though. It's hard to see this in GDB, because the CFI does not express
%eflags, so "print $eflags" won't track up the stack. I don't think
there's a handy dwarf register number for it at the moment. But you
can print out the struct sigcontext by hand once you locate it on the
stack.
--
Daniel Jacobowitz
CodeSourcery, LLC
On Sun, Mar 06, 2005 at 12:03:22PM -0800, Linus Torvalds wrote:
> I _think_ your test-case would work right if you just moved that code from
> the special-case in do_debug(), and moved it to the top of
> setup_sigcontext() instead. I've not tested it, though, and haven't really
> given it any "deep thought". Maybe somebody smarter can say "yeah, that's
> obviously the right thing to do" or "no, that won't work because.."
I bought it, but the GDB testsuite didn't. Both copies seem to be
necessary; there's generally no signal handler for SIGTRAP, so moving
it disables the test in the most common case. I didn't poke at it long
enough to figure out what the failing case was, but it introduced a
different situation which could leave TF enabled. This, however,
worked:
If a debugger set the TF bit, make sure to clear it when creating a
signal context. Otherwise, TF will be incorrectly restored by
sigreturn.
Signed-off-by: Daniel Jacobowitz <[email protected]>
===== arch/i386/kernel/signal.c 1.53 vs edited =====
--- 1.53/arch/i386/kernel/signal.c 2005-01-31 01:20:14 -05:00
+++ edited/arch/i386/kernel/signal.c 2005-03-06 15:36:41 -05:00
@@ -277,6 +277,18 @@
{
int tmp, err = 0;
+ /*
+ * If TF is set due to a debugger (PT_DTRACE), clear the TF
+ * flag so that register information in the sigcontext is
+ * correct.
+ */
+ if (unlikely(regs->eflags & TF_MASK)) {
+ if (likely(current->ptrace & PT_DTRACE)) {
+ current->ptrace &= ~PT_DTRACE;
+ regs->eflags &= ~TF_MASK;
+ }
+ }
+
tmp = 0;
__asm__("movl %%gs,%0" : "=r"(tmp): "0"(tmp));
err |= __put_user(tmp, (unsigned int __user *)&sc->gs);
--
Daniel Jacobowitz
CodeSourcery, LLC
> I _think_ your test-case would work right if you just moved that code from
> the special-case in do_debug(), and moved it to the top of
> setup_sigcontext() instead. I've not tested it, though, and haven't really
> given it any "deep thought". Maybe somebody smarter can say "yeah, that's
> obviously the right thing to do" or "no, that won't work because.."
Indeed, this is what my original changes for this did, before you started
cleaning things up to be nice to TF users other than PTRACE_SINGLESTEP.
I note, btw, that the x86_64 code is still at that prior stage. So I think
it doesn't have this new wrinkle, but it also doesn't have the advantages
of the more recent i386 changes. Once we're sure about the i386 state, we
should update the x86_64 code to match.
I'm not sure what kind of smart this makes me, but I'll say that your plan
would work and no, it's obviously not the right thing to do. ;-) I haven't
tested the following, not having tracked down the specific problem case you
folks are talking about. But I think this is the right solution. The
difference is that when we stop for some signal and report to the debugger,
the debugger looking at our registers will see TF clear instead of set,
before it decides whether to continue us with the signal or what to do.
With the change yo suggested, (I think) if the debugger decides to eat the
signal and resume, we would get a spurious single-step trap after executing
the next instruction, instead of resuming normally as requested.
Thanks,
Roland
Signed-off-by: Roland McGrath <[email protected]>
--- linux-2.6/include/asm-i386/signal.h
+++ linux-2.6/include/asm-i386/signal.h
@@ -223,7 +223,14 @@ static __inline__ int sigfindinword(unsi
struct pt_regs;
extern int FASTCALL(do_signal(struct pt_regs *regs, sigset_t *oldset));
-#define ptrace_signal_deliver(regs, cookie) do { } while (0)
+
+#define ptrace_signal_deliver(regs, cookie) \
+ do { \
+ if (current->ptrace & PT_DTRACE) { \
+ current->ptrace &= ~PT_DTRACE; \
+ (regs)->eflags &= ~TF_MASK; \
+ } \
+ } while (0)
#endif /* __KERNEL__ */
--- linux-2.6/arch/i386/kernel/traps.c
+++ linux-2.6/arch/i386/kernel/traps.c
@@ -707,8 +707,6 @@ fastcall void do_debug(struct pt_regs *
/*
* Single-stepping through TF: make sure we ignore any events in
* kernel space (but re-enable TF when returning to user mode).
- * And if the event was due to a debugger (PT_DTRACE), clear the
- * TF flag so that register information is correct.
*/
if (condition & DR_STEP) {
/*
@@ -718,11 +716,6 @@ fastcall void do_debug(struct pt_regs *
*/
if ((regs->xcs & 3) == 0)
goto clear_TF_reenable;
-
- if (likely(tsk->ptrace & PT_DTRACE)) {
- tsk->ptrace &= ~PT_DTRACE;
- regs->eflags &= ~TF_MASK;
- }
}
/* Ok, finally something we can handle */
On Sun, Mar 06, 2005 at 01:22:25PM -0800, Roland McGrath wrote:
> > I _think_ your test-case would work right if you just moved that code from
> > the special-case in do_debug(), and moved it to the top of
> > setup_sigcontext() instead. I've not tested it, though, and haven't really
> > given it any "deep thought". Maybe somebody smarter can say "yeah, that's
> > obviously the right thing to do" or "no, that won't work because.."
>
> Indeed, this is what my original changes for this did, before you started
> cleaning things up to be nice to TF users other than PTRACE_SINGLESTEP.
>
> I note, btw, that the x86_64 code is still at that prior stage. So I think
> it doesn't have this new wrinkle, but it also doesn't have the advantages
> of the more recent i386 changes. Once we're sure about the i386 state, we
> should update the x86_64 code to match.
>
> I'm not sure what kind of smart this makes me, but I'll say that your plan
> would work and no, it's obviously not the right thing to do. ;-) I haven't
> tested the following, not having tracked down the specific problem case you
> folks are talking about. But I think this is the right solution. The
> difference is that when we stop for some signal and report to the debugger,
> the debugger looking at our registers will see TF clear instead of set,
> before it decides whether to continue us with the signal or what to do.
> With the change yo suggested, (I think) if the debugger decides to eat the
> signal and resume, we would get a spurious single-step trap after executing
> the next instruction, instead of resuming normally as requested.
Roland, the sigstep.exp test in the GDB testsuite will show this
problem; if your patch monotonically improves GDB HEAD testsuite
results and removes all the FAILs for sigstep.exp, then it's probably
equivalent to the one I just posted for this testcase.
I think mine is more correct; the problem doesn't occur because the
debugger cancelled a signal, it occurs because a bogus TF bit was saved
to the signal context. I like keeping solutions close to their
problems. But that's just aesthetic.
--
Daniel Jacobowitz
CodeSourcery, LLC
On Sun, 6 Mar 2005, Daniel Jacobowitz wrote:
>
> I bought it, but the GDB testsuite didn't. Both copies seem to be
> necessary; there's generally no signal handler for SIGTRAP
Ahh, duh, yes. I was looking at it and saying "debug fault always
generates a sigtrap", but you're right, it obviously doesn't - usually
it's caught by the tracer.
Your patch looks fine.
Linus
On Sun, Mar 06, 2005 at 07:16:37PM -0800, Roland McGrath wrote:
> > I think mine is more correct; the problem doesn't occur because the
> > debugger cancelled a signal, it occurs because a bogus TF bit was saved
> > to the signal context. I like keeping solutions close to their
> > problems. But that's just aesthetic.
>
> I understand the scenario. Understanding how it comes about made me
> recognize there is another scenario that is also handled wrong.
> I didn't say the second scenario was what you are seeing.
>
> Dan's patch covers the case of PTRACE_SINGLESTEP called to deliver a signal
> that has a handler to run. That's because there TF is set after the ptrace
> stop, when it's resuming. This is a "normalize register state" operation.
> I think it would be a little clearer to do this in handle_signal where the
> similar case of tweaking register state to back up a system call is done.
>
> The patch I posted moves the resetting of TF from the trap handler to
> ptrace_signal_deliver. This is necessary to ensure that TF is not shown as
> set in the registers retrieved by the debugger when the process stops for
> something other than the single-step trap requested by PTRACE_SINGLESTEP.
Is this semantically different from the patch I posted, i.e. is there
any case which one of them covers and not the other?
> Here is a patch that does both of those things. This had no effect on any
> of the gdb testsuite cases (for good or ill) aside from sigstep.exp, and:
>
> $ grep 'FAIL.*sigstep' testsuite/gdb.sum
> KFAIL: gdb.base/sigstep.exp: finish from handleri; leave handler (could not set breakpoint) (PRMS: gdb/1736)
>
> I don't know what that one is about, but it was KFAIL before the change too.
That is an inability to set breakpoints in the vsyscall page. Andrew
told me (last May, wow) that he thought this worked in Fedora, but I
haven't seen any signs of the code. It would certainly be a Good Thing
if it is possible!
>
--
Daniel Jacobowitz
CodeSourcery, LLC
Roland McGrath <[email protected]> writes:
>
> I note, btw, that the x86_64 code is still at that prior stage. So I think
> it doesn't have this new wrinkle, but it also doesn't have the advantages
> of the more recent i386 changes. Once we're sure about the i386 state, we
> should update the x86_64 code to match.
I have it fixed in my tree, but not pushed out yet because of lack of
testing.
In general I track all i386 changes that make sense for modern systems
and 64bit, but sometimes merging takes some time.
However given the flood of bugs in the i386 code I'm starting to have
doubts again if all these changes were really a good idea.
However x86-64 has to be bug-to-bug compatible to i386, so there
is not much choice.
-Andi
> Is this semantically different from the patch I posted, i.e. is there
> any case which one of them covers and not the other?
Yes, the second case that I described when I said there were two cases!
(Sheesh.) To repeat, when the process was doing PTRACE_SINGLESTEP and then
stops on some other signal rather than because of the single-step trap
(e.g. single-stepping an instruction that faults), ptrace will show TF set
in its registers. With my patch, it will show TF clear.
> That is an inability to set breakpoints in the vsyscall page. Andrew
> told me (last May, wow) that he thought this worked in Fedora, but I
> haven't seen any signs of the code. It would certainly be a Good Thing
> if it is possible!
Fedora kernels use a normal mapping (with randomized location) for the
page, rather than the fixed high address in the vanilla kernel. The
FIXADDR_USER_START area is globally mapped in a special way not using
normal vma data structures, and is permanently read-only in all tasks.
COW via ptrace works normally for Fedora's flavor, but no writing is ever
possible to the fixmap page.
Thanks,
Roland
On Mon, Mar 07, 2005 at 01:29:12PM -0800, Roland McGrath wrote:
> > Is this semantically different from the patch I posted, i.e. is there
> > any case which one of them covers and not the other?
>
> Yes, the second case that I described when I said there were two cases!
> (Sheesh.)
Calm down, there were already two cases. I reread your message and
couldn't pick out the answer, or I wouldn't have asked.
> To repeat, when the process was doing PTRACE_SINGLESTEP and then
> stops on some other signal rather than because of the single-step trap
> (e.g. single-stepping an instruction that faults), ptrace will show TF set
> in its registers. With my patch, it will show TF clear.
I can reproduce this problem with the patch that Linus committed, so
you should probably update your patch for a current snapshot and nag
him about it.
> > That is an inability to set breakpoints in the vsyscall page. Andrew
> > told me (last May, wow) that he thought this worked in Fedora, but I
> > haven't seen any signs of the code. It would certainly be a Good Thing
> > if it is possible!
>
> Fedora kernels use a normal mapping (with randomized location) for the
> page, rather than the fixed high address in the vanilla kernel. The
> FIXADDR_USER_START area is globally mapped in a special way not using
> normal vma data structures, and is permanently read-only in all tasks.
> COW via ptrace works normally for Fedora's flavor, but no writing is ever
> possible to the fixmap page.
Blech. I assume that there is no way to map a normal VMA over top of
the fixed page, for a particular process? This makes debugging the
vsyscall DSO a real pain.
--
Daniel Jacobowitz
CodeSourcery, LLC
This patch further cleans up the appearance of TF in eflags when ptrace is
involved. With this, PTRACE_SINGLESTEP will not cause TF to appear in
eflags as seen by PTRACE_GETREGS and the like, when the instruction faulted
for some reason other than the single-step trap.
This moves the check added by Dan's patch from setup_sigcontext to
handle_signal. This is a cosmetic difference, but I think it makes more
sense to consolidate all the "reset registers to canonical state" work in
the same place (i.e. put it with the syscall rollback code), separate from
the signal handler setup. The change that matters is moving the similar
check out of do_debug, where it only covers the case of a single-step trap.
Instead, it goes into the ptrace_signal_deliver macro, which is called
before the ptrace stop for whatever signal results from whatever kind of
fault in that instruction (or asynchronous signal). With that, the
handle_signal check is still needed only for the case of PTRACE_SINGLESTEP
with a handled signal.
Thanks,
Roland
Signed-off-by: Roland McGrath <[email protected]>
--- linux-2.6/arch/i386/kernel/signal.c
+++ linux-2.6/arch/i386/kernel/signal.c
@@ -277,18 +277,6 @@ setup_sigcontext(struct sigcontext __use
{
int tmp, err = 0;
- /*
- * If TF is set due to a debugger (PT_DTRACE), clear the TF
- * flag so that register information in the sigcontext is
- * correct.
- */
- if (unlikely(regs->eflags & TF_MASK)) {
- if (likely(current->ptrace & PT_DTRACE)) {
- current->ptrace &= ~PT_DTRACE;
- regs->eflags &= ~TF_MASK;
- }
- }
-
tmp = 0;
__asm__("movl %%gs,%0" : "=r"(tmp): "0"(tmp));
err |= __put_user(tmp, (unsigned int __user *)&sc->gs);
@@ -569,6 +557,16 @@ handle_signal(unsigned long sig, siginfo
}
}
+ /*
+ * If TF is set due to a debugger (PT_DTRACE), clear the TF flag so
+ * that register information in the sigcontext is correct.
+ */
+ if (unlikely(regs->eflags & TF_MASK)
+ && likely(current->ptrace & PT_DTRACE)) {
+ current->ptrace &= ~PT_DTRACE;
+ regs->eflags &= ~TF_MASK;
+ }
+
/* Set up the stack frame */
if (ka->sa.sa_flags & SA_SIGINFO)
setup_rt_frame(sig, ka, info, oldset, regs);
--- linux-2.6/arch/i386/kernel/traps.c
+++ linux-2.6/arch/i386/kernel/traps.c
@@ -707,8 +707,6 @@ fastcall void do_debug(struct pt_regs *
/*
* Single-stepping through TF: make sure we ignore any events in
* kernel space (but re-enable TF when returning to user mode).
- * And if the event was due to a debugger (PT_DTRACE), clear the
- * TF flag so that register information is correct.
*/
if (condition & DR_STEP) {
/*
@@ -718,11 +716,6 @@ fastcall void do_debug(struct pt_regs *
*/
if ((regs->xcs & 3) == 0)
goto clear_TF_reenable;
-
- if (likely(tsk->ptrace & PT_DTRACE)) {
- tsk->ptrace &= ~PT_DTRACE;
- regs->eflags &= ~TF_MASK;
- }
}
/* Ok, finally something we can handle */
--- linux-2.6/include/asm-i386/signal.h
+++ linux-2.6/include/asm-i386/signal.h
@@ -223,7 +223,14 @@ static __inline__ int sigfindinword(unsi
struct pt_regs;
extern int FASTCALL(do_signal(struct pt_regs *regs, sigset_t *oldset));
-#define ptrace_signal_deliver(regs, cookie) do { } while (0)
+
+#define ptrace_signal_deliver(regs, cookie) \
+ do { \
+ if (current->ptrace & PT_DTRACE) { \
+ current->ptrace &= ~PT_DTRACE; \
+ (regs)->eflags &= ~TF_MASK; \
+ } \
+ } while (0)
#endif /* __KERNEL__ */
On Sun, Mar 13, 2005 at 12:27:58AM -0800, Roland McGrath wrote:
> This patch further cleans up the appearance of TF in eflags when ptrace is
> involved. With this, PTRACE_SINGLESTEP will not cause TF to appear in
> eflags as seen by PTRACE_GETREGS and the like, when the instruction faulted
> for some reason other than the single-step trap.
>
> This moves the check added by Dan's patch from setup_sigcontext to
> handle_signal. This is a cosmetic difference, but I think it makes more
> sense to consolidate all the "reset registers to canonical state" work in
> the same place (i.e. put it with the syscall rollback code), separate from
> the signal handler setup. The change that matters is moving the similar
> check out of do_debug, where it only covers the case of a single-step trap.
> Instead, it goes into the ptrace_signal_deliver macro, which is called
> before the ptrace stop for whatever signal results from whatever kind of
> fault in that instruction (or asynchronous signal). With that, the
> handle_signal check is still needed only for the case of PTRACE_SINGLESTEP
> with a handled signal.
>
>
> Thanks,
> Roland
Thanks, looks right to me!
--
Daniel Jacobowitz
CodeSourcery, LLC
Andi Kleen wrote:
> > Once we're sure about the i386 state, we should update the x86_64 code to match.
>
> I have it fixed in my tree, but not pushed out yet because of lack of testing.
Andi,
I have someone who thinks they might be experiencing the same problem
that I reported but now on x86-64. Could I get the patches from your
tree so they can be tested?
Jesse
Roland, Daniel,
I was the one that reported the ptrace problems which caused all that
hoopla in Nov and Dec. I have tested your new patches and find that
there are no new regressions.
Jesse