On 08/18, Oleg Nesterov wrote:
>
> In short. Perhaps I missed something, but I think it would be "safer" if
> ptrace_attach() did utrace_set_events() after setting PT_PTRACED, but this
> is not possible because attach sends SIGSTOP. That is why initially I was
> going to implement attach this way:
>
> utrace_attach_task(UTRACE_ATTACH_CREATE);
>
> write_lock(tasklist);
> if (child->ptrace)
> abort();
> child->ptrace = PT_PTRACED;
> write_unlock(tasklist);
>
> utrace_set_events(...);
> send_sig_info(SIGSTOP);
>
> But then decided to revert this nightmire and keep the code simple.
Hmm. Strange, but I didn't think of another option...
ptrace_attach_task() can check ->ptrace == 0 ! Something like
static int ptrace_attach_task(struct task_struct *tracee)
{
struct utrace_engine *engine;
unsigned long events;
engine = utrace_attach_task(tracee, UTRACE_ATTACH_CREATE |
UTRACE_ATTACH_EXCLUSIVE
|
UTRACE_ATTACH_MATCH_OPS,
&ptrace_utrace_ops,
NULL);
if (unlikely(IS_ERR(engine))) {
int err = PTR_ERR(engine);
if (err != -ESRCH && err != -ERESTARTNOINTR)
err = -EPERM ;
return err;
}
// !!!!!!!!!!!!!!!!!!!!!!!!
// We can rely on UTRACE_ATTACH_EXCLUSIVE and check ->ptrace
lockless
// !!!!!!!!!!!!!!!!!!!!!!!!
if (unlikely(tracee->ptrace)) {
utrace_control(engine, UTRACE_DETACH);
utrace_engine_put(engine);
return -EPERM;
}
/*
* We need QUIESCE for resume handling, CLONE to check
* for CLONE_PTRACE, other events are always reported.
*/
events = UTRACE_EVENT(QUIESCE) | UTRACE_EVENT(CLONE) |
UTRACE_EVENT(EXEC) | UTRACE_EVENT_SIGNAL_ALL;
/*
* It can fail only if the tracee is dead, the caller
* must notice this before setting PT_PTRACED.
*/
utrace_set_events(tracee, engine, events);
utrace_engine_put(engine);
return 0;
}
Then we change ptrace_attach:
write_lock_irq(&tasklist_lock);
retval = -EPERM;
if (unlikely(task->exit_state))
goto unlock_tasklist;
- if (task->ptrace)
- goto unlock_tasklist;
+ BUG_ON(task->ptrace);
....
unlock_tasklist:
write_unlock_irq(&tasklist_lock);
- if (retval)
- ptrace_abort_attach(task);
ptrace_traceme() can be changed the same way (except it should take
parent's PF_EXITING into account).
I'll recheck my thinking with a fresh head. But currently this looks
like a good change to me. Even if (most probably) I was wrong and we
shouldn't really worry about ptrace_abort_attach()'s subtleness.
What do you think?
Oleg.
