On 04/21, Roland McGrath wrote:
>
> [We have been on fine details here that are quite purely ptrace innards for
> a while now. I think discussion at this level of detail about this stuff
> quite far from utrace per se belongs on LKML.]
Agreed. s/utrace-devel/lkml/.
> > > The clean-up should get rid of PT_DTRACE entirely.
> >
> > Agreed. But this needs another patch...
>
> Yes, or several. It always gets fiddly when to get lots of little arch
> changes merged. The 90% that are just one-liner removal of wholly unused
> PT_DTRACE can probably go in as a single patch to Linus instead of tiny
> ones through each arch tree.
OK. I'd like to finally do at least something. Please look at the 4 patches
attached.
Unfortunately, I know nothing about these arches, so I can only rely on grep.
But it really looks like nobody except arch/um actually uses DTRACE.
There are also some strange defines in blackfin and m68k, PF_DTRACE_BIT and
PT_DTRACE_BIT, which seems to be unused too. At least I failed to find
anything related in asm. Perhaps I should learn how to cross compile.
Oleg.
On 04/23, Oleg Nesterov wrote:
>
> OK. I'd like to finally do at least something. Please look at the 4 patches
> attached.
Damn. Forgot to change the subject. These patches remove PT_DTRACE from
arch/*/ except arch/um/
Oleg.
(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?
Oleg.
On Wed, Apr 22, 2009 at 18:04, Oleg Nesterov wrote:
> On 04/21, Roland McGrath wrote:
>> [We have been on fine details here that are quite purely ptrace innards for
>> a while now. I think discussion at this level of detail about this stuff
>> quite far from utrace per se belongs on LKML.]
>
> Agreed. s/utrace-devel/lkml/.
>
>> > > The clean-up should get rid of PT_DTRACE entirely.
>> >
>> > Agreed. But this needs another patch...
>>
>> Yes, or several. It always gets fiddly when to get lots of little arch
>> changes merged. The 90% that are just one-liner removal of wholly unused
>> PT_DTRACE can probably go in as a single patch to Linus instead of tiny
>> ones through each arch tree.
>
> OK. I'd like to finally do at least something. Please look at the 4 patches
> attached.
>
> Unfortunately, I know nothing about these arches, so I can only rely on grep.
> But it really looks like nobody except arch/um actually uses DTRACE.
>
> There are also some strange defines in blackfin and m68k, PF_DTRACE_BIT and
> PT_DTRACE_BIT, which seems to be unused too. At least I failed to find
> anything related in asm. Perhaps I should learn how to cross compile.
Blackfin was based on m68knommu, so it is most likely unused by us.
the only ptrace consumers are gdb(server) and strace and neither use
it afaik.
-mike
> Unfortunately, I know nothing about these arches, so I can only rely on grep.
> But it really looks like nobody except arch/um actually uses DTRACE.
Same with me. But I do trust grep, and my knowledge that PT_DTRACE was
widely used old cruft. To be really paranoid, you can grep (painfully)
for all ->ptrace ('[>.]ptrace\>') in arch code, plus find any that use
it in asm-offsets.c, and then grep for TSK_PTRACE or whatever macro is
defined there. (Chances are high that there are no uses that you missed.)
Or you could just leave it to the arch people to pipe up if we're wrong.
The patches all look quite right and harmless to me.
Thanks,
Roland
> (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
> Blackfin was based on m68knommu, so it is most likely unused by us.
> the only ptrace consumers are gdb(server) and strace and neither use
> it afaik.
These are not user-visible bits, just implementation cruft in the kernel.
If no arch code in the kernel really does anything with the bit, that is
all there is to it.
Thanks,
Roland
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. The one odd part is the use in the signal delivery
code. That looks like a bug to me.
Jeff
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;
On 04/27, Oleg Nesterov wrote:
>
> Do you see other problems with this patch? (uncompiled, untested).
dup_task_struct()->setup_thread_stack() copies parent's ti->flags.
Why? Which flags should be actually copied? I must have missed
something, but whats wrong with the patch below?
OK, it is wrong. On x86 we should at least copy TIF_IA32. But
why should we copy, say, TIF_DEBUG?
Actually, I don't understand why don't we use TS_IA32 instead of
TIF_IA32. Only current can change this flag, perhaps it makes sense
to move it in thread_info->status.
copy_process()->clear_tsk_thread_flag(TIF_SIGPENDING) looks unneeded
in any case...
Oleg.
--- kernel/fork.c
+++ kernel/fork.c
@@ -241,6 +241,7 @@ static struct task_struct *dup_task_stru
goto out;
setup_thread_stack(tsk, orig);
+ ti->flags = 0;
stackend = end_of_stack(tsk);
*stackend = STACK_END_MAGIC; /* for overflow detection */
@@ -1027,7 +1028,6 @@ static struct task_struct *copy_process(
p->vfork_done = NULL;
spin_lock_init(&p->alloc_lock);
- clear_tsk_thread_flag(p, TIF_SIGPENDING);
init_sigpending(&p->pending);
p->utime = cputime_zero;
@@ -1163,14 +1163,6 @@ static struct task_struct *copy_process(
if ((clone_flags & (CLONE_VM|CLONE_VFORK)) == CLONE_VM)
p->sas_ss_sp = p->sas_ss_size = 0;
- /*
- * Syscall tracing should be turned off in the child regardless
- * of CLONE_PTRACE.
- */
- clear_tsk_thread_flag(p, TIF_SYSCALL_TRACE);
-#ifdef TIF_SYSCALL_EMU
- clear_tsk_thread_flag(p, TIF_SYSCALL_EMU);
-#endif
clear_all_latency_tracing(p);
/* ok, now we should be set up.. */
> dup_task_struct()->setup_thread_stack() copies parent's ti->flags.
>
> Why? Which flags should be actually copied? I must have missed
> something, but whats wrong with the patch below?
I suspect it just evolved that way as the default case of how
copy_process() is written: copy whole structs, and then fix them up.
Almost all the details of struct thread_info are arch-specific, so
whether copy-and-fix or start-from-zero is better really has to be
decided by each arch maintainer.
Your next two questions are not about UML, but about arch/x86 code.
Those should be directed to the x86 maintainers, whom you did not CC.
> OK, it is wrong. On x86 we should at least copy TIF_IA32. But
> why should we copy, say, TIF_DEBUG?
TIF_DEBUG is set when task->thread.debugregN fields have interesting
values. The semantics today are that those values are copied, so
copying TIF_DEBUG too makes the child's context switches do what they
should.
This illustrates the general point: since the overall policy defaults
to copying, then what actually keeps the code simpler overall is to
have the same default at each substructure (and so it is for
thread_struct and thread_info). If some things should be cleared,
they are cleared explicitly. Hence, to clear TIF_DEBUG one would have
to do it on purpose, and would be required to think about what
TIF_DEBUG means and that clearing thread->debugregN goes along with
it. (And at this point, one would then realize that one can't do it
without changing the user-visible semantics.)
> Actually, I don't understand why don't we use TS_IA32 instead of
> TIF_IA32. Only current can change this flag, perhaps it makes sense
> to move it in thread_info->status.
However, it is tested by other threads asynchronously. The stated use
of TS_* flags is things only tested by current or when current is
stopped (e.g. TS_COMPAT). In other uses like TASK_SIZE_OF(),
get_gate_vma(), etc., that might not be so. It might be so on x86
that there can never be any modification to ti->status that could
momentarily clear some bit, but that is not formally said to be
required.
> copy_process()->clear_tsk_thread_flag(TIF_SIGPENDING) looks unneeded
> in any case...
It goes along with init_sigpending(). But it's actually potentially
wrong in case of shared_pending signals. Do recalc_sigpending_tsk()
if anything. As I think you intend to point out, it is always pretty
harmless to leave TIF_SIGPENDING set. (get_signal_to_deliver will
just figure it out.) So I think that should just be removed,
independent of your other questions.
Thanks,
Roland