On 04/16, Roland McGrath wrote:
>
> But even that is a lot of hair for the incremental patches in the first
> several stages, I think. So just never deallocate it, and:
>
> static inline int task_ptrace(struct task_struct *task)
> {
> return unlikely(task->ptrace_child) ? task->ptrace_child->flags
> : 0;
> }
OK, agreed.
Except... personally I dislike ->flags, it is not greppable.
> > even ptracer can't safely access
> > child->ptrace_child->ptrace_flags. Once sys_ptrace() drops tasklist,
> > child's sub-thread can do exec, and de_thread() can untrace the child
> > and free ->ptrace_child.
>
> This kind of trouble is why going to dynamic allocation should start
> with never-deallocate (i.e. until release_task or something).
Yes, except release_task() doesn't work. I think it should be freed
by __put_task_struct(). Proc can read ->ptrace_child and race with
release_task().
> > // was task_struct->parent
> > struct task_struct *ptrace_parent;
>
> struct ptrace_task.tracer, please. :-)
OK. I'll send the first patch (or 2 patches) which introduces ptrace_task
and moves ->ptrace into it tomorrow. The patch is simple, but intrusive.
Now the questions. First of all, what does task_lock() currently mean
from the ptrace pov ?
Afaics ptrace_attach() needs this lock only to pin ->mm, no other other
reasons. ptrace_traceme() doesn't need it at all.
The comment above tracehook_unsafe_exec() says "Called with task_lock()",
this is wrong, check_unsafe_exec() doesn't take task_lock().
The only place which believes task_lock() can help is sys_execve() on
some arches, the code clears PT_DTRACE under task_lock(). Why, and what
PT_DTRACE actually means?
I don't understand PT_DTRACE, but looking at the code I assume it can
only be set when the task is ptraced. Right?
arch/m68k/kernel/traps.c:trap_c() does current->ptrace |= PT_DTRACE.
Again, can I assume this can only happen when the task is already
traced? Otherwise, we have a problem with allocationg of ->ptrace_task
(and this is racy with ptrace_attach of course).
Perhaps it makes sense to do a little cleanup first, introduce
ptrace_clear_dtrace(void)
{
if (unlikely(current->ptrace & PT_DTRACE))
current->ptrace &= ~PT_DTRACE;
}
This can't race with the tracer, it never changes ->ptrace flags until
the tracee is TASK_TRACED.
The last question. ptrace_attach() does
task->ptrace |= PT_PTRACED;
Why can't we use the plain "=" instead of "|=" ? This looks as if ->ptrace
can be nonzero even if the task is not traced. But I assume this is not
possible? And any code which does "if (p->ptrace & PT_TRACED)" could just
do "if (p->ptrace)", right?
Oleg.
