Re: Is not locking task_lock in cgroup_fork() safe?

[Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

Hey, Frederic.

On Mon, Oct 08, 2012 at 02:48:58PM +0200, Frederic Weisbecker wrote:
> Yeah I missed this one.
> Now the whole cgroup_attach_task() is clusteracy without the

Clusteracy?

> threadgroup lock anyway:
> 
> * The PF_EXITING check is racy (we are neither holding tsk->flags nor
> threagroup lock).

PF_EXITING is *always* protected by threadgroup_change_begin/end().

> * The cgrp == oldcgrp is racy (exit() can change the oldcgrp anytime.

So, as long as this happens after PF_EXITING check, it should be safe.

> * can_attach / attach / cancel_attach can race against fork/exit (and
> post_fork if you consider those interested in cgroup task link like
> the freezer. But that is racy in any case already even with
> threadgroup lock)

Against exit, no.  Against forking a new process, can they?  If so, we
need to fix it.

> It has been designed to be called under that lock. So I suspect the

Ummm.... threadgroup_lock is a recent addition so things couldn't have
been designed to be called under that lock.  threadgroup_lock protects
the *threadgroup* - creating a new task in the same process or a task
of the process exiting.  It doesn't do anything about other processes.
In fact, the lock itself is per-process.

> best, at least for now, is to threadgroup lock from
> cgroup_attach_task_all(). And also make cgroup_attach_task() static to
> avoid future unsafe callers.

Oh, from that call path, sure.  Can someone teach me why we need that
one at all?  I think we're confusing each other here.  I was talking
about the usual migration path not protected against forking a new
process.

> There is no harm yet because the only user of it calls that with
> current as the "task" parameter, in a place that is
> not in the middle of a fork. So no need to worry about some stable backport.
> 
> Also, looking at cgroup_attach_task_all(), what guarantee do we have
> that "from" is not concurrently exiting and removing its cgrp. Which
> is a separate problem. But we probably need to do some css_set_get()
> before playing with it.

I really don't know.  Why isn't it locking the threadgroup to begin
with?

Thanks.

-- 
tejun