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

[PATCH v3 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

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 v3 1/9] device_cgroup: prepare exception list handling functions for two lists

[aris]

[-- Attachment #1: dev_exception_helpers.patch --]
[-- Type: text/plain, Size: 2701 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.

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;
 	}

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

[Tejun Heo]

On Tue, Jan 29, 2013 at 02:08:00PM -0500, aris@redhat.com wrote:
> 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.
> 
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Serge Hallyn <serge.hallyn@canonical.com>
> Signed-off-by: Aristeu Rozanski <aris@redhat.com>

Acked-by: Tejun Heo <tj@kernel.org>

Can you please keep acks across postings?  That makes it a bit easier
for reviewers.

Thanks.

-- 
tejun

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

[Aristeu Rozanski]

On Tue, Jan 29, 2013 at 11:41:55AM -0800, Tejun Heo wrote:
> On Tue, Jan 29, 2013 at 02:08:00PM -0500, aris@redhat.com wrote:
> > 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.
> > 
> > Cc: Tejun Heo <tj@kernel.org>
> > Cc: Serge Hallyn <serge.hallyn@canonical.com>
> > Signed-off-by: Aristeu Rozanski <aris@redhat.com>
> 
> Acked-by: Tejun Heo <tj@kernel.org>
> 
> Can you please keep acks across postings?  That makes it a bit easier
> for reviewers.

sure, will do it in the future submissions.

-- 
Aristeu

Re: [PATCH v3 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.
> 
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Serge Hallyn <serge.hallyn@canonical.com>

Hi,

near as I can tell this is identical to the one I acked on Nov 20?

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 v3 1/9] device_cgroup: prepare exception list handling functions for two lists

[Aristeu Rozanski]

On Wed, Jan 30, 2013 at 05:41:40PM +0000, Serge E. Hallyn wrote:
> 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.
> > 
> > Cc: Tejun Heo <tj@kernel.org>
> > Cc: Serge Hallyn <serge.hallyn@canonical.com>
> 
> Hi,
> 
> near as I can tell this is identical to the one I acked on Nov 20?

argh, yes. sorry about that.

-- 
Aristeu

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

[aris]

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

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

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)

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

[Tejun Heo]

On Tue, Jan 29, 2013 at 02:08:01PM -0500, aris@redhat.com wrote:
> In preparation for the next patch, reorder dev_exception_add() and
> dev_exception_rm().

It would be nice to have something like "this change doesn't introduce
any functional changes".

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

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

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

[aris]

[-- Attachment #1: hierarchy.patch --]
[-- Type: text/plain, Size: 5169 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.

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;
 	}

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

[Tejun Heo]

On Tue, Jan 29, 2013 at 02:08:02PM -0500, aris@redhat.com wrote:
> 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.
> 
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Serge Hallyn <serge.hallyn@canonical.com>
> Signed-off-by: Aristeu Rozanski <aris@redhat.com>

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

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

[aris]

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

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

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 |   15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

--- github.orig/security/device_cgroup.c	2013-01-29 11:49:15.244665037 -0500
+++ github/security/device_cgroup.c	2013-01-29 11:49:15.514669057 -0500
@@ -382,15 +382,18 @@ 		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;
+	if (dev_cgroup->behavior == DEVCG_DEFAULT_DENY) {
+		if (match)
+			return 1;
+	} else {
+		if (!match)
+			return 1;
+	}
 	return 0;
 }
 

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

[Tejun Heo]

On Tue, Jan 29, 2013 at 02:08:03PM -0500, aris@redhat.com wrote:
> In order to make the next patch more clear, expand may_access() logic.
> 
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Serge Hallyn <serge.hallyn@canonical.com>
> Signed-off-by: Aristeu Rozanski <aris@redhat.com>

Maybe convert the function to return bool while at it?

Acked-by: Tejun Heo <tj@kernel.org>

-- 
tejun

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

[aris]

[-- Attachment #1: strengthen-may_access.patch --]
[-- Type: text/plain, Size: 3223 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()

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, 27 insertions(+), 17 deletions(-)

--- github.orig/security/device_cgroup.c	2013-01-29 11:49:15.514669057 -0500
+++ github/security/device_cgroup.c	2013-01-29 11:49:15.795673240 -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 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,
+		       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)
-			return 1;
+	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)
-			return 1;
+		/* only behavior == DEVCG_DEFAULT_DENY allowed here */
+		if (match)
+			/* parent has an exception that matches the proposed */
+			return true;
+		else
+			return false;
 	}
 	return 0;
 }
@@ -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)

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

[Tejun Heo]

On Tue, Jan 29, 2013 at 02:08:04PM -0500, aris@redhat.com wrote:
> 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()
> 
> 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, 27 insertions(+), 17 deletions(-)
> 
> --- github.orig/security/device_cgroup.c	2013-01-29 11:49:15.514669057 -0500
> +++ github/security/device_cgroup.c	2013-01-29 11:49:15.795673240 -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 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,
> +		       enum devcg_behavior behavior)

Probably belongs to the previous patch.  Other than that,

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

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

[aris]

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

Allocate resources and change behavior only when online.
This patch is in preparation for hierarchy support.

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-29 11:49:15.795673240 -0500
+++ github/security/device_cgroup.c	2013-01-29 11:49:16.076677425 -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,
 

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

[Tejun Heo]

On Tue, Jan 29, 2013 at 02:08:05PM -0500, aris@redhat.com wrote:
> Allocate resources and change behavior only when online.
> This patch is in preparation for hierarchy support.

I think the description is a bit lacking.  We need this because we
need to reliably determine whether a certain node is on the hierarchy
or not for config propagation and inheritance.  It would be nice if
you explain the synchornization implications.

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

Other than that,

Acked-by: Tejun Heo <tj@kernel.org>

-- 
tejun

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

[aris]

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

This patch is in preparation for hierarchy support

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;

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

[Tejun Heo]

On Tue, Jan 29, 2013 at 02:08:06PM -0500, aris@redhat.com wrote:
> This patch is in preparation for hierarchy support

It would be nice to note this is purely organizational and doesn't
make any functional changes.

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

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

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

[aris]

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

This patch is in preparation for hierarchy support.

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;

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

[Tejun Heo]

On Tue, Jan 29, 2013 at 02:08:07PM -0500, aris@redhat.com wrote:
> This patch is in preparation for hierarchy support.

Ditto.

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

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

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

[aris]

[-- Attachment #1: propagate.patch --]
[-- Type: text/plain, Size: 12241 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.

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


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          |  170 ++++++++++++++++++++++++++++++++++++--
 2 files changed, 230 insertions(+), 6 deletions(-)

--- github.orig/security/device_cgroup.c	2013-01-29 11:49:16.653686016 -0500
+++ github/security/device_cgroup.c	2013-01-29 13:58:07.537099962 -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,139 @@ 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;
+}
+
+/**
+ * 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, *pos;
+	struct dev_cgroup *parent, *devcg, *tmp;
+	int rc = 0;
+	LIST_HEAD(pending);
+
+	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, &pending);
+	}
+	rcu_read_unlock();
+
+	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, *pos;
+	struct dev_cgroup *devcg, *parent, *tmp;
+	int rc = 0;
+	LIST_HEAD(pending);
+
+	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, &pending);
+	}
+	rcu_read_unlock();
+
+	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 +657,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 +754,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 +770,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 11:44:51.258734330 -0500
+++ github/Documentation/cgroups/devices.txt	2013-01-29 11:49:16.935690216 -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 v3 9/9] devcg: propagate local changes down the hierarchy

[Tejun Heo]

Hey,

Generally looks good to me although I haven't really delved into the
behavior (you're gonna be there for the fallouts, right?).  Just some
minor comments.

> +static int propagate_behavior(struct dev_cgroup *devcg_root)
> +{
> +	struct cgroup *root = devcg_root->css.cgroup, *pos;
> +	struct dev_cgroup *parent, *devcg, *tmp;
> +	int rc = 0;
> +	LIST_HEAD(pending);
> +
> +	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, &pending);
> +	}
> +	rcu_read_unlock();

I think it could be a good idea to factor out the above and document
what's going on and why.

Other than that,

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

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

[Aristeu Rozanski]

On Tue, Jan 29, 2013 at 12:35:00PM -0800, Tejun Heo wrote:
> Generally looks good to me although I haven't really delved into the
> behavior (you're gonna be there for the fallouts, right?).  Just some
> minor comments.

yes, I'll.

> > +static int propagate_behavior(struct dev_cgroup *devcg_root)
> > +{
> > +	struct cgroup *root = devcg_root->css.cgroup, *pos;
> > +	struct dev_cgroup *parent, *devcg, *tmp;
> > +	int rc = 0;
> > +	LIST_HEAD(pending);
> > +
> > +	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, &pending);
> > +	}
> > +	rcu_read_unlock();
> 
> I think it could be a good idea to factor out the above and document
> what's going on and why.

ok, will fix the missing bits based on your comments and resubmit.

-- 
Aristeu