Re: [PATCH][BUGFIX] cgroups: more safe tasklist locking in cgroup_attach_proc

[Oleg Nesterov <oleg@redhat.com>]

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?

> 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
list.

How can tasklist make any difference?

> > 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.

Oleg.