Re: ptrace single-stepping change breaks Wine

[Linus Torvalds]

On Wed, 29 Dec 2004, Jesse Allen wrote:
> > 
> > So instead of removing the setting of TIF_SINGLESTEP in set_singlestep(),
> > can you test whether removing the _testing_ of it in do_syscall_trace()
> > makes things happier for you? Hmm?
> > 
> 
> Yes, doing that does work.  But I still have to remove the conditional
> TF clear.  Here's the diff now to show you.

Ok. That's a good piece of information. 

I don't actually understand why do_syscall_trace() does that 
TIF_SINGLESTEP test in the first place, since the ptrace_notify() should 
happen as part of the _normal_ TIF_SINGLESTEP handling, afaiks. No 
apparent need to do it in syscall tracing, and do_syscall_trace() does 
that bogus fake signal sending.

Hmm.. That TF_SINGLESTEP test was added by Davide Libenzi in August, and I
think Davide had some test for exactly this. Davide? Can you recall why
you did this in the system call tracing code, rather than depend on the
regular TIF_SINGLESTEP handling in arch/i386/kernel/signal.c:
do_notify_resume()?

(Jesse's patch re-appended here in-line for Davide's "reading pleasure").

That still leaves the problem of the clearing of TF_MASK. I _appears_ that 
the problem is that TF was set both by Wine doing a PTRACE_SINGLESTEP 
(since PT_DTRACE is set) _and_ the application having TF enabled in eflags 
from before (since it seems to want to keep it enabled).

How about something like the attachment for the TF_MASK issue (replacing
your "don't clear" code)? The comments should make the intention pretty
obvious, but I have equally obviously not actually _tested_ any of this..

		Linus

---
--- linux/arch/i386/kernel/ptrace.c	2004-12-29 14:10:34.000000000 -0700
+++ linux-mod/arch/i386/kernel/ptrace.c	2004-12-29 14:22:33.000000000 -0700
@@ -568,8 +568,7 @@
 			audit_syscall_exit(current, regs->eax);
 	}
 
-	if (!test_thread_flag(TIF_SYSCALL_TRACE) &&
-	    !test_thread_flag(TIF_SINGLESTEP))
+	if (!test_thread_flag(TIF_SYSCALL_TRACE))
 		return;
 	if (!(current->ptrace & PT_PTRACED))
 		return;
--- linux/arch/i386/kernel/signal.c	2004-12-29 14:10:34.000000000 -0700
+++ linux-mod/arch/i386/kernel/signal.c	2004-12-29 14:23:04.000000000 -0700
@@ -299,8 +299,8 @@
 	 * don't want debugging to change state.
 	 */
 	eflags = regs->eflags;
-	if (current->ptrace & PT_DTRACE)
-		eflags &= ~TF_MASK;
+//	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);===== arch/i386/kernel/ptrace.c 1.28 vs edited =====
--- 1.28/arch/i386/kernel/ptrace.c	2004-11-22 09:44:52 -08:00
+++ edited/arch/i386/kernel/ptrace.c	2004-12-29 16:42:04 -08:00
@@ -142,18 +142,31 @@
 {
 	long eflags;
 
+	/*
+	 * Always set TIF_SINGLESTEP - this guarantees that 
+	 * we single-step system calls etc.. 
+	 */
 	set_tsk_thread_flag(child, TIF_SINGLESTEP);
+
+	/*
+	 * If TF was already set, don't do anything else
+	 */
 	eflags = get_stack_long(child, EFL_OFFSET);
+	if (flags & TRAP_FLAG)
+		return;
 	put_stack_long(child, EFL_OFFSET, eflags | TRAP_FLAG);
 	child->ptrace |= PT_DTRACE;
 }
 
 static void clear_singlestep(struct task_struct *child)
 {
+	/* Always clear TIF_SINGLESTEP... */
+	clear_tsk_thread_flag(child, TIF_SINGLESTEP);
+
+	/* But touch TF only if it was set by us.. */
 	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;