Is not locking task_lock in cgroup_fork() safe?
tj at kernel.org
Tue Oct 16 19:33:41 UTC 2012
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
> 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
> 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
More information about the Containers