[PATCH][BUGFIX] cgroups: more safe tasklist locking in cgroup_attach_proc
bblum at andrew.cmu.edu
Wed Sep 7 19:11:42 PDT 2011
On Fri, Sep 02, 2011 at 02:32:51PM +0200, Oleg Nesterov wrote:
> On 09/01, Ben Blum wrote:
> > On Mon, Aug 15, 2011 at 08:49:57PM +0200, Oleg Nesterov wrote:
> > >
> > > But can't we avoid the global list? thread_group_leader() or not, we do
> > > not really care. We only need to ensure we can safely find all threads.
> > >
> > > How about the patch below?
> > I was content with the tasklist_lock because cgroup_attach_proc is
> > already a pretty heavyweight operation, and probably pretty rare that a
> > user would want to do multiple of them at once quickly.
> Perhaps. But this is the global lock, no matter what. Why do you prefer
> to take it instead of per-process ->siglock?
Basically just because I wanted to find the simplest race-free
implementation - at that point I was just shooting to have a complete
set of patches, and didn't judge the global lock to be bad enough to
think about replacing. I could yet be convinced that siglock is better.
> > I asked Andrew
> > to take the simple tasklist_lock patch just now, since it does fix the
> > bug at least.
> I disagree.
> > Anyway, looking at this, hmm. I am not sure if this protects adequately?
> > In de_thread, the sighand lock is held only around the first half
> > (around zap_other_threads),
> We only need this lock to find all threads,
> > and not around the following section where
> > leadership is transferred (esp. around the list_replace calls).
> > tasklist_lock is held here, though, so it seems like the right lock to
> > hold.
> This doesn't matter at all, I think. The code under while_each_thread()
> doesn't need the stable ->group_leader, and it can be changed right after
> you drop tasklist. list_replace() calls do not play with ->thread_group
> How can tasklist make any difference?
Oops. I misread the list_replace calls as being on ->thread_group,
instead of on ->tasks as they actually are. Hm, and I see __exit_signal
takes the sighand lock around the call to __unhash_process.
OK, I believe it will work. Oh, and I guess the sighand lock also
subsumes the group_leader check that you were objecting to in the other
Sorry for the delay. Any way this change goes through, though, the
commenting should be clear on how de_thread interacts with it. Something
/* If the leader exits, its links on the thread_group list become
* invalid. One way this can happen is if a sub-thread does exec() when
* de_thread() calls release_task(leader) (and leader->sighand gets set
* to NULL, in which case lock_task_sighand will fail). Since in that
* case the threadgroup is still around, cgroup_procs_write should try
* again (finding the new leader), which EAGAIN indicates here. This is
* "double-double-toil-and-trouble-check locking". */
(I rather like the phrase "double-double-..." to describe the overall
approach, and would prefer if it stayed in the comment.)
> > > With or without this/your patch this leader can die right after we
> > > drop the lock. ss->can_attach(leader) and ss->attach(leader) look
> > > suspicious. If a sub-thread execs, this task_struct has nothing to
> > > do with the threadgroup.
> > hmm. I thought I had this case covered, but it's been so long since I
> > actually wrote the code that if I did I can't remember how.
> This all doesn't look right... Hopefully addressed by Tejun patches.
Will look into those separately.
More information about the Containers