Messages in this thread | | | Date | Fri, 2 Sep 2011 10:15:50 -0400 | From | Ben Blum <> | Subject | Re: + cgroups-more-safe-tasklist-locking-in-cgroup_attach_proc.patch added to -mm tree |
| |
On Fri, Sep 02, 2011 at 04:00:15PM +0200, Oleg Nesterov wrote: > Forgot to mention, sorry... > > That said, I believe the patch is correct and should fix the problem.
Thanks!
But I don't think the check becomes pointless? If a sub-thread execs right before read_lock(&tasklist_lock) (but after the find_task_by_vpid in attach_task_by_pid), that causes the case that the comment refers to.
-- Ben
> > On 09/02, Oleg Nesterov wrote: > > > > > From: Ben Blum <bblum@andrew.cmu.edu> > > > > > > Fix unstable tasklist locking in cgroup_attach_proc. > > > > > > According to this thread - https://lkml.org/lkml/2011/7/27/243 - RCU is > > > not sufficient to guarantee the tasklist is stable w.r.t. de_thread and > > > exit. Taking tasklist_lock for reading, instead of rcu_read_lock, ensures > > > proper exclusion. > > > > I still think we should avoid the global lock. > > > > In any case, with tasklist or siglock, > > > > > - rcu_read_lock(); > > > + read_lock(&tasklist_lock); > > > if (!thread_group_leader(leader)) { > > > /* > > > * a race with de_thread from another thread's exec() may strip > > > @@ -2036,7 +2036,7 @@ int cgroup_attach_proc(struct cgroup *cg > > > * throw this task away and try again (from cgroup_procs_write); > > > * this is "double-double-toil-and-trouble-check locking". > > > */ > > > - rcu_read_unlock(); > > > + read_unlock(&tasklist_lock); > > > retval = -EAGAIN; > > > > this check+comment becomes completely pointless and imho very confusing. > > > > Oleg. > >
| |