Re: PT_DTRACE && uml

Re: PT_DTRACE && uml

[Roland McGrath]

> (cc Jeff Dike)

I've also CC'd the UML hackers' mailing list.

> So, arch/um/ seems to be the only user of PT_DTRACE.
> 
> I do not understand this code at all. It looks as if we can just
> s/PT_DTRACE/TIF_SINGLESTEP/.
> 
> But it can't be that simple?

I don't really understand off hand how UML is using PT_DTRACE either.
But clearly it (or any real arch) that needs a flag for an arch purpose
can use a TIF_* bit instead of touching ->ptrace, and should do that.


Thanks,
Roland

Re: PT_DTRACE && uml

[Oleg Nesterov]

On 04/23, Jeff Dike wrote:
>
> On Thu, Apr 23, 2009 at 12:17:26AM +0200, Oleg Nesterov wrote:
> > (cc Jeff Dike)
> >
> > So, arch/um/ seems to be the only user of PT_DTRACE.
> >
> > I do not understand this code at all. It looks as if we can just
> > s/PT_DTRACE/TIF_SINGLESTEP/.
> >
> > But it can't be that simple?
>
> It looks like it.

OK. Please look at the patch below. It is literally s/PT_DTRACE/TIF_SINGLESTEP/
and nothing more.

Except, it removes task_lock() arch/um/kernel/exec.c:execve1(). We don't
need this lock to clear bit (actually we could clear PT_DTRACE without
this lock too). But what about SUBARCH_EXECVE1(), does it need this lock ?
grep finds nothing about SUBARCH_EXECVE1.

> The one odd part is the use in the signal delivery
> code.  That looks like a bug to me.

Cough. Where?

arch/um/sys-i386/signal.c:setup_signal_stack_sc() and setup_signal_stack_si()
looks suspicious. Why do they play with single-step? And why arch/um/sys-x86_64/
does not?

It seems to me we should kill this code, and change handle_signal() to call
tracehook_signal_handler(test_thread_flag(TIF_SINGLESTEP)).

UML hooks ptrace_disable() which calls set_singlestepping(0), so we can't
leak TIF_SINGLESTEP after ptrace_detach(). Unfortunately, if the tracer
dies nobody will clear this flag. But currently this is common problem.

Do you see other problems with this patch? (uncompiled, untested).

Oleg.


--- PTRACE/arch/um/include/asm/thread_info.h~DT_5_um	2009-04-06 00:03:36.000000000 +0200
+++ PTRACE/arch/um/include/asm/thread_info.h	2009-04-26 21:14:05.000000000 +0200
@@ -69,6 +69,7 @@ static inline struct thread_info *curren
 #define TIF_MEMDIE	 	5
 #define TIF_SYSCALL_AUDIT	6
 #define TIF_RESTORE_SIGMASK	7
+#define TIF_SINGLESTEP		8
 #define TIF_FREEZE		16	/* is freezing for suspend */
 
 #define _TIF_SYSCALL_TRACE	(1 << TIF_SYSCALL_TRACE)
@@ -78,6 +79,7 @@ static inline struct thread_info *curren
 #define _TIF_MEMDIE		(1 << TIF_MEMDIE)
 #define _TIF_SYSCALL_AUDIT	(1 << TIF_SYSCALL_AUDIT)
 #define _TIF_RESTORE_SIGMASK	(1 << TIF_RESTORE_SIGMASK)
+#define _TIF_SINGLESTEP		(1 << TIF_SINGLESTEP)
 #define _TIF_FREEZE		(1 << TIF_FREEZE)
 
 #endif
--- PTRACE/arch/um/kernel/exec.c~DT_5_um	2009-04-06 00:03:36.000000000 +0200
+++ PTRACE/arch/um/kernel/exec.c	2009-04-26 23:29:11.000000000 +0200
@@ -50,12 +50,10 @@ static long execve1(char *file, char __u
 
 	error = do_execve(file, argv, env, &current->thread.regs);
 	if (error == 0) {
-		task_lock(current);
-		current->ptrace &= ~PT_DTRACE;
+		clear_thread_flag(TIF_SINGLESTEP);
 #ifdef SUBARCH_EXECVE1
 		SUBARCH_EXECVE1(&current->thread.regs.regs);
 #endif
-		task_unlock(current);
 	}
 	return error;
 }
--- PTRACE/arch/um/kernel/process.c~DT_5_um	2009-04-06 00:03:36.000000000 +0200
+++ PTRACE/arch/um/kernel/process.c	2009-04-27 00:03:26.000000000 +0200
@@ -384,7 +384,7 @@ int singlestepping(void * t)
 {
 	struct task_struct *task = t ? t : current;
 
-	if (!(task->ptrace & PT_DTRACE))
+	if (!test_tsk_thread_flag(task, TIF_SINGLESTEP))
 		return 0;
 
 	if (task->thread.singlestep_syscall)
--- PTRACE/arch/um/kernel/ptrace.c~DT_5_um	2009-04-06 00:03:36.000000000 +0200
+++ PTRACE/arch/um/kernel/ptrace.c	2009-04-26 23:24:18.000000000 +0200
@@ -15,9 +15,9 @@
 static inline void set_singlestepping(struct task_struct *child, int on)
 {
 	if (on)
-		child->ptrace |= PT_DTRACE;
+		set_tsk_thread_flag(child, TIF_SINGLESTEP);
 	else
-		child->ptrace &= ~PT_DTRACE;
+		clear_tsk_thread_flag(child, TIF_SINGLESTEP);
 	child->thread.singlestep_syscall = 0;
 
 #ifdef SUBARCH_SET_SINGLESTEPPING
@@ -247,12 +247,11 @@ static void send_sigtrap(struct task_str
 }
 
 /*
- * XXX Check PT_DTRACE vs TIF_SINGLESTEP for singlestepping check and
- * PT_PTRACED vs TIF_SYSCALL_TRACE for syscall tracing check
+ * XXX Check PT_PTRACED vs TIF_SYSCALL_TRACE for syscall tracing check
  */
 void syscall_trace(struct uml_pt_regs *regs, int entryexit)
 {
-	int is_singlestep = (current->ptrace & PT_DTRACE) && entryexit;
+	int is_singlestep = test_thread_flag(TIF_SINGLESTEP) && entryexit;
 	int tracesysgood;
 
 	if (unlikely(current->audit_context)) {
--- PTRACE/arch/um/kernel/signal.c~DT_5_um	2009-04-06 00:03:36.000000000 +0200
+++ PTRACE/arch/um/kernel/signal.c	2009-04-26 21:56:02.000000000 +0200
@@ -138,7 +138,7 @@ static int kern_do_signal(struct pt_regs
 	 * on the host.  The tracing thread will check this flag and
 	 * PTRACE_SYSCALL if necessary.
 	 */
-	if (current->ptrace & PT_DTRACE)
+	if (test_thread_flag(TIF_SINGLESTEP))
 		current->thread.singlestep_syscall =
 			is_syscall(PT_REGS_IP(&current->thread.regs));
 
--- PTRACE/arch/um/sys-i386/signal.c~DT_5_um	2009-04-06 00:03:36.000000000 +0200
+++ PTRACE/arch/um/sys-i386/signal.c	2009-04-26 23:26:14.000000000 +0200
@@ -377,7 +377,7 @@ int setup_signal_stack_sc(unsigned long 
 	PT_REGS_EDX(regs) = (unsigned long) 0;
 	PT_REGS_ECX(regs) = (unsigned long) 0;
 
-	if ((current->ptrace & PT_DTRACE) && (current->ptrace & PT_PTRACED))
+	if (test_thread_flag(TIF_SINGLESTEP))
 		ptrace_notify(SIGTRAP);
 	return 0;
 
@@ -434,7 +434,7 @@ int setup_signal_stack_si(unsigned long 
 	PT_REGS_EDX(regs) = (unsigned long) &frame->info;
 	PT_REGS_ECX(regs) = (unsigned long) &frame->uc;
 
-	if ((current->ptrace & PT_DTRACE) && (current->ptrace & PT_PTRACED))
+	if (test_thread_flag(TIF_SINGLESTEP))
 		ptrace_notify(SIGTRAP);
 	return 0;