Newsgroups: fa.linux.kernel
From: Linus Torvalds <torvalds@osdl.org>
Subject: Re: ptrace single-stepping change breaks Wine
Original-Message-ID: <Pine.LNX.4.58.0411211414460.20993@ppc970.osdl.org>
Date: Sun, 21 Nov 2004 22:41:15 GMT
Message-ID: <fa.itn7no7.14k683p@ifi.uio.no>

On Sun, 21 Nov 2004, Davide Libenzi wrote:
>
> I'd agree with Linus here. A signal handler is part of the application, so
> it should be single stepped in the same way other application code does.
> My original patch simply reenabled the flag before returning to userspace,
> and this had the consequence to single step into signal handlers too.

Hmmm.. I think I may have a test-case for the problem.

Lookie here:

	#include <signal.h>
	#include <sys/mman.h>

	void function(void)
	{
		printf("Copy protected: ok\n");
	}

	void handler(int signo)
	{
		extern char smc;
		smc++;
	}

	#define TF 0x100

	int main(int argc, char **argv)
	{
		void (*fnp)(void);

		signal(SIGTRAP, handler);
		mprotect((void *)(0xfffff000 & (unsigned long)main), 4096, PROT_READ | PROT_WRITE);
		asm volatile("pushfl ; orl %0,(%%esp) ; popfl"
			: :"i" (TF):"memory");
		asm volatile("pushfl ; andl %0,(%%esp) ; popfl"
			: :"i" (~TF):"memory");
		asm volatile("\nsmc:\n\t"
			".byte 0xb7\n\t"
			".long function"
			:"=d" (fnp));
		fnp();
		exit(1);
	}

Compile it, run it, and it should say

	Copy protected: ok

Now, try to "strace" it, or debug it with gdb, and see if you can repeat
the behaviour.

Roland? Think of it as a challenge,

		Linus

Newsgroups: fa.linux.kernel
From: Linus Torvalds <torvalds@osdl.org>
Subject: Re: ptrace single-stepping change breaks Wine
Original-Message-ID: <Pine.LNX.4.58.0411211707280.20993@ppc970.osdl.org>
Date: Mon, 22 Nov 2004 01:17:26 GMT
Message-ID: <fa.isnhnga.17ks8bq@ifi.uio.no>

On Sun, 21 Nov 2004, Davide Libenzi wrote:
>
> You know you're sick, don't you? Making traps inc's to get you in the
> correct opcode to move function in edx->fnp, is indeed fruit of a sick
> mind :=)

I prefer "creative" over "sick". It's so much less judgemental.

I thought it was a fun way to illustrate the issue, and I'm sure somebody
had fun trying to figure out what the thing did.

I could have _obfuscated_ the thing if I had wanted to make it hard to
follow, but instead I tried to make it as obvious as possible so that it's
quite clear from reading the code what it actually does.

But imagine hitting that thing without seeing the source code. NOT a lot
of fun to debug, even if the debugger _worked_ on it.

		Linus

Newsgroups: fa.linux.kernel
From: Linus Torvalds <torvalds@osdl.org>
Subject: Re: ptrace single-stepping change breaks Wine
Original-Message-ID: <Pine.LNX.4.58.0411211703160.20993@ppc970.osdl.org>
Date: Mon, 22 Nov 2004 01:15:09 GMT
Message-ID: <fa.is7dng6.16408bu@ifi.uio.no>

On Mon, 22 Nov 2004, Andreas Schwab wrote:
>
> Linus Torvalds <torvalds@osdl.org> writes:
>
> > Now, try to "strace" it, or debug it with gdb, and see if you can repeat
> > the behaviour.
>
> You'll always have hard time repeating that under strace or gdb, since a
> debugger uses SIGTRAP for it's own purpose and does not pass it to the
> program.

Sure. But "hard time" and "impossible" are two different things.

It _should_ be perfectly easy to debug this, by using

	signal SIGTRAP

instead of "continue" when you get a SIGTRAP that wasn't due to anything
you did.

But try it. It doesn't work. Why? Because the kernel will have cleared TF
on the signal stack, so even when you do a "signal SIGTRAP", it will only
run the trap handler _once_, even though it should run it three times
(once for each instruction in between the "popfl"s.

I do think this is actually a bug, although whether it's the bug that
causes problems for Wine is not clear at all. I'mm too lazy to build
and boot an older kernel, but I bet that on an older kernel you _can_ do
"signal SIGTRAP" three times, and it will work correctly. That would
indeed make this a regression.

		Linus

Newsgroups: fa.linux.kernel
From: Linus Torvalds <torvalds@osdl.org>
Subject: Re: ptrace single-stepping change breaks Wine
Original-Message-ID: <Pine.LNX.4.58.0411212022510.20993@ppc970.osdl.org>
Date: Mon, 22 Nov 2004 04:31:41 GMT
Message-ID: <fa.iumlo05.15k08rv@ifi.uio.no>

On Sun, 21 Nov 2004, Davide Libenzi wrote:
>
> So it seems it did not work even before, the gdb-SIGTRAP stepping. In
> 2.6.8 I get a straight segfault just for running it.

Ok, that at least means it's not a regression, although it may be that the
altered behaviour is enough to make some program work/not-work depending
on exactly what it is testing. My example is certainly not the only way to
try to mess up a debugger.

I'm by no means 100% sure that we should encourage the kind of programming
"skills" I showed with that example program, so in that sense this may not
be worth worrying about. That said, I do hate the notion of having
programs that are basically undebuggable, so from a QoI standpoint I'd
really like to say that you can run my horrid little program under the
debugger and see it work...

		Linus

Newsgroups: fa.linux.kernel
From: Linus Torvalds <torvalds@osdl.org>
Subject: Re: ptrace single-stepping change breaks Wine
Original-Message-ID: <Pine.LNX.4.58.0411212212530.20993@ppc970.osdl.org>
Date: Mon, 22 Nov 2004 06:27:18 GMT
Message-ID: <fa.iumtno5.15k083v@ifi.uio.no>

On Sun, 21 Nov 2004, Linus Torvalds wrote:
>
> I'm by no means 100% sure that we should encourage the kind of programming
> "skills" I showed with that example program, so in that sense this may not
> be worth worrying about. That said, I do hate the notion of having
> programs that are basically undebuggable, so from a QoI standpoint I'd
> really like to say that you can run my horrid little program under the
> debugger and see it work...

Ok, how about this patch?

It does basically two things:

 - it makes the x86 version of ptrace be a lot more careful about the TF
   bit in eflags, and in particular it never touches it _unless_ the
   tracer has explicitly asked for it (ie we set TF only when doing a
   PTRACE_SINGESTEP, and we clear it only when it has been set by us, not
   if it has been set by the program itself).

   This patch also cleans up the codepaths by doing all the common stuff
   in set_singlestep()/clear_singlestep().

 - It clarifies signal handling, and makes it clear that we always push
   the full eflags onto the signal stack, _except_ if the TF bit was set
   by an external ptrace user, in which case we hide it so that the tracee
   doesn't see it when it looks at its stack contents.

   It also adds a few comments, and makes it clear that the signal handler
   itself is always set up with TF _clear_. But if we were single-stepped
   into it, we will have notified the debugger, so the debugger obviously
   can (and often will) decide to continue single-stepping.

IMHO, this is a nice cleanup, and it also means that I can actually debug
my "program from hell":

	[torvalds@evo ~]$ gdb ./a.out
	GNU gdb Red Hat Linux (6.1post-1.20040607.41rh)
	Copyright 2004 Free Software Foundation, Inc.
	GDB is free software, covered by the GNU General Public License, and you are
	welcome to change it and/or distribute copies of it under certain conditions.
	Type "show copying" to see the conditions.
	There is absolutely no warranty for GDB.  Type "show warranty" for details.
	This GDB was configured as "i386-redhat-linux-gnu"...(no debugging symbols
	found)...Using host libthread_db library "/lib/tls/libthread_db.so.1".

	(gdb) run
	Starting program: /home/torvalds/a.out
	Reading symbols from shared object read from target memory...(no debugging
	symbols found)...done.
	Loaded system supplied DSO at 0xffffe000
	(no debugging symbols found)...(no debugging symbols found)...
	Program received signal SIGTRAP, Trace/breakpoint trap.
	0x08048480 in main ()
	(gdb) signal SIGTRAP
	Continuing with signal SIGTRAP.

	Program received signal SIGTRAP, Trace/breakpoint trap.
	0x08048487 in main ()
	(gdb) signal SIGTRAP
	Continuing with signal SIGTRAP.

	Program received signal SIGTRAP, Trace/breakpoint trap.
	0x08048488 in smc ()
	(gdb) signal SIGTRAP
	Continuing with signal SIGTRAP.
	Copy protected: ok

	Program exited with code 01.
	(gdb)

which I think is a sign that this patch actually fixes ptrace.

Does this help with wine? I dunno. Maybe some wine people can comment..

Roland, mind take a look? I assume you have some gdb test-suite that you
use to test the things?

		Linus

----

===== arch/i386/kernel/ptrace.c 1.27 vs edited =====
--- 1.27/arch/i386/kernel/ptrace.c	2004-11-07 18:10:34 -08:00
+++ edited/arch/i386/kernel/ptrace.c	2004-11-21 21:34:58 -08:00
@@ -138,6 +138,28 @@
 	return retval;
 }

+static void set_singlestep(struct task_struct *child)
+{
+	long eflags;
+
+	set_tsk_thread_flag(child, TIF_SINGLESTEP);
+	eflags = get_stack_long(child, EFL_OFFSET);
+	put_stack_long(child, EFL_OFFSET, eflags | TRAP_FLAG);
+	child->ptrace |= PT_DTRACE;
+}
+
+static void clear_singlestep(struct task_struct *child)
+{
+	if (child->ptrace & PT_DTRACE) {
+		long eflags;
+
+		clear_tsk_thread_flag(child, TIF_SINGLESTEP);
+		eflags = get_stack_long(child, EFL_OFFSET);
+		put_stack_long(child, EFL_OFFSET, eflags & ~TRAP_FLAG);
+		child->ptrace &= ~PT_DTRACE;
+	}
+}
+
 /*
  * Called by kernel/ptrace.c when detaching..
  *
@@ -145,11 +167,7 @@
  */
 void ptrace_disable(struct task_struct *child)
 {
-	long tmp;

-	clear_tsk_thread_flag(child, TIF_SINGLESTEP);
-	tmp = get_stack_long(child, EFL_OFFSET) & ~TRAP_FLAG;
-	put_stack_long(child, EFL_OFFSET, tmp);
+	clear_singlestep(child);
 }

 /*
@@ -388,10 +406,8 @@
 		  }
 		  break;

-	case PTRACE_SYSCALL: /* continue and stop at next (return from) syscall */
-	case PTRACE_CONT: { /* restart after signal. */
-		long tmp;

+	case PTRACE_SYSCALL:	/* continue and stop at next (return from) syscall */
+	case PTRACE_CONT:	/* restart after signal. */
 		ret = -EIO;
 		if ((unsigned long) data > _NSIG)
 			break;
@@ -401,56 +417,39 @@
 		else {
 			clear_tsk_thread_flag(child, TIF_SYSCALL_TRACE);
 		}
-		clear_tsk_thread_flag(child, TIF_SINGLESTEP);
 		child->exit_code = data;
-	/* make sure the single step bit is not set. */
-		tmp = get_stack_long(child, EFL_OFFSET) & ~TRAP_FLAG;
-		put_stack_long(child, EFL_OFFSET,tmp);
+		/* make sure the single step bit is not set. */
+		clear_singlestep(child);
 		wake_up_process(child);
 		ret = 0;
 		break;
-	}

 /*
  * make the child exit.  Best I can do is send it a sigkill.
  * perhaps it should be put in the status that it wants to
  * exit.
  */
-	case PTRACE_KILL: {
-		long tmp;

+	case PTRACE_KILL:
 		ret = 0;
 		if (child->exit_state == EXIT_ZOMBIE)	/* already dead */
 			break;
 		child->exit_code = SIGKILL;
-		clear_tsk_thread_flag(child, TIF_SINGLESTEP);
 		/* make sure the single step bit is not set. */
-		tmp = get_stack_long(child, EFL_OFFSET) & ~TRAP_FLAG;
-		put_stack_long(child, EFL_OFFSET, tmp);
+		clear_singlestep(child);
 		wake_up_process(child);
 		break;
-	}

-	case PTRACE_SINGLESTEP: {  /* set the trap flag. */
-		long tmp;

+	case PTRACE_SINGLESTEP:	/* set the trap flag. */
 		ret = -EIO;
 		if ((unsigned long) data > _NSIG)
 			break;
 		clear_tsk_thread_flag(child, TIF_SYSCALL_TRACE);
-		if ((child->ptrace & PT_DTRACE) == 0) {
-			/* Spurious delayed TF traps may occur */
-			child->ptrace |= PT_DTRACE;
-		}
-		tmp = get_stack_long(child, EFL_OFFSET) | TRAP_FLAG;
-		put_stack_long(child, EFL_OFFSET, tmp);
-		set_tsk_thread_flag(child, TIF_SINGLESTEP);
+		set_singlestep(child);
 		child->exit_code = data;
 		/* give it a chance to run. */
 		wake_up_process(child);
 		ret = 0;
 		break;
-	}

 	case PTRACE_DETACH:
 		/* detach a process that was attached. */
===== arch/i386/kernel/signal.c 1.48 vs edited =====
--- 1.48/arch/i386/kernel/signal.c	2004-11-15 00:56:24 -08:00
+++ edited/arch/i386/kernel/signal.c	2004-11-21 21:33:21 -08:00
@@ -292,10 +292,15 @@
 	err |= __put_user(current->thread.error_code, &sc->err);
 	err |= __put_user(regs->eip, &sc->eip);
 	err |= __put_user(regs->xcs, (unsigned int __user *)&sc->cs);
+
+	/*
+	 * Iff TF was set because the program is being single-stepped by a
+	 * debugger, don't save that information on the signal stack.. We
+	 * don't want debugging to change state.
+	 */
 	eflags = regs->eflags;
-	if (current->ptrace & PT_PTRACED) {
+	if (current->ptrace & PT_DTRACE)
 		eflags &= ~TF_MASK;
-	}
 	err |= __put_user(eflags, &sc->eflags);
 	err |= __put_user(regs->esp, &sc->esp_at_signal);
 	err |= __put_user(regs->xss, (unsigned int __user *)&sc->ss);
@@ -412,12 +417,17 @@
 	regs->xes = __USER_DS;
 	regs->xss = __USER_DS;
 	regs->xcs = __USER_CS;
+
+	/*
+	 * Clear TF when entering the signal handler, but
+	 * notify any tracer that was single-stepping it.
+	 * The tracer may want to single-step inside the
+	 * handler too.
+	 */
 	if (regs->eflags & TF_MASK) {
-		if ((current->ptrace & (PT_PTRACED | PT_DTRACE)) == (PT_PTRACED | PT_DTRACE)) {
+		regs->eflags &= ~TF_MASK;
+		if (current->ptrace & PT_DTRACE)
 			ptrace_notify(SIGTRAP);
-		} else {
-			regs->eflags &= ~TF_MASK;
-		}
 	}

 #if DEBUG_SIG
@@ -502,12 +512,17 @@
 	regs->xes = __USER_DS;
 	regs->xss = __USER_DS;
 	regs->xcs = __USER_CS;
+
+	/*
+	 * Clear TF when entering the signal handler, but
+	 * notify any tracer that was single-stepping it.
+	 * The tracer may want to single-step inside the
+	 * handler too.
+	 */
 	if (regs->eflags & TF_MASK) {
-		if (current->ptrace & PT_PTRACED) {
+		regs->eflags &= ~TF_MASK;
+		if (current->ptrace & PT_DTRACE)
 			ptrace_notify(SIGTRAP);
-		} else {
-			regs->eflags &= ~TF_MASK;
-		}
 	}

 #if DEBUG_SIG

Newsgroups: fa.linux.kernel
From: Linus Torvalds <torvalds@osdl.org>
Subject: Re: ptrace single-stepping change breaks Wine
Original-Message-ID: <Pine.LNX.4.58.0411220823581.20993@ppc970.osdl.org>
Date: Mon, 22 Nov 2004 16:52:59 GMT
Message-ID: <fa.itnjo87.14k28jt@ifi.uio.no>

On Mon, 22 Nov 2004, Andreas Schwab wrote:

> Linus Torvalds <torvalds@osdl.org> writes:
>
> > IMHO, this is a nice cleanup, and it also means that I can actually debug
> > my "program from hell":
>
> Does it also work when trying to single step over it?  I guess all bets
> are off then.

If you single-step over the "popfl", then you need to generate the
SIGTRAP's by hand too. IOW, it's _possible_ to emulate the behaviour from
within the debugger, but it gets really really nasty very quickly.

I think the nastyness in that case is at least acceptable, since if you
single-step, you actually _see_ what is happening, and thus you have a
chance in hell of figuring it out. Practical? No. But debuggable at least
in theory, which it really wasn't before.

		Linus

Newsgroups: fa.linux.kernel
From: Linus Torvalds <torvalds@osdl.org>
Subject: Re: ptrace single-stepping change breaks Wine
Original-Message-ID: <Pine.LNX.4.58.0411221300460.20993@ppc970.osdl.org>
Date: Mon, 22 Nov 2004 21:19:01 GMT
Message-ID: <fa.itn5ng4.14k88bu@ifi.uio.no>

On Mon, 22 Nov 2004, Eric Pouech wrote:
>
> For the linux folks, here a small comparison of what happens in the working
> (old) case and in the non-working (new) case:
>
> In both case
>
> - Wine gets a first SIGTRAP (in it's sig_trap handler)
> 	+ Wine converts it into a Windows exception (w-exception in short).
> 	  This includes creating a context for the generic CPU registers
> 	+ This w-exception is sent to the w-exception handler the program
> 	  installed (this one can modifiy the all registers)
> 		o this handler touches one of the i386 debug registers
> 	+ as we need to update the debug registers values (and we don't do in
> 	  the signal handler return), this task is delegated to the Wine server
> 	  (our central process, which is in charge of synchronisation...)
> 		> the Wine server ptrace-attach:es to the process which handled
> 		  the SIGTRAP.
> 		> Wine server wait4:s on the SIGSTOP (after ptrace:attach)
> 		> modify (with ptrace) the debug registers
> 		> and resumes excution (ptrace: cont)
> 	+ wine terminates the sig trap handler and resumes the execution with
> 	  the modified basic registers (from the saved context), and the
> 	  modified debug registers (from the Wine server round trip)
> - a second sig trap is generated
> 	> since the wine server is still ptrace:attached, it gets the signal.
>
> What differs then in both execution:
> - in the working case, the sig trap handler is called on the client side,
> whereas it's never called in the non-working case. We do have a couple of
> protection (to avoid some misbehaving apps), but none of them get triggered. So
> it seems like the trap handler is not called (ugh).

This actually implies that the current -bk tree with my latest patch may
actually fix it.

One of the things that 2.6.9 did wrong was exactly that it cleared TF too
much in the ptrace interface. The current development tree is a lot more
careful about that, and it fixes the horrid test-case that I used to debug
it. The test-case (when run under gdb) actually does something similar to
what Wine appears to do.

> - in Windows trap handling, the TF is explictly reset before calling the windows
> exception handler (which is what Wine does, before calling the window exception
> handler). Of course the handler can set it back if it wants to continue single
> stepping.

TF will be still set in Linux when ptrace gets access, but the ptracer can
choose to clear it with PTRACE_PEEKUSR/PTRACE_POKEUSR (or with
PTRACE_GETREGS/SETREGS). I assume you already do that, since I think that
has been true forever (although maybe you don't: PTRACE_CONTINUE used to
unconditionally clear TF, so it may be that Wine may need some minor
modification to not do that - but the good news is that mod should be
backwards-compatible, so it should be pretty easy).

I actually broke down and am downloading the latest source tree of wine,
let's see if I can find the place you do this.

		Linus