[PATCH v5 0/4] devcg: introduce proper hierarchy support

[PATCH v5 0/4] devcg: introduce proper hierarchy support

[Aristeu Rozanski]

This patchset implements device cgroup hierarchy. Exceptions will be
propagated down in the tree and local preferences will be re-evaluated
everytime a change in its parent occours, reapplying them if it's still
possible.

git://github.com/aristeu/linux-2.6.git
branch: devcg_hierarchy_review

v5:
- reworked the patchset to not propagate behavior changes

v4:
- minor fixes pointed by Tejun

v3:
- update documentation
- move css_online/css_offline changes to a new patch
- use cgroup_for_each_descendant_pre() instead of own descendant walk
- move exception_copy rework to a separared patch
- move exception_clean rework to a separated patch
- new patch to just move dev_exception_rm() before dev_exception_add()
  as requested by Tejun.
- updated patch description for may_access() changes
- new patch to expand the may_access() logic before changing it
- fixed argument description order in may_access()

v2:
- rebase on top "device_cgroup: don't grab mutex in rcu callback"
- in case parent changes behavior or exceptions and the local exceptions won't
  apply anymore, remove them instead of keeping them around.

Cc: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>                                                                                               
Cc: Serge Hallyn <serge.hallyn-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>                                                                               
Signed-off-by: Aristeu Rozanski <aris-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>                                                                           

-- 
Aristeu

[PATCH v5 1/4] devcg: expand may_access() logic

[Aristeu Rozanski]

[-- Attachment #1: split_may_access_logic.patch --]
[-- Type: text/plain, Size: 1820 bytes --]

In order to make the next patch more clear, expand may_access() logic.

v2: may_access() returns bool now

Acked-by: Tejun Heo <tj@kernel.org>
Acked-by: Serge Hallyn <serge.hallyn@canonical.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: Serge Hallyn <serge.hallyn@canonical.com>
Signed-off-by: Aristeu Rozanski <aris@redhat.com>

---
 security/device_cgroup.c |   21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

--- github.orig/security/device_cgroup.c	2013-02-14 10:46:45.717808064 -0500
+++ github/security/device_cgroup.c	2013-02-14 10:47:12.146177936 -0500
@@ -302,8 +302,8 @@ 	return 0;
  * @dev_cgroup: dev cgroup to be tested against
  * @refex: new exception
  */
-static int may_access(struct dev_cgroup *dev_cgroup,
-		      struct dev_exception_item *refex)
+static bool may_access(struct dev_cgroup *dev_cgroup,
+		       struct dev_exception_item *refex)
 {
 	struct dev_exception_item *ex;
 	bool match = false;
@@ -329,16 +329,19 @@ 		if (ex->minor != ~0 && ex->minor != re
 
 	/*
 	 * In two cases we'll consider this new exception valid:
-	 * - the dev cgroup has its default policy to allow + exception list:
-	 *   the new exception should *not* match any of the exceptions
-	 *   (behavior == DEVCG_DEFAULT_ALLOW, !match)
 	 * - the dev cgroup has its default policy to deny + exception list:
 	 *   the new exception *should* match the exceptions
-	 *   (behavior == DEVCG_DEFAULT_DENY, match)
+	 * - the dev cgroup has its default policy to allow + exception list:
+	 *   the new exception should *not* match any of the exceptions
 	 */
-	if ((dev_cgroup->behavior == DEVCG_DEFAULT_DENY) == match)
-		return 1;
-	return 0;
+	if (dev_cgroup->behavior == DEVCG_DEFAULT_DENY) {
+		if (match)
+			return true;
+	} else {
+		if (!match)
+			return true;
+	}
+	return false;
 }
 
 /*

[PATCH v5 2/4] devcg: prepare may_access() for hierarchy support

[Aristeu Rozanski]

[-- Attachment #1: strengthen-may_access.patch --]
[-- Type: text/plain, Size: 3824 bytes --]

Currently may_access() is only able to verify if an exception is valid for the
current cgroup, which has the same behavior. With hierarchy, it'll be also used
to verify if a cgroup local exception is valid towards its cgroup parent, which
might have different behavior.

v2:
- updated patch description
- rebased on top of a new patch to expand the may_access() logic to make it
  more clear
- fixed argument description order in may_access()

Acked-by: Tejun Heo <tj@kernel.org>
Acked-by: Serge Hallyn <serge.hallyn@canonical.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: Serge Hallyn <serge.hallyn@canonical.com>
Signed-off-by: Aristeu Rozanski <aris@redhat.com>

---
 security/device_cgroup.c |   49 +++++++++++++++++++++++++++++------------------
 1 file changed, 31 insertions(+), 18 deletions(-)

--- github.orig/security/device_cgroup.c	2013-02-14 10:47:12.146177936 -0500
+++ github/security/device_cgroup.c	2013-02-14 10:47:12.411181646 -0500
@@ -25,6 +25,12 @@  * Copyright 2007 IBM Corp
 
 static DEFINE_MUTEX(devcgroup_mutex);
 
+enum devcg_behavior {
+	DEVCG_DEFAULT_NONE,
+	DEVCG_DEFAULT_ALLOW,
+	DEVCG_DEFAULT_DENY,
+};
+
 /*
  * exception list locking rules:
  * hold devcgroup_mutex for update/read.
@@ -42,10 +48,7 @@ struct dev_exception_item {
 struct dev_cgroup {
 	struct cgroup_subsys_state css;
 	struct list_head exceptions;
-	enum {
-		DEVCG_DEFAULT_ALLOW,
-		DEVCG_DEFAULT_DENY,
-	} behavior;
+	enum devcg_behavior behavior;
 };
 
 static inline struct dev_cgroup *css_to_devcgroup(struct cgroup_subsys_state *s)
@@ -301,9 +304,11 @@ 	return 0;
  *		verify if a certain access is allowed.
  * @dev_cgroup: dev cgroup to be tested against
  * @refex: new exception
+ * @behavior: behavior of the exception
  */
 static bool may_access(struct dev_cgroup *dev_cgroup,
-		       struct dev_exception_item *refex)
+		       struct dev_exception_item *refex,
+		       enum devcg_behavior behavior)
 {
 	struct dev_exception_item *ex;
 	bool match = false;
@@ -327,19 +332,27 @@ 		if (ex->minor != ~0 && ex->minor != re
 		break;
 	}
 
-	/*
-	 * In two cases we'll consider this new exception valid:
-	 * - the dev cgroup has its default policy to deny + exception list:
-	 *   the new exception *should* match the exceptions
-	 * - the dev cgroup has its default policy to allow + exception list:
-	 *   the new exception should *not* match any of the exceptions
-	 */
-	if (dev_cgroup->behavior == DEVCG_DEFAULT_DENY) {
-		if (match)
+	if (dev_cgroup->behavior == DEVCG_DEFAULT_ALLOW) {
+		if (behavior == DEVCG_DEFAULT_ALLOW) {
+			/* the exception will deny access to certain devices */
+			return true;
+		} else {
+			/* the exception will allow access to certain devices */
+			if (match)
+				/*
+				 * a new exception allowing access shouldn't
+				 * match an parent's exception
+				 */
+				return false;
 			return true;
+		}
 	} else {
-		if (!match)
+		/* only behavior == DEVCG_DEFAULT_DENY allowed here */
+		if (match)
+			/* parent has an exception that matches the proposed */
 			return true;
+		else
+			return false;
 	}
 	return false;
 }
@@ -358,7 +371,7 @@ static int parent_has_perm(struct dev_cg
 	if (!pcg)
 		return 1;
 	parent = cgroup_to_devcgroup(pcg);
-	return may_access(parent, ex);
+	return may_access(parent, ex, childcg->behavior);
 }
 
 /**
@@ -392,7 +405,7 @@ static int devcgroup_update_access(struc
 {
 	const char *b;
 	char temp[12];		/* 11 + 1 characters needed for a u32 */
-	int count, rc;
+	int count, rc = 0;
 	struct dev_exception_item ex;
 	struct cgroup *p = devcgroup->css.cgroup;
 	struct dev_cgroup *parent = NULL;
@@ -609,7 +622,7 @@ 	memset(&ex, 0, sizeof(ex));
 
 	rcu_read_lock();
 	dev_cgroup = task_devcgroup(current);
-	rc = may_access(dev_cgroup, &ex);
+	rc = may_access(dev_cgroup, &ex, dev_cgroup->behavior);
 	rcu_read_unlock();
 
 	if (!rc)

[PATCH v5 3/4] devcg: use css_online and css_offline

[Aristeu Rozanski]

[-- Attachment #1: online.patch --]
[-- Type: text/plain, Size: 3431 bytes --]

Allocate resources and change behavior only when online. This is needed in
order to determine if a node is suitable for hierarchy propagation or if it's
being removed.

Locking:
Both functions take devcgroup_mutex to make changes to device_cgroup structure.
Hierarchy propagation will also take devcgroup_mutex before walking the
tree while walking the tree itself is protected by rcu lock.

Acked-by: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Acked-by: Serge Hallyn <serge.hallyn-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
Cc: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Serge Hallyn <serge.hallyn-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
Signed-off-by: Aristeu Rozanski <aris-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

---
 security/device_cgroup.c |   59 +++++++++++++++++++++++++++++++++--------------
 1 file changed, 42 insertions(+), 17 deletions(-)

--- github.orig/security/device_cgroup.c	2013-02-14 10:47:12.411181646 -0500
+++ github/security/device_cgroup.c	2013-02-14 10:47:12.701185704 -0500
@@ -180,36 +180,59 @@ static void dev_exception_clean(struct d
 	}
 }
 
+/**
+ * devcgroup_online - initializes devcgroup's behavior and exceptions based on
+ * 		      parent's
+ * @cgroup: cgroup getting online
+ * returns 0 in case of success, error code otherwise
+ */
+static int devcgroup_online(struct cgroup *cgroup)
+{
+	struct dev_cgroup *dev_cgroup, *parent_dev_cgroup = NULL;
+	int ret = 0;
+
+	mutex_lock(&devcgroup_mutex);
+	dev_cgroup = cgroup_to_devcgroup(cgroup);
+	if (cgroup->parent)
+		parent_dev_cgroup = cgroup_to_devcgroup(cgroup->parent);
+
+	if (parent_dev_cgroup == NULL)
+		dev_cgroup->behavior = DEVCG_DEFAULT_ALLOW;
+	else {
+		ret = dev_exceptions_copy(&dev_cgroup->exceptions,
+					  &parent_dev_cgroup->exceptions);
+		if (!ret)
+			dev_cgroup->behavior = parent_dev_cgroup->behavior;
+	}
+	mutex_unlock(&devcgroup_mutex);
+
+	return ret;
+}
+
+static void devcgroup_offline(struct cgroup *cgroup)
+{
+	struct dev_cgroup *dev_cgroup = cgroup_to_devcgroup(cgroup);
+
+	mutex_lock(&devcgroup_mutex);
+	dev_cgroup->behavior = DEVCG_DEFAULT_NONE;
+	mutex_unlock(&devcgroup_mutex);
+}
+
 /*
  * called from kernel/cgroup.c with cgroup_lock() held.
  */
 static struct cgroup_subsys_state *devcgroup_css_alloc(struct cgroup *cgroup)
 {
-	struct dev_cgroup *dev_cgroup, *parent_dev_cgroup;
+	struct dev_cgroup *dev_cgroup;
 	struct cgroup *parent_cgroup;
-	int ret;
 
 	dev_cgroup = kzalloc(sizeof(*dev_cgroup), GFP_KERNEL);
 	if (!dev_cgroup)
 		return ERR_PTR(-ENOMEM);
 	INIT_LIST_HEAD(&dev_cgroup->exceptions);
+	dev_cgroup->behavior = DEVCG_DEFAULT_NONE;
 	parent_cgroup = cgroup->parent;
 
-	if (parent_cgroup == NULL)
-		dev_cgroup->behavior = DEVCG_DEFAULT_ALLOW;
-	else {
-		parent_dev_cgroup = cgroup_to_devcgroup(parent_cgroup);
-		mutex_lock(&devcgroup_mutex);
-		ret = dev_exceptions_copy(&dev_cgroup->exceptions,
-					  &parent_dev_cgroup->exceptions);
-		dev_cgroup->behavior = parent_dev_cgroup->behavior;
-		mutex_unlock(&devcgroup_mutex);
-		if (ret) {
-			kfree(dev_cgroup);
-			return ERR_PTR(ret);
-		}
-	}
-
 	return &dev_cgroup->css;
 }
 
@@ -584,6 +607,8 @@ struct cgroup_subsys devices_subsys = {
 	.can_attach = devcgroup_can_attach,
 	.css_alloc = devcgroup_css_alloc,
 	.css_free = devcgroup_css_free,
+	.css_online = devcgroup_online,
+	.css_offline = devcgroup_offline,
 	.subsys_id = devices_subsys_id,
 	.base_cftypes = dev_cgroup_files,
 

[PATCH v5 4/4] devcg: propagate local changes down the hierarchy

[Aristeu Rozanski]

[-- Attachment #1: propagate.patch --]
[-- Type: text/plain, Size: 11859 bytes --]

This patch makes exception changes to propagate down in hierarchy respecting
when possible local exceptions.

New exceptions allowing additional access to devices won't be propagated, but
it'll be possible to add an exception to access all of part of the newly
allowed device(s).

New exceptions disallowing access to devices will be propagated down and the
local group's exceptions will be revalidated for the new situation.
Example:
      A
     / \
        B

    group        behavior          exceptions
    A            allow             "b 8:* rwm", "c 116:1 rw"
    B            deny              "c 1:3 rwm", "c 116:2 rwm", "b 3:* rwm"

If a new exception is added to group A:
	# echo "c 116:* r" > A/devices.deny
it'll propagate down and after revalidating B's local exceptions, the exception
"c 116:2 rwm" will be removed.

In case parent's exceptions change and local exceptions are not allowed anymore,
they'll be deleted.

v7:
- do not allow behavior change when the cgroup has children
- update documentation

v6: fixed issues pointed by Serge Hallyn
- only copy parent's exceptions while propagating behavior if the local
  behavior is different
- while propagating exceptions, do not clear and copy parent's: it'd be against
  the premise we don't propagate access to more devices

v5: fixed issues pointed by Serge Hallyn
- updated documentation
- not propagating when an exception is written to devices.allow
- when propagating a new behavior, clean the local exceptions list if they're
  for a different behavior

v4: fixed issues pointed by Tejun Heo
- separated function to walk the tree and collect valid propagation targets

v3: fixed issues pointed by Tejun Heo
- update documentation
- move css_online/css_offline changes to a new patch
- use cgroup_for_each_descendant_pre() instead of own descendant walk
- move exception_copy rework to a separared patch
- move exception_clean rework to a separated patch

v2: fixed issues pointed by Tejun Heo
- instead of keeping the local settings that won't apply anymore, remove them

Cc: Tejun Heo <tj@kernel.org>
Cc: Serge Hallyn <serge.hallyn@canonical.com>
Signed-off-by: Aristeu Rozanski <aris@redhat.com>

---
 Documentation/cgroups/devices.txt |   70 ++++++++++++++++++-
 security/device_cgroup.c          |  139 ++++++++++++++++++++++++++++++++++++--
 2 files changed, 199 insertions(+), 10 deletions(-)

--- github.orig/security/device_cgroup.c	2013-02-15 11:30:25.450085632 -0500
+++ github/security/device_cgroup.c	2013-02-15 11:30:37.510258447 -0500
@@ -49,6 +49,8 @@ struct dev_cgroup {
 	struct cgroup_subsys_state css;
 	struct list_head exceptions;
 	enum devcg_behavior behavior;
+	/* temporary list for pending propagation operations */
+	struct list_head propagate_pending;
 };
 
 static inline struct dev_cgroup *css_to_devcgroup(struct cgroup_subsys_state *s)
@@ -180,6 +182,11 @@ static void dev_exception_clean(struct d
 	}
 }
 
+static inline bool is_devcg_online(const struct dev_cgroup *devcg)
+{
+	return (devcg->behavior != DEVCG_DEFAULT_NONE);
+}
+
 /**
  * devcgroup_online - initializes devcgroup's behavior and exceptions based on
  * 		      parent's
@@ -230,6 +237,7 @@ static struct cgroup_subsys_state *devcg
 	if (!dev_cgroup)
 		return ERR_PTR(-ENOMEM);
 	INIT_LIST_HEAD(&dev_cgroup->exceptions);
+	INIT_LIST_HEAD(&dev_cgroup->propagate_pending);
 	dev_cgroup->behavior = DEVCG_DEFAULT_NONE;
 	parent_cgroup = cgroup->parent;
 
@@ -410,6 +418,111 @@ static inline int may_allow_all(struct d
 	return parent->behavior == DEVCG_DEFAULT_ALLOW;
 }
 
+/**
+ * revalidate_active_exceptions - walks through the active exception list and
+ * 				  revalidates the exceptions based on parent's
+ * 				  behavior and exceptions. The exceptions that
+ * 				  are no longer valid will be removed.
+ * 				  Called with devcgroup_mutex held.
+ * @devcg: cgroup which exceptions will be checked
+ *
+ * This is one of the three key functions for hierarchy implementation.
+ * This function is responsible for re-evaluating all the cgroup's active
+ * exceptions due to a parent's exception change.
+ * Refer to Documentation/cgroups/devices.txt for more details.
+ */
+static void revalidate_active_exceptions(struct dev_cgroup *devcg)
+{
+	struct dev_exception_item *ex;
+	struct list_head *this, *tmp;
+
+	list_for_each_safe(this, tmp, &devcg->exceptions) {
+		ex = container_of(this, struct dev_exception_item, list);
+		if (!parent_has_perm(devcg, ex))
+			dev_exception_rm(devcg, ex);
+	}
+}
+
+/**
+ * get_online_devcg - walks the cgroup tree and fills a list with the online
+ * 		      groups
+ * @root: cgroup used as starting point
+ * @online: list that will be filled with online groups
+ *
+ * Must be called with devcgroup_mutex held. Grabs RCU lock.
+ * Because devcgroup_mutex is held, no devcg will become online or offline
+ * during the tree walk (see devcgroup_online, devcgroup_offline)
+ * A separated list is needed because propagate_behavior() and
+ * propagate_exception() need to allocate memory and can block.
+ */
+static void get_online_devcg(struct cgroup *root, struct list_head *online)
+{
+	struct cgroup *pos;
+	struct dev_cgroup *devcg;
+
+	lockdep_assert_held(&devcgroup_mutex);
+
+	rcu_read_lock();
+	cgroup_for_each_descendant_pre(pos, root) {
+		devcg = cgroup_to_devcgroup(pos);
+		if (is_devcg_online(devcg))
+			list_add_tail(&devcg->propagate_pending, online);
+	}
+	rcu_read_unlock();
+}
+
+/**
+ * propagate_exception - propagates a new exception to the children
+ * @devcg_root: device cgroup that added a new exception
+ * @ex: new exception to be propagated
+ *
+ * returns: 0 in case of success, != 0 in case of error
+ */
+static int propagate_exception(struct dev_cgroup *devcg_root,
+			       struct dev_exception_item *ex)
+{
+	struct cgroup *root = devcg_root->css.cgroup;
+	struct dev_cgroup *devcg, *parent, *tmp;
+	int rc = 0;
+	LIST_HEAD(pending);
+
+	get_online_devcg(root, &pending);
+
+	list_for_each_entry_safe(devcg, tmp, &pending, propagate_pending) {
+		parent = cgroup_to_devcgroup(devcg->css.cgroup->parent);
+
+		/*
+		 * in case both root's behavior and devcg is allow, a new
+		 * restriction means adding to the exception list
+		 */
+		if (devcg_root->behavior == DEVCG_DEFAULT_ALLOW &&
+		    devcg->behavior == DEVCG_DEFAULT_ALLOW) {
+			rc = dev_exception_add(devcg, ex);
+			if (rc)
+				break;
+		} else {
+			/*
+			 * in the other possible cases:
+			 * root's behavior: allow, devcg's: deny
+			 * root's behavior: deny, devcg's: deny
+			 * the exception will be removed
+			 */
+			dev_exception_rm(devcg, ex);
+		}
+		revalidate_active_exceptions(devcg);
+
+		list_del_init(&devcg->propagate_pending);
+	}
+	return rc;
+}
+
+static inline bool has_children(struct dev_cgroup *devcgroup)
+{
+	struct cgroup *cgrp = devcgroup->css.cgroup;
+
+	return !list_empty(&cgrp->children);
+}
+
 /*
  * Modify the exception list using allow/deny rules.
  * CAP_SYS_ADMIN is needed for this.  It's at least separate from CAP_MKNOD
@@ -446,6 +559,9 @@ 	memset(&ex, 0, sizeof(ex));
 	case 'a':
 		switch (filetype) {
 		case DEVCG_ALLOW:
+			if (has_children(devcgroup))
+				return -EINVAL;
+
 			if (!may_allow_all(parent))
 				return -EPERM;
 			dev_exception_clean(devcgroup);
@@ -459,6 +575,9 @@ 	memset(&ex, 0, sizeof(ex));
 				return rc;
 			break;
 		case DEVCG_DENY:
+			if (has_children(devcgroup))
+				return -EINVAL;
+
 			dev_exception_clean(devcgroup);
 			devcgroup->behavior = DEVCG_DEFAULT_DENY;
 			break;
@@ -553,22 +672,28 @@ 		case '\0':
 			dev_exception_rm(devcgroup, &ex);
 			return 0;
 		}
-		return dev_exception_add(devcgroup, &ex);
+		rc = dev_exception_add(devcgroup, &ex);
+		break;
 	case DEVCG_DENY:
 		/*
 		 * If the default policy is to deny by default, try to remove
 		 * an matching exception instead. And be silent about it: we
 		 * don't want to break compatibility
 		 */
-		if (devcgroup->behavior == DEVCG_DEFAULT_DENY) {
+		if (devcgroup->behavior == DEVCG_DEFAULT_DENY)
 			dev_exception_rm(devcgroup, &ex);
-			return 0;
-		}
-		return dev_exception_add(devcgroup, &ex);
+		else
+			rc = dev_exception_add(devcgroup, &ex);
+
+		if (rc)
+			break;
+		/* we only propagate new restrictions */
+		rc = propagate_exception(devcgroup, &ex);
+		break;
 	default:
-		return -EINVAL;
+		rc = -EINVAL;
 	}
-	return 0;
+	return rc;
 }
 
 static int devcgroup_access_write(struct cgroup *cgrp, struct cftype *cft,
--- github.orig/Documentation/cgroups/devices.txt	2013-02-15 11:30:20.907020535 -0500
+++ github/Documentation/cgroups/devices.txt	2013-02-15 11:30:37.511258462 -0500
@@ -13,9 +13,7 @@ either an integer or * for all.  Access 
 The root device cgroup starts with rwm to 'all'.  A child device
 cgroup gets a copy of the parent.  Administrators can then remove
 devices from the whitelist or add new entries.  A child cgroup can
-never receive a device access which is denied by its parent.  However
-when a device access is removed from a parent it will not also be
-removed from the child(ren).
+never receive a device access which is denied by its parent.
 
 2. User Interface
 
@@ -50,3 +48,69 @@ task to a new cgroup.  (Again we'll prob
 
 A cgroup may not be granted more permissions than the cgroup's
 parent has.
+
+4. Hierarchy
+
+device cgroups maintain hierarchy by making sure a cgroup never has more
+access permissions than its parent.  Every time an entry is written to
+a cgroup's devices.deny file, all its children will have that entry removed
+from their whitelist and all the locally set whitelist entries will be
+re-evaluated.  In case one of the locally set whitelist entries would provide
+more access than the cgroup's parent, it'll be removed from the whitelist.
+
+Example:
+      A
+     / \
+        B
+
+    group        behavior	exceptions
+    A            allow		"b 8:* rwm", "c 116:1 rw"
+    B            deny		"c 1:3 rwm", "c 116:2 rwm", "b 3:* rwm"
+
+If a device is denied in group A:
+	# echo "c 116:* r" > A/devices.deny
+it'll propagate down and after revalidating B's entries, the whitelist entry
+"c 116:2 rwm" will be removed:
+
+    group        whitelist entries                        denied devices
+    A            all                                      "b 8:* rwm", "c 116:* rw"
+    B            "c 1:3 rwm", "b 3:* rwm"                 all the rest
+
+In case parent's exceptions change and local exceptions are not allowed
+anymore, they'll be deleted.
+
+Notice that new whitelist entries will not be propagated:
+      A
+     / \
+        B
+
+    group        whitelist entries                        denied devices
+    A            "c 1:3 rwm", "c 1:5 r"                   all the rest
+    B            "c 1:3 rwm", "c 1:5 r"                   all the rest
+
+when adding "c *:3 rwm":
+	# echo "c *:3 rwm" >A/devices.allow
+
+the result:
+    group        whitelist entries                        denied devices
+    A            "c *:3 rwm", "c 1:5 r"                   all the rest
+    B            "c 1:3 rwm", "c 1:5 r"                   all the rest
+
+but now it'll be possible to add new entries to B:
+	# echo "c 2:3 rwm" >B/devices.allow
+	# echo "c 50:3 r" >B/devices.allow
+or even
+	# echo "c *:3 rwm" >B/devices.allow
+
+Allowing or denying all by writing 'a' to devices.allow or devices.deny will
+not be possible once the device cgroups has children.
+
+4.1 Hierarchy (internal implementation)
+
+device cgroups is implemented internally using a behavior (ALLOW, DENY) and a
+list of exceptions.  The internal state is controlled using the same user
+interface to preserve compatibility with the previous whitelist-only
+implementation.  Removal or addition of exceptions that will reduce the access
+to devices will be propagated down the hierarchy.
+For every propagated exception, the effective rules will be re-evaluated based
+on current parent's access rules.

Re: [PATCH v5 4/4] devcg: propagate local changes down the hierarchy

[Serge E. Hallyn]

Quoting Aristeu Rozanski (aris-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org):
> This patch makes exception changes to propagate down in hierarchy respecting
> when possible local exceptions.
> 
> New exceptions allowing additional access to devices won't be propagated, but
> it'll be possible to add an exception to access all of part of the newly
> allowed device(s).
> 
> New exceptions disallowing access to devices will be propagated down and the
> local group's exceptions will be revalidated for the new situation.
> Example:
>       A
>      / \
>         B
> 
>     group        behavior          exceptions
>     A            allow             "b 8:* rwm", "c 116:1 rw"
>     B            deny              "c 1:3 rwm", "c 116:2 rwm", "b 3:* rwm"
> 
> If a new exception is added to group A:
> 	# echo "c 116:* r" > A/devices.deny
> it'll propagate down and after revalidating B's local exceptions, the exception
> "c 116:2 rwm" will be removed.
> 
> In case parent's exceptions change and local exceptions are not allowed anymore,
> they'll be deleted.
> 
> v7:
> - do not allow behavior change when the cgroup has children
> - update documentation
> 
> v6: fixed issues pointed by Serge Hallyn
> - only copy parent's exceptions while propagating behavior if the local
>   behavior is different
> - while propagating exceptions, do not clear and copy parent's: it'd be against
>   the premise we don't propagate access to more devices
> 
> v5: fixed issues pointed by Serge Hallyn
> - updated documentation
> - not propagating when an exception is written to devices.allow
> - when propagating a new behavior, clean the local exceptions list if they're
>   for a different behavior
> 
> v4: fixed issues pointed by Tejun Heo
> - separated function to walk the tree and collect valid propagation targets
> 
> v3: fixed issues pointed by Tejun Heo
> - update documentation
> - move css_online/css_offline changes to a new patch
> - use cgroup_for_each_descendant_pre() instead of own descendant walk
> - move exception_copy rework to a separared patch
> - move exception_clean rework to a separated patch
> 
> v2: fixed issues pointed by Tejun Heo
> - instead of keeping the local settings that won't apply anymore, remove them
> 
> Cc: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Serge Hallyn <serge.hallyn-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>

Thanks!

Reviewed-by: Serge Hallyn <serge.hallyn-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>

(one minor comment below)

> Signed-off-by: Aristeu Rozanski <aris-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> 
> ---
>  Documentation/cgroups/devices.txt |   70 ++++++++++++++++++-
>  security/device_cgroup.c          |  139 ++++++++++++++++++++++++++++++++++++--
>  2 files changed, 199 insertions(+), 10 deletions(-)
> 
> --- github.orig/security/device_cgroup.c	2013-02-15 11:30:25.450085632 -0500
> +++ github/security/device_cgroup.c	2013-02-15 11:30:37.510258447 -0500
> @@ -49,6 +49,8 @@ struct dev_cgroup {
>  	struct cgroup_subsys_state css;
>  	struct list_head exceptions;
>  	enum devcg_behavior behavior;
> +	/* temporary list for pending propagation operations */
> +	struct list_head propagate_pending;
>  };
>  
>  static inline struct dev_cgroup *css_to_devcgroup(struct cgroup_subsys_state *s)
> @@ -180,6 +182,11 @@ static void dev_exception_clean(struct d
>  	}
>  }
>  
> +static inline bool is_devcg_online(const struct dev_cgroup *devcg)
> +{
> +	return (devcg->behavior != DEVCG_DEFAULT_NONE);
> +}
> +
>  /**
>   * devcgroup_online - initializes devcgroup's behavior and exceptions based on
>   * 		      parent's
> @@ -230,6 +237,7 @@ static struct cgroup_subsys_state *devcg
>  	if (!dev_cgroup)
>  		return ERR_PTR(-ENOMEM);
>  	INIT_LIST_HEAD(&dev_cgroup->exceptions);
> +	INIT_LIST_HEAD(&dev_cgroup->propagate_pending);
>  	dev_cgroup->behavior = DEVCG_DEFAULT_NONE;
>  	parent_cgroup = cgroup->parent;
>  
> @@ -410,6 +418,111 @@ static inline int may_allow_all(struct d
>  	return parent->behavior == DEVCG_DEFAULT_ALLOW;
>  }
>  
> +/**
> + * revalidate_active_exceptions - walks through the active exception list and
> + * 				  revalidates the exceptions based on parent's
> + * 				  behavior and exceptions. The exceptions that
> + * 				  are no longer valid will be removed.
> + * 				  Called with devcgroup_mutex held.
> + * @devcg: cgroup which exceptions will be checked
> + *
> + * This is one of the three key functions for hierarchy implementation.
> + * This function is responsible for re-evaluating all the cgroup's active
> + * exceptions due to a parent's exception change.
> + * Refer to Documentation/cgroups/devices.txt for more details.
> + */
> +static void revalidate_active_exceptions(struct dev_cgroup *devcg)
> +{
> +	struct dev_exception_item *ex;
> +	struct list_head *this, *tmp;
> +
> +	list_for_each_safe(this, tmp, &devcg->exceptions) {
> +		ex = container_of(this, struct dev_exception_item, list);
> +		if (!parent_has_perm(devcg, ex))
> +			dev_exception_rm(devcg, ex);
> +	}
> +}
> +
> +/**
> + * get_online_devcg - walks the cgroup tree and fills a list with the online
> + * 		      groups
> + * @root: cgroup used as starting point
> + * @online: list that will be filled with online groups
> + *
> + * Must be called with devcgroup_mutex held. Grabs RCU lock.
> + * Because devcgroup_mutex is held, no devcg will become online or offline
> + * during the tree walk (see devcgroup_online, devcgroup_offline)
> + * A separated list is needed because propagate_behavior() and
> + * propagate_exception() need to allocate memory and can block.
> + */
> +static void get_online_devcg(struct cgroup *root, struct list_head *online)
> +{
> +	struct cgroup *pos;
> +	struct dev_cgroup *devcg;
> +
> +	lockdep_assert_held(&devcgroup_mutex);
> +
> +	rcu_read_lock();
> +	cgroup_for_each_descendant_pre(pos, root) {
> +		devcg = cgroup_to_devcgroup(pos);
> +		if (is_devcg_online(devcg))
> +			list_add_tail(&devcg->propagate_pending, online);
> +	}
> +	rcu_read_unlock();
> +}
> +
> +/**
> + * propagate_exception - propagates a new exception to the children
> + * @devcg_root: device cgroup that added a new exception
> + * @ex: new exception to be propagated
> + *
> + * returns: 0 in case of success, != 0 in case of error
> + */
> +static int propagate_exception(struct dev_cgroup *devcg_root,
> +			       struct dev_exception_item *ex)
> +{
> +	struct cgroup *root = devcg_root->css.cgroup;
> +	struct dev_cgroup *devcg, *parent, *tmp;
> +	int rc = 0;
> +	LIST_HEAD(pending);
> +
> +	get_online_devcg(root, &pending);
> +
> +	list_for_each_entry_safe(devcg, tmp, &pending, propagate_pending) {
> +		parent = cgroup_to_devcgroup(devcg->css.cgroup->parent);
> +
> +		/*
> +		 * in case both root's behavior and devcg is allow, a new
> +		 * restriction means adding to the exception list
> +		 */
> +		if (devcg_root->behavior == DEVCG_DEFAULT_ALLOW &&
> +		    devcg->behavior == DEVCG_DEFAULT_ALLOW) {
> +			rc = dev_exception_add(devcg, ex);
> +			if (rc)
> +				break;
> +		} else {
> +			/*
> +			 * in the other possible cases:
> +			 * root's behavior: allow, devcg's: deny
> +			 * root's behavior: deny, devcg's: deny
> +			 * the exception will be removed
> +			 */

Technically this case isn't needed, right?  Will the dev_exception_rm()
also be done by revalidate_active_exceptions()?  So it's safe (but
not necessary) to drop the else here.  Though the comment is very
informative, and it might be worth keeping the code as is for clarity.

> +			dev_exception_rm(devcg, ex);
> +		}
> +		revalidate_active_exceptions(devcg);
> +
> +		list_del_init(&devcg->propagate_pending);
> +	}
> +	return rc;
> +}
> +
> +static inline bool has_children(struct dev_cgroup *devcgroup)
> +{
> +	struct cgroup *cgrp = devcgroup->css.cgroup;
> +
> +	return !list_empty(&cgrp->children);
> +}
> +
>  /*
>   * Modify the exception list using allow/deny rules.
>   * CAP_SYS_ADMIN is needed for this.  It's at least separate from CAP_MKNOD
> @@ -446,6 +559,9 @@ 	memset(&ex, 0, sizeof(ex));
>  	case 'a':
>  		switch (filetype) {
>  		case DEVCG_ALLOW:
> +			if (has_children(devcgroup))
> +				return -EINVAL;
> +
>  			if (!may_allow_all(parent))
>  				return -EPERM;
>  			dev_exception_clean(devcgroup);
> @@ -459,6 +575,9 @@ 	memset(&ex, 0, sizeof(ex));
>  				return rc;
>  			break;
>  		case DEVCG_DENY:
> +			if (has_children(devcgroup))
> +				return -EINVAL;
> +
>  			dev_exception_clean(devcgroup);
>  			devcgroup->behavior = DEVCG_DEFAULT_DENY;
>  			break;
> @@ -553,22 +672,28 @@ 		case '\0':
>  			dev_exception_rm(devcgroup, &ex);
>  			return 0;
>  		}
> -		return dev_exception_add(devcgroup, &ex);
> +		rc = dev_exception_add(devcgroup, &ex);
> +		break;
>  	case DEVCG_DENY:
>  		/*
>  		 * If the default policy is to deny by default, try to remove
>  		 * an matching exception instead. And be silent about it: we
>  		 * don't want to break compatibility
>  		 */
> -		if (devcgroup->behavior == DEVCG_DEFAULT_DENY) {
> +		if (devcgroup->behavior == DEVCG_DEFAULT_DENY)
>  			dev_exception_rm(devcgroup, &ex);
> -			return 0;
> -		}
> -		return dev_exception_add(devcgroup, &ex);
> +		else
> +			rc = dev_exception_add(devcgroup, &ex);
> +
> +		if (rc)
> +			break;
> +		/* we only propagate new restrictions */
> +		rc = propagate_exception(devcgroup, &ex);
> +		break;
>  	default:
> -		return -EINVAL;
> +		rc = -EINVAL;
>  	}
> -	return 0;
> +	return rc;
>  }
>  
>  static int devcgroup_access_write(struct cgroup *cgrp, struct cftype *cft,
> --- github.orig/Documentation/cgroups/devices.txt	2013-02-15 11:30:20.907020535 -0500
> +++ github/Documentation/cgroups/devices.txt	2013-02-15 11:30:37.511258462 -0500
> @@ -13,9 +13,7 @@ either an integer or * for all.  Access 
>  The root device cgroup starts with rwm to 'all'.  A child device
>  cgroup gets a copy of the parent.  Administrators can then remove
>  devices from the whitelist or add new entries.  A child cgroup can
> -never receive a device access which is denied by its parent.  However
> -when a device access is removed from a parent it will not also be
> -removed from the child(ren).
> +never receive a device access which is denied by its parent.
>  
>  2. User Interface
>  
> @@ -50,3 +48,69 @@ task to a new cgroup.  (Again we'll prob
>  
>  A cgroup may not be granted more permissions than the cgroup's
>  parent has.
> +
> +4. Hierarchy
> +
> +device cgroups maintain hierarchy by making sure a cgroup never has more
> +access permissions than its parent.  Every time an entry is written to
> +a cgroup's devices.deny file, all its children will have that entry removed
> +from their whitelist and all the locally set whitelist entries will be
> +re-evaluated.  In case one of the locally set whitelist entries would provide
> +more access than the cgroup's parent, it'll be removed from the whitelist.
> +
> +Example:
> +      A
> +     / \
> +        B
> +
> +    group        behavior	exceptions
> +    A            allow		"b 8:* rwm", "c 116:1 rw"
> +    B            deny		"c 1:3 rwm", "c 116:2 rwm", "b 3:* rwm"
> +
> +If a device is denied in group A:
> +	# echo "c 116:* r" > A/devices.deny
> +it'll propagate down and after revalidating B's entries, the whitelist entry
> +"c 116:2 rwm" will be removed:
> +
> +    group        whitelist entries                        denied devices
> +    A            all                                      "b 8:* rwm", "c 116:* rw"
> +    B            "c 1:3 rwm", "b 3:* rwm"                 all the rest
> +
> +In case parent's exceptions change and local exceptions are not allowed
> +anymore, they'll be deleted.
> +
> +Notice that new whitelist entries will not be propagated:
> +      A
> +     / \
> +        B
> +
> +    group        whitelist entries                        denied devices
> +    A            "c 1:3 rwm", "c 1:5 r"                   all the rest
> +    B            "c 1:3 rwm", "c 1:5 r"                   all the rest
> +
> +when adding "c *:3 rwm":
> +	# echo "c *:3 rwm" >A/devices.allow
> +
> +the result:
> +    group        whitelist entries                        denied devices
> +    A            "c *:3 rwm", "c 1:5 r"                   all the rest
> +    B            "c 1:3 rwm", "c 1:5 r"                   all the rest
> +
> +but now it'll be possible to add new entries to B:
> +	# echo "c 2:3 rwm" >B/devices.allow
> +	# echo "c 50:3 r" >B/devices.allow
> +or even
> +	# echo "c *:3 rwm" >B/devices.allow
> +
> +Allowing or denying all by writing 'a' to devices.allow or devices.deny will
> +not be possible once the device cgroups has children.
> +
> +4.1 Hierarchy (internal implementation)
> +
> +device cgroups is implemented internally using a behavior (ALLOW, DENY) and a
> +list of exceptions.  The internal state is controlled using the same user
> +interface to preserve compatibility with the previous whitelist-only
> +implementation.  Removal or addition of exceptions that will reduce the access
> +to devices will be propagated down the hierarchy.
> +For every propagated exception, the effective rules will be re-evaluated based
> +on current parent's access rules.
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v5 4/4] devcg: propagate local changes down the hierarchy

[Aristeu Rozanski]

On Tue, Feb 19, 2013 at 09:12:08PM +0000, Serge E. Hallyn wrote:
> Quoting Aristeu Rozanski (aris-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org):
> > +		} else {
> > +			/*
> > +			 * in the other possible cases:
> > +			 * root's behavior: allow, devcg's: deny
> > +			 * root's behavior: deny, devcg's: deny
> > +			 * the exception will be removed
> > +			 */
> 
> Technically this case isn't needed, right?  Will the dev_exception_rm()
> also be done by revalidate_active_exceptions()?  So it's safe (but
> not necessary) to drop the else here.  Though the comment is very
> informative, and it might be worth keeping the code as is for clarity.

that's correct, it'll end up being removed by
revalidate_active_exceptions(). if others have no objection, I'll keep
it

Thanks for the reviews Serge, much appreciated

-- 
Aristeu

Re: [PATCH v5 4/4] devcg: propagate local changes down the hierarchy

[Aristeu Rozanski]

Hi Tejun,
On Tue, Feb 19, 2013 at 04:20:16PM -0500, Aristeu Rozanski wrote:
> On Tue, Feb 19, 2013 at 09:12:08PM +0000, Serge E. Hallyn wrote:
> > Quoting Aristeu Rozanski (aris-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org):
> > > +		} else {
> > > +			/*
> > > +			 * in the other possible cases:
> > > +			 * root's behavior: allow, devcg's: deny
> > > +			 * root's behavior: deny, devcg's: deny
> > > +			 * the exception will be removed
> > > +			 */
> > 
> > Technically this case isn't needed, right?  Will the dev_exception_rm()
> > also be done by revalidate_active_exceptions()?  So it's safe (but
> > not necessary) to drop the else here.  Though the comment is very
> > informative, and it might be worth keeping the code as is for clarity.
> 
> that's correct, it'll end up being removed by
> revalidate_active_exceptions(). if others have no objection, I'll keep
> it
> 
> Thanks for the reviews Serge, much appreciated

did you pick this series up for 3.10 or you want me to resubmit?

-- 
Aristeu

Re: [PATCH v5 0/4] devcg: introduce proper hierarchy support

[Serge Hallyn]

Quoting Aristeu Rozanski (aris-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org):
> This patchset implements device cgroup hierarchy. Exceptions will be
> propagated down in the tree and local preferences will be re-evaluated
> everytime a change in its parent occours, reapplying them if it's still
> possible.

Thanks Aristeu!  I may not be able to look at it until monday, but will
take a close look.

-serge

Re: [PATCH v5 0/4] devcg: introduce proper hierarchy support

[Tejun Heo]

On Fri, Feb 15, 2013 at 11:55:43AM -0500, Aristeu Rozanski wrote:
> This patchset implements device cgroup hierarchy. Exceptions will be
> propagated down in the tree and local preferences will be re-evaluated
> everytime a change in its parent occours, reapplying them if it's still
> possible.

Applied to cgroup/for-3.10.

Thanks!

-- 
tejun