lkml.org 
[lkml]   [2011]   [Sep]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [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.



\
 
 \ /
  Last update: 2011-09-11 20:21    [W:0.079 / U:0.580 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site