ptracee data structures cleanup

[Oleg Nesterov]

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.