Messages in this thread Patch in this message | | | Date | Sun, 11 Sep 2011 20:14:42 +0200 | From | Oleg Nesterov <> | Subject | Re: [PATCH v2] Make PTRACE_SEIZE set ptrace options specified in 'data' parameter |
| |
Hello,
On 09/11, Tejun Heo wrote: > > On Thu, Sep 08, 2011 at 08:17:47PM +0200, Oleg Nesterov wrote: > > > There are places which assume ->ptrace is protected by siglock. > > > > Really? Once again, I agree. But _afaics_ currently this is not strictly > > needed. PT_PTRACED/PT_SEIZED should not go away under ->siglock, yes, but > > it seems that it is fine to set them. > > Hmmm.... I haven't checked each direction. Maybe we don't strictly > need it on setting it but I definitely was assuming that ->ptrace was > protected by siglock while coding recent ptrace changes. Can't the > following happen? > > * ptracer seizes child, sets PT_PTRACED and then OR PT_SEIZED. > > * ptracee enters signal delivery path with group stop scheduled. > PT_PTRACED is visible and group stop is transformed into > JOBCTL_TRAP_STOP. > > * ptracee enters do_jobct_trap(). PT_SEIZED is still not visible and > it takes the path for the old behavior. > > * ptracer SEIZE'd and expects PTRACE_EVENT_STOP but it gets the old > no-siginfo trap.
Heh ;) Please look at http://marc.info/?l=linux-kernel&m=131541614232539
> @@ -263,7 +267,7 @@ static int ptrace_attach(struct task_struct *task, long request, > if (task->ptrace) > goto unlock_tasklist; > > - task->ptrace = PT_PTRACED; > + task->ptrace = PT_PTRACED | (flags << PT_OPT_FLAG_SHIFT); > if (seize) > task->ptrace |= PT_SEIZED;
Hmm. Tejun, Denys, this doesn't look exactly right.
I already thought about this before, but somehow I convinced myself this is fine.
I think we should set both PT_PTRACED | PT_SEIZED "atomically", at once. Otherwise, say, the tracee can do do_jobctl_trap() in between, no? Nothing really bad can happen, but we shouldn't lose EVENT_STOP code.
Yes, we need to set them both at once.
And yes, I agree, it is better to do this under ->siglock even if currently this is not strictly necessary.
> > > and linking are protected by siglock > > > > Hmm. Could you explain this? Why do want __ptrace_link under ->siglock ? > > Because it's much simpler to assume that w/ siglock locked, everything > including ->parent is set up properly w.r.t. ->ptrace.
Well, but then we shouldn't rely on tracee's ->siglock. The tracee simply do not care about its ->ptrace_entry, only the tracer does. We need to rework the locking, yes. But we need the lock which protects the parent's list_head (currently we rely on tasklist). Yes, a single lock can't help. We already use ->cred_guard_mutex though.
This needs more thinking, but imho child->siglock is pointless here.
Oleg.
| |