Re: PT_DTRACE && uml
From: Oleg Nesterov
Date: Sun Apr 26 2009 - 18:14:19 EST
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, ¤t->thread.regs);
if (error == 0) {
- task_lock(current);
- current->ptrace &= ~PT_DTRACE;
+ clear_thread_flag(TIF_SINGLESTEP);
#ifdef SUBARCH_EXECVE1
SUBARCH_EXECVE1(¤t->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(¤t->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;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/