All of lore.kernel.org
 help / color / mirror / Atom feed
From: ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org (Eric W. Biederman)
To: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: "Serge E. Hallyn" <serge-A9i7LUbDfNHQT0dZR+AlfA@public.gmane.org>,
	Aditya Kali <adityakali-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>,
	cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Subject: [PATCH 1/3] cgroupns: Fix the locking in copy_cgroup_ns
Date: Fri, 15 Jul 2016 06:35:24 -0500	[thread overview]
Message-ID: <87shvb16df.fsf_-_@x220.int.ebiederm.org> (raw)
In-Reply-To: <8760s72lu5.fsf-JOvCrm2gF+uungPnsOpG7nhyD016LWXt@public.gmane.org> (Eric W. Biederman's message of "Fri, 15 Jul 2016 06:16:02 -0500")


If "clone(CLONE_NEWCGROUP...)" is called it results in a nice lockdep
valid splat.

In __cgroup_proc_write the lock ordering is:
     cgroup_mutex -- through cgroup_kn_lock_live
     cgroup_threadgroup_rwsem

In copy_process the guts of clone the lock ordering is:
     cgroup_threadgroup_rwsem -- through threadgroup_change_begin
     cgroup_mutex -- through copy_namespaces -- copy_cgroup_ns

lockdep reports some a different call chains for the first ordering of
cgroup_mutex and cgroup_threadgroup_rwsem but it is harder to trace.
This is most definitely deadlock potential under the right
circumstances.

Fix this by by skipping the cgroup_mutex and making the locking in
copy_cgroup_ns mirror the locking in cgroup_post_fork which also runs
during fork under the cgroup_threadgroup_rwsem.

Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Fixes: a79a908fd2b0 ("cgroup: introduce cgroup namespaces")
Signed-off-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
---
 kernel/cgroup.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 75c0ff00aca6..5f01e00cffc4 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -6309,14 +6309,11 @@ struct cgroup_namespace *copy_cgroup_ns(unsigned long flags,
 	if (!ns_capable(user_ns, CAP_SYS_ADMIN))
 		return ERR_PTR(-EPERM);
 
-	mutex_lock(&cgroup_mutex);
+	/* It is not safe to take cgroup_mutex here */
 	spin_lock_irq(&css_set_lock);
-
 	cset = task_css_set(current);
 	get_css_set(cset);
-
 	spin_unlock_irq(&css_set_lock);
-	mutex_unlock(&cgroup_mutex);
 
 	new_ns = alloc_cgroup_ns();
 	if (IS_ERR(new_ns)) {
-- 
2.8.3

  parent reply	other threads:[~2016-07-15 11:35 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <87h9br4h80.fsf@x220.int.ebiederm.org>
     [not found] ` <87h9br4h80.fsf-JOvCrm2gF+uungPnsOpG7nhyD016LWXt@public.gmane.org>
2016-07-15  5:15   ` [PATCH 1/3] cgroupns: Fix the locking in copy_cgroup_ns Eric W. Biederman
2016-07-15  5:16   ` [PATCH 2/3] cgroupns: Close race between cgroup_post_fork and copy_cgroup_ns Eric W. Biederman
2016-07-15  5:17   ` [PATCH 3/3] cgroupns: Only allow creation of hierarchies in the initial cgroup namespace Eric W. Biederman
     [not found]     ` <87r3av32g1.fsf-JOvCrm2gF+uungPnsOpG7nhyD016LWXt@public.gmane.org>
2016-07-15 11:16       ` Tejun Heo
     [not found]         ` <20160715111659.GB3078-qYNAdHglDFBN0TnZuCh8vA@public.gmane.org>
2016-07-15 11:16           ` Eric W. Biederman
     [not found]             ` <8760s72lu5.fsf-JOvCrm2gF+uungPnsOpG7nhyD016LWXt@public.gmane.org>
2016-07-15 11:35               ` Eric W. Biederman [this message]
2016-07-15 11:35               ` [PATCH 2/3] cgroupns: Close race between cgroup_post_fork and copy_cgroup_ns Eric W. Biederman
     [not found]                 ` <87mvlj16co.fsf_-_-JOvCrm2gF+uungPnsOpG7nhyD016LWXt@public.gmane.org>
2016-07-15 11:58                   ` Tejun Heo
2016-07-15 11:36               ` [PATCH 3/3] cgroupns: Only allow creation of hierarchies in the initial cgroup namespace Eric W. Biederman
     [not found]                 ` <87h9br16b7.fsf-JOvCrm2gF+uungPnsOpG7nhyD016LWXt@public.gmane.org>
2016-07-15 12:05                   ` Tejun Heo
     [not found]                     ` <20160715120501.GF3078-qYNAdHglDFBN0TnZuCh8vA@public.gmane.org>
2016-07-15 16:39                       ` Serge E. Hallyn
     [not found] ` <20160715111847.GC3078@mtj.duckdns.org>
     [not found]   ` <20160715111847.GC3078-qYNAdHglDFBN0TnZuCh8vA@public.gmane.org>
2016-07-15 11:34     ` [PATCH 0/3] cgroupns: Locking and semantic fixes Eric W. Biederman

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87shvb16df.fsf_-_@x220.int.ebiederm.org \
    --to=ebiederm-as9lmozglivwk0htik3j/w@public.gmane.org \
    --cc=adityakali-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org \
    --cc=cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
    --cc=serge-A9i7LUbDfNHQT0dZR+AlfA@public.gmane.org \
    --cc=tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.