[PATCH 0/5] devcg: introduce proper hierarchy support

[PATCH 0/5] devcg: introduce proper hierarchy support

[Aristeu Rozanski]

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.

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

-- 
Aristeu

[PATCH 1/5] device_cgroup: fix locking in devcgroup_destroy()

[Aristeu Rozanski]

[-- Attachment #1: devcg-devcgroup_destroy_locking.patch --]
[-- Type: text/plain, Size: 690 bytes --]

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 |    4 ++++
 1 file changed, 4 insertions(+)

Index: github/security/device_cgroup.c
===================================================================
--- github.orig/security/device_cgroup.c	2012-11-26 17:13:54.008153189 -0500
+++ github/security/device_cgroup.c	2012-11-27 11:58:43.574251075 -0500
@@ -215,7 +215,11 @@
 	struct dev_cgroup *dev_cgroup;
 
 	dev_cgroup = cgroup_to_devcgroup(cgroup);
+
+	mutex_lock(&devcgroup_mutex);
 	dev_exception_clean(dev_cgroup);
+	mutex_unlock(&devcgroup_mutex);
+
 	kfree(dev_cgroup);
 }
 

Re: [PATCH 1/5] device_cgroup: fix locking in devcgroup_destroy()

[Serge E. Hallyn]

Quoting Aristeu Rozanski (aris@redhat.com):
> 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 |    4 ++++
>  1 file changed, 4 insertions(+)
> 
> Index: github/security/device_cgroup.c
> ===================================================================
> --- github.orig/security/device_cgroup.c	2012-11-26 17:13:54.008153189 -0500
> +++ github/security/device_cgroup.c	2012-11-27 11:58:43.574251075 -0500
> @@ -215,7 +215,11 @@
>  	struct dev_cgroup *dev_cgroup;
>  
>  	dev_cgroup = cgroup_to_devcgroup(cgroup);
> +
> +	mutex_lock(&devcgroup_mutex);
>  	dev_exception_clean(dev_cgroup);
> +	mutex_unlock(&devcgroup_mutex);
> +
>  	kfree(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 1/5] device_cgroup: fix locking in devcgroup_destroy()

[Tejun Heo]

On Tue, Nov 27, 2012 at 02:35:02PM -0500, Aristeu Rozanski wrote:
> 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 2/5] device_cgroup: prepare exception list handling functions for two lists

[Aristeu Rozanski]

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

Index: github/security/device_cgroup.c
===================================================================
--- github.orig/security/device_cgroup.c	2012-11-26 17:13:55.972206330 -0500
+++ github/security/device_cgroup.c	2012-11-27 11:58:28.025841706 -0500
@@ -104,7 +104,7 @@
 /*
  * 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 @@
 	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 @@
 	}
 
 	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)
@@ -513,10 +513,10 @@
 		 * 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
@@ -524,10 +524,10 @@
 		 * 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 2/5] device_cgroup: prepare exception list handling functions for two lists

[Serge E. Hallyn]

Quoting Aristeu Rozanski (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>

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(-)
> 
> Index: github/security/device_cgroup.c
> ===================================================================
> --- github.orig/security/device_cgroup.c	2012-11-26 17:13:55.972206330 -0500
> +++ github/security/device_cgroup.c	2012-11-27 11:58:28.025841706 -0500
> @@ -104,7 +104,7 @@
>  /*
>   * 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 @@
>  	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 @@
>  	}
>  
>  	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)
> @@ -513,10 +513,10 @@
>  		 * 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
> @@ -524,10 +524,10 @@
>  		 * 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 2/5] device_cgroup: prepare exception list handling functions for two lists

[Tejun Heo]

On Tue, Nov 27, 2012 at 02:35:03PM -0500, Aristeu Rozanski 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>

Thanks.

-- 
tejun

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

[Aristeu Rozanski]

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

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 |  108 ++++++++++++++++++++++++++++++++++-------------
 1 file changed, 80 insertions(+), 28 deletions(-)

Index: github/security/device_cgroup.c
===================================================================
--- github.orig/security/device_cgroup.c	2012-11-26 17:27:52.710834676 -0500
+++ github/security/device_cgroup.c	2012-11-27 11:58:09.333349468 -0500
@@ -39,13 +39,27 @@
 	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,41 @@
 /*
  * called under devcgroup_mutex
  */
-static int dev_exception_add(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;
+
+	lockdep_assert_held(&devcgroup_mutex);
+
+	list_for_each_entry_safe(walk, tmp, exceptions, list) {
+		if (walk->type != ex->type)
+			continue;
+		if (walk->major != ex->major)
+			continue;
+		if (walk->minor != ex->minor)
+			continue;
+
+		walk->access &= ~ex->access;
+		if (!walk->access) {
+			list_del_rcu(&walk->list);
+			kfree_rcu(walk, rcu);
+		}
+	}
+}
+
+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)
 {
 	struct dev_exception_item *excopy, *walk;
 
@@ -133,30 +180,26 @@
 	return 0;
 }
 
-/*
- * called under devcgroup_mutex
- */
-static void dev_exception_rm(struct list_head *exceptions,
+static int dev_exception_add(struct dev_cgroup *devcgroup,
 			     struct dev_exception_item *ex)
 {
-	struct dev_exception_item *walk, *tmp;
+	int rc;
 
 	lockdep_assert_held(&devcgroup_mutex);
 
-	list_for_each_entry_safe(walk, tmp, exceptions, list) {
-		if (walk->type != ex->type)
-			continue;
-		if (walk->major != ex->major)
-			continue;
-		if (walk->minor != ex->minor)
-			continue;
+	/*
+	 * 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);
 
-		walk->access &= ~ex->access;
-		if (!walk->access) {
-			list_del_rcu(&walk->list);
-			kfree_rcu(walk, rcu);
-		}
-	}
+	return rc;
 }
 
 /**
@@ -175,6 +218,11 @@
 		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);
+	}
 }
 
 /*
@@ -190,6 +238,8 @@
 	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,6 +463,7 @@
 				return -EPERM;
 			dev_exception_clean(devcgroup);
 			devcgroup->behavior = DEVCG_DEFAULT_ALLOW;
+			devcgroup->local.behavior = DEVCG_DEFAULT_ALLOW;
 			if (!parent)
 				break;
 
@@ -424,6 +475,7 @@
 		case DEVCG_DENY:
 			dev_exception_clean(devcgroup);
 			devcgroup->behavior = DEVCG_DEFAULT_DENY;
+			devcgroup->local.behavior = DEVCG_DEFAULT_DENY;
 			break;
 		default:
 			return -EINVAL;
@@ -513,10 +565,10 @@
 		 * 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
@@ -524,10 +576,10 @@
 		 * 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 3/5] device_cgroup: keep track of local group settings

[Serge E. Hallyn]

Quoting Aristeu Rozanski (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.
> 
> 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 |  108 ++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 80 insertions(+), 28 deletions(-)

Hi,

thanks for doing this.  I've got one concern though.  I don't see
any place where devcgroup_create() was updated to create the
local exceptions list.  I think we need a guarantee that at
any time the local exceptions list will contain all the entries
contained in the cgroup->exceptions list.  Otherwise you cannot
use the exception_add() the way you do (or you can't use RCU),
since there could be a window between the successful addition
of a rule to cgroup->local.exceptions and the failed addition to
cgroup->exceptions, during which a task (since it won't need
the mutex for devcg_allow()) could exceed allowed permissions.

It's possible I'm misunderstanding.  If you think that's the case
just kick me and I'll take a fresh look.

(Btw is there a git tree or gitweb view I could look at alongside
the patchset?)

thanks,
-serge

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

[Aristeu Rozanski]

On Thu, Nov 29, 2012 at 07:29:45PM +0000, Serge E. Hallyn wrote:
> Quoting Aristeu Rozanski (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.
> > 
> > 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 |  108 ++++++++++++++++++++++++++++++++++-------------
> >  1 file changed, 80 insertions(+), 28 deletions(-)
> 
> Hi,
> 
> thanks for doing this.  I've got one concern though.  I don't see
> any place where devcgroup_create() was updated to create the
> local exceptions list.  I think we need a guarantee that at

the local exceptions list is part of the dev_cgroup structure and it's
initialized on the patch 'device_cgroup: keep track of local group
setting':

@@ -190,6 +238,8 @@
        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)

> any time the local exceptions list will contain all the entries
> contained in the cgroup->exceptions list.  Otherwise you cannot

hm, no. the local exceptions are meant to be used for stuff written
locally. the devcgroup->exceptions list is the list in effect.

> use the exception_add() the way you do (or you can't use RCU),
> since there could be a window between the successful addition
> of a rule to cgroup->local.exceptions and the failed addition to
> cgroup->exceptions, during which a task (since it won't need
> the mutex for devcg_allow()) could exceed allowed permissions.
>
> It's possible I'm misunderstanding.  If you think that's the case
> just kick me and I'll take a fresh look.

I see your point. it's indeed a problem. in dev_exception_add(), it
needs to check for permissions before actually adding to
devcgroup->exceptions.

> (Btw is there a git tree or gitweb view I could look at alongside
> the patchset?)

I'll rebase the patchset along with a fix for this and resubmit with a
link to the git repo.

-- 
Aristeu

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

[Serge E. Hallyn]

Quoting Aristeu Rozanski (aris@redhat.com):
> On Thu, Nov 29, 2012 at 07:29:45PM +0000, Serge E. Hallyn wrote:
> > Quoting Aristeu Rozanski (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.
> > > 
> > > 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 |  108 ++++++++++++++++++++++++++++++++++-------------
> > >  1 file changed, 80 insertions(+), 28 deletions(-)
> > 
> > Hi,
> > 
> > thanks for doing this.  I've got one concern though.  I don't see
> > any place where devcgroup_create() was updated to create the
> > local exceptions list.  I think we need a guarantee that at
> 
> the local exceptions list is part of the dev_cgroup structure and it's
> initialized on the patch 'device_cgroup: keep track of local group
> setting':
> 
> @@ -190,6 +238,8 @@
>         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)
> 
> > any time the local exceptions list will contain all the entries
> > contained in the cgroup->exceptions list.  Otherwise you cannot
> 
> hm, no. the local exceptions are meant to be used for stuff written
> locally. the devcgroup->exceptions list is the list in effect.
> 
> > use the exception_add() the way you do (or you can't use RCU),
> > since there could be a window between the successful addition
> > of a rule to cgroup->local.exceptions and the failed addition to
> > cgroup->exceptions, during which a task (since it won't need
> > the mutex for devcg_allow()) could exceed allowed permissions.
> >
> > It's possible I'm misunderstanding.  If you think that's the case
> > just kick me and I'll take a fresh look.
> 
> I see your point. it's indeed a problem. in dev_exception_add(), it
> needs to check for permissions before actually adding to
> devcgroup->exceptions.
> 
> > (Btw is there a git tree or gitweb view I could look at alongside
> > the patchset?)
> 
> I'll rebase the patchset along with a fix for this and resubmit with a
> link to the git repo.

Thanks, Aristeu!

-serge

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

[Aristeu Rozanski]

On Thu, Nov 29, 2012 at 08:26:08PM +0000, Serge E. Hallyn wrote:
> Quoting Aristeu Rozanski (aris@redhat.com):
> > I see your point. it's indeed a problem. in dev_exception_add(), it
> > needs to check for permissions before actually adding to
> > devcgroup->exceptions.

actually, checked again, it's done correctly. when adding an exception
that will allow extra device access (DEVCG_ALLOW), it does check it
before. It means that you can't add local exceptions unless at a certain
point of the time it was allowed to do so.

> > 
> > > (Btw is there a git tree or gitweb view I could look at alongside
> > > the patchset?)
> > 
> > I'll rebase the patchset along with a fix for this and resubmit with a
> > link to the git repo.

just pushed the same version to:
git://github.com/aristeu/linux-2.6.git

please use branch devcg_hiearchy_review

-- 
Aristeu

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

[Serge E. Hallyn]

Quoting Aristeu Rozanski (aris@redhat.com):
> On Thu, Nov 29, 2012 at 08:26:08PM +0000, Serge E. Hallyn wrote:
> > Quoting Aristeu Rozanski (aris@redhat.com):
> > > I see your point. it's indeed a problem. in dev_exception_add(), it
> > > needs to check for permissions before actually adding to
> > > devcgroup->exceptions.
> 
> actually, checked again, it's done correctly. when adding an exception
> that will allow extra device access (DEVCG_ALLOW), it does check it
> before. It means that you can't add local exceptions unless at a certain
> point of the time it was allowed to do so.

Thanks.

...

> git://github.com/aristeu/linux-2.6.git
> 
> please use branch devcg_hiearchy_review

Thanks!

I have a few remaining concerns.

First, generally, I don't think 'allows' added to parent should be 
automatically propagated to descendents.

In devcgroup_update_access: (around line 625)
	there is a period of time where cgroup members have
	default allow without the parent's exceptions.

propagate_behavior (line 505):
	1. doesn't follow the same ordering as devcgroup_update_access(), in
	particular cleaning exceptions before setting behavior.
	2. When changing a parent from deny to allow, I don't think children
	should be updated.

propagate_exception:
	1. the WARN_ONCE doesn't seem helpful.
	2. Again, I don't think allows to a parent should be propagated, only
	denies.

-serge

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

[Aristeu Rozanski]

On Mon, Dec 03, 2012 at 06:01:25PM +0000, Serge E. Hallyn wrote:
> First, generally, I don't think 'allows' added to parent should be 
> automatically propagated to descendents.

that's what I think too and what I tried to do

> In devcgroup_update_access: (around line 625)
> 	there is a period of time where cgroup members have
> 	default allow without the parent's exceptions.

true, will fix that one and look for more cases

> propagate_behavior (line 505):
> 	1. doesn't follow the same ordering as devcgroup_update_access(), in
> 	particular cleaning exceptions before setting behavior.

I see, will update that

> 	2. When changing a parent from deny to allow, I don't think children
> 	should be updated.

I disagree on this one. since there'll be local preferences, it'll try
to revalidate them everytime there's a change. so, for example, an
exception that might not be possible now, will be possible when its
parent changes in a way that allows that.

-- 
Aristeu

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

[Serge E. Hallyn]

Quoting Aristeu Rozanski (aris@redhat.com):
> On Mon, Dec 03, 2012 at 06:01:25PM +0000, Serge E. Hallyn wrote:
> > First, generally, I don't think 'allows' added to parent should be 
> > automatically propagated to descendents.
> 
> that's what I think too and what I tried to do
> 
> > In devcgroup_update_access: (around line 625)
> > 	there is a period of time where cgroup members have
> > 	default allow without the parent's exceptions.
> 
> true, will fix that one and look for more cases
> 
> > propagate_behavior (line 505):
> > 	1. doesn't follow the same ordering as devcgroup_update_access(), in
> > 	particular cleaning exceptions before setting behavior.
> 
> I see, will update that
> 
> > 	2. When changing a parent from deny to allow, I don't think children
> > 	should be updated.
> 
> I disagree on this one. since there'll be local preferences, it'll try
> to revalidate them everytime there's a change. so, for example, an
> exception that might not be possible now, will be possible when its
> parent changes in a way that allows that.

My concern is just practical - if I've started a bunch of containers,
and another admin decides to make a change to the root devices cgroup,
I don't want the container's device accesses now changing.

Maybe that's better solved by having all of userspace sit in /system
while containers and vms sit under /lxc and /libvirt...

-serge

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

[Aristeu Rozanski]

On Thu, Nov 29, 2012 at 07:29:45PM +0000, Serge E. Hallyn wrote:
> thanks for doing this.  I've got one concern though.  I don't see
> any place where devcgroup_create() was updated to create the
> local exceptions list.  I think we need a guarantee that at
> any time the local exceptions list will contain all the entries
> contained in the cgroup->exceptions list.  Otherwise you cannot
> use the exception_add() the way you do (or you can't use RCU),
> since there could be a window between the successful addition
> of a rule to cgroup->local.exceptions and the failed addition to
> cgroup->exceptions, during which a task (since it won't need
> the mutex for devcg_allow()) could exceed allowed permissions.
> 
> It's possible I'm misunderstanding.  If you think that's the case
> just kick me and I'll take a fresh look.
> 
> (Btw is there a git tree or gitweb view I could look at alongside
> the patchset?)

ah, yes, the missing thing you're not seeing is
4b1c7840b7d01b14a1a00fa0e61b761d4391ba67 from Tejun's tree
('device_cgroup: add lockdep asserts')

-- 
Aristeu

[PATCH 4/5] device_cgroup: make may_access() stronger

[Aristeu Rozanski]

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

In order to revalidate local exceptions for the hierarchy change propagation,
make may_access() stronger.

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 |   36 ++++++++++++++++++++++++------------
 1 file changed, 24 insertions(+), 12 deletions(-)

Index: github/security/device_cgroup.c
===================================================================
--- github.orig/security/device_cgroup.c	2012-11-26 17:28:04.399150628 -0500
+++ github/security/device_cgroup.c	2012-11-27 11:57:55.932996663 -0500
@@ -352,13 +352,14 @@
  *		won't have more privileges than its parent or to
  *		verify if a certain access is allowed.
  * @dev_cgroup: dev cgroup to be tested against
+ * @behavior: behavior of the exception
  * @refex: new exception
  */
 static int may_access(struct dev_cgroup *dev_cgroup,
 		      struct dev_exception_item *refex)
 {
 	struct dev_exception_item *ex;
-	bool match = false;
+	int match = 0;
 
 	rcu_lockdep_assert(rcu_read_lock_held() ||
 			   lockdep_is_held(&devcgroup_mutex),
@@ -375,22 +376,33 @@
 			continue;
 		if (refex->access & (~ex->access))
 			continue;
-		match = true;
+		match = 1;
 		break;
 	}
 
 	/*
-	 * 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 only three possibilities are:
+	 * devcg->behavior == ALLOW, rule behavior == ALLOW
+	 * devcg->behavior == ALLOW, rule behavior == DENY
+	 * devcg->behavior == DENY, rule behavior == DENY
+	 * the remaining
+	 * devcg->behavior == DENY, rule behavior == ALLOW
+	 * won't be possible by hierarchy
+	 *
+	 * Since we want to simplify the code, here're the possibilites to
+	 * make easier to understand:
+	 *
+	 * devcg behavior   rule behavior  match  result
+	 * allow            allow          1      0
+	 * allow            allow          0      1
+	 * allow            deny           1      0
+	 * allow            deny           0      1
+	 * deny             deny           1      1
+	 * deny             deny           0      0
 	 */
-	if ((dev_cgroup->behavior == DEVCG_DEFAULT_DENY) == match)
-		return 1;
-	return 0;
+	if (dev_cgroup->behavior == DEVCG_DEFAULT_ALLOW)
+		return !match;
+	return match;
 }
 
 /*

Re: [PATCH 4/5] device_cgroup: make may_access() stronger

[Tejun Heo]

Hello,

On Tue, Nov 27, 2012 at 02:35:05PM -0500, Aristeu Rozanski wrote:
> @@ -375,22 +376,33 @@
>  			continue;
>  		if (refex->access & (~ex->access))
>  			continue;
> -		match = true;
> +		match = 1;
>  		break;
>  	}
>  
>  	/*
> -	 * 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 only three possibilities are:
> +	 * devcg->behavior == ALLOW, rule behavior == ALLOW
> +	 * devcg->behavior == ALLOW, rule behavior == DENY
> +	 * devcg->behavior == DENY, rule behavior == DENY
> +	 * the remaining
> +	 * devcg->behavior == DENY, rule behavior == ALLOW
> +	 * won't be possible by hierarchy
> +	 *
> +	 * Since we want to simplify the code, here're the possibilites to
> +	 * make easier to understand:
> +	 *
> +	 * devcg behavior   rule behavior  match  result
> +	 * allow            allow          1      0
> +	 * allow            allow          0      1
> +	 * allow            deny           1      0
> +	 * allow            deny           0      1
> +	 * deny             deny           1      1
> +	 * deny             deny           0      0
>  	 */
> -	if ((dev_cgroup->behavior == DEVCG_DEFAULT_DENY) == match)
> -		return 1;
> -	return 0;
> +	if (dev_cgroup->behavior == DEVCG_DEFAULT_ALLOW)
> +		return !match;
> +	return match;

I kinda dislike this.  This isn't a performanc critical path where we
must try our best to shave off a few condition checks.  There's no
reason to encode the test like this.  Please just spell the conditions
out in code rather than trying to build a magic series of equality
tests which somehow ends up spewing out the correct results.

Thanks.

-- 
tejun

Re: [PATCH 4/5] device_cgroup: make may_access() stronger

[Aristeu Rozanski]

On Mon, Dec 03, 2012 at 09:44:14AM -0800, Tejun Heo wrote:
> I kinda dislike this.  This isn't a performanc critical path where we
> must try our best to shave off a few condition checks.  There's no
> reason to encode the test like this.  Please just spell the conditions
> out in code rather than trying to build a magic series of equality
> tests which somehow ends up spewing out the correct results.

sure, will do. but this is not "magic", that's just logic simplification.

-- 
Aristeu

[PATCH 5/5] device_cgroup: propagate local changes down the hierarchy

[Aristeu Rozanski]

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

The local preferences will be kept and applied when allowed by its parent.

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 |  215 ++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 187 insertions(+), 28 deletions(-)

Index: github/security/device_cgroup.c
===================================================================
--- github.orig/security/device_cgroup.c	2012-11-26 17:28:12.415367313 -0500
+++ github/security/device_cgroup.c	2012-11-26 17:28:17.023491874 -0500
@@ -89,28 +89,38 @@
 	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_rcu(&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;
 
 free_and_exit:
 	list_for_each_entry_safe(ex, tmp, dest, list) {
-		list_del(&ex->list);
-		kfree(ex);
+		list_del_rcu(&ex->list);
+		kfree_rcu(ex, rcu);
 	}
 	return -ENOMEM;
 }
@@ -202,29 +212,30 @@
 	return rc;
 }
 
-/**
- * dev_exception_clean - 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(struct list_head *exceptions)
 {
 	struct dev_exception_item *ex, *tmp;
 
 	lockdep_assert_held(&devcgroup_mutex);
 
-	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);
 	}
 }
 
+/**
+ * dev_exception_clean - 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)
+{
+	__dev_exception_clean(&dev_cgroup->exceptions);
+	__dev_exception_clean(&dev_cgroup->local.exceptions);
+}
+
 /*
  * called from kernel/cgroup.c with cgroup_lock() held.
  */
@@ -435,6 +446,138 @@
 	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
+ */
+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;
+		}
+	}
+	return 0;
+
+error:
+	__dev_exception_clean(&devcg->exceptions);
+	return -ENOMEM;
+}
+
+/**
+ * propagate_behavior - propagates a change in the behavior to the children
+ * @devcg: device cgroup that changed behavior
+ *
+ * returns: 0 in case of success, != 0 in case of error
+ */
+static int propagate_behavior(struct dev_cgroup *devcg)
+{
+	struct cgroup *root = devcg->css.cgroup, *pos, *saved = NULL,
+			*prev = NULL;
+	struct dev_cgroup *parent;
+	int rc = 0;
+
+	while (1) {
+		rcu_read_lock();
+		cgroup_for_each_descendant_pre(pos, root) {
+			if (saved && prev != saved) {
+				prev = pos;
+				continue;
+			}
+			break;
+		}
+		rcu_read_unlock();
+
+		if (!pos)
+			break;
+
+		devcg = cgroup_to_devcgroup(pos);
+		parent = cgroup_to_devcgroup(pos->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;
+		saved = pos;
+	}
+
+	return rc;
+}
+
+/**
+ * propagate_exception - propagates a new exception to the children
+ * @devcg: 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)
+{
+	struct cgroup *root = devcg->css.cgroup, *pos, *saved = NULL,
+			*prev = NULL;
+	struct dev_cgroup *parent;
+	int rc = 0;
+
+	while(1) {
+		rcu_read_lock();
+		cgroup_for_each_descendant_pre(pos, root) {
+			if (saved && prev != saved) {
+				prev = pos;
+				continue;
+			}
+			break;
+		}
+		rcu_read_unlock();
+
+		if (!pos)
+			break;
+
+		devcg = cgroup_to_devcgroup(pos);
+		parent = cgroup_to_devcgroup(pos->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;
+		}
+		saved = pos;
+	}
+	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
@@ -453,7 +596,7 @@
 {
 	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;
@@ -483,16 +626,18 @@
 						 &parent->exceptions);
 			if (rc)
 				return rc;
+			rc = propagate_behavior(devcgroup);
 			break;
 		case DEVCG_DENY:
 			dev_exception_clean(devcgroup);
 			devcgroup->behavior = DEVCG_DEFAULT_DENY;
 			devcgroup->local.behavior = DEVCG_DEFAULT_DENY;
+			rc = propagate_behavior(devcgroup);
 			break;
 		default:
-			return -EINVAL;
+			rc = -EINVAL;
 		}
-		return 0;
+		return rc;
 	case 'b':
 		ex.type = DEV_BLOCK;
 		break;
@@ -578,9 +723,14 @@
 		 */
 		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
@@ -589,13 +739,22 @@
 		 */
 		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,

Re: [PATCH 5/5] device_cgroup: propagate local changes down the hierarchy

[Tejun Heo]

Hello, Aristeu.

On Tue, Nov 27, 2012 at 02:35:06PM -0500, Aristeu Rozanski wrote:
> 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.

I think the inheritance policy needs to be documented in detail
listing the possible cases and the implemented behavior preferably
with rationale.  Can you please do that?

> +/**
> + * __revalidate_exceptions - walks through the exception list and revalidates
> + *			     the exceptions based on parents' behavior and
> + *			     exceptions. Called with devcgroup_mutex held.

new line

> + * @devcg: cgroup which exceptions will be checked
> + * returns: 0 in success, -ENOMEM in case of out of memory
> + */
> +static int __revalidate_exceptions(struct dev_cgroup *devcg)

Why __?

> +{
> +	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;
> +		}
> +	}
> +	return 0;
> +
> +error:
> +	__dev_exception_clean(&devcg->exceptions);

Ditto.

> +	return -ENOMEM;
> +}
> +
> +/**
> + * propagate_behavior - propagates a change in the behavior to the children
> + * @devcg: device cgroup that changed behavior
> + *
> + * returns: 0 in case of success, != 0 in case of error
> + */
> +static int propagate_behavior(struct dev_cgroup *devcg)
> +{
> +	struct cgroup *root = devcg->css.cgroup, *pos, *saved = NULL,
> +			*prev = NULL;
> +	struct dev_cgroup *parent;
> +	int rc = 0;
> +
> +	while (1) {
> +		rcu_read_lock();
> +		cgroup_for_each_descendant_pre(pos, root) {
> +			if (saved && prev != saved) {
> +				prev = pos;
> +				continue;
> +			}
> +			break;
> +		}
> +		rcu_read_unlock();

Hmmm... this can race with new cgroup creation and a new child can
escape propagation.  devcg currently inherits from css_alloc() at
which it isn't visible to cgroup_for_each_*() iteration.  The
inheriting step should be moved to css_online() with explicit online
marking.  Please take a look at the recently posted cpuset for an
example.

> +
> +		if (!pos)
> +			break;
> +
> +		devcg = cgroup_to_devcgroup(pos);
> +		parent = cgroup_to_devcgroup(pos->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;
> +		saved = pos;
> +	}
> +
> +	return rc;
> +}
> +
> +/**
> + * propagate_exception - propagates a new exception to the children
> + * @devcg: 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)
> +{
> +	struct cgroup *root = devcg->css.cgroup, *pos, *saved = NULL,
> +			*prev = NULL;
> +	struct dev_cgroup *parent;
> +	int rc = 0;
> +
> +	while(1) {
> +		rcu_read_lock();
> +		cgroup_for_each_descendant_pre(pos, root) {
> +			if (saved && prev != saved) {
> +				prev = pos;
> +				continue;
> +			}
> +			break;
> +		}
> +		rcu_read_unlock();

Ditto.  Racy.

> +		if (!pos)
> +			break;
> +
> +		devcg = cgroup_to_devcgroup(pos);
> +		parent = cgroup_to_devcgroup(pos->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;
> +		}
> +		saved = pos;
> +	}
> +	return rc;
> +}

Maybe I'm misunderstanding something but the behavior seems a bit
inconsistent.  So, you can't add an exception which isn't allowed by
your parent, right?  But, if your parent disallows an existing
exception, you get to keep it?  I think it would be more consistent to
go either

* Allow all settings but apply only as allowed by the parent.

* Deny settings disallowed by the parent.  If parent's config changes,
  delete configs which fall outside the new config.

It seems the implemented behavior is something inbetween which can be
quite confusing.

Thanks.

-- 
tejun

Re: [PATCH 5/5] device_cgroup: propagate local changes down the hierarchy

[Aristeu Rozanski]

On Mon, Dec 03, 2012 at 10:01:45AM -0800, Tejun Heo wrote:
> On Tue, Nov 27, 2012 at 02:35:06PM -0500, Aristeu Rozanski wrote:
> > 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.
> 
> I think the inheritance policy needs to be documented in detail
> listing the possible cases and the implemented behavior preferably
> with rationale.  Can you please do that?

ok, will do

> > +/**
> > + * __revalidate_exceptions - walks through the exception list and revalidates
> > + *			     the exceptions based on parents' behavior and
> > + *			     exceptions. Called with devcgroup_mutex held.
> 
> new line

ok

> > + * @devcg: cgroup which exceptions will be checked
> > + * returns: 0 in success, -ENOMEM in case of out of memory
> > + */
> > +static int __revalidate_exceptions(struct dev_cgroup *devcg)
> 
> Why __?

hm. I think I had a version with locking in the past, forgot to take
those out. will fix that too

> > +/**
> > + * propagate_behavior - propagates a change in the behavior to the children
> > + * @devcg: device cgroup that changed behavior
> > + *
> > + * returns: 0 in case of success, != 0 in case of error
> > + */
> > +static int propagate_behavior(struct dev_cgroup *devcg)
> > +{
> > +	struct cgroup *root = devcg->css.cgroup, *pos, *saved = NULL,
> > +			*prev = NULL;
> > +	struct dev_cgroup *parent;
> > +	int rc = 0;
> > +
> > +	while (1) {
> > +		rcu_read_lock();
> > +		cgroup_for_each_descendant_pre(pos, root) {
> > +			if (saved && prev != saved) {
> > +				prev = pos;
> > +				continue;
> > +			}
> > +			break;
> > +		}
> > +		rcu_read_unlock();
> 
> Hmmm... this can race with new cgroup creation and a new child can
> escape propagation.  devcg currently inherits from css_alloc() at
> which it isn't visible to cgroup_for_each_*() iteration.  The
> inheriting step should be moved to css_online() with explicit online
> marking.  Please take a look at the recently posted cpuset for an
> example.

ok, will do
> > +/**
> > + * propagate_exception - propagates a new exception to the children
> > + * @devcg: 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)
> > +{
> > +	struct cgroup *root = devcg->css.cgroup, *pos, *saved = NULL,
> > +			*prev = NULL;
> > +	struct dev_cgroup *parent;
> > +	int rc = 0;
> > +
> > +	while(1) {
> > +		rcu_read_lock();
> > +		cgroup_for_each_descendant_pre(pos, root) {
> > +			if (saved && prev != saved) {
> > +				prev = pos;
> > +				continue;
> > +			}
> > +			break;
> > +		}
> > +		rcu_read_unlock();
> 
> Ditto.  Racy.
> 
> > +		if (!pos)
> > +			break;
> > +
> > +		devcg = cgroup_to_devcgroup(pos);
> > +		parent = cgroup_to_devcgroup(pos->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;
> > +		}
> > +		saved = pos;
> > +	}
> > +	return rc;
> > +}
> 
> Maybe I'm misunderstanding something but the behavior seems a bit
> inconsistent.  So, you can't add an exception which isn't allowed by
> your parent, right?  But, if your parent disallows an existing
> exception, you get to keep it?  I think it would be more consistent to
> go either
> 
> * Allow all settings but apply only as allowed by the parent.
> 
> * Deny settings disallowed by the parent.  If parent's config changes,
>   delete configs which fall outside the new config.

I prefer this one, in fact that's what was happening before and you
suggested to not remove local preferences when they're not valid
anymore.

In this case, Serge is right about not propagating 'allow' exceptions.

-- 
Aristeu

Re: [PATCH 5/5] device_cgroup: propagate local changes down the hierarchy

[Tejun Heo]

Hello, Aristeu.

On Mon, Dec 03, 2012 at 02:14:12PM -0500, Aristeu Rozanski wrote:
> > Maybe I'm misunderstanding something but the behavior seems a bit
> > inconsistent.  So, you can't add an exception which isn't allowed by
> > your parent, right?  But, if your parent disallows an existing
> > exception, you get to keep it?  I think it would be more consistent to
> > go either
> > 
> > * Allow all settings but apply only as allowed by the parent.
> > 
> > * Deny settings disallowed by the parent.  If parent's config changes,
> >   delete configs which fall outside the new config.
> 
> I prefer this one, in fact that's what was happening before and you
> suggested to not remove local preferences when they're not valid
> anymore.

If I'm contradicting my past self, my apologies.  I'm a bit conflicted
about what to do myself.  So, you prefer to change configurations
downstream as the parent gets updated.  I think you're right.  Let's
go with that.

Thanks.

-- 
tejun