From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tejun Heo Subject: Re: Is not locking task_lock in cgroup_fork() safe? Date: Tue, 16 Oct 2012 12:33:41 -0700 Message-ID: <20121016193341.GD16166@google.com> References: <20121008020000.GB2575@localhost> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=sender:date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; bh=VxYrnZIPvGwPD1TM3Gq7AUSYOLNBloShm8SmkfLzyYM=; b=SyPyb7tqtUklCAnD8DRQx3A9vMJTeIOwIcGSyhFKUxSV1yrvwUfQpRzdTYTkls1EV0 ha1Ks1YN5V1eL2WNOtx6tDxKhwPN8xzetlxMsEcrWW9vVag8bp+A9q45DKNg74jFBv6X V0ULrRqTVt9GAr1WddlttNc/b/yywoIFwhKUVgRS6rcmMAgkrMjmbpDk8m9TzlGlMS20 B2DMykMJrROIoX7t5DfwdhL8290AFaaE9qlr1GPOeAm8L9d4XM/K79HN/g4Zt2FiTD92 emIvnkmqq0pDiGeHympMkUZPbcHsbMptzDnndqgQ+xMZv92ANjunZWSUxM5+wLLAxKQv gcxw== Content-Disposition: inline In-Reply-To: List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: containers-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org Errors-To: containers-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org To: Frederic Weisbecker Cc: cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@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