[PATCH v4 0/9] devcg: introduce proper hierarchy support

[PATCH v4 0/9] devcg: introduce proper hierarchy support

[aris]

This patchset implements device cgroup hierarchy. Behaviors and 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

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@kernel.org>                                                                                               
Cc: Serge Hallyn <serge.hallyn@canonical.com>                                                                               
Signed-off-by: Aristeu Rozanski <aris@redhat.com>                                                                           

-- 
Aristeu

[PATCH v4 1/9] device_cgroup: prepare exception list handling functions for two lists

[aris]

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

In the following patches, device_cgroup structure will have two sets of
behavior and exceptions list (actual one, another with the local settings)
so rework the functions to use exception list, not a device_cgroup.

Acked-by: Tejun Heo <tj@kernel.org>
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 |   18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

--- github.orig/security/device_cgroup.c	2013-01-29 11:48:50.603298122 -0500
+++ github/security/device_cgroup.c	2013-01-29 11:49:14.739657516 -0500
@@ -104,7 +104,7 @@ free_and_exit:
 /*
  * called under devcgroup_mutex
  */
-static int dev_exception_add(struct dev_cgroup *dev_cgroup,
+static int dev_exception_add(struct list_head *exceptions,
 			     struct dev_exception_item *ex)
 {
 	struct dev_exception_item *excopy, *walk;
@@ -115,7 +115,7 @@ static int dev_exception_add(struct dev_
 	if (!excopy)
 		return -ENOMEM;
 
-	list_for_each_entry(walk, &dev_cgroup->exceptions, list) {
+	list_for_each_entry(walk, exceptions, list) {
 		if (walk->type != ex->type)
 			continue;
 		if (walk->major != ex->major)
@@ -129,21 +129,21 @@ static int dev_exception_add(struct dev_
 	}
 
 	if (excopy != NULL)
-		list_add_tail_rcu(&excopy->list, &dev_cgroup->exceptions);
+		list_add_tail_rcu(&excopy->list, exceptions);
 	return 0;
 }
 
 /*
  * called under devcgroup_mutex
  */
-static void dev_exception_rm(struct dev_cgroup *dev_cgroup,
+static void dev_exception_rm(struct list_head *exceptions,
 			     struct dev_exception_item *ex)
 {
 	struct dev_exception_item *walk, *tmp;
 
 	lockdep_assert_held(&devcgroup_mutex);
 
-	list_for_each_entry_safe(walk, tmp, &dev_cgroup->exceptions, list) {
+	list_for_each_entry_safe(walk, tmp, exceptions, list) {
 		if (walk->type != ex->type)
 			continue;
 		if (walk->major != ex->major)
@@ -514,10 +514,10 @@ 		case '\0':
 		 * don't want to break compatibility
 		 */
 		if (devcgroup->behavior == DEVCG_DEFAULT_ALLOW) {
-			dev_exception_rm(devcgroup, &ex);
+			dev_exception_rm(&devcgroup->exceptions, &ex);
 			return 0;
 		}
-		return dev_exception_add(devcgroup, &ex);
+		return dev_exception_add(&devcgroup->exceptions, &ex);
 	case DEVCG_DENY:
 		/*
 		 * If the default policy is to deny by default, try to remove
@@ -525,10 +525,10 @@ 			return 0;
 		 * don't want to break compatibility
 		 */
 		if (devcgroup->behavior == DEVCG_DEFAULT_DENY) {
-			dev_exception_rm(devcgroup, &ex);
+			dev_exception_rm(&devcgroup->exceptions, &ex);
 			return 0;
 		}
-		return dev_exception_add(devcgroup, &ex);
+		return dev_exception_add(&devcgroup->exceptions, &ex);
 	default:
 		return -EINVAL;
 	}

[PATCH v4 2/9] devcg: reorder device exception functions

[aris]

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

In preparation for the next patch, reorder dev_exception_add() and
dev_exception_rm().

This patch doesn't introduce any functional changes.

Acked-by: Tejun Heo <tj@kernel.org>
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 |   44 ++++++++++++++++++++++----------------------
 1 file changed, 22 insertions(+), 22 deletions(-)

--- github.orig/security/device_cgroup.c	2013-01-29 11:49:14.739657516 -0500
+++ github/security/device_cgroup.c	2013-01-29 11:49:14.987661210 -0500
@@ -104,18 +104,14 @@ free_and_exit:
 /*
  * called under devcgroup_mutex
  */
-static int dev_exception_add(struct list_head *exceptions,
+static void dev_exception_rm(struct list_head *exceptions,
 			     struct dev_exception_item *ex)
 {
-	struct dev_exception_item *excopy, *walk;
+	struct dev_exception_item *walk, *tmp;
 
 	lockdep_assert_held(&devcgroup_mutex);
 
-	excopy = kmemdup(ex, sizeof(*ex), GFP_KERNEL);
-	if (!excopy)
-		return -ENOMEM;
-
-	list_for_each_entry(walk, exceptions, list) {
+	list_for_each_entry_safe(walk, tmp, exceptions, list) {
 		if (walk->type != ex->type)
 			continue;
 		if (walk->major != ex->major)
@@ -123,27 +119,29 @@ static int dev_exception_add(struct list
 		if (walk->minor != ex->minor)
 			continue;
 
-		walk->access |= ex->access;
-		kfree(excopy);
-		excopy = NULL;
+		walk->access &= ~ex->access;
+		if (!walk->access) {
+			list_del_rcu(&walk->list);
+			kfree_rcu(walk, rcu);
+		}
 	}
-
-	if (excopy != NULL)
-		list_add_tail_rcu(&excopy->list, exceptions);
-	return 0;
 }
 
 /*
  * called under devcgroup_mutex
  */
-static void dev_exception_rm(struct list_head *exceptions,
+static int dev_exception_add(struct list_head *exceptions,
 			     struct dev_exception_item *ex)
 {
-	struct dev_exception_item *walk, *tmp;
+	struct dev_exception_item *excopy, *walk;
 
 	lockdep_assert_held(&devcgroup_mutex);
 
-	list_for_each_entry_safe(walk, tmp, exceptions, list) {
+	excopy = kmemdup(ex, sizeof(*ex), GFP_KERNEL);
+	if (!excopy)
+		return -ENOMEM;
+
+	list_for_each_entry(walk, exceptions, list) {
 		if (walk->type != ex->type)
 			continue;
 		if (walk->major != ex->major)
@@ -151,12 +149,14 @@ static void dev_exception_rm(struct list
 		if (walk->minor != ex->minor)
 			continue;
 
-		walk->access &= ~ex->access;
-		if (!walk->access) {
-			list_del_rcu(&walk->list);
-			kfree_rcu(walk, rcu);
-		}
+		walk->access |= ex->access;
+		kfree(excopy);
+		excopy = NULL;
 	}
+
+	if (excopy != NULL)
+		list_add_tail_rcu(&excopy->list, exceptions);
+	return 0;
 }
 
 static void __dev_exception_clean(struct dev_cgroup *dev_cgroup)

[PATCH v4 3/9] device_cgroup: keep track of local group settings

[aris]

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

In preparation for better hierarchy support, it's needed to retain the local
settings in order to try to reapply them after a propagated change if they're
still valid.

v2: split this patch in two, one to just move dev_exception_rm() before
    dev_exception_add() while keeping functional changes in this patch as
    requested by Tejun.

Acked-by: Tejun Heo <tj@kernel.org>
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 |   83 +++++++++++++++++++++++++++++++++++++----------
 1 file changed, 67 insertions(+), 16 deletions(-)

--- github.orig/security/device_cgroup.c	2013-01-29 11:49:14.987661210 -0500
+++ github/security/device_cgroup.c	2013-01-29 11:49:15.244665037 -0500
@@ -39,13 +39,27 @@ struct dev_exception_item {
 	struct rcu_head rcu;
 };
 
+enum devcg_behavior {
+	DEVCG_DEFAULT_NONE,
+	DEVCG_DEFAULT_ALLOW,
+	DEVCG_DEFAULT_DENY,
+};
+
 struct dev_cgroup {
 	struct cgroup_subsys_state css;
+
+	/* result of merging the parent's rules with local ones */
 	struct list_head exceptions;
-	enum {
-		DEVCG_DEFAULT_ALLOW,
-		DEVCG_DEFAULT_DENY,
-	} behavior;
+	enum devcg_behavior behavior;
+
+	/*
+	 * local set rules, saved so when a parent propagates new rules, the
+	 * local preferences can be preserved
+	 */
+	struct {
+		struct list_head exceptions;
+		enum devcg_behavior behavior;
+	} local;
 };
 
 static inline struct dev_cgroup *css_to_devcgroup(struct cgroup_subsys_state *s)
@@ -104,8 +118,8 @@ free_and_exit:
 /*
  * called under devcgroup_mutex
  */
-static void dev_exception_rm(struct list_head *exceptions,
-			     struct dev_exception_item *ex)
+static void __dev_exception_rm(struct list_head *exceptions,
+			       struct dev_exception_item *ex)
 {
 	struct dev_exception_item *walk, *tmp;
 
@@ -127,11 +141,18 @@ static void dev_exception_rm(struct list
 	}
 }
 
+static void dev_exception_rm(struct dev_cgroup *devcgroup,
+			     struct dev_exception_item *ex)
+{
+	__dev_exception_rm(&devcgroup->local.exceptions, ex);
+	__dev_exception_rm(&devcgroup->exceptions, ex);
+}
+
 /*
  * called under devcgroup_mutex
  */
-static int dev_exception_add(struct list_head *exceptions,
-			     struct dev_exception_item *ex)
+static int __dev_exception_add(struct list_head *exceptions,
+			       struct dev_exception_item *ex)
 {
 	struct dev_exception_item *excopy, *walk;
 
@@ -159,6 +180,28 @@ static int dev_exception_add(struct list
 	return 0;
 }
 
+static int dev_exception_add(struct dev_cgroup *devcgroup,
+			     struct dev_exception_item *ex)
+{
+	int rc;
+
+	lockdep_assert_held(&devcgroup_mutex);
+
+	/*
+	 * we add to the local list so we can preserve local preferences if
+	 * the parent propagates down new rules
+	 */
+	rc = __dev_exception_add(&devcgroup->local.exceptions, ex);
+	if (rc)
+		return rc;
+
+	rc = __dev_exception_add(&devcgroup->exceptions, ex);
+	if (rc)
+		__dev_exception_rm(&devcgroup->local.exceptions, ex);
+
+	return rc;
+}
+
 static void __dev_exception_clean(struct dev_cgroup *dev_cgroup)
 {
 	struct dev_exception_item *ex, *tmp;
@@ -167,6 +210,11 @@ static void __dev_exception_clean(struct
 		list_del_rcu(&ex->list);
 		kfree_rcu(ex, rcu);
 	}
+	list_for_each_entry_safe(ex, tmp, &dev_cgroup->local.exceptions,
+				 list) {
+		list_del_rcu(&ex->list);
+		kfree_rcu(ex, rcu);
+	}
 }
 
 /**
@@ -195,6 +243,8 @@ static struct cgroup_subsys_state *devcg
 	if (!dev_cgroup)
 		return ERR_PTR(-ENOMEM);
 	INIT_LIST_HEAD(&dev_cgroup->exceptions);
+	INIT_LIST_HEAD(&dev_cgroup->local.exceptions);
+	dev_cgroup->local.behavior = DEVCG_DEFAULT_NONE;
 	parent_cgroup = cgroup->parent;
 
 	if (parent_cgroup == NULL)
@@ -413,18 +463,19 @@ 	memset(&ex, 0, sizeof(ex));
 			if (!may_allow_all(parent))
 				return -EPERM;
 			dev_exception_clean(devcgroup);
+			if (parent)
+				rc = dev_exceptions_copy(&devcgroup->exceptions,
+							 &parent->exceptions);
 			devcgroup->behavior = DEVCG_DEFAULT_ALLOW;
-			if (!parent)
-				break;
+			devcgroup->local.behavior = DEVCG_DEFAULT_ALLOW;
 
-			rc = dev_exceptions_copy(&devcgroup->exceptions,
-						 &parent->exceptions);
 			if (rc)
 				return rc;
 			break;
 		case DEVCG_DENY:
 			dev_exception_clean(devcgroup);
 			devcgroup->behavior = DEVCG_DEFAULT_DENY;
+			devcgroup->local.behavior = DEVCG_DEFAULT_DENY;
 			break;
 		default:
 			return -EINVAL;
@@ -514,10 +565,10 @@ 		case '\0':
 		 * don't want to break compatibility
 		 */
 		if (devcgroup->behavior == DEVCG_DEFAULT_ALLOW) {
-			dev_exception_rm(&devcgroup->exceptions, &ex);
+			dev_exception_rm(devcgroup, &ex);
 			return 0;
 		}
-		return dev_exception_add(&devcgroup->exceptions, &ex);
+		return dev_exception_add(devcgroup, &ex);
 	case DEVCG_DENY:
 		/*
 		 * If the default policy is to deny by default, try to remove
@@ -525,10 +576,10 @@ 			return 0;
 		 * don't want to break compatibility
 		 */
 		if (devcgroup->behavior == DEVCG_DEFAULT_DENY) {
-			dev_exception_rm(&devcgroup->exceptions, &ex);
+			dev_exception_rm(devcgroup, &ex);
 			return 0;
 		}
-		return dev_exception_add(&devcgroup->exceptions, &ex);
+		return dev_exception_add(devcgroup, &ex);
 	default:
 		return -EINVAL;
 	}

[PATCH v4 4/9] devcg: expand may_access() logic

[aris]

[-- Attachment #1: split_may_access_logic.patch --]
[-- Type: text/plain, Size: 1769 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>
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-01-30 08:56:29.532063723 -0500
+++ github/security/device_cgroup.c	2013-01-30 08:58:02.934460404 -0500
@@ -355,8 +355,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;
@@ -382,16 +382,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 v4 5/9] devcg: prepare may_access() for hierarchy support

[aris]

[-- Attachment #1: strengthen-may_access.patch --]
[-- Type: text/plain, Size: 3176 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>
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 |   38 ++++++++++++++++++++++++--------------
 1 file changed, 24 insertions(+), 14 deletions(-)

--- github.orig/security/device_cgroup.c	2013-01-30 08:58:02.000000000 -0500
+++ github/security/device_cgroup.c	2013-01-30 09:00:09.435351867 -0500
@@ -354,9 +354,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;
@@ -380,19 +382,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;
 }
@@ -411,7 +421,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);
 }
 
 /**
@@ -445,7 +455,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;
@@ -663,7 +673,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 v4 6/9] devcg: use css_online and css_offline

[aris]

[-- Attachment #1: online.patch --]
[-- Type: text/plain, Size: 3408 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@kernel.org>
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 |   59 +++++++++++++++++++++++++++++++++--------------
 1 file changed, 42 insertions(+), 17 deletions(-)

--- github.orig/security/device_cgroup.c	2013-01-30 09:00:09.435351867 -0500
+++ github/security/device_cgroup.c	2013-01-30 09:09:12.572464122 -0500
@@ -230,14 +230,51 @@ static void dev_exception_clean(struct d
 	__dev_exception_clean(dev_cgroup);
 }
 
+/**
+ * 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)
@@ -245,23 +282,9 @@ static struct cgroup_subsys_state *devcg
 	INIT_LIST_HEAD(&dev_cgroup->exceptions);
 	INIT_LIST_HEAD(&dev_cgroup->local.exceptions);
 	dev_cgroup->local.behavior = DEVCG_DEFAULT_NONE;
+	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;
 }
 
@@ -635,6 +658,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 v4 7/9] devcg: split single exception copy from dev_exceptions_copy()

[aris]

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

This patch is in preparation for hierarchy support

This patch doesn't introduce any functional changes.

Acked-by: Tejun Heo <tj@kernel.org>
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 |   18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

--- github.orig/security/device_cgroup.c	2013-01-29 11:49:16.076677425 -0500
+++ github/security/device_cgroup.c	2013-01-29 11:49:16.374681863 -0500
@@ -89,20 +89,30 @@ static int devcgroup_can_attach(struct c
 	return 0;
 }
 
+static int dev_exception_copy(struct list_head *dest,
+			      struct dev_exception_item *ex)
+{
+	struct dev_exception_item *new;
+
+	new = kmemdup(ex, sizeof(*ex), GFP_KERNEL);
+	if (!new)
+		return -ENOMEM;
+	list_add_tail(&new->list, dest);
+	return 0;
+}
+
 /*
  * called under devcgroup_mutex
  */
 static int dev_exceptions_copy(struct list_head *dest, struct list_head *orig)
 {
-	struct dev_exception_item *ex, *tmp, *new;
+	struct dev_exception_item *ex, *tmp;
 
 	lockdep_assert_held(&devcgroup_mutex);
 
 	list_for_each_entry(ex, orig, list) {
-		new = kmemdup(ex, sizeof(*ex), GFP_KERNEL);
-		if (!new)
+		if (dev_exception_copy(dest, ex))
 			goto free_and_exit;
-		list_add_tail(&new->list, dest);
 	}
 
 	return 0;

[PATCH v4 8/9] devcg: refactor dev_exception_clean()

[aris]

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

This patch is in preparation for hierarchy support.

This patch doesn't introduce any functional changes.

Acked-by: Tejun Heo <tj@kernel.org>
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 |   34 ++++++++++++++++------------------
 1 file changed, 16 insertions(+), 18 deletions(-)

--- github.orig/security/device_cgroup.c	2013-01-29 11:49:16.374681863 -0500
+++ github/security/device_cgroup.c	2013-01-29 11:49:16.653686016 -0500
@@ -212,32 +212,33 @@ static int dev_exception_add(struct dev_
 	return rc;
 }
 
-static void __dev_exception_clean(struct dev_cgroup *dev_cgroup)
+static void dev_exception_clean(struct list_head *exceptions)
 {
 	struct dev_exception_item *ex, *tmp;
 
-	list_for_each_entry_safe(ex, tmp, &dev_cgroup->exceptions, list) {
-		list_del_rcu(&ex->list);
-		kfree_rcu(ex, rcu);
-	}
-	list_for_each_entry_safe(ex, tmp, &dev_cgroup->local.exceptions,
-				 list) {
+	list_for_each_entry_safe(ex, tmp, exceptions, list) {
 		list_del_rcu(&ex->list);
 		kfree_rcu(ex, rcu);
 	}
 }
 
+static void __dev_exception_clean_all(struct dev_cgroup *dev_cgroup)
+{
+	dev_exception_clean(&dev_cgroup->exceptions);
+	dev_exception_clean(&dev_cgroup->local.exceptions);
+}
+
 /**
- * dev_exception_clean - frees all entries of the exception list
+ * dev_exception_clean_all - frees all entries of the exception list
  * @dev_cgroup: dev_cgroup with the exception list to be cleaned
  *
  * called under devcgroup_mutex
  */
-static void dev_exception_clean(struct dev_cgroup *dev_cgroup)
+static void dev_exception_clean_all(struct dev_cgroup *dev_cgroup)
 {
 	lockdep_assert_held(&devcgroup_mutex);
 
-	__dev_exception_clean(dev_cgroup);
+	__dev_exception_clean_all(dev_cgroup);
 }
 
 /**
@@ -303,7 +304,7 @@ static void devcgroup_css_free(struct cg
 	struct dev_cgroup *dev_cgroup;
 
 	dev_cgroup = cgroup_to_devcgroup(cgroup);
-	__dev_exception_clean(dev_cgroup);
+	__dev_exception_clean_all(dev_cgroup);
 	kfree(dev_cgroup);
 }
 
@@ -508,25 +509,22 @@ 	memset(&ex, 0, sizeof(ex));
 		case DEVCG_ALLOW:
 			if (!may_allow_all(parent))
 				return -EPERM;
-			dev_exception_clean(devcgroup);
+			dev_exception_clean_all(devcgroup);
 			if (parent)
 				rc = dev_exceptions_copy(&devcgroup->exceptions,
 							 &parent->exceptions);
 			devcgroup->behavior = DEVCG_DEFAULT_ALLOW;
 			devcgroup->local.behavior = DEVCG_DEFAULT_ALLOW;
-
-			if (rc)
-				return rc;
 			break;
 		case DEVCG_DENY:
-			dev_exception_clean(devcgroup);
+			dev_exception_clean_all(devcgroup);
 			devcgroup->behavior = DEVCG_DEFAULT_DENY;
 			devcgroup->local.behavior = DEVCG_DEFAULT_DENY;
 			break;
 		default:
-			return -EINVAL;
+			rc = -EINVAL;
 		}
-		return 0;
+		return rc;
 	case 'b':
 		ex.type = DEV_BLOCK;
 		break;

[PATCH v4 9/9] devcg: propagate local changes down the hierarchy

[aris]

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

This patch makes all changes propagate down in hierarchy respecting when
possible local configurations.

Behavior changes will clean up exceptions in all the children except when the
parent changes the behavior from allow to deny and the child's behavior was
already deny, in which case the local exceptions will be reused. The inverse
is not possible: you can't have a parent with behavior deny and a child with
behavior accept.

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 behavior or exceptions change and local settings are not
allowed anymore, they'll be deleted.

v4:
- separated function to walk the tree and collect valid propagation targets

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

v2:
- instead of keeping the local settings that won't apply anymore, remove them

Acked-by: Tejun Heo <tj@kernel.org>
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 |   66 +++++++++++++
 security/device_cgroup.c          |  186 ++++++++++++++++++++++++++++++++++++--
 2 files changed, 246 insertions(+), 6 deletions(-)

--- github.orig/security/device_cgroup.c	2013-01-30 10:03:16.943873992 -0500
+++ github/security/device_cgroup.c	2013-01-30 10:44:23.693586209 -0500
@@ -60,6 +60,9 @@ struct dev_cgroup {
 		struct list_head exceptions;
 		enum devcg_behavior behavior;
 	} local;
+
+	/* temporary list for pending propagation operations */
+	struct list_head propagate_pending;
 };
 
 static inline struct dev_cgroup *css_to_devcgroup(struct cgroup_subsys_state *s)
@@ -241,6 +244,11 @@ static void dev_exception_clean_all(stru
 	__dev_exception_clean_all(dev_cgroup);
 }
 
+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
@@ -292,6 +300,7 @@ static struct cgroup_subsys_state *devcg
 		return ERR_PTR(-ENOMEM);
 	INIT_LIST_HEAD(&dev_cgroup->exceptions);
 	INIT_LIST_HEAD(&dev_cgroup->local.exceptions);
+	INIT_LIST_HEAD(&dev_cgroup->propagate_pending);
 	dev_cgroup->local.behavior = DEVCG_DEFAULT_NONE;
 	dev_cgroup->behavior = DEVCG_DEFAULT_NONE;
 	parent_cgroup = cgroup->parent;
@@ -471,6 +480,155 @@ static inline int may_allow_all(struct d
 	return parent->behavior == DEVCG_DEFAULT_ALLOW;
 }
 
+/**
+ * revalidate_exceptions - walks through the exception list and revalidates
+ *			     the exceptions based on parents' behavior and
+ *			     exceptions. Called with devcgroup_mutex held.
+ * @devcg: cgroup which exceptions will be checked
+ *
+ * returns: 0 in success, -ENOMEM in case of out of memory
+ *
+ * This is one of the two key functions for hierarchy implementation.
+ * This function is responsible for re-evaluating all the cgroup's locally
+ * set exceptions due to a parent's behavior or exception change.
+ * Refer to Documentation/cgroups/devices.txt for more details.
+ */
+static int revalidate_exceptions(struct dev_cgroup *devcg)
+{
+	struct dev_exception_item *ex;
+	struct list_head *this, *tmp;
+
+	list_for_each_safe(this, tmp, &devcg->local.exceptions) {
+		ex = container_of(this, struct dev_exception_item, list);
+		if (parent_has_perm(devcg, ex)) {
+			if (dev_exception_copy(&devcg->exceptions, ex))
+				goto error;
+		} else
+			__dev_exception_rm(&devcg->local.exceptions, ex);
+	}
+	return 0;
+
+error:
+	dev_exception_clean(&devcg->exceptions);
+	return -ENOMEM;
+}
+
+/**
+ * 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_behavior - propagates a change in the behavior down in hierarchy
+ * @devcg_root: device cgroup that changed behavior
+ *
+ * returns: 0 in case of success, != 0 in case of error
+ *
+ * This is one of the two key functions for hierarchy implementation.
+ * All cgroup's children recursively will have the behavior changed and
+ * exceptions copied from the parent then its local behavior and exceptions
+ * re-evaluated and applied if they're still allowed.  Refer to
+ * Documentation/cgroups/devices.txt for more details.
+ */
+static int propagate_behavior(struct dev_cgroup *devcg_root)
+{
+	struct cgroup *root = devcg_root->css.cgroup;
+	struct dev_cgroup *parent, *devcg, *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);
+
+		/* first copy parent's state */
+		devcg->behavior = parent->behavior;
+		dev_exception_clean(&devcg->exceptions);
+		rc = dev_exceptions_copy(&devcg->exceptions, &parent->exceptions);
+		if (rc) {
+			devcg->behavior = DEVCG_DEFAULT_DENY;
+			break;
+		}
+
+		if (devcg->local.behavior == DEVCG_DEFAULT_DENY &&
+		    devcg->behavior == DEVCG_DEFAULT_ALLOW) {
+			devcg->behavior = DEVCG_DEFAULT_DENY;
+		}
+		if (devcg->local.behavior == devcg->behavior)
+			rc = revalidate_exceptions(devcg);
+		if (rc)
+			break;
+		list_del_init(&devcg->propagate_pending);
+	}
+
+	return rc;
+}
+
+/**
+ * propagate_exception - propagates a new exception to the children
+ * @devcg_root: device cgroup that added a new exception
+ *
+ * returns: 0 in case of success, != 0 in case of error
+ */
+static int propagate_exception(struct dev_cgroup *devcg_root)
+{
+	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);
+
+		dev_exception_clean(&devcg->exceptions);
+		if (devcg->behavior == parent->behavior) {
+			rc = dev_exceptions_copy(&devcg->exceptions, &parent->exceptions);
+			if (rc) {
+				devcg->behavior = DEVCG_DEFAULT_DENY;
+				break;
+			}
+			rc = revalidate_exceptions(devcg);
+			if (rc)
+				break;
+		} else {
+			/* we never give more permissions to the child */
+			WARN_ONCE(devcg->behavior == DEVCG_DEFAULT_ALLOW,
+				  "devcg: parent/child behavior is inconsistent");
+			rc = revalidate_exceptions(devcg);
+			if (rc)
+				break;
+		}
+		list_del_init(&devcg->propagate_pending);
+	}
+	return rc;
+}
+
 /*
  * Modify the exception list using allow/deny rules.
  * CAP_SYS_ADMIN is needed for this.  It's at least separate from CAP_MKNOD
@@ -515,11 +673,13 @@ 	memset(&ex, 0, sizeof(ex));
 							 &parent->exceptions);
 			devcgroup->behavior = DEVCG_DEFAULT_ALLOW;
 			devcgroup->local.behavior = DEVCG_DEFAULT_ALLOW;
+			rc = propagate_behavior(devcgroup);
 			break;
 		case DEVCG_DENY:
 			dev_exception_clean_all(devcgroup);
 			devcgroup->behavior = DEVCG_DEFAULT_DENY;
 			devcgroup->local.behavior = DEVCG_DEFAULT_DENY;
+			rc = propagate_behavior(devcgroup);
 			break;
 		default:
 			rc = -EINVAL;
@@ -610,9 +770,14 @@ 		case '\0':
 		 */
 		if (devcgroup->behavior == DEVCG_DEFAULT_ALLOW) {
 			dev_exception_rm(devcgroup, &ex);
-			return 0;
+			rc = propagate_exception(devcgroup);
+			return rc;
 		}
-		return dev_exception_add(devcgroup, &ex);
+		rc = dev_exception_add(devcgroup, &ex);
+		if (!rc)
+			/* if a local behavior wasn't explicitely choosen, pick it */
+			devcgroup->local.behavior = devcgroup->behavior;
+		break;
 	case DEVCG_DENY:
 		/*
 		 * If the default policy is to deny by default, try to remove
@@ -621,13 +786,22 @@ 			return 0;
 		 */
 		if (devcgroup->behavior == DEVCG_DEFAULT_DENY) {
 			dev_exception_rm(devcgroup, &ex);
-			return 0;
+			rc = propagate_exception(devcgroup);
+			return rc;
 		}
-		return dev_exception_add(devcgroup, &ex);
+		rc = dev_exception_add(devcgroup, &ex);
+		if (rc)
+			return rc;
+		/* we only propagate new restrictions */
+		rc = propagate_exception(devcgroup);
+		if (!rc)
+			/* if a local behavior wasn't explicitely choosen, pick it */
+			devcgroup->local.behavior = devcgroup->behavior;
+		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-01-29 14:39:17.721843991 -0500
+++ github/Documentation/cgroups/devices.txt	2013-01-30 10:03:30.536076528 -0500
@@ -50,3 +50,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 never a cgroup 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 the the whitelist and all the locally set whitelist entries 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 behavior or exceptions change and local settings 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 not 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
+
+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.  A change in behavior (writing "a" to
+devices.deny or devices.allow) will be propagated down the hierarchy as well
+new exceptions that will reduce the access to devices (an exception when
+behavior is ALLOW).  Each cgroup will have its local behavior and exception
+list to keep track what was set by the user for that cgroup in specific.  For
+every propagated change, the effective rules will be built starting with
+parent's rules and the locally set behavior and exceptions in case they still
+apply, otherwise those will be lost.

Re: [PATCH v4 1/9] device_cgroup: prepare exception list handling functions for two lists

[Serge E. Hallyn]

Quoting aris@redhat.com (aris@redhat.com):
> In the following patches, device_cgroup structure will have two sets of
> behavior and exceptions list (actual one, another with the local settings)
> so rework the functions to use exception list, not a device_cgroup.
> 
> Acked-by: Tejun Heo <tj@kernel.org>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Serge Hallyn <serge.hallyn@canonical.com>

Sorry - just to keep it all nicely in one thread,

Acked-by: Serge Hallyn <serge.hallyn@canonical.com>

> Signed-off-by: Aristeu Rozanski <aris@redhat.com>
> 
> ---
>  security/device_cgroup.c |   18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> --- github.orig/security/device_cgroup.c	2013-01-29 11:48:50.603298122 -0500
> +++ github/security/device_cgroup.c	2013-01-29 11:49:14.739657516 -0500
> @@ -104,7 +104,7 @@ free_and_exit:
>  /*
>   * called under devcgroup_mutex
>   */
> -static int dev_exception_add(struct dev_cgroup *dev_cgroup,
> +static int dev_exception_add(struct list_head *exceptions,
>  			     struct dev_exception_item *ex)
>  {
>  	struct dev_exception_item *excopy, *walk;
> @@ -115,7 +115,7 @@ static int dev_exception_add(struct dev_
>  	if (!excopy)
>  		return -ENOMEM;
>  
> -	list_for_each_entry(walk, &dev_cgroup->exceptions, list) {
> +	list_for_each_entry(walk, exceptions, list) {
>  		if (walk->type != ex->type)
>  			continue;
>  		if (walk->major != ex->major)
> @@ -129,21 +129,21 @@ static int dev_exception_add(struct dev_
>  	}
>  
>  	if (excopy != NULL)
> -		list_add_tail_rcu(&excopy->list, &dev_cgroup->exceptions);
> +		list_add_tail_rcu(&excopy->list, exceptions);
>  	return 0;
>  }
>  
>  /*
>   * called under devcgroup_mutex
>   */
> -static void dev_exception_rm(struct dev_cgroup *dev_cgroup,
> +static void dev_exception_rm(struct list_head *exceptions,
>  			     struct dev_exception_item *ex)
>  {
>  	struct dev_exception_item *walk, *tmp;
>  
>  	lockdep_assert_held(&devcgroup_mutex);
>  
> -	list_for_each_entry_safe(walk, tmp, &dev_cgroup->exceptions, list) {
> +	list_for_each_entry_safe(walk, tmp, exceptions, list) {
>  		if (walk->type != ex->type)
>  			continue;
>  		if (walk->major != ex->major)
> @@ -514,10 +514,10 @@ 		case '\0':
>  		 * don't want to break compatibility
>  		 */
>  		if (devcgroup->behavior == DEVCG_DEFAULT_ALLOW) {
> -			dev_exception_rm(devcgroup, &ex);
> +			dev_exception_rm(&devcgroup->exceptions, &ex);
>  			return 0;
>  		}
> -		return dev_exception_add(devcgroup, &ex);
> +		return dev_exception_add(&devcgroup->exceptions, &ex);
>  	case DEVCG_DENY:
>  		/*
>  		 * If the default policy is to deny by default, try to remove
> @@ -525,10 +525,10 @@ 			return 0;
>  		 * don't want to break compatibility
>  		 */
>  		if (devcgroup->behavior == DEVCG_DEFAULT_DENY) {
> -			dev_exception_rm(devcgroup, &ex);
> +			dev_exception_rm(&devcgroup->exceptions, &ex);
>  			return 0;
>  		}
> -		return dev_exception_add(devcgroup, &ex);
> +		return dev_exception_add(&devcgroup->exceptions, &ex);
>  	default:
>  		return -EINVAL;
>  	}
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4 2/9] devcg: reorder device exception functions

[Serge E. Hallyn]

Quoting aris@redhat.com (aris@redhat.com):
> In preparation for the next patch, reorder dev_exception_add() and
> dev_exception_rm().
> 
> This patch doesn't introduce any functional changes.
> 
> Acked-by: Tejun Heo <tj@kernel.org>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Serge Hallyn <serge.hallyn@canonical.com>

Acked-by: Serge Hallyn <serge.hallyn@canonical.com>

> Signed-off-by: Aristeu Rozanski <aris@redhat.com>
> 
> ---
>  security/device_cgroup.c |   44 ++++++++++++++++++++++----------------------
>  1 file changed, 22 insertions(+), 22 deletions(-)
> 
> --- github.orig/security/device_cgroup.c	2013-01-29 11:49:14.739657516 -0500
> +++ github/security/device_cgroup.c	2013-01-29 11:49:14.987661210 -0500
> @@ -104,18 +104,14 @@ free_and_exit:
>  /*
>   * called under devcgroup_mutex
>   */
> -static int dev_exception_add(struct list_head *exceptions,
> +static void dev_exception_rm(struct list_head *exceptions,
>  			     struct dev_exception_item *ex)
>  {
> -	struct dev_exception_item *excopy, *walk;
> +	struct dev_exception_item *walk, *tmp;
>  
>  	lockdep_assert_held(&devcgroup_mutex);
>  
> -	excopy = kmemdup(ex, sizeof(*ex), GFP_KERNEL);
> -	if (!excopy)
> -		return -ENOMEM;
> -
> -	list_for_each_entry(walk, exceptions, list) {
> +	list_for_each_entry_safe(walk, tmp, exceptions, list) {
>  		if (walk->type != ex->type)
>  			continue;
>  		if (walk->major != ex->major)
> @@ -123,27 +119,29 @@ static int dev_exception_add(struct list
>  		if (walk->minor != ex->minor)
>  			continue;
>  
> -		walk->access |= ex->access;
> -		kfree(excopy);
> -		excopy = NULL;
> +		walk->access &= ~ex->access;
> +		if (!walk->access) {
> +			list_del_rcu(&walk->list);
> +			kfree_rcu(walk, rcu);
> +		}
>  	}
> -
> -	if (excopy != NULL)
> -		list_add_tail_rcu(&excopy->list, exceptions);
> -	return 0;
>  }
>  
>  /*
>   * called under devcgroup_mutex
>   */
> -static void dev_exception_rm(struct list_head *exceptions,
> +static int dev_exception_add(struct list_head *exceptions,
>  			     struct dev_exception_item *ex)
>  {
> -	struct dev_exception_item *walk, *tmp;
> +	struct dev_exception_item *excopy, *walk;
>  
>  	lockdep_assert_held(&devcgroup_mutex);
>  
> -	list_for_each_entry_safe(walk, tmp, exceptions, list) {
> +	excopy = kmemdup(ex, sizeof(*ex), GFP_KERNEL);
> +	if (!excopy)
> +		return -ENOMEM;
> +
> +	list_for_each_entry(walk, exceptions, list) {
>  		if (walk->type != ex->type)
>  			continue;
>  		if (walk->major != ex->major)
> @@ -151,12 +149,14 @@ static void dev_exception_rm(struct list
>  		if (walk->minor != ex->minor)
>  			continue;
>  
> -		walk->access &= ~ex->access;
> -		if (!walk->access) {
> -			list_del_rcu(&walk->list);
> -			kfree_rcu(walk, rcu);
> -		}
> +		walk->access |= ex->access;
> +		kfree(excopy);
> +		excopy = NULL;
>  	}
> +
> +	if (excopy != NULL)
> +		list_add_tail_rcu(&excopy->list, exceptions);
> +	return 0;
>  }
>  
>  static void __dev_exception_clean(struct dev_cgroup *dev_cgroup)
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4 3/9] device_cgroup: keep track of local group settings

[Serge E. Hallyn]

Quoting aris@redhat.com (aris@redhat.com):
> In preparation for better hierarchy support, it's needed to retain the local
> settings in order to try to reapply them after a propagated change if they're
> still valid.
> 
> v2: split this patch in two, one to just move dev_exception_rm() before
>     dev_exception_add() while keeping functional changes in this patch as
>     requested by Tejun.
> 
> Acked-by: Tejun Heo <tj@kernel.org>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Serge Hallyn <serge.hallyn@canonical.com>

Acked-by: Serge Hallyn <serge.hallyn@canonical.com>

> Signed-off-by: Aristeu Rozanski <aris@redhat.com>
>  
> ---
>  security/device_cgroup.c |   83 +++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 67 insertions(+), 16 deletions(-)
> 
> --- github.orig/security/device_cgroup.c	2013-01-29 11:49:14.987661210 -0500
> +++ github/security/device_cgroup.c	2013-01-29 11:49:15.244665037 -0500
> @@ -39,13 +39,27 @@ struct dev_exception_item {
>  	struct rcu_head rcu;
>  };
>  
> +enum devcg_behavior {
> +	DEVCG_DEFAULT_NONE,
> +	DEVCG_DEFAULT_ALLOW,
> +	DEVCG_DEFAULT_DENY,
> +};
> +
>  struct dev_cgroup {
>  	struct cgroup_subsys_state css;
> +
> +	/* result of merging the parent's rules with local ones */
>  	struct list_head exceptions;
> -	enum {
> -		DEVCG_DEFAULT_ALLOW,
> -		DEVCG_DEFAULT_DENY,
> -	} behavior;
> +	enum devcg_behavior behavior;
> +
> +	/*
> +	 * local set rules, saved so when a parent propagates new rules, the
> +	 * local preferences can be preserved
> +	 */
> +	struct {
> +		struct list_head exceptions;
> +		enum devcg_behavior behavior;
> +	} local;
>  };
>  
>  static inline struct dev_cgroup *css_to_devcgroup(struct cgroup_subsys_state *s)
> @@ -104,8 +118,8 @@ free_and_exit:
>  /*
>   * called under devcgroup_mutex
>   */
> -static void dev_exception_rm(struct list_head *exceptions,
> -			     struct dev_exception_item *ex)
> +static void __dev_exception_rm(struct list_head *exceptions,
> +			       struct dev_exception_item *ex)
>  {
>  	struct dev_exception_item *walk, *tmp;
>  
> @@ -127,11 +141,18 @@ static void dev_exception_rm(struct list
>  	}
>  }
>  
> +static void dev_exception_rm(struct dev_cgroup *devcgroup,
> +			     struct dev_exception_item *ex)
> +{
> +	__dev_exception_rm(&devcgroup->local.exceptions, ex);
> +	__dev_exception_rm(&devcgroup->exceptions, ex);
> +}
> +
>  /*
>   * called under devcgroup_mutex
>   */
> -static int dev_exception_add(struct list_head *exceptions,
> -			     struct dev_exception_item *ex)
> +static int __dev_exception_add(struct list_head *exceptions,
> +			       struct dev_exception_item *ex)
>  {
>  	struct dev_exception_item *excopy, *walk;
>  
> @@ -159,6 +180,28 @@ static int dev_exception_add(struct list
>  	return 0;
>  }
>  
> +static int dev_exception_add(struct dev_cgroup *devcgroup,
> +			     struct dev_exception_item *ex)
> +{
> +	int rc;
> +
> +	lockdep_assert_held(&devcgroup_mutex);
> +
> +	/*
> +	 * we add to the local list so we can preserve local preferences if
> +	 * the parent propagates down new rules
> +	 */
> +	rc = __dev_exception_add(&devcgroup->local.exceptions, ex);
> +	if (rc)
> +		return rc;
> +
> +	rc = __dev_exception_add(&devcgroup->exceptions, ex);
> +	if (rc)
> +		__dev_exception_rm(&devcgroup->local.exceptions, ex);
> +
> +	return rc;
> +}
> +
>  static void __dev_exception_clean(struct dev_cgroup *dev_cgroup)
>  {
>  	struct dev_exception_item *ex, *tmp;
> @@ -167,6 +210,11 @@ static void __dev_exception_clean(struct
>  		list_del_rcu(&ex->list);
>  		kfree_rcu(ex, rcu);
>  	}
> +	list_for_each_entry_safe(ex, tmp, &dev_cgroup->local.exceptions,
> +				 list) {
> +		list_del_rcu(&ex->list);
> +		kfree_rcu(ex, rcu);
> +	}
>  }
>  
>  /**
> @@ -195,6 +243,8 @@ static struct cgroup_subsys_state *devcg
>  	if (!dev_cgroup)
>  		return ERR_PTR(-ENOMEM);
>  	INIT_LIST_HEAD(&dev_cgroup->exceptions);
> +	INIT_LIST_HEAD(&dev_cgroup->local.exceptions);
> +	dev_cgroup->local.behavior = DEVCG_DEFAULT_NONE;
>  	parent_cgroup = cgroup->parent;
>  
>  	if (parent_cgroup == NULL)
> @@ -413,18 +463,19 @@ 	memset(&ex, 0, sizeof(ex));
>  			if (!may_allow_all(parent))
>  				return -EPERM;
>  			dev_exception_clean(devcgroup);
> +			if (parent)
> +				rc = dev_exceptions_copy(&devcgroup->exceptions,
> +							 &parent->exceptions);
>  			devcgroup->behavior = DEVCG_DEFAULT_ALLOW;
> -			if (!parent)
> -				break;
> +			devcgroup->local.behavior = DEVCG_DEFAULT_ALLOW;
>  
> -			rc = dev_exceptions_copy(&devcgroup->exceptions,
> -						 &parent->exceptions);
>  			if (rc)
>  				return rc;
>  			break;
>  		case DEVCG_DENY:
>  			dev_exception_clean(devcgroup);
>  			devcgroup->behavior = DEVCG_DEFAULT_DENY;
> +			devcgroup->local.behavior = DEVCG_DEFAULT_DENY;
>  			break;
>  		default:
>  			return -EINVAL;
> @@ -514,10 +565,10 @@ 		case '\0':
>  		 * don't want to break compatibility
>  		 */
>  		if (devcgroup->behavior == DEVCG_DEFAULT_ALLOW) {
> -			dev_exception_rm(&devcgroup->exceptions, &ex);
> +			dev_exception_rm(devcgroup, &ex);
>  			return 0;
>  		}
> -		return dev_exception_add(&devcgroup->exceptions, &ex);
> +		return dev_exception_add(devcgroup, &ex);
>  	case DEVCG_DENY:
>  		/*
>  		 * If the default policy is to deny by default, try to remove
> @@ -525,10 +576,10 @@ 			return 0;
>  		 * don't want to break compatibility
>  		 */
>  		if (devcgroup->behavior == DEVCG_DEFAULT_DENY) {
> -			dev_exception_rm(&devcgroup->exceptions, &ex);
> +			dev_exception_rm(devcgroup, &ex);
>  			return 0;
>  		}
> -		return dev_exception_add(&devcgroup->exceptions, &ex);
> +		return dev_exception_add(devcgroup, &ex);
>  	default:
>  		return -EINVAL;
>  	}
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4 4/9] devcg: expand may_access() logic

[Serge E. Hallyn]

Quoting aris@redhat.com (aris@redhat.com):
> 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>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Serge Hallyn <serge.hallyn@canonical.com>

Acked-by: 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-01-30 08:56:29.532063723 -0500
> +++ github/security/device_cgroup.c	2013-01-30 08:58:02.934460404 -0500
> @@ -355,8 +355,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;
> @@ -382,16 +382,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;
>  }
>  
>  /*
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4 5/9] devcg: prepare may_access() for hierarchy support

[Serge E. Hallyn]

Quoting aris@redhat.com (aris@redhat.com):
> 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>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Serge Hallyn <serge.hallyn@canonical.com>

Acked-by: Serge Hallyn <serge.hallyn@canonical.com>

> Signed-off-by: Aristeu Rozanski <aris@redhat.com>
> 
> ---
>  security/device_cgroup.c |   38 ++++++++++++++++++++++++--------------
>  1 file changed, 24 insertions(+), 14 deletions(-)
> 
> --- github.orig/security/device_cgroup.c	2013-01-30 08:58:02.000000000 -0500
> +++ github/security/device_cgroup.c	2013-01-30 09:00:09.435351867 -0500
> @@ -354,9 +354,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;
> @@ -380,19 +382,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;
>  }
> @@ -411,7 +421,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);
>  }
>  
>  /**
> @@ -445,7 +455,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;

This here points out that (unrelated to yor set) devcgroup_update_access()
really needs a cleanup.

>  	struct dev_exception_item ex;
>  	struct cgroup *p = devcgroup->css.cgroup;
>  	struct dev_cgroup *parent = NULL;
> @@ -663,7 +673,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)
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4 6/9] devcg: use css_online and css_offline

[Serge E. Hallyn]

Quoting aris@redhat.com (aris@redhat.com):
> 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@kernel.org>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Serge Hallyn <serge.hallyn@canonical.com>

Acked-by: Serge Hallyn <serge.hallyn@canonical.com>

> Signed-off-by: Aristeu Rozanski <aris@redhat.com>
> 
> ---
>  security/device_cgroup.c |   59 +++++++++++++++++++++++++++++++++--------------
>  1 file changed, 42 insertions(+), 17 deletions(-)
> 
> --- github.orig/security/device_cgroup.c	2013-01-30 09:00:09.435351867 -0500
> +++ github/security/device_cgroup.c	2013-01-30 09:09:12.572464122 -0500
> @@ -230,14 +230,51 @@ static void dev_exception_clean(struct d
>  	__dev_exception_clean(dev_cgroup);
>  }
>  
> +/**
> + * 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)
> @@ -245,23 +282,9 @@ static struct cgroup_subsys_state *devcg
>  	INIT_LIST_HEAD(&dev_cgroup->exceptions);
>  	INIT_LIST_HEAD(&dev_cgroup->local.exceptions);
>  	dev_cgroup->local.behavior = DEVCG_DEFAULT_NONE;
> +	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;
>  }
>  
> @@ -635,6 +658,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,
>  
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4 7/9] devcg: split single exception copy from dev_exceptions_copy()

[Serge E. Hallyn]

Quoting aris@redhat.com (aris@redhat.com):
> This patch is in preparation for hierarchy support
> 
> This patch doesn't introduce any functional changes.
> 
> Acked-by: Tejun Heo <tj@kernel.org>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Serge Hallyn <serge.hallyn@canonical.com>

Acked-by: Serge Hallyn <serge.hallyn@canonical.com>

> Signed-off-by: Aristeu Rozanski <aris@redhat.com>
> 
> ---
>  security/device_cgroup.c |   18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> --- github.orig/security/device_cgroup.c	2013-01-29 11:49:16.076677425 -0500
> +++ github/security/device_cgroup.c	2013-01-29 11:49:16.374681863 -0500
> @@ -89,20 +89,30 @@ static int devcgroup_can_attach(struct c
>  	return 0;
>  }
>  
> +static int dev_exception_copy(struct list_head *dest,
> +			      struct dev_exception_item *ex)
> +{
> +	struct dev_exception_item *new;
> +
> +	new = kmemdup(ex, sizeof(*ex), GFP_KERNEL);
> +	if (!new)
> +		return -ENOMEM;
> +	list_add_tail(&new->list, dest);
> +	return 0;
> +}
> +
>  /*
>   * called under devcgroup_mutex
>   */
>  static int dev_exceptions_copy(struct list_head *dest, struct list_head *orig)
>  {
> -	struct dev_exception_item *ex, *tmp, *new;
> +	struct dev_exception_item *ex, *tmp;
>  
>  	lockdep_assert_held(&devcgroup_mutex);
>  
>  	list_for_each_entry(ex, orig, list) {
> -		new = kmemdup(ex, sizeof(*ex), GFP_KERNEL);
> -		if (!new)
> +		if (dev_exception_copy(dest, ex))
>  			goto free_and_exit;
> -		list_add_tail(&new->list, dest);
>  	}
>  
>  	return 0;
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4 8/9] devcg: refactor dev_exception_clean()

[Serge E. Hallyn]

Quoting aris@redhat.com (aris@redhat.com):
> This patch is in preparation for hierarchy support.
> 
> This patch doesn't introduce any functional changes.
> 
> Acked-by: Tejun Heo <tj@kernel.org>
> 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 |   34 ++++++++++++++++------------------
>  1 file changed, 16 insertions(+), 18 deletions(-)
> 
> --- github.orig/security/device_cgroup.c	2013-01-29 11:49:16.374681863 -0500
> +++ github/security/device_cgroup.c	2013-01-29 11:49:16.653686016 -0500
> @@ -212,32 +212,33 @@ static int dev_exception_add(struct dev_
>  	return rc;
>  }
>  
> -static void __dev_exception_clean(struct dev_cgroup *dev_cgroup)
> +static void dev_exception_clean(struct list_head *exceptions)
>  {
>  	struct dev_exception_item *ex, *tmp;
>  
> -	list_for_each_entry_safe(ex, tmp, &dev_cgroup->exceptions, list) {
> -		list_del_rcu(&ex->list);
> -		kfree_rcu(ex, rcu);
> -	}
> -	list_for_each_entry_safe(ex, tmp, &dev_cgroup->local.exceptions,
> -				 list) {
> +	list_for_each_entry_safe(ex, tmp, exceptions, list) {
>  		list_del_rcu(&ex->list);
>  		kfree_rcu(ex, rcu);
>  	}
>  }
>  
> +static void __dev_exception_clean_all(struct dev_cgroup *dev_cgroup)
> +{
> +	dev_exception_clean(&dev_cgroup->exceptions);
> +	dev_exception_clean(&dev_cgroup->local.exceptions);
> +}
> +
>  /**
> - * dev_exception_clean - frees all entries of the exception list
> + * dev_exception_clean_all - frees all entries of the exception list
>   * @dev_cgroup: dev_cgroup with the exception list to be cleaned
>   *
>   * called under devcgroup_mutex
>   */
> -static void dev_exception_clean(struct dev_cgroup *dev_cgroup)
> +static void dev_exception_clean_all(struct dev_cgroup *dev_cgroup)
>  {
>  	lockdep_assert_held(&devcgroup_mutex);
>  
> -	__dev_exception_clean(dev_cgroup);
> +	__dev_exception_clean_all(dev_cgroup);
>  }
>  
>  /**
> @@ -303,7 +304,7 @@ static void devcgroup_css_free(struct cg
>  	struct dev_cgroup *dev_cgroup;
>  
>  	dev_cgroup = cgroup_to_devcgroup(cgroup);
> -	__dev_exception_clean(dev_cgroup);
> +	__dev_exception_clean_all(dev_cgroup);
>  	kfree(dev_cgroup);
>  }
>  
> @@ -508,25 +509,22 @@ 	memset(&ex, 0, sizeof(ex));
>  		case DEVCG_ALLOW:
>  			if (!may_allow_all(parent))
>  				return -EPERM;
> -			dev_exception_clean(devcgroup);
> +			dev_exception_clean_all(devcgroup);
>  			if (parent)
>  				rc = dev_exceptions_copy(&devcgroup->exceptions,
>  							 &parent->exceptions);
>  			devcgroup->behavior = DEVCG_DEFAULT_ALLOW;
>  			devcgroup->local.behavior = DEVCG_DEFAULT_ALLOW;
> -
> -			if (rc)
> -				return rc;

Was this intentional?

I see that you next add

	rc = propagate_behavior(devcgroup);

right here, but this means you're ignoring failure in the
dev_exceptions_copy() call above.

>  			break;
>  		case DEVCG_DENY:
> -			dev_exception_clean(devcgroup);
> +			dev_exception_clean_all(devcgroup);
>  			devcgroup->behavior = DEVCG_DEFAULT_DENY;
>  			devcgroup->local.behavior = DEVCG_DEFAULT_DENY;
>  			break;
>  		default:
> -			return -EINVAL;
> +			rc = -EINVAL;
>  		}
> -		return 0;
> +		return rc;
>  	case 'b':
>  		ex.type = DEV_BLOCK;
>  		break;
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4 8/9] devcg: refactor dev_exception_clean()

[Aristeu Rozanski]

On Wed, Jan 30, 2013 at 08:47:30PM +0000, Serge E. Hallyn wrote:
> Quoting aris@redhat.com (aris@redhat.com):
> > -
> > -			if (rc)
> > -				return rc;
> 
> Was this intentional?
> 
> I see that you next add
> 
> 	rc = propagate_behavior(devcgroup);
> 
> right here, but this means you're ignoring failure in the
> dev_exceptions_copy() call above.

that's not intentional. thanks for catching this

Tejun, you want me to resubmit the whole series or just the next patch
(where I was supposed to move that chunk)?

-- 
Aristeu

Re: [PATCH v4 8/9] devcg: refactor dev_exception_clean()

[Tejun Heo]

Hello,

On Wed, Jan 30, 2013 at 12:49 PM, Aristeu Rozanski > that's not
intentional. thanks for catching this
>
> Tejun, you want me to resubmit the whole series or just the next patch
> (where I was supposed to move that chunk)?

If it doesn't affect the next patch, just posting an updated version
of this patch should be enough, I think. Which tree are these patches
planned to go through?

Thanks.

-- 
tejun

Re: [PATCH v4 9/9] devcg: propagate local changes down the hierarchy

[Serge E. Hallyn]

Quoting aris@redhat.com (aris@redhat.com):
Still looking over the code changes, but:

>  static int devcgroup_access_write(struct cgroup *cgrp, struct cftype *cft,
> --- github.orig/Documentation/cgroups/devices.txt	2013-01-29 14:39:17.721843991 -0500
> +++ github/Documentation/cgroups/devices.txt	2013-01-30 10:03:30.536076528 -0500
> @@ -50,3 +50,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 never a cgroup has more

making sure a cgroup never has...

> +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 the the whitelist and all the locally set whitelist entries re-evaluated.

s/the the/their/

and

...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 behavior or exceptions change and local settings 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 not it'll be possible to add new entries to B:

"but now" ?

> +	# 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
> +
> +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.  A change in behavior (writing "a" to

... to preserve compatibility with the previous whitelist-only
implementation.

> +devices.deny or devices.allow) will be propagated down the hierarchy as well

"as well as" ?

> +new exceptions that will reduce the access to devices (an exception when
> +behavior is ALLOW).  Each cgroup will have its local behavior and exception
> +list to keep track what was set by the user for that cgroup in specific.  For
> +every propagated change, the effective rules will be built starting with
> +parent's rules and the locally set behavior and exceptions in case they still
> +apply, otherwise those will be lost.
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4 8/9] devcg: refactor dev_exception_clean()

[Li Zefan]

On 2013/1/31 4:50, Tejun Heo wrote:
> Hello,
> 
> On Wed, Jan 30, 2013 at 12:49 PM, Aristeu Rozanski > that's not
> intentional. thanks for catching this
>>
>> Tejun, you want me to resubmit the whole series or just the next patch
>> (where I was supposed to move that chunk)?
> 
> If it doesn't affect the next patch, just posting an updated version
> of this patch should be enough, I think. Which tree are these patches
> planned to go through?
> 

It used to be -mm tree, but I bet Andrew would be happy to see you
route them through your tree. ;)

Re: [PATCH v4 9/9] devcg: propagate local changes down the hierarchy

[Serge E. Hallyn]

Quoting aris@redhat.com (aris@redhat.com):
> +/**
> + * propagate_behavior - propagates a change in the behavior down in hierarchy
> + * @devcg_root: device cgroup that changed behavior
> + *
> + * returns: 0 in case of success, != 0 in case of error
> + *
> + * This is one of the two key functions for hierarchy implementation.
> + * All cgroup's children recursively will have the behavior changed and
> + * exceptions copied from the parent then its local behavior and exceptions
> + * re-evaluated and applied if they're still allowed.  Refer to
> + * Documentation/cgroups/devices.txt for more details.
> + */
> +static int propagate_behavior(struct dev_cgroup *devcg_root)
> +{
> +	struct cgroup *root = devcg_root->css.cgroup;
> +	struct dev_cgroup *parent, *devcg, *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);
> +
> +		/* first copy parent's state */
> +		devcg->behavior = parent->behavior;
> +		dev_exception_clean(&devcg->exceptions);
> +		rc = dev_exceptions_copy(&devcg->exceptions, &parent->exceptions);
> +		if (rc) {
> +			devcg->behavior = DEVCG_DEFAULT_DENY;
> +			break;
> +		}
> +
> +		if (devcg->local.behavior == DEVCG_DEFAULT_DENY &&
> +		    devcg->behavior == DEVCG_DEFAULT_ALLOW) {
> +			devcg->behavior = DEVCG_DEFAULT_DENY;
> +		}

I think you might need another special case here.  If A and it's
child B are both ALLOW, and A switches to DENY, then if I read this
right B will be switched to DENY, but its local->exceptions will
not be cleared.  They won't be immediately applied, so at first it's
ok.  But if B then adds an exception, what happens?  It'll call
revalidate_exceptions on the full old list plus new exception.  If
any exceptions aren't allowed by the parent then it won't be applied,
but it's possible that it is allowed in the parent but (its sense
now being inverted from blacklist to whitelist) not intended to be
allowed in the child.  But there will be nothing to stop it.

So do you need

	if (devcg->local.behavior == DEVCG_DEFAULT_ALLOW &&
		devcg->behavior == DEVCG_DEFAULT_DENY) {
		dev_exception_clean(&devcg->local.exceptions);
	}

here?

> +		if (devcg->local.behavior == devcg->behavior)
> +			rc = revalidate_exceptions(devcg);
> +		if (rc)
> +			break;
> +		list_del_init(&devcg->propagate_pending);
> +	}
> +
> +	return rc;
> +}

Re: [PATCH v4 9/9] devcg: propagate local changes down the hierarchy

[Serge E. Hallyn]

Quoting aris@redhat.com (aris@redhat.com):
...
> 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).

Is that intended to apply only to only in the DEFAULT_DENY case?  If so
that should be made clear.  If not,

...

> @@ -515,11 +673,13 @@ 	memset(&ex, 0, sizeof(ex));
>  							 &parent->exceptions);
>  			devcgroup->behavior = DEVCG_DEFAULT_ALLOW;
>  			devcgroup->local.behavior = DEVCG_DEFAULT_ALLOW;
> +			rc = propagate_behavior(devcgroup);
>  			break;
>  		case DEVCG_DENY:
>  			dev_exception_clean_all(devcgroup);
>  			devcgroup->behavior = DEVCG_DEFAULT_DENY;
>  			devcgroup->local.behavior = DEVCG_DEFAULT_DENY;
> +			rc = propagate_behavior(devcgroup);
>  			break;
>  		default:
>  			rc = -EINVAL;
> @@ -610,9 +770,14 @@ 		case '\0':
>  		 */
>  		if (devcgroup->behavior == DEVCG_DEFAULT_ALLOW) {
>  			dev_exception_rm(devcgroup, &ex);
> -			return 0;
> +			rc = propagate_exception(devcgroup);

Let's say the default in both parent A and child B is ALLOW, and both
have a blacklist entry for "b 8:* rwm".  Now I

	echo "b 8:* rwm" > A/devices.allow

removing the blacklist entry.  Here you are propagating that to the
child B, which I would argue is actually propagating a new allow to
a child.  Which you said you wouldn't do.

> +			return rc;
>  		}
> -		return dev_exception_add(devcgroup, &ex);
> +		rc = dev_exception_add(devcgroup, &ex);
> +		if (!rc)
> +			/* if a local behavior wasn't explicitely choosen, pick it */
> +			devcgroup->local.behavior = devcgroup->behavior;
> +		break;
>  	case DEVCG_DENY:
>  		/*
>  		 * If the default policy is to deny by default, try to remove
> @@ -621,13 +786,22 @@ 			return 0;
>  		 */
>  		if (devcgroup->behavior == DEVCG_DEFAULT_DENY) {
>  			dev_exception_rm(devcgroup, &ex);
> -			return 0;
> +			rc = propagate_exception(devcgroup);
> +			return rc;
>  		}
> -		return dev_exception_add(devcgroup, &ex);
> +		rc = dev_exception_add(devcgroup, &ex);
> +		if (rc)
> +			return rc;
> +		/* we only propagate new restrictions */
> +		rc = propagate_exception(devcgroup);
> +		if (!rc)
> +			/* if a local behavior wasn't explicitely choosen, pick it */
> +			devcgroup->local.behavior = devcgroup->behavior;
> +		break;

Re: [PATCH v4 8/9] devcg: refactor dev_exception_clean()

[Aristeu Rozanski]

On Wed, Jan 30, 2013 at 12:50:38PM -0800, Tejun Heo wrote:
> On Wed, Jan 30, 2013 at 12:49 PM, Aristeu Rozanski > that's not
> intentional. thanks for catching this
> >
> > Tejun, you want me to resubmit the whole series or just the next patch
> > (where I was supposed to move that chunk)?
> 
> If it doesn't affect the next patch, just posting an updated version
> of this patch should be enough, I think. Which tree are these patches
> planned to go through?

the fix should actually happen on the next patch, not this one. and it
seems I'll have to refresh it anyway (just had a glance on Serge's
emails).

-- 
Aristeu

Re: [PATCH v4 9/9] devcg: propagate local changes down the hierarchy

[Aristeu Rozanski]

On Thu, Jan 31, 2013 at 04:19:32AM +0000, Serge E. Hallyn wrote:
> Quoting aris@redhat.com (aris@redhat.com):
> > +/**
> > + * propagate_behavior - propagates a change in the behavior down in hierarchy
> > + * @devcg_root: device cgroup that changed behavior
> > + *
> > + * returns: 0 in case of success, != 0 in case of error
> > + *
> > + * This is one of the two key functions for hierarchy implementation.
> > + * All cgroup's children recursively will have the behavior changed and
> > + * exceptions copied from the parent then its local behavior and exceptions
> > + * re-evaluated and applied if they're still allowed.  Refer to
> > + * Documentation/cgroups/devices.txt for more details.
> > + */
> > +static int propagate_behavior(struct dev_cgroup *devcg_root)
> > +{
> > +	struct cgroup *root = devcg_root->css.cgroup;
> > +	struct dev_cgroup *parent, *devcg, *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);
> > +
> > +		/* first copy parent's state */
> > +		devcg->behavior = parent->behavior;
> > +		dev_exception_clean(&devcg->exceptions);
> > +		rc = dev_exceptions_copy(&devcg->exceptions, &parent->exceptions);
> > +		if (rc) {
> > +			devcg->behavior = DEVCG_DEFAULT_DENY;
> > +			break;
> > +		}
> > +
> > +		if (devcg->local.behavior == DEVCG_DEFAULT_DENY &&
> > +		    devcg->behavior == DEVCG_DEFAULT_ALLOW) {
> > +			devcg->behavior = DEVCG_DEFAULT_DENY;
> > +		}
> 
> I think you might need another special case here.  If A and it's
> child B are both ALLOW, and A switches to DENY, then if I read this
> right B will be switched to DENY, but its local->exceptions will
> not be cleared.  They won't be immediately applied, so at first it's
> ok.  But if B then adds an exception, what happens?  It'll call
> revalidate_exceptions on the full old list plus new exception.  If
> any exceptions aren't allowed by the parent then it won't be applied,
> but it's possible that it is allowed in the parent but (its sense
> now being inverted from blacklist to whitelist) not intended to be
> allowed in the child.  But there will be nothing to stop it.
> 
> So do you need
> 
> 	if (devcg->local.behavior == DEVCG_DEFAULT_ALLOW &&
> 		devcg->behavior == DEVCG_DEFAULT_DENY) {
> 		dev_exception_clean(&devcg->local.exceptions);
> 	}
> 
> here?
> 
> > +		if (devcg->local.behavior == devcg->behavior)
> > +			rc = revalidate_exceptions(devcg);

I think:
		else
			dev_exception_clean(&devcg->local.exceptions);

here instead

-- 
Aristeu

Re: [PATCH v4 9/9] devcg: propagate local changes down the hierarchy

[Aristeu Rozanski]

Hi Serge,
On Thu, Jan 31, 2013 at 04:38:39AM +0000, Serge E. Hallyn wrote:
> > @@ -610,9 +770,14 @@ 		case '\0':
> >  		 */
> >  		if (devcgroup->behavior == DEVCG_DEFAULT_ALLOW) {
> >  			dev_exception_rm(devcgroup, &ex);
> > -			return 0;
> > +			rc = propagate_exception(devcgroup);
> 
> Let's say the default in both parent A and child B is ALLOW, and both
> have a blacklist entry for "b 8:* rwm".  Now I
> 
> 	echo "b 8:* rwm" > A/devices.allow
> 
> removing the blacklist entry.  Here you are propagating that to the
> child B, which I would argue is actually propagating a new allow to
> a child.  Which you said you wouldn't do.

yep, that's a bug. will fix it up

thanks!

-- 
Aristeu

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

[Aristeu Rozanski]

devcg: propagate local changes down the hierarchy

This patch makes all changes propagate down in hierarchy respecting when
possible local configurations.

Behavior changes will clean up exceptions in all the children except when the
parent changes the behavior from allow to deny and the child's behavior was
already deny, in which case the local exceptions will be reused. The inverse
is not possible: you can't have a parent with behavior deny and a child with
behavior accept.

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 behavior or exceptions change and local settings are not
allowed anymore, they'll be deleted.

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:
- separated function to walk the tree and collect valid propagation targets

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

v2:
- instead of keeping the local settings that won't apply anymore, remove them

Acked-by: Tejun Heo <tj@kernel.org>
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 |   67 ++++++++++++
 security/device_cgroup.c          |  196 ++++++++++++++++++++++++++++++++++++--
 2 files changed, 257 insertions(+), 6 deletions(-)

--- github.orig/security/device_cgroup.c	2013-01-31 10:15:22.458209721 -0500
+++ github/security/device_cgroup.c	2013-02-01 14:09:04.067917557 -0500
@@ -60,6 +60,9 @@ struct dev_cgroup {
 		struct list_head exceptions;
 		enum devcg_behavior behavior;
 	} local;
+
+	/* temporary list for pending propagation operations */
+	struct list_head propagate_pending;
 };
 
 static inline struct dev_cgroup *css_to_devcgroup(struct cgroup_subsys_state *s)
@@ -241,6 +244,11 @@ static void dev_exception_clean_all(stru
 	__dev_exception_clean_all(dev_cgroup);
 }
 
+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
@@ -292,6 +300,7 @@ static struct cgroup_subsys_state *devcg
 		return ERR_PTR(-ENOMEM);
 	INIT_LIST_HEAD(&dev_cgroup->exceptions);
 	INIT_LIST_HEAD(&dev_cgroup->local.exceptions);
+	INIT_LIST_HEAD(&dev_cgroup->propagate_pending);
 	dev_cgroup->local.behavior = DEVCG_DEFAULT_NONE;
 	dev_cgroup->behavior = DEVCG_DEFAULT_NONE;
 	parent_cgroup = cgroup->parent;
@@ -471,6 +480,163 @@ static inline int may_allow_all(struct d
 	return parent->behavior == DEVCG_DEFAULT_ALLOW;
 }
 
+/**
+ * revalidate_exceptions - walks through the exception list and revalidates
+ *			     the exceptions based on parents' behavior and
+ *			     exceptions. Called with devcgroup_mutex held.
+ * @devcg: cgroup which exceptions will be checked
+ *
+ * returns: 0 in success, -ENOMEM in case of out of memory
+ *
+ * This is one of the two key functions for hierarchy implementation.
+ * This function is responsible for re-evaluating all the cgroup's locally
+ * set exceptions due to a parent's behavior or exception change.
+ * Refer to Documentation/cgroups/devices.txt for more details.
+ */
+static int revalidate_exceptions(struct dev_cgroup *devcg)
+{
+	struct dev_exception_item *ex;
+	struct list_head *this, *tmp;
+
+	list_for_each_safe(this, tmp, &devcg->local.exceptions) {
+		ex = container_of(this, struct dev_exception_item, list);
+		if (parent_has_perm(devcg, ex)) {
+			if (dev_exception_copy(&devcg->exceptions, ex))
+				goto error;
+		} else
+			__dev_exception_rm(&devcg->local.exceptions, ex);
+	}
+	return 0;
+
+error:
+	dev_exception_clean(&devcg->exceptions);
+	return -ENOMEM;
+}
+
+/**
+ * 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_behavior - propagates a change in the behavior down in hierarchy
+ * @devcg_root: device cgroup that changed behavior
+ *
+ * returns: 0 in case of success, != 0 in case of error
+ *
+ * This is one of the two key functions for hierarchy implementation.
+ * All cgroup's children recursively will have the behavior changed and
+ * exceptions copied from the parent then its local behavior and exceptions
+ * re-evaluated and applied if they're still allowed.  Refer to
+ * Documentation/cgroups/devices.txt for more details.
+ */
+static int propagate_behavior(struct dev_cgroup *devcg_root)
+{
+	struct cgroup *root = devcg_root->css.cgroup;
+	struct dev_cgroup *parent, *devcg, *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);
+
+		/* first copy parent's state */
+		devcg->behavior = parent->behavior;
+		dev_exception_clean(&devcg->exceptions);
+		rc = dev_exceptions_copy(&devcg->exceptions, &parent->exceptions);
+		if (rc) {
+			devcg->behavior = DEVCG_DEFAULT_DENY;
+			break;
+		}
+
+		if (devcg->local.behavior == DEVCG_DEFAULT_DENY &&
+		    devcg->behavior == DEVCG_DEFAULT_ALLOW) {
+			devcg->behavior = DEVCG_DEFAULT_DENY;
+		}
+		if (devcg->local.behavior == devcg->behavior) {
+			rc = revalidate_exceptions(devcg);
+		} else {
+			dev_exception_clean(&devcg->local.exceptions);
+			/*
+			 * if the local behavior couldn't be applied,
+			 * reset it
+			 */
+			devcg->local.behavior = DEVCG_DEFAULT_NONE;
+		}
+		if (rc)
+			break;
+		list_del_init(&devcg->propagate_pending);
+	}
+
+	return rc;
+}
+
+/**
+ * propagate_exception - propagates a new exception to the children
+ * @devcg_root: device cgroup that added a new exception
+ *
+ * returns: 0 in case of success, != 0 in case of error
+ */
+static int propagate_exception(struct dev_cgroup *devcg_root)
+{
+	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);
+
+		dev_exception_clean(&devcg->exceptions);
+		if (devcg->behavior == parent->behavior) {
+			rc = dev_exceptions_copy(&devcg->exceptions, &parent->exceptions);
+			if (rc) {
+				devcg->behavior = DEVCG_DEFAULT_DENY;
+				break;
+			}
+			rc = revalidate_exceptions(devcg);
+			if (rc)
+				break;
+		} else {
+			/* we never give more permissions to the child */
+			WARN_ONCE(devcg->behavior == DEVCG_DEFAULT_ALLOW,
+				  "devcg: parent/child behavior is inconsistent");
+			rc = revalidate_exceptions(devcg);
+			if (rc)
+				break;
+		}
+		list_del_init(&devcg->propagate_pending);
+	}
+	return rc;
+}
+
 /*
  * Modify the exception list using allow/deny rules.
  * CAP_SYS_ADMIN is needed for this.  It's at least separate from CAP_MKNOD
@@ -510,16 +676,21 @@ 	memset(&ex, 0, sizeof(ex));
 			if (!may_allow_all(parent))
 				return -EPERM;
 			dev_exception_clean_all(devcgroup);
-			if (parent)
+			if (parent) {
 				rc = dev_exceptions_copy(&devcgroup->exceptions,
 							 &parent->exceptions);
+				if (rc)
+					break;
+			}
 			devcgroup->behavior = DEVCG_DEFAULT_ALLOW;
 			devcgroup->local.behavior = DEVCG_DEFAULT_ALLOW;
+			rc = propagate_behavior(devcgroup);
 			break;
 		case DEVCG_DENY:
 			dev_exception_clean_all(devcgroup);
 			devcgroup->behavior = DEVCG_DEFAULT_DENY;
 			devcgroup->local.behavior = DEVCG_DEFAULT_DENY;
+			rc = propagate_behavior(devcgroup);
 			break;
 		default:
 			rc = -EINVAL;
@@ -612,7 +783,11 @@ 		case '\0':
 			dev_exception_rm(devcgroup, &ex);
 			return 0;
 		}
-		return dev_exception_add(devcgroup, &ex);
+		rc = dev_exception_add(devcgroup, &ex);
+		if (!rc)
+			/* if a local behavior wasn't explicitely choosen, pick it */
+			devcgroup->local.behavior = devcgroup->behavior;
+		break;
 	case DEVCG_DENY:
 		/*
 		 * If the default policy is to deny by default, try to remove
@@ -621,13 +796,22 @@ 			return 0;
 		 */
 		if (devcgroup->behavior == DEVCG_DEFAULT_DENY) {
 			dev_exception_rm(devcgroup, &ex);
-			return 0;
+			rc = propagate_exception(devcgroup);
+			break;
 		}
-		return dev_exception_add(devcgroup, &ex);
+		rc = dev_exception_add(devcgroup, &ex);
+		if (rc)
+			break;
+		/* we only propagate new restrictions */
+		rc = propagate_exception(devcgroup);
+		if (!rc)
+			/* if a local behavior wasn't explicitely choosen, pick it */
+			devcgroup->local.behavior = devcgroup->behavior;
+		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-01-30 13:46:53.906746370 -0500
+++ github/Documentation/cgroups/devices.txt	2013-01-31 10:33:10.278177681 -0500
@@ -50,3 +50,70 @@ 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 behavior or exceptions change and local settings 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
+
+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.  A change in behavior (writing "a" to devices.deny or
+devices.allow) will be propagated down the hierarchy as well as new exceptions
+that will reduce the access to devices (an exception when behavior is ALLOW).
+Each cgroup will have its local behavior and exception list to keep track what
+was set by the user for that cgroup in specific.  For every propagated change,
+the effective rules will be built starting with parent's rules and the locally
+set behavior and exceptions in case they still apply, otherwise those will be
+lost.

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

[Serge E. Hallyn]

Quoting Aristeu Rozanski (aris@redhat.com):
> devcg: propagate local changes down the hierarchy
> 
> This patch makes all changes propagate down in hierarchy respecting when
> possible local configurations.
> 
> Behavior changes will clean up exceptions in all the children except when the
> parent changes the behavior from allow to deny and the child's behavior was
> already deny, in which case the local exceptions will be reused. The inverse
> is not possible: you can't have a parent with behavior deny and a child with
> behavior accept.
> 
> 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 behavior or exceptions change and local settings are not
> allowed anymore, they'll be deleted.
> 
> 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:
> - separated function to walk the tree and collect valid propagation targets
> 
> 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
> 
> v2:
> - instead of keeping the local settings that won't apply anymore, remove them
> 
> Acked-by: Tejun Heo <tj@kernel.org>
> 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 |   67 ++++++++++++
>  security/device_cgroup.c          |  196 ++++++++++++++++++++++++++++++++++++--
>  2 files changed, 257 insertions(+), 6 deletions(-)
> 
> --- github.orig/security/device_cgroup.c	2013-01-31 10:15:22.458209721 -0500
> +++ github/security/device_cgroup.c	2013-02-01 14:09:04.067917557 -0500
> @@ -60,6 +60,9 @@ struct dev_cgroup {
>  		struct list_head exceptions;
>  		enum devcg_behavior behavior;
>  	} local;
> +
> +	/* temporary list for pending propagation operations */
> +	struct list_head propagate_pending;
>  };
>  
>  static inline struct dev_cgroup *css_to_devcgroup(struct cgroup_subsys_state *s)
> @@ -241,6 +244,11 @@ static void dev_exception_clean_all(stru
>  	__dev_exception_clean_all(dev_cgroup);
>  }
>  
> +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
> @@ -292,6 +300,7 @@ static struct cgroup_subsys_state *devcg
>  		return ERR_PTR(-ENOMEM);
>  	INIT_LIST_HEAD(&dev_cgroup->exceptions);
>  	INIT_LIST_HEAD(&dev_cgroup->local.exceptions);
> +	INIT_LIST_HEAD(&dev_cgroup->propagate_pending);
>  	dev_cgroup->local.behavior = DEVCG_DEFAULT_NONE;
>  	dev_cgroup->behavior = DEVCG_DEFAULT_NONE;
>  	parent_cgroup = cgroup->parent;
> @@ -471,6 +480,163 @@ static inline int may_allow_all(struct d
>  	return parent->behavior == DEVCG_DEFAULT_ALLOW;
>  }
>  
> +/**
> + * revalidate_exceptions - walks through the exception list and revalidates
> + *			     the exceptions based on parents' behavior and
> + *			     exceptions. Called with devcgroup_mutex held.
> + * @devcg: cgroup which exceptions will be checked
> + *
> + * returns: 0 in success, -ENOMEM in case of out of memory
> + *
> + * This is one of the two key functions for hierarchy implementation.
> + * This function is responsible for re-evaluating all the cgroup's locally
> + * set exceptions due to a parent's behavior or exception change.
> + * Refer to Documentation/cgroups/devices.txt for more details.
> + */
> +static int revalidate_exceptions(struct dev_cgroup *devcg)
> +{
> +	struct dev_exception_item *ex;
> +	struct list_head *this, *tmp;
> +
> +	list_for_each_safe(this, tmp, &devcg->local.exceptions) {
> +		ex = container_of(this, struct dev_exception_item, list);
> +		if (parent_has_perm(devcg, ex)) {
> +			if (dev_exception_copy(&devcg->exceptions, ex))
> +				goto error;
> +		} else
> +			__dev_exception_rm(&devcg->local.exceptions, ex);
> +	}
> +	return 0;
> +
> +error:
> +	dev_exception_clean(&devcg->exceptions);
> +	return -ENOMEM;
> +}
> +
> +/**
> + * 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_behavior - propagates a change in the behavior down in hierarchy
> + * @devcg_root: device cgroup that changed behavior
> + *
> + * returns: 0 in case of success, != 0 in case of error
> + *
> + * This is one of the two key functions for hierarchy implementation.
> + * All cgroup's children recursively will have the behavior changed and
> + * exceptions copied from the parent then its local behavior and exceptions
> + * re-evaluated and applied if they're still allowed.  Refer to
> + * Documentation/cgroups/devices.txt for more details.
> + */
> +static int propagate_behavior(struct dev_cgroup *devcg_root)
> +{
> +	struct cgroup *root = devcg_root->css.cgroup;
> +	struct dev_cgroup *parent, *devcg, *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);
> +
> +		/* first copy parent's state */
> +		devcg->behavior = parent->behavior;
> +		dev_exception_clean(&devcg->exceptions);
> +		rc = dev_exceptions_copy(&devcg->exceptions, &parent->exceptions);

You may not want to do this if parent->behavior == DENY and
devcg->local.behavior == ALLOW.  You'll end up with matches
in may_access() in the child, where you assume that if
devcg->behavior != ALLOW it is DENY.

Now maybe that *was* your intent, but if so then I think you're
better off explicitly changing the child to DENY below.  (See my
related question below)

> +		if (rc) {
> +			devcg->behavior = DEVCG_DEFAULT_DENY;
> +			break;
> +		}
> +
> +		if (devcg->local.behavior == DEVCG_DEFAULT_DENY &&
> +		    devcg->behavior == DEVCG_DEFAULT_ALLOW) {
> +			devcg->behavior = DEVCG_DEFAULT_DENY;
> +		}
> +		if (devcg->local.behavior == devcg->behavior) {
> +			rc = revalidate_exceptions(devcg);
> +		} else {
> +			dev_exception_clean(&devcg->local.exceptions);
> +			/*
> +			 * if the local behavior couldn't be applied,
> +			 * reset it
> +			 */
> +			devcg->local.behavior = DEVCG_DEFAULT_NONE;

So the only way this will happen is if the parent and child were
originally both ALLOW, and the parent switches to DENY.

Now in general I'd discourage starting containers in ALLOW mode
at all, but I think if someone does so, then changes the host to
DENY, the container should not be blindly switched to having no
access.  Now as I say the way you have the code it will actually
behave a bit like a DENY...

Really I don't know what the right thing to do is.  The best I can
come up with is a big fat syslog warning, and keep the child as
ALLOW with exactly its original set of exceptions.

What you're doing iiuc is switching them to DENY behavior (but refusing
future exception additions) with a copy of the parent's rules.

-serge

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

[Serge E. Hallyn]

Quoting Aristeu Rozanski (aris@redhat.com):
> +/**
> + * propagate_exception - propagates a new exception to the children
> + * @devcg_root: device cgroup that added a new exception
> + *
> + * returns: 0 in case of success, != 0 in case of error
> + */
> +static int propagate_exception(struct dev_cgroup *devcg_root)
> +{
> +	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);
> +
> +		dev_exception_clean(&devcg->exceptions);
> +		if (devcg->behavior == parent->behavior) {
> +			rc = dev_exceptions_copy(&devcg->exceptions, &parent->exceptions);

Let's say parent A and child B both have DEFAULT_DENY, with a set of let's
say 5 whitelist exceptions.  Now the parent adds two more whitelist
exceptions.  As you say, we don't propagate those.

Now the parent removes one of it's whitelist exceptions.
devcgroup_update_access() calls dev_exception_rm() followed by
propagate_exception(), which comes here and copies the parent's
whitelist - including the two new whitelist rules - to the
child.

-serge

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

[Aristeu Rozanski]

On Sat, Feb 02, 2013 at 04:13:41PM +0000, Serge E. Hallyn wrote:
> Quoting Aristeu Rozanski (aris@redhat.com):
> > +static int propagate_behavior(struct dev_cgroup *devcg_root)
> > +{
> > +	struct cgroup *root = devcg_root->css.cgroup;
> > +	struct dev_cgroup *parent, *devcg, *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);
> > +
> > +		/* first copy parent's state */
> > +		devcg->behavior = parent->behavior;
> > +		dev_exception_clean(&devcg->exceptions);
> > +		rc = dev_exceptions_copy(&devcg->exceptions, &parent->exceptions);
> 
> You may not want to do this if parent->behavior == DENY and
> devcg->local.behavior == ALLOW.  You'll end up with matches
> in may_access() in the child, where you assume that if
> devcg->behavior != ALLOW it is DENY.
> 
> Now maybe that *was* your intent, but if so then I think you're
> better off explicitly changing the child to DENY below.  (See my
> related question below)

hm, could go either way. if the local behavior is allowed and it's
different, clearing the local list should be enough.

but yes, you're right, the way it is now is wrong.

> > +		if (devcg->local.behavior == devcg->behavior) {
> > +			rc = revalidate_exceptions(devcg);
> > +		} else {
> > +			dev_exception_clean(&devcg->local.exceptions);
> > +			/*
> > +			 * if the local behavior couldn't be applied,
> > +			 * reset it
> > +			 */
> > +			devcg->local.behavior = DEVCG_DEFAULT_NONE;
> 
> So the only way this will happen is if the parent and child were
> originally both ALLOW, and the parent switches to DENY.

hm, no. it could also happen if it was deny/deny. if a new exception is
written in the child (consider that the parent just had a new exception
written) it'll change automatically local.behavior to
DEVCG_DEFAULT_DENY.

> Now in general I'd discourage starting containers in ALLOW mode
> at all, but I think if someone does so, then changes the host to
> DENY, the container should not be blindly switched to having no
> access.  Now as I say the way you have the code it will actually
> behave a bit like a DENY...
> 
> Really I don't know what the right thing to do is.  The best I can
> come up with is a big fat syslog warning, and keep the child as
> ALLOW with exactly its original set of exceptions.

hm, if you do that you're breaking the hierarchy and this patchset is
useless :)

> What you're doing iiuc is switching them to DENY behavior (but refusing
> future exception additions) with a copy of the parent's rules.

hm, no. I think you misunderstood "local.behavior = DEVCG_DEFAULT_NONE".
This means "there's no local preference for behavior". local.* are just
the local preferences that need to be revalidated everytime something is
propagated. Or did you mean something else?

-- 
Aristeu

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

[Aristeu Rozanski]

On Sat, Feb 02, 2013 at 04:20:52PM +0000, Serge E. Hallyn wrote:
> Quoting Aristeu Rozanski (aris@redhat.com):
> > +static int propagate_exception(struct dev_cgroup *devcg_root)
> > +{
> > +	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);
> > +
> > +		dev_exception_clean(&devcg->exceptions);
> > +		if (devcg->behavior == parent->behavior) {
> > +			rc = dev_exceptions_copy(&devcg->exceptions, &parent->exceptions);
> 
> Let's say parent A and child B both have DEFAULT_DENY, with a set of let's
> say 5 whitelist exceptions.  Now the parent adds two more whitelist
> exceptions.  As you say, we don't propagate those.
> 
> Now the parent removes one of it's whitelist exceptions.
> devcgroup_update_access() calls dev_exception_rm() followed by
> propagate_exception(), which comes here and copies the parent's
> whitelist - including the two new whitelist rules - to the
> child.

ugh, I see your point. This gonna be trickier to fix.

-- 
Aristeu

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

[Serge Hallyn]

Quoting Aristeu Rozanski (aris@redhat.com):
> hm, no. I think you misunderstood "local.behavior = DEVCG_DEFAULT_NONE".
> This means "there's no local preference for behavior". local.* are just
> the local preferences that need to be revalidated everytime something is
> propagated. Or did you mean something else?

Ah, yes, I didn't understand that correctly, thanks.

-serge

[PATCH v6 9/9] devcg: propagate local changes down the hierarchy

[Aristeu Rozanski]

devcg: propagate local changes down the hierarchy

This patch makes all changes propagate down in hierarchy respecting when
possible local configurations.

Behavior changes will clean up exceptions in all the children except when the
parent changes the behavior from allow to deny and the child's behavior was
already deny, in which case the local exceptions will be reused. The inverse
is not possible: you can't have a parent with behavior deny and a child with
behavior accept.

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 behavior or exceptions change and local settings are not
allowed anymore, they'll be deleted.

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

Acked-by: Tejun Heo <tj@kernel.org>
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 |   67 +++++++++++
 security/device_cgroup.c          |  228 +++++++++++++++++++++++++++++++++++++-
 2 files changed, 289 insertions(+), 6 deletions(-)

--- github.orig/security/device_cgroup.c	2013-02-04 10:12:59.648315261 -0500
+++ github/security/device_cgroup.c	2013-02-05 11:35:26.925224339 -0500
@@ -60,6 +60,9 @@ struct dev_cgroup {
 		struct list_head exceptions;
 		enum devcg_behavior behavior;
 	} local;
+
+	/* temporary list for pending propagation operations */
+	struct list_head propagate_pending;
 };
 
 static inline struct dev_cgroup *css_to_devcgroup(struct cgroup_subsys_state *s)
@@ -241,6 +244,11 @@ static void dev_exception_clean_all(stru
 	__dev_exception_clean_all(dev_cgroup);
 }
 
+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
@@ -292,6 +300,7 @@ static struct cgroup_subsys_state *devcg
 		return ERR_PTR(-ENOMEM);
 	INIT_LIST_HEAD(&dev_cgroup->exceptions);
 	INIT_LIST_HEAD(&dev_cgroup->local.exceptions);
+	INIT_LIST_HEAD(&dev_cgroup->propagate_pending);
 	dev_cgroup->local.behavior = DEVCG_DEFAULT_NONE;
 	dev_cgroup->behavior = DEVCG_DEFAULT_NONE;
 	parent_cgroup = cgroup->parent;
@@ -471,6 +480,195 @@ 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->exceptions, ex);
+	}
+}
+
+/**
+ * revalidate_local_exceptions - walks through the local exception list and
+ * 				 revalidates the exceptions based on
+ * 				 parent's behavior and exceptions. The
+ * 				 exceptions that are still valid will be copied
+ * 				 to the active exception list and the others
+ * 				 will be removed.
+ * @devcg: cgroup which exceptions will be checked
+ *
+ * returns: 0 in success, -ENOMEM in case of out of memory
+ *
+ * Called with devcgroup_mutex held.
+ * This is one of the three key functions for hierarchy implementation.
+ * This function is responsible for re-evaluating all the cgroup's locally
+ * set exceptions due to a parent's behavior or exception change.
+ * Refer to Documentation/cgroups/devices.txt for more details.
+ */
+static int revalidate_local_exceptions(struct dev_cgroup *devcg)
+{
+	struct dev_exception_item *ex;
+	struct list_head *this, *tmp;
+
+	list_for_each_safe(this, tmp, &devcg->local.exceptions) {
+		ex = container_of(this, struct dev_exception_item, list);
+		if (parent_has_perm(devcg, ex)) {
+			if (dev_exception_copy(&devcg->exceptions, ex))
+				goto error;
+		} else
+			__dev_exception_rm(&devcg->local.exceptions, ex);
+	}
+	return 0;
+
+error:
+	dev_exception_clean(&devcg->exceptions);
+	return -ENOMEM;
+}
+
+/**
+ * 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_behavior - propagates a change in the behavior down in hierarchy
+ * @devcg_root: device cgroup that changed behavior
+ *
+ * returns: 0 in case of success, != 0 in case of error
+ *
+ * This is one of the two key functions for hierarchy implementation.
+ * All cgroup's children recursively will have the behavior changed and
+ * exceptions copied from the parent then its local behavior and exceptions
+ * re-evaluated and applied if they're still allowed.  Refer to
+ * Documentation/cgroups/devices.txt for more details.
+ */
+static int propagate_behavior(struct dev_cgroup *devcg_root)
+{
+	struct cgroup *root = devcg_root->css.cgroup;
+	struct dev_cgroup *parent, *devcg, *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);
+
+		/* first copy parent's state */
+		devcg->behavior = parent->behavior;
+		dev_exception_clean(&devcg->exceptions);
+
+		if (devcg->local.behavior == DEVCG_DEFAULT_DENY &&
+		    devcg->behavior == DEVCG_DEFAULT_ALLOW) {
+			/*
+			 * the only case when the local exceptions
+			 * will be reused
+			 */
+			devcg->behavior = DEVCG_DEFAULT_DENY;
+			rc = revalidate_local_exceptions(devcg);
+		} else {
+			dev_exception_clean(&devcg->local.exceptions);
+			devcg->local.behavior = DEVCG_DEFAULT_NONE;
+
+			/* just copy the parent's exceptions over */
+			rc = dev_exceptions_copy(&devcg->exceptions,
+						 &parent->exceptions);
+			if (rc)
+				devcg->behavior = DEVCG_DEFAULT_DENY;
+		}
+		if (rc)
+			break;
+		list_del_init(&devcg->propagate_pending);
+	}
+
+	return rc;
+}
+
+/**
+ * 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;
+}
+
 /*
  * Modify the exception list using allow/deny rules.
  * CAP_SYS_ADMIN is needed for this.  It's at least separate from CAP_MKNOD
@@ -510,16 +708,21 @@ 	memset(&ex, 0, sizeof(ex));
 			if (!may_allow_all(parent))
 				return -EPERM;
 			dev_exception_clean_all(devcgroup);
-			if (parent)
+			if (parent) {
 				rc = dev_exceptions_copy(&devcgroup->exceptions,
 							 &parent->exceptions);
+				if (rc)
+					break;
+			}
 			devcgroup->behavior = DEVCG_DEFAULT_ALLOW;
 			devcgroup->local.behavior = DEVCG_DEFAULT_ALLOW;
+			rc = propagate_behavior(devcgroup);
 			break;
 		case DEVCG_DENY:
 			dev_exception_clean_all(devcgroup);
 			devcgroup->behavior = DEVCG_DEFAULT_DENY;
 			devcgroup->local.behavior = DEVCG_DEFAULT_DENY;
+			rc = propagate_behavior(devcgroup);
 			break;
 		default:
 			rc = -EINVAL;
@@ -612,7 +815,11 @@ 		case '\0':
 			dev_exception_rm(devcgroup, &ex);
 			return 0;
 		}
-		return dev_exception_add(devcgroup, &ex);
+		rc = dev_exception_add(devcgroup, &ex);
+		if (!rc)
+			/* if a local behavior wasn't explicitely choosen, pick it */
+			devcgroup->local.behavior = devcgroup->behavior;
+		break;
 	case DEVCG_DENY:
 		/*
 		 * If the default policy is to deny by default, try to remove
@@ -621,13 +828,22 @@ 			return 0;
 		 */
 		if (devcgroup->behavior == DEVCG_DEFAULT_DENY) {
 			dev_exception_rm(devcgroup, &ex);
-			return 0;
+			rc = propagate_exception(devcgroup, &ex);
+			break;
 		}
-		return dev_exception_add(devcgroup, &ex);
+		rc = dev_exception_add(devcgroup, &ex);
+		if (rc)
+			break;
+		/* we only propagate new restrictions */
+		rc = propagate_exception(devcgroup, &ex);
+		if (!rc)
+			/* if a local behavior wasn't explicitely choosen, pick it */
+			devcgroup->local.behavior = devcgroup->behavior;
+		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-04 10:12:51.104189035 -0500
+++ github/Documentation/cgroups/devices.txt	2013-02-04 10:12:59.674315645 -0500
@@ -50,3 +50,70 @@ 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 behavior or exceptions change and local settings 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
+
+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.  A change in behavior (writing "a" to devices.deny or
+devices.allow) will be propagated down the hierarchy as well as new exceptions
+that will reduce the access to devices (an exception when behavior is ALLOW).
+Each cgroup will have its local behavior and exception list to keep track what
+was set by the user for that cgroup in specific.  For every propagated change,
+the effective rules will be built starting with parent's rules and the locally
+set behavior and exceptions in case they still apply, otherwise those will be
+lost.

Re: [PATCH v6 9/9] devcg: propagate local changes down the hierarchy

[Serge E. Hallyn]

Quoting Aristeu Rozanski (aris@redhat.com):

Thanks, Aristeu.  I'm sorry it feels like I'm just trying to give you a
hard time, I'm really not :)

I do feel like you're really close.  At the same time this is so subtle
and complicated that I wonder if there must not be a simpler way,
perhaps a small change in assumptions that would help do away with a lot
of this.  It's not just about the iterations you're having to do, but
more about how subtle it still all feels, suggesting it will be hard to
prevent regressions as it gets maintained.

(But maybe I'm wrong about that)

Anyway, I really appreciate all the work you're doing on this.

After you reply to my questions below, if there are no further changes
I'd like to take one more long look at the whole thing before acking.

> +/**
> + * propagate_behavior - propagates a change in the behavior down in hierarchy
> + * @devcg_root: device cgroup that changed behavior
> + *
> + * returns: 0 in case of success, != 0 in case of error
> + *
> + * This is one of the two key functions for hierarchy implementation.
> + * All cgroup's children recursively will have the behavior changed and
> + * exceptions copied from the parent then its local behavior and exceptions
> + * re-evaluated and applied if they're still allowed.  Refer to
> + * Documentation/cgroups/devices.txt for more details.
> + */
> +static int propagate_behavior(struct dev_cgroup *devcg_root)
> +{
> +	struct cgroup *root = devcg_root->css.cgroup;
> +	struct dev_cgroup *parent, *devcg, *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);
> +
> +		/* first copy parent's state */
> +		devcg->behavior = parent->behavior;
> +		dev_exception_clean(&devcg->exceptions);
> +
> +		if (devcg->local.behavior == DEVCG_DEFAULT_DENY &&
> +		    devcg->behavior == DEVCG_DEFAULT_ALLOW) {
> +			/*
> +			 * the only case when the local exceptions
> +			 * will be reused
> +			 */
> +			devcg->behavior = DEVCG_DEFAULT_DENY;
> +			rc = revalidate_local_exceptions(devcg);
> +		} else {

So, what if parent A and child B are both deny, then parent A switches
to allow, then parent A switches back to deny?  You'll be dropping B's
local exceptions, is that what you want?  (Maybe it is, I'm not sure)

> +			dev_exception_clean(&devcg->local.exceptions);
> +			devcg->local.behavior = DEVCG_DEFAULT_NONE;
> +
> +			/* just copy the parent's exceptions over */
> +			rc = dev_exceptions_copy(&devcg->exceptions,
> +						 &parent->exceptions);
> +			if (rc)
> +				devcg->behavior = DEVCG_DEFAULT_DENY;
> +		}
> +		if (rc)
> +			break;
> +		list_del_init(&devcg->propagate_pending);
> +	}
> +
> +	return rc;
> +}
> +
> +/**
> + * 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);

this looks good.

> +
> +		list_del_init(&devcg->propagate_pending);
> +	}
> +	return rc;
> +}
> +
>  /*
>   * Modify the exception list using allow/deny rules.
>   * CAP_SYS_ADMIN is needed for this.  It's at least separate from CAP_MKNOD
> @@ -510,16 +708,21 @@ 	memset(&ex, 0, sizeof(ex));
>  			if (!may_allow_all(parent))
>  				return -EPERM;
>  			dev_exception_clean_all(devcgroup);
> -			if (parent)
> +			if (parent) {
>  				rc = dev_exceptions_copy(&devcgroup->exceptions,
>  							 &parent->exceptions);
> +				if (rc)
> +					break;
> +			}
>  			devcgroup->behavior = DEVCG_DEFAULT_ALLOW;
>  			devcgroup->local.behavior = DEVCG_DEFAULT_ALLOW;
> +			rc = propagate_behavior(devcgroup);
>  			break;
>  		case DEVCG_DENY:
>  			dev_exception_clean_all(devcgroup);
>  			devcgroup->behavior = DEVCG_DEFAULT_DENY;
>  			devcgroup->local.behavior = DEVCG_DEFAULT_DENY;
> +			rc = propagate_behavior(devcgroup);
>  			break;
>  		default:
>  			rc = -EINVAL;
> @@ -612,7 +815,11 @@ 		case '\0':
>  			dev_exception_rm(devcgroup, &ex);
>  			return 0;
>  		}
> -		return dev_exception_add(devcgroup, &ex);
> +		rc = dev_exception_add(devcgroup, &ex);
> +		if (!rc)
> +			/* if a local behavior wasn't explicitely choosen, pick it */
> +			devcgroup->local.behavior = devcgroup->behavior;
> +		break;
>  	case DEVCG_DENY:
>  		/*
>  		 * If the default policy is to deny by default, try to remove
> @@ -621,13 +828,22 @@ 			return 0;
>  		 */
>  		if (devcgroup->behavior == DEVCG_DEFAULT_DENY) {
>  			dev_exception_rm(devcgroup, &ex);
> -			return 0;
> +			rc = propagate_exception(devcgroup, &ex);

Note that this is a case where we made a change to the cgroups
exceptions, but do not set the cgroup's local behavior explicitly.
That's important bc we have parent A and child B, where child B
made a change, but when child A changes its behavior, child B's
behavior will be changed as well despite having made local changes.

I assume you were thinking that local.behavior gets set if a
local.exception gets added, not if a devcg->exception gets removed
with no change to local.exceptions.

While the reasoning may be clear looking at it from this level,
I think that as an admin configuring cgroups on a system, the
rules about when the behavior change to A are propagated to B
will seem random.

(Or, it may just be an oversight and you meant to set local.behavior
here? :)

Oh, that brings up another point,

in the two cases where you do dev_exception_rm(devcgroup, &ex)
instead of dev_exception_add(devcgroup, &ex), should that
action be recorded locally somehow, so it can be reproduced
after a parent behavior change?

-serge

Re: [PATCH v6 9/9] devcg: propagate local changes down the hierarchy

[Serge E. Hallyn]

Quoting Aristeu Rozanski (aris@redhat.com):
> devcg: propagate local changes down the hierarchy
> 
> This patch makes all changes propagate down in hierarchy respecting when
> possible local configurations.
> 
> Behavior changes will clean up exceptions in all the children except when the
> parent changes the behavior from allow to deny and the child's behavior was
> already deny, in which case the local exceptions will be reused. The inverse
> is not possible: you can't have a parent with behavior deny and a child with
> behavior accept.
> 
> 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 behavior or exceptions change and local settings are not
> allowed anymore, they'll be deleted.

Do you have a use case which would be broken if we simply refuse to
allow behavior changes for any cgroup with children?

It seems like that would drastically simplify much of this.  We would
no longer need local.exceptions at all, right?  Your comment says

         * local set rules, saved so when a parent propagates new rules, the
         * local preferences can be preserved

but if there were no parent behavior changes, then any exception change
in a parent could be enforced by simply removing violating exceptions
in the child, and subsequently refusing the addition of new rules in the
child which are not allowed in the parent.  Both of which you already do.

Or am I thinking wrongly?

-serge

Re: [PATCH v6 9/9] devcg: propagate local changes down the hierarchy

[Aristeu Rozanski]

On Sat, Feb 09, 2013 at 03:53:57AM +0000, Serge E. Hallyn wrote:
> Quoting Aristeu Rozanski (aris@redhat.com):
> 
> Thanks, Aristeu.  I'm sorry it feels like I'm just trying to give you a
> hard time, I'm really not :)

no worries on that, it's important this is done right and I appreciate
your reviews.

> I do feel like you're really close.  At the same time this is so subtle
> and complicated that I wonder if there must not be a simpler way,
> perhaps a small change in assumptions that would help do away with a lot
> of this.  It's not just about the iterations you're having to do, but
> more about how subtle it still all feels, suggesting it will be hard to
> prevent regressions as it gets maintained.

what could make it simpler is simply drop the notion of local settings:
if it's changed locally, just check if it's allowed and do it.
propagation simply will mirror the parent cgroup. But other than that, I
don't see any easier way.

> (But maybe I'm wrong about that)
> 
> Anyway, I really appreciate all the work you're doing on this.
> 
> After you reply to my questions below, if there are no further changes
> I'd like to take one more long look at the whole thing before acking.
> 
> > +/**
> > + * propagate_behavior - propagates a change in the behavior down in hierarchy
> > + * @devcg_root: device cgroup that changed behavior
> > + *
> > + * returns: 0 in case of success, != 0 in case of error
> > + *
> > + * This is one of the two key functions for hierarchy implementation.
> > + * All cgroup's children recursively will have the behavior changed and
> > + * exceptions copied from the parent then its local behavior and exceptions
> > + * re-evaluated and applied if they're still allowed.  Refer to
> > + * Documentation/cgroups/devices.txt for more details.
> > + */
> > +static int propagate_behavior(struct dev_cgroup *devcg_root)
> > +{
> > +	struct cgroup *root = devcg_root->css.cgroup;
> > +	struct dev_cgroup *parent, *devcg, *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);
> > +
> > +		/* first copy parent's state */
> > +		devcg->behavior = parent->behavior;
> > +		dev_exception_clean(&devcg->exceptions);
> > +
> > +		if (devcg->local.behavior == DEVCG_DEFAULT_DENY &&
> > +		    devcg->behavior == DEVCG_DEFAULT_ALLOW) {
> > +			/*
> > +			 * the only case when the local exceptions
> > +			 * will be reused
> > +			 */
> > +			devcg->behavior = DEVCG_DEFAULT_DENY;
> > +			rc = revalidate_local_exceptions(devcg);
> > +		} else {
> 
> So, what if parent A and child B are both deny, then parent A switches
> to allow, then parent A switches back to deny?  You'll be dropping B's
> local exceptions, is that what you want?  (Maybe it is, I'm not sure)

so, doing the two steps slowly:
1) parent changes to ALLOW: B gets to keep deny because it's a local
   setting and it's allowed. revalidate_local_exceptions() will probably
   keep all the local exceptions since parent's change to ALLOW will
   reset its exception list; so any exception to DENY in B will be
   allowed.
1.1) parent gets new exceptions (or not): some of the local exceptions
     in B might be dropped since they'll not be allowed anymore
2) parent changes to DENY: B keeps DENY but ends up indirectly losing
   all its local exceptions anyway because parent's behavior change will
   also clear all exceptions and will deny everything.

the whole idea is to never a child should have access to more devices than
its parent.

> > @@ -621,13 +828,22 @@ 			return 0;
> >  		 */
> >  		if (devcgroup->behavior == DEVCG_DEFAULT_DENY) {
> >  			dev_exception_rm(devcgroup, &ex);
> > -			return 0;
> > +			rc = propagate_exception(devcgroup, &ex);
> 
> Note that this is a case where we made a change to the cgroups
> exceptions, but do not set the cgroup's local behavior explicitly.
> That's important bc we have parent A and child B, where child B
> made a change, but when child A changes its behavior, child B's
> behavior will be changed as well despite having made local changes.
> 
> I assume you were thinking that local.behavior gets set if a
> local.exception gets added, not if a devcg->exception gets removed
> with no change to local.exceptions.

that is correct. the idea of setting local.behavior is to make sure
local.exceptions are interpreted correctly: are they exceptions to
DENY or ALLOW?

> While the reasoning may be clear looking at it from this level,
> I think that as an admin configuring cgroups on a system, the
> rules about when the behavior change to A are propagated to B
> will seem random.
> 
> (Or, it may just be an oversight and you meant to set local.behavior
> here? :)
> 
> Oh, that brings up another point,
> 
> in the two cases where you do dev_exception_rm(devcgroup, &ex)
> instead of dev_exception_add(devcgroup, &ex), should that
> action be recorded locally somehow, so it can be reproduced
> after a parent behavior change?

that makes sense. not sure how to do that though. will think in
something.

-- 
Aristeu

Re: [PATCH v6 9/9] devcg: propagate local changes down the hierarchy

[Aristeu Rozanski]

On Sat, Feb 09, 2013 at 04:04:02AM +0000, Serge E. Hallyn wrote:
> Quoting Aristeu Rozanski (aris@redhat.com):
> > devcg: propagate local changes down the hierarchy
> > 
> > This patch makes all changes propagate down in hierarchy respecting when
> > possible local configurations.
> > 
> > Behavior changes will clean up exceptions in all the children except when the
> > parent changes the behavior from allow to deny and the child's behavior was
> > already deny, in which case the local exceptions will be reused. The inverse
> > is not possible: you can't have a parent with behavior deny and a child with
> > behavior accept.
> > 
> > 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 behavior or exceptions change and local settings are not
> > allowed anymore, they'll be deleted.
> 
> Do you have a use case which would be broken if we simply refuse to
> allow behavior changes for any cgroup with children?
> 
> It seems like that would drastically simplify much of this.  We would
> no longer need local.exceptions at all, right?  Your comment says
> 
>          * local set rules, saved so when a parent propagates new rules, the
>          * local preferences can be preserved
> 
> but if there were no parent behavior changes, then any exception change
> in a parent could be enforced by simply removing violating exceptions
> in the child, and subsequently refusing the addition of new rules in the
> child which are not allowed in the parent.  Both of which you already do.
> 
> Or am I thinking wrongly?

That would be an option even simpler than not keeping local settings. In
production I doubt the sysadmin will keep playing with permissions,
although until one gets right, it'll be annoying as hell to have to
remove the whole hierarchy because you forgot to add a certain device to
the list.

-- 
Aristeu

Re: [PATCH v6 9/9] devcg: propagate local changes down the hierarchy

[Serge E. Hallyn]

Quoting Aristeu Rozanski (aris@redhat.com):
> On Sat, Feb 09, 2013 at 04:04:02AM +0000, Serge E. Hallyn wrote:
> > Quoting Aristeu Rozanski (aris@redhat.com):
> > > devcg: propagate local changes down the hierarchy
> > > 
> > > This patch makes all changes propagate down in hierarchy respecting when
> > > possible local configurations.
> > > 
> > > Behavior changes will clean up exceptions in all the children except when the
> > > parent changes the behavior from allow to deny and the child's behavior was
> > > already deny, in which case the local exceptions will be reused. The inverse
> > > is not possible: you can't have a parent with behavior deny and a child with
> > > behavior accept.
> > > 
> > > 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 behavior or exceptions change and local settings are not
> > > allowed anymore, they'll be deleted.
> > 
> > Do you have a use case which would be broken if we simply refuse to
> > allow behavior changes for any cgroup with children?
> > 
> > It seems like that would drastically simplify much of this.  We would
> > no longer need local.exceptions at all, right?  Your comment says
> > 
> >          * local set rules, saved so when a parent propagates new rules, the
> >          * local preferences can be preserved
> > 
> > but if there were no parent behavior changes, then any exception change
> > in a parent could be enforced by simply removing violating exceptions
> > in the child, and subsequently refusing the addition of new rules in the
> > child which are not allowed in the parent.  Both of which you already do.
> > 
> > Or am I thinking wrongly?
> 
> That would be an option even simpler than not keeping local settings. In
> production I doubt the sysadmin will keep playing with permissions,
> although until one gets right, it'll be annoying as hell to have to
> remove the whole hierarchy because you forgot to add a certain device to
> the list.

Note I said only forbid behavior changes - not exception changes - to
cgroups with children.

-serge

Re: [PATCH v6 9/9] devcg: propagate local changes down the hierarchy

[Aristeu Rozanski]

On Mon, Feb 11, 2013 at 05:42:59PM +0000, Serge E. Hallyn wrote:
> Note I said only forbid behavior changes - not exception changes - to
> cgroups with children.

that won't do much. behavior change is the simplest: if you change a
behavior, you wipe the local exceptions unless the special (deny, deny)
-> (allow, deny) case.
exception propagation is harder
getting rid of local settings would buy more simplicity

-- 
Aristeu

Re: [PATCH v6 9/9] devcg: propagate local changes down the hierarchy

[Serge E. Hallyn]

Quoting Aristeu Rozanski (aris@redhat.com):
> On Mon, Feb 11, 2013 at 05:42:59PM +0000, Serge E. Hallyn wrote:
> > Note I said only forbid behavior changes - not exception changes - to
> > cgroups with children.
> 
> that won't do much. behavior change is the simplest: if you change a
> behavior, you wipe the local exceptions unless the special (deny, deny)
> -> (allow, deny) case.

Right but that is what has the most non-sensical implications for
propagation.

> exception propagation is harder

But if we removed behavior change propagation, then your exception
propagation not only would be correct, it could be simplified by
removing the .local stuff altogether.  Local would be obvious:
if it's in current and not in parent, it's local.  Or when adding
a new exception e2, if exception e1 is in current and not in parent
minus the new exception e2, then e1 is local.

> getting rid of local settings would buy more simplicity

(Not sure which you mean here by 'getting rid of local settings')

-serge

Re: [PATCH v6 9/9] devcg: propagate local changes down the hierarchy

[Aristeu Rozanski]

On Mon, Feb 11, 2013 at 06:52:39PM +0000, Serge E. Hallyn wrote:
> > getting rid of local settings would buy more simplicity
> 
> (Not sure which you mean here by 'getting rid of local settings')

no local.{behavior,exceptions}, which still would allow behavior
propagation, but simply wouldn't keep local behavior or exceptions.
a change in behavior on parent would simply reset the child to parent's
state. exception propagation would mean inserting/removing the new
exception and making sure the others are still valid.

-- 
Aristeu

Re: [PATCH v6 9/9] devcg: propagate local changes down the hierarchy

[Serge Hallyn]

Quoting Aristeu Rozanski (aris@redhat.com):
> On Mon, Feb 11, 2013 at 06:52:39PM +0000, Serge E. Hallyn wrote:
> > > getting rid of local settings would buy more simplicity
> > 
> > (Not sure which you mean here by 'getting rid of local settings')
> 
> no local.{behavior,exceptions}, which still would allow behavior
> propagation, but simply wouldn't keep local behavior or exceptions.
> a change in behavior on parent would simply reset the child to parent's

Why would that be necessary?  If I add permission to the parent, I just
don't propagate it.  If I remove permission from the parent, I make sure
all children don't have that permission, but keep everything else.
The child's permission is never allowed to exceed its parent's.

We don't need to reset the child's to the parent's.

> state. exception propagation would mean inserting/removing the new
> exception and making sure the others are still valid.
> 
> -- 
> Aristeu
>