[PATCH 0/4] devcg: Store local settings for each device cgroup

[PATCH 0/4] devcg: Store local settings for each device cgroup

[aris-H+wXaHxf7aLQT0dZR+AlfA]

This patchset makes possible for device cgroups to store locally set rules to
be reapplied whenever possible. This is also desired when moving device cgroups
around.

Consider:

          A
        /
       B

Behavior in both is deny. 'A' has this exception list:
	b 1:* rw
	c *:* rw
One can explicitely set on 'B':
	b 1:5 r
If for some reason 'A' removes 'b 1:* rw', 'B' exception 'b 1:5 r' will be
removed because it's not allowed anymore and would be forever lost.

Now, consider that in the parent more fine grained exceptions were added:
	b 1:1 r
	b 1:2 rw
	b 1:4 rwc
	b 1:5 r

B would still be out of exceptions (we only propagate restrictions, not added
access).

With this patchset, the 'b 1:5 r' exception will be kept and whenever possible
(more specifically when the parent gets access to more devices) it'll be
re-evaluated and applied if allowed. In this specific case, since it's allowed
again, the exception 'b 1:5 r' will be reapplied to B.

This patchset is based on for-3.12 branch on Tejun's tree
(bd8815a6d802fc16a7a106e170593aa05dc17e72) and it's available for review on my
git tree:
	git://github.com/aristeu/linux-2.6.git
	branch: devcg-rename

Signed-off-by: Aristeu Rozanski <aris-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

[PATCH 1/4] devcg: move behavior and exceptions into its own structure

[aris-H+wXaHxf7aLQT0dZR+AlfA]

From: Aristeu Rozanski <aris-moeOTchvdi7YtjvyW6yDsg@public.gmane.org>

This patch is in preparation for the next one that will allow saving locally
set rules.

Signed-off-by: Aristeu Rozanski <aris-moeOTchvdi7YtjvyW6yDsg@public.gmane.org>
---
 security/device_cgroup.c |   72 ++++++++++++++++++++++++++-------------------
 1 files changed, 42 insertions(+), 30 deletions(-)

diff --git a/security/device_cgroup.c b/security/device_cgroup.c
index c123628..a4b7df4 100644
--- a/security/device_cgroup.c
+++ b/security/device_cgroup.c
@@ -45,12 +45,23 @@ struct dev_exception_item {
 	struct rcu_head rcu;
 };
 
-struct dev_cgroup {
-	struct cgroup_subsys_state css;
+struct devcg_rules {
 	struct list_head exceptions;
 	enum devcg_behavior behavior;
 };
 
+struct dev_cgroup {
+	struct cgroup_subsys_state css;
+	struct devcg_rules active_rules;
+};
+
+static inline struct list_head *active_exceptions(struct dev_cgroup *devcg)
+{
+	return &(devcg->active_rules.exceptions);
+}
+
+#define active_behavior(devcg) ((devcg)->active_rules.behavior)
+
 static inline struct dev_cgroup *css_to_devcgroup(struct cgroup_subsys_state *s)
 {
 	return s ? container_of(s, struct dev_cgroup, css) : NULL;
@@ -113,7 +124,7 @@ static int dev_exception_add(struct dev_cgroup *dev_cgroup,
 	if (!excopy)
 		return -ENOMEM;
 
-	list_for_each_entry(walk, &dev_cgroup->exceptions, list) {
+	list_for_each_entry(walk, active_exceptions(dev_cgroup), list) {
 		if (walk->type != ex->type)
 			continue;
 		if (walk->major != ex->major)
@@ -127,7 +138,7 @@ static int dev_exception_add(struct dev_cgroup *dev_cgroup,
 	}
 
 	if (excopy != NULL)
-		list_add_tail_rcu(&excopy->list, &dev_cgroup->exceptions);
+		list_add_tail_rcu(&excopy->list, active_exceptions(dev_cgroup));
 	return 0;
 }
 
@@ -141,7 +152,7 @@ static void dev_exception_rm(struct dev_cgroup *dev_cgroup,
 
 	lockdep_assert_held(&devcgroup_mutex);
 
-	list_for_each_entry_safe(walk, tmp, &dev_cgroup->exceptions, list) {
+	list_for_each_entry_safe(walk, tmp, active_exceptions(dev_cgroup), list) {
 		if (walk->type != ex->type)
 			continue;
 		if (walk->major != ex->major)
@@ -161,7 +172,7 @@ static void __dev_exception_clean(struct dev_cgroup *dev_cgroup)
 {
 	struct dev_exception_item *ex, *tmp;
 
-	list_for_each_entry_safe(ex, tmp, &dev_cgroup->exceptions, list) {
+	list_for_each_entry_safe(ex, tmp, active_exceptions(dev_cgroup), list) {
 		list_del_rcu(&ex->list);
 		kfree_rcu(ex, rcu);
 	}
@@ -182,7 +193,7 @@ static void dev_exception_clean(struct dev_cgroup *dev_cgroup)
 
 static inline bool is_devcg_online(const struct dev_cgroup *devcg)
 {
-	return (devcg->behavior != DEVCG_DEFAULT_NONE);
+	return (active_behavior(devcg) != DEVCG_DEFAULT_NONE);
 }
 
 /**
@@ -200,12 +211,12 @@ static int devcgroup_online(struct cgroup_subsys_state *css)
 	mutex_lock(&devcgroup_mutex);
 
 	if (parent_dev_cgroup == NULL)
-		dev_cgroup->behavior = DEVCG_DEFAULT_ALLOW;
+		active_behavior(dev_cgroup) = DEVCG_DEFAULT_ALLOW;
 	else {
-		ret = dev_exceptions_copy(&dev_cgroup->exceptions,
-					  &parent_dev_cgroup->exceptions);
+		ret = dev_exceptions_copy(active_exceptions(dev_cgroup),
+					  active_exceptions(parent_dev_cgroup));
 		if (!ret)
-			dev_cgroup->behavior = parent_dev_cgroup->behavior;
+			active_behavior(dev_cgroup) = active_behavior(parent_dev_cgroup);
 	}
 	mutex_unlock(&devcgroup_mutex);
 
@@ -217,7 +228,7 @@ static void devcgroup_offline(struct cgroup_subsys_state *css)
 	struct dev_cgroup *dev_cgroup = css_to_devcgroup(css);
 
 	mutex_lock(&devcgroup_mutex);
-	dev_cgroup->behavior = DEVCG_DEFAULT_NONE;
+	active_behavior(dev_cgroup) = DEVCG_DEFAULT_NONE;
 	mutex_unlock(&devcgroup_mutex);
 }
 
@@ -232,8 +243,8 @@ devcgroup_css_alloc(struct cgroup_subsys_state *parent_css)
 	dev_cgroup = kzalloc(sizeof(*dev_cgroup), GFP_KERNEL);
 	if (!dev_cgroup)
 		return ERR_PTR(-ENOMEM);
-	INIT_LIST_HEAD(&dev_cgroup->exceptions);
-	dev_cgroup->behavior = DEVCG_DEFAULT_NONE;
+	INIT_LIST_HEAD(active_exceptions(dev_cgroup));
+	active_behavior(dev_cgroup) = DEVCG_DEFAULT_NONE;
 
 	return &dev_cgroup->css;
 }
@@ -298,14 +309,15 @@ static int devcgroup_seq_read(struct cgroup_subsys_state *css,
 	 * - List the exceptions in case the default policy is to deny
 	 * This way, the file remains as a "whitelist of devices"
 	 */
-	if (devcgroup->behavior == DEVCG_DEFAULT_ALLOW) {
+	if (active_behavior(devcgroup) == DEVCG_DEFAULT_ALLOW) {
 		set_access(acc, ACC_MASK);
 		set_majmin(maj, ~0);
 		set_majmin(min, ~0);
 		seq_printf(m, "%c %s:%s %s\n", type_to_char(DEV_ALL),
 			   maj, min, acc);
 	} else {
-		list_for_each_entry_rcu(ex, &devcgroup->exceptions, list) {
+		list_for_each_entry_rcu(ex, active_exceptions(devcgroup),
+					list) {
 			set_access(acc, ex->access);
 			set_majmin(maj, ex->major);
 			set_majmin(min, ex->minor);
@@ -339,7 +351,7 @@ static bool may_access(struct dev_cgroup *dev_cgroup,
 			   lockdep_is_held(&devcgroup_mutex),
 			   "device_cgroup::may_access() called without proper synchronization");
 
-	list_for_each_entry_rcu(ex, &dev_cgroup->exceptions, list) {
+	list_for_each_entry_rcu(ex, active_exceptions(dev_cgroup), list) {
 		if ((refex->type & DEV_BLOCK) && !(ex->type & DEV_BLOCK))
 			continue;
 		if ((refex->type & DEV_CHAR) && !(ex->type & DEV_CHAR))
@@ -354,7 +366,7 @@ static bool may_access(struct dev_cgroup *dev_cgroup,
 		break;
 	}
 
-	if (dev_cgroup->behavior == DEVCG_DEFAULT_ALLOW) {
+	if (active_behavior(dev_cgroup) == DEVCG_DEFAULT_ALLOW) {
 		if (behavior == DEVCG_DEFAULT_ALLOW) {
 			/* the exception will deny access to certain devices */
 			return true;
@@ -391,7 +403,7 @@ static int parent_has_perm(struct dev_cgroup *childcg,
 
 	if (!parent)
 		return 1;
-	return may_access(parent, ex, childcg->behavior);
+	return may_access(parent, ex, active_behavior(childcg));
 }
 
 /**
@@ -404,7 +416,7 @@ static inline int may_allow_all(struct dev_cgroup *parent)
 {
 	if (!parent)
 		return 1;
-	return parent->behavior == DEVCG_DEFAULT_ALLOW;
+	return active_behavior(parent) == DEVCG_DEFAULT_ALLOW;
 }
 
 /**
@@ -425,7 +437,7 @@ static void revalidate_active_exceptions(struct dev_cgroup *devcg)
 	struct dev_exception_item *ex;
 	struct list_head *this, *tmp;
 
-	list_for_each_safe(this, tmp, &devcg->exceptions) {
+	list_for_each_safe(this, tmp, active_exceptions(devcg)) {
 		ex = container_of(this, struct dev_exception_item, list);
 		if (!parent_has_perm(devcg, ex))
 			dev_exception_rm(devcg, ex);
@@ -465,8 +477,8 @@ static int propagate_exception(struct dev_cgroup *devcg_root,
 		 * in case both root's behavior and devcg is allow, a new
 		 * restriction means adding to the exception list
 		 */
-		if (devcg_root->behavior == DEVCG_DEFAULT_ALLOW &&
-		    devcg->behavior == DEVCG_DEFAULT_ALLOW) {
+		if (active_behavior(devcg_root) == DEVCG_DEFAULT_ALLOW &&
+		    active_behavior(devcg) == DEVCG_DEFAULT_ALLOW) {
 			rc = dev_exception_add(devcg, ex);
 			if (rc)
 				break;
@@ -533,12 +545,12 @@ static int devcgroup_update_access(struct dev_cgroup *devcgroup,
 			if (!may_allow_all(parent))
 				return -EPERM;
 			dev_exception_clean(devcgroup);
-			devcgroup->behavior = DEVCG_DEFAULT_ALLOW;
+			active_behavior(devcgroup) = DEVCG_DEFAULT_ALLOW;
 			if (!parent)
 				break;
 
-			rc = dev_exceptions_copy(&devcgroup->exceptions,
-						 &parent->exceptions);
+			rc = dev_exceptions_copy(active_exceptions(devcgroup),
+						 active_exceptions(parent));
 			if (rc)
 				return rc;
 			break;
@@ -547,7 +559,7 @@ static int devcgroup_update_access(struct dev_cgroup *devcgroup,
 				return -EINVAL;
 
 			dev_exception_clean(devcgroup);
-			devcgroup->behavior = DEVCG_DEFAULT_DENY;
+			active_behavior(devcgroup) = DEVCG_DEFAULT_DENY;
 			break;
 		default:
 			return -EINVAL;
@@ -636,7 +648,7 @@ static int devcgroup_update_access(struct dev_cgroup *devcgroup,
 		 * an matching exception instead. And be silent about it: we
 		 * don't want to break compatibility
 		 */
-		if (devcgroup->behavior == DEVCG_DEFAULT_ALLOW) {
+		if (active_behavior(devcgroup) == DEVCG_DEFAULT_ALLOW) {
 			dev_exception_rm(devcgroup, &ex);
 			return 0;
 		}
@@ -648,7 +660,7 @@ static int devcgroup_update_access(struct dev_cgroup *devcgroup,
 		 * an matching exception instead. And be silent about it: we
 		 * don't want to break compatibility
 		 */
-		if (devcgroup->behavior == DEVCG_DEFAULT_DENY)
+		if (active_behavior(devcgroup) == DEVCG_DEFAULT_DENY)
 			dev_exception_rm(devcgroup, &ex);
 		else
 			rc = dev_exception_add(devcgroup, &ex);
@@ -731,7 +743,7 @@ static int __devcgroup_check_permission(short type, u32 major, u32 minor,
 
 	rcu_read_lock();
 	dev_cgroup = task_devcgroup(current);
-	rc = may_access(dev_cgroup, &ex, dev_cgroup->behavior);
+	rc = may_access(dev_cgroup, &ex, active_behavior(dev_cgroup));
 	rcu_read_unlock();
 
 	if (!rc)
-- 
1.7.1

[PATCH 2/4] devcg: make dev_exception_ functions to use lists

[aris-H+wXaHxf7aLQT0dZR+AlfA]

From: Aristeu Rozanski <arozansk-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

In preparation for having active and locally set list of rules.

Signed-off-by: Aristeu Rozanski <arozansk-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 security/device_cgroup.c |   24 ++++++++++++------------
 1 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/security/device_cgroup.c b/security/device_cgroup.c
index a4b7df4..8461e0f 100644
--- a/security/device_cgroup.c
+++ b/security/device_cgroup.c
@@ -113,7 +113,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;
@@ -124,7 +124,7 @@ static int dev_exception_add(struct dev_cgroup *dev_cgroup,
 	if (!excopy)
 		return -ENOMEM;
 
-	list_for_each_entry(walk, active_exceptions(dev_cgroup), list) {
+	list_for_each_entry(walk, exceptions, list) {
 		if (walk->type != ex->type)
 			continue;
 		if (walk->major != ex->major)
@@ -138,21 +138,21 @@ static int dev_exception_add(struct dev_cgroup *dev_cgroup,
 	}
 
 	if (excopy != NULL)
-		list_add_tail_rcu(&excopy->list, active_exceptions(dev_cgroup));
+		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, active_exceptions(dev_cgroup), list) {
+	list_for_each_entry_safe(walk, tmp, exceptions, list) {
 		if (walk->type != ex->type)
 			continue;
 		if (walk->major != ex->major)
@@ -440,7 +440,7 @@ static void revalidate_active_exceptions(struct dev_cgroup *devcg)
 	list_for_each_safe(this, tmp, active_exceptions(devcg)) {
 		ex = container_of(this, struct dev_exception_item, list);
 		if (!parent_has_perm(devcg, ex))
-			dev_exception_rm(devcg, ex);
+			dev_exception_rm(active_exceptions(devcg), ex);
 	}
 }
 
@@ -479,7 +479,7 @@ static int propagate_exception(struct dev_cgroup *devcg_root,
 		 */
 		if (active_behavior(devcg_root) == DEVCG_DEFAULT_ALLOW &&
 		    active_behavior(devcg) == DEVCG_DEFAULT_ALLOW) {
-			rc = dev_exception_add(devcg, ex);
+			rc = dev_exception_add(active_exceptions(devcg), ex);
 			if (rc)
 				break;
 		} else {
@@ -489,7 +489,7 @@ static int propagate_exception(struct dev_cgroup *devcg_root,
 			 * root's behavior: deny, devcg's: deny
 			 * the exception will be removed
 			 */
-			dev_exception_rm(devcg, ex);
+			dev_exception_rm(active_exceptions(devcg), ex);
 		}
 		revalidate_active_exceptions(devcg);
 
@@ -649,10 +649,10 @@ static int devcgroup_update_access(struct dev_cgroup *devcgroup,
 		 * don't want to break compatibility
 		 */
 		if (active_behavior(devcgroup) == DEVCG_DEFAULT_ALLOW) {
-			dev_exception_rm(devcgroup, &ex);
+			dev_exception_rm(active_exceptions(devcgroup), &ex);
 			return 0;
 		}
-		rc = dev_exception_add(devcgroup, &ex);
+		rc = dev_exception_add(active_exceptions(devcgroup), &ex);
 		break;
 	case DEVCG_DENY:
 		/*
@@ -661,9 +661,9 @@ static int devcgroup_update_access(struct dev_cgroup *devcgroup,
 		 * don't want to break compatibility
 		 */
 		if (active_behavior(devcgroup) == DEVCG_DEFAULT_DENY)
-			dev_exception_rm(devcgroup, &ex);
+			dev_exception_rm(active_exceptions(devcgroup), &ex);
 		else
-			rc = dev_exception_add(devcgroup, &ex);
+			rc = dev_exception_add(active_exceptions(devcgroup), &ex);
 
 		if (rc)
 			break;
-- 
1.7.1

[PATCH 3/4] devcg: save locally saved settings

[aris-H+wXaHxf7aLQT0dZR+AlfA]

From: Aristeu Rozanski <arozansk-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Whenever writing rules in the current directory, save these rules so it
can be used whenever parent's rules change. This patch prepares for
revalidating the local rules based on parent's current state, which is
not only needed to retain whenever possible the local settings but to
allow moving the group to another part of the tree.

Signed-off-by: Aristeu Rozanski <arozansk-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 security/device_cgroup.c |   35 +++++++++++++++++++++++++++++++----
 1 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/security/device_cgroup.c b/security/device_cgroup.c
index 8461e0f..7f55bb7 100644
--- a/security/device_cgroup.c
+++ b/security/device_cgroup.c
@@ -53,6 +53,7 @@ struct devcg_rules {
 struct dev_cgroup {
 	struct cgroup_subsys_state css;
 	struct devcg_rules active_rules;
+	struct devcg_rules local_rules;
 };
 
 static inline struct list_head *active_exceptions(struct dev_cgroup *devcg)
@@ -62,6 +63,13 @@ static inline struct list_head *active_exceptions(struct dev_cgroup *devcg)
 
 #define active_behavior(devcg) ((devcg)->active_rules.behavior)
 
+static inline struct list_head *local_exceptions(struct dev_cgroup *devcg)
+{
+	return &(devcg->local_rules.exceptions);
+}
+
+#define local_behavior(devcg) ((devcg)->local_rules.behavior)
+
 static inline struct dev_cgroup *css_to_devcgroup(struct cgroup_subsys_state *s)
 {
 	return s ? container_of(s, struct dev_cgroup, css) : NULL;
@@ -168,16 +176,22 @@ static void dev_exception_rm(struct list_head *exceptions,
 	}
 }
 
-static void __dev_exception_clean(struct dev_cgroup *dev_cgroup)
+static void __dev_exception_clean_one(struct list_head *exceptions)
 {
 	struct dev_exception_item *ex, *tmp;
 
-	list_for_each_entry_safe(ex, tmp, active_exceptions(dev_cgroup), list) {
+	list_for_each_entry_safe(ex, tmp, exceptions, list) {
 		list_del_rcu(&ex->list);
 		kfree_rcu(ex, rcu);
 	}
 }
 
+static void __dev_exception_clean(struct dev_cgroup *dev_cgroup)
+{
+	__dev_exception_clean_one(active_exceptions(dev_cgroup));
+	__dev_exception_clean_one(local_exceptions(dev_cgroup));
+}
+
 /**
  * dev_exception_clean - frees all entries of the exception list
  * @dev_cgroup: dev_cgroup with the exception list to be cleaned
@@ -245,6 +259,8 @@ devcgroup_css_alloc(struct cgroup_subsys_state *parent_css)
 		return ERR_PTR(-ENOMEM);
 	INIT_LIST_HEAD(active_exceptions(dev_cgroup));
 	active_behavior(dev_cgroup) = DEVCG_DEFAULT_NONE;
+	INIT_LIST_HEAD(local_exceptions(dev_cgroup));
+	local_behavior(dev_cgroup) = DEVCG_DEFAULT_NONE;
 
 	return &dev_cgroup->css;
 }
@@ -546,6 +562,9 @@ static int devcgroup_update_access(struct dev_cgroup *devcgroup,
 				return -EPERM;
 			dev_exception_clean(devcgroup);
 			active_behavior(devcgroup) = DEVCG_DEFAULT_ALLOW;
+			if (local_behavior(devcgroup) == DEVCG_DEFAULT_DENY)
+				__dev_exception_clean_one(local_exceptions(devcgroup));
+			local_behavior(devcgroup) = DEVCG_DEFAULT_ALLOW;
 			if (!parent)
 				break;
 
@@ -560,6 +579,9 @@ static int devcgroup_update_access(struct dev_cgroup *devcgroup,
 
 			dev_exception_clean(devcgroup);
 			active_behavior(devcgroup) = DEVCG_DEFAULT_DENY;
+			if (local_behavior(devcgroup) == DEVCG_DEFAULT_ALLOW)
+				__dev_exception_clean_one(local_exceptions(devcgroup));
+			local_behavior(devcgroup) = DEVCG_DEFAULT_DENY;
 			break;
 		default:
 			return -EINVAL;
@@ -650,9 +672,11 @@ static int devcgroup_update_access(struct dev_cgroup *devcgroup,
 		 */
 		if (active_behavior(devcgroup) == DEVCG_DEFAULT_ALLOW) {
 			dev_exception_rm(active_exceptions(devcgroup), &ex);
+			dev_exception_rm(local_exceptions(devcgroup), &ex);
 			return 0;
 		}
 		rc = dev_exception_add(active_exceptions(devcgroup), &ex);
+		rc = dev_exception_add(local_exceptions(devcgroup), &ex);
 		break;
 	case DEVCG_DENY:
 		/*
@@ -660,10 +684,13 @@ static int devcgroup_update_access(struct dev_cgroup *devcgroup,
 		 * an matching exception instead. And be silent about it: we
 		 * don't want to break compatibility
 		 */
-		if (active_behavior(devcgroup) == DEVCG_DEFAULT_DENY)
+		if (active_behavior(devcgroup) == DEVCG_DEFAULT_DENY) {
 			dev_exception_rm(active_exceptions(devcgroup), &ex);
-		else
+			dev_exception_rm(local_exceptions(devcgroup), &ex);
+		} else {
 			rc = dev_exception_add(active_exceptions(devcgroup), &ex);
+			rc = dev_exception_add(local_exceptions(devcgroup), &ex);
+		}
 
 		if (rc)
 			break;
-- 
1.7.1

[PATCH 4/4] devcg: try to reapply local settings

[aris-H+wXaHxf7aLQT0dZR+AlfA]

From: Aristeu Rozanski <arozansk-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Whenever there's a change in parent's rules, try to apply the locally
set rules. For now, local and parent's behavior must match.

Signed-off-by: Aristeu Rozanski <arozansk-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 security/device_cgroup.c |   83 +++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 82 insertions(+), 1 deletions(-)

diff --git a/security/device_cgroup.c b/security/device_cgroup.c
index 7f55bb7..5996888 100644
--- a/security/device_cgroup.c
+++ b/security/device_cgroup.c
@@ -497,7 +497,7 @@ static int propagate_exception(struct dev_cgroup *devcg_root,
 		    active_behavior(devcg) == DEVCG_DEFAULT_ALLOW) {
 			rc = dev_exception_add(active_exceptions(devcg), ex);
 			if (rc)
-				break;
+				goto out;
 		} else {
 			/*
 			 * in the other possible cases:
@@ -513,6 +513,73 @@ static int propagate_exception(struct dev_cgroup *devcg_root,
 	}
 
 	rcu_read_unlock();
+
+out:
+	return rc;
+}
+
+/**
+ * apply_local_rules - apply locally set rules if permitted by parent's rules
+ * @devcg: cgroup which local rules will be exceptions will be checked
+ * returns: 0 in case of success or error code
+ */
+static int apply_local_rules(struct dev_cgroup *devcg)
+{
+	struct dev_exception_item *ex;
+	struct list_head *this, *tmp;
+	int rc = 0;
+
+	if (active_behavior(devcg) != local_behavior(devcg))
+		return rc;
+
+	list_for_each_safe(this, tmp, local_exceptions(devcg)) {
+		ex = container_of(this, struct dev_exception_item, list);
+		if (parent_has_perm(devcg, ex)) {
+			rc = dev_exception_add(active_exceptions(devcg), ex);
+			if (rc)
+				break;
+		}
+	}
+
+	return rc;
+}
+
+/**
+ * apply_local_rules_tree - scans a dev_cgroup and its descendents for local
+ * 			    rules that might be used. This is called after
+ * 			    devcg gets access to more devices.
+ * @devcg_root: the first cgroup to be checked
+ * returns: 0 in case of success or error code
+ */
+static int apply_local_rules_tree(struct dev_cgroup *devcg_root)
+{
+	struct cgroup_subsys_state *pos;
+	int rc = 0;
+
+	rcu_read_lock();
+
+	css_for_each_descendant_pre(pos, &devcg_root->css) {
+		struct dev_cgroup *devcg = css_to_devcgroup(pos);
+
+		/*
+		 * Because devcgroup_mutex is held, no devcg will become
+		 * online or offline during the tree walk (see on/offline
+		 * methods), and online ones are safe to access outside RCU
+		 * read lock without bumping refcnt.
+		 */
+		if (pos == &devcg_root->css || !is_devcg_online(devcg))
+			continue;
+
+		rcu_read_unlock();
+
+		rc = apply_local_rules(devcg);
+		if (rc)
+			goto out;
+
+		rcu_read_lock();
+	}
+	rcu_read_unlock();
+out:
 	return rc;
 }
 
@@ -676,7 +743,17 @@ static int devcgroup_update_access(struct dev_cgroup *devcgroup,
 			return 0;
 		}
 		rc = dev_exception_add(active_exceptions(devcgroup), &ex);
+		if (rc)
+			break;
+		if (local_behavior(devcgroup) == DEVCG_DEFAULT_NONE)
+			local_behavior(devcgroup) = active_behavior(devcgroup);
 		rc = dev_exception_add(local_exceptions(devcgroup), &ex);
+		if (rc)
+			break;
+		/* whenever a cgroup gains access to something else, it's time
+		 * to re-evaluate if it's possible to apply any new local
+		 * settings that aren't in effect */
+		rc = apply_local_rules_tree(devcgroup);
 		break;
 	case DEVCG_DENY:
 		/*
@@ -689,6 +766,10 @@ static int devcgroup_update_access(struct dev_cgroup *devcgroup,
 			dev_exception_rm(local_exceptions(devcgroup), &ex);
 		} else {
 			rc = dev_exception_add(active_exceptions(devcgroup), &ex);
+			if (rc)
+				break;
+			if (local_behavior(devcgroup) == DEVCG_DEFAULT_NONE)
+				local_behavior(devcgroup) = active_behavior(devcgroup);
 			rc = dev_exception_add(local_exceptions(devcgroup), &ex);
 		}
 
-- 
1.7.1

Re: [PATCH 0/4] devcg: Store local settings for each device cgroup

[Tejun Heo]

Hello, Aristeu.

On Thu, Aug 15, 2013 at 11:34:10AM -0400, aris-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org wrote:
> With this patchset, the 'b 1:5 r' exception will be kept and whenever possible
> (more specifically when the parent gets access to more devices) it'll be
> re-evaluated and applied if allowed. In this specific case, since it's allowed
> again, the exception 'b 1:5 r' will be reapplied to B.

So, while this patchset is headed in the right direction, some stuff
still bothers me.

* The configurations are finicky and complex.  There are many ways to
  configure it and some may fail depending on some conditions.  I
  really wish it were a lot simpler, at least when sane_behavior.

* Using separate propagation paths for allows and denys feels a bit
  weird.  Can't config just update local config and always propagate
  the change downwards?

When sane_behavior, can't we have something like the following?

* Setting local config is not affected by what ancestors or
  descendants are doing.  It just sets local config and triggers
  propagation and never fails (except for things like alloc failure).

* Config defaults to allow-all unconfigured and there are only two
  modes - allow-all or allow-only-listed with an easy way to flip
  between the two and clear the list, which lists either specific
  maj:min or maj.

Thanks.

-- 
tejun

Re: [PATCH 0/4] devcg: Store local settings for each device cgroup

[Aristeu Rozanski]

Hi Tejun,
On Thu, Aug 15, 2013 at 03:59:41PM -0400, Tejun Heo wrote:
> On Thu, Aug 15, 2013 at 11:34:10AM -0400, aris-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org wrote:
> > With this patchset, the 'b 1:5 r' exception will be kept and whenever possible
> > (more specifically when the parent gets access to more devices) it'll be
> > re-evaluated and applied if allowed. In this specific case, since it's allowed
> > again, the exception 'b 1:5 r' will be reapplied to B.
> 
> So, while this patchset is headed in the right direction, some stuff
> still bothers me.
> 
> * The configurations are finicky and complex.  There are many ways to
>   configure it and some may fail depending on some conditions.  I
>   really wish it were a lot simpler, at least when sane_behavior.

The ways it can fail is based on parent's permission. Yes, it's complex,
but part of it was based on decisions taken to make the code simpler.
For example: one thing left out from the last patchset was propagating
access to new devices: let's say you add an exception to a behavior deny
cgroup, it'd not be propagated down.

I'd prefer just top down changes. Parent allows X? cgroup allows X
unless local rule says otherwise.

> * Using separate propagation paths for allows and denys feels a bit
>   weird.  Can't config just update local config and always propagate
>   the change downwards?

That was a design decision back then. It might be doable to keep the
same way it is by simply not adding extra access to devices in cgroups
when propagating an exception like that. But that will only make both
paths call the same function, not really any different.

> When sane_behavior, can't we have something like the following?
> 
> * Setting local config is not affected by what ancestors or
>   descendants are doing.  It just sets local config and triggers
>   propagation and never fails (except for things like alloc failure).

That could be done but would certanly break the interface. Perhaps a
good time to introduce a newer interface that can mirror the
behavior/exception model to userspace?

> * Config defaults to allow-all unconfigured and there are only two
>   modes - allow-all or allow-only-listed with an easy way to flip
>   between the two and clear the list, which lists either specific
>   maj:min or maj.

That would get rid of the 'allow all but with some exceptions' model
which is useful in cases such when you just don't want scsi devices to be
written, but pretty much everything else is ok. Wonder how many people
would miss it?

-- 
Aristeu

Re: [PATCH 0/4] devcg: Store local settings for each device cgroup

[Tejun Heo]

Hello,

On Thu, Aug 15, 2013 at 04:48:04PM -0400, Aristeu Rozanski wrote:
> > * The configurations are finicky and complex.  There are many ways to
> >   configure it and some may fail depending on some conditions.  I
> >   really wish it were a lot simpler, at least when sane_behavior.
> 
> The ways it can fail is based on parent's permission. Yes, it's complex,
> but part of it was based on decisions taken to make the code simpler.
> For example: one thing left out from the last patchset was propagating
> access to new devices: let's say you add an exception to a behavior deny
> cgroup, it'd not be propagated down.
> 
> I'd prefer just top down changes. Parent allows X? cgroup allows X
> unless local rule says otherwise.

I think the configuration rules are too complex.  Without being privy
to the implementation itself, the restrictions seem mostly arbitrary.
The thing with this sort of greedy restrictions is that it might seem
okay for a while after enough tweaking (the current state) but it gets
weird as soon as the circumstance changes a bit and all the tweakings
quickly become extra baggages to carry around.

e.g. if a cgroup will be allowed to be moved to another position in
the hierarchy, the cgroup shouldn't change its configuration across
such migration, right?  If so, we'd be allowing creating a cgroup
outside the hierarchy with any configuration moving it under another
while disallowing creating it under there and creating the same
configuration, which would be an outright buggy behavior.

> > * Using separate propagation paths for allows and denys feels a bit
> >   weird.  Can't config just update local config and always propagate
> >   the change downwards?
> 
> That was a design decision back then. It might be doable to keep the
> same way it is by simply not adding extra access to devices in cgroups
> when propagating an exception like that. But that will only make both
> paths call the same function, not really any different.

The thing is I really don't like specific config change triggering
different actions.  It really should be "this chagned, recalculate all
rules in this sub-hierarchy" rather than "this may affect XXX and YYY
but not ZZZ so let's trigger this subpart".  The latter is way more
tricky to fragile and difficult to follow and verify.  In addition, to
support migration, it should be ready for any set of config changes in
a go anyway.

> > When sane_behavior, can't we have something like the following?
> > 
> > * Setting local config is not affected by what ancestors or
> >   descendants are doing.  It just sets local config and triggers
> >   propagation and never fails (except for things like alloc failure).
> 
> That could be done but would certanly break the interface. Perhaps a
> good time to introduce a newer interface that can mirror the
> behavior/exception model to userspace?

That's what cgroup_sane_behavior() is for.  It's essentially the next
revision of cgroup interface.

> > * Config defaults to allow-all unconfigured and there are only two
> >   modes - allow-all or allow-only-listed with an easy way to flip
> >   between the two and clear the list, which lists either specific
> >   maj:min or maj.
> 
> That would get rid of the 'allow all but with some exceptions' model
> which is useful in cases such when you just don't want scsi devices to be
> written, but pretty much everything else is ok. Wonder how many people
> would miss it?

The reason why I want to get rid of disallow w/ exceptions is that
it's difficult to stack properly and ends up with this hodgepodge of
restrictions which can serve a set of contorted requirements at the
cost of overall consitent design which can be evolved and maintained
in the long term.  If the majority of use cases are whitelisting, I
think it'd be a better idea to just stick with whitelisting.

Thanks.

-- 
tejun

Re: [PATCH 0/4] devcg: Store local settings for each device cgroup

[Aristeu Rozanski]

Hi Tejun,
On Thu, Aug 15, 2013 at 05:09:37PM -0400, Tejun Heo wrote:
> I think the configuration rules are too complex.  Without being privy
> to the implementation itself, the restrictions seem mostly arbitrary.
> The thing with this sort of greedy restrictions is that it might seem
> okay for a while after enough tweaking (the current state) but it gets
> weird as soon as the circumstance changes a bit and all the tweakings
> quickly become extra baggages to carry around.

Agreed.

> e.g. if a cgroup will be allowed to be moved to another position in
> the hierarchy, the cgroup shouldn't change its configuration across
> such migration, right?  If so, we'd be allowing creating a cgroup
> outside the hierarchy with any configuration moving it under another
> while disallowing creating it under there and creating the same
> configuration, which would be an outright buggy behavior.

Hm, then I think I got the requirements wrong. On my understanding you
cannot have a cgroup with more access than its parent. And the reason
for that is at that time the idea of having a container able to control
its own hierarchy was valid.
The way it is right now, when moved, a cgroup would have its active
rules re-checked for permission and it'd be made an attempt to apply
local rules if the new parent allows it.

> The thing is I really don't like specific config change triggering
> different actions.  It really should be "this chagned, recalculate all
> rules in this sub-hierarchy" rather than "this may affect XXX and YYY
> but not ZZZ so let's trigger this subpart".  The latter is way more
> tricky to fragile and difficult to follow and verify.  In addition, to
> support migration, it should be ready for any set of config changes in
> a go anyway.

IIRC it was to simplify the code. That can change of course.

> That's what cgroup_sane_behavior() is for.  It's essentially the next
> revision of cgroup interface.

Understood, I think after we get the internals sorted out (like going
for allow all or allow the listed) first

> The reason why I want to get rid of disallow w/ exceptions is that
> it's difficult to stack properly and ends up with this hodgepodge of
> restrictions which can serve a set of contorted requirements at the
> cost of overall consitent design which can be evolved and maintained
> in the long term.  If the majority of use cases are whitelisting, I
> think it'd be a better idea to just stick with whitelisting.

OK, will work on that

-- 
Aristeu

Re: [PATCH 0/4] devcg: Store local settings for each device cgroup

[Tejun Heo]

Hello, Aristeu.

On Fri, Aug 16, 2013 at 11:20:26AM -0400, Aristeu Rozanski wrote:
> > e.g. if a cgroup will be allowed to be moved to another position in
> > the hierarchy, the cgroup shouldn't change its configuration across
> > such migration, right?  If so, we'd be allowing creating a cgroup
> > outside the hierarchy with any configuration moving it under another
> > while disallowing creating it under there and creating the same
> > configuration, which would be an outright buggy behavior.
> 
> Hm, then I think I got the requirements wrong. On my understanding you
> cannot have a cgroup with more access than its parent. And the reason
> for that is at that time the idea of having a container able to control
> its own hierarchy was valid.

Oh yes, sure, a descendant shouldn't have access to devices which
aren't allowed by its ancestors but that doesn't mean it can't have a
rule to allow such access.  It just wouldn't make any difference at
that point.  If the ancestors later change the configuration such that
the device is allowed, the cgroup will have access to that
automatically.  Rules configured and rules being applied need to be
distinguished and the latter doesn't need to affect the former.

> The way it is right now, when moved, a cgroup would have its active
> rules re-checked for permission and it'd be made an attempt to apply
> local rules if the new parent allows it.

Yeah, that's the correct behavior, if I'm not misunderstanding you,
but to be consistent we also need to allow creating rules which allow
devices which aren't allowed by ancestors.  It won't be applicable at
rule creation but may later become effective later on.

Thanks a lot for working on this!

-- 
tejun

Re: [PATCH 0/4] devcg: Store local settings for each device cgroup

[Aristeu Rozanski]

Hi Tejun,
On Fri, Aug 16, 2013 at 11:47:57AM -0400, Tejun Heo wrote:
> On Fri, Aug 16, 2013 at 11:20:26AM -0400, Aristeu Rozanski wrote:
> > > e.g. if a cgroup will be allowed to be moved to another position in
> > > the hierarchy, the cgroup shouldn't change its configuration across
> > > such migration, right?  If so, we'd be allowing creating a cgroup
> > > outside the hierarchy with any configuration moving it under another
> > > while disallowing creating it under there and creating the same
> > > configuration, which would be an outright buggy behavior.
> > 
> > Hm, then I think I got the requirements wrong. On my understanding you
> > cannot have a cgroup with more access than its parent. And the reason
> > for that is at that time the idea of having a container able to control
> > its own hierarchy was valid.
> 
> Oh yes, sure, a descendant shouldn't have access to devices which
> aren't allowed by its ancestors but that doesn't mean it can't have a
> rule to allow such access.  It just wouldn't make any difference at
> that point.  If the ancestors later change the configuration such that
> the device is allowed, the cgroup will have access to that
> automatically.  Rules configured and rules being applied need to be
> distinguished and the latter doesn't need to affect the former.
> 
> > The way it is right now, when moved, a cgroup would have its active
> > rules re-checked for permission and it'd be made an attempt to apply
> > local rules if the new parent allows it.
> 
> Yeah, that's the correct behavior, if I'm not misunderstanding you,
> but to be consistent we also need to allow creating rules which allow
> devices which aren't allowed by ancestors.  It won't be applicable at
> rule creation but may later become effective later on.

Oh, I see, it's just matter of allowing to set the desired set or rules
locally even if they're not possible at the moment.

So, considering we drop in sane_behavior the allow + exceptions case,
the interface in sane_behavior mode would look like:
- policy: {allow_all,deny}
	writing either will clear the active aw
- active_whitelist
	list of in effect rules, read only
- whitelist
	list of locally set rules, read only
- whitelist_add
	write only, adds rule to the local list and active lists
- whitelist_remove
	write only, removes rule from the local and active lists

What you think?

-- 
Aristeu

Re: [PATCH 0/4] devcg: Store local settings for each device cgroup

[Tejun Heo]

Hello,

On Fri, Aug 16, 2013 at 12:02:04PM -0400, Aristeu Rozanski wrote:
> > Yeah, that's the correct behavior, if I'm not misunderstanding you,
> > but to be consistent we also need to allow creating rules which allow
> > devices which aren't allowed by ancestors.  It won't be applicable at
> > rule creation but may later become effective later on.
> 
> Oh, I see, it's just matter of allowing to set the desired set or rules
> locally even if they're not possible at the moment.

Yeah, otherwise, we'd get into situation where setting rules in place
isn't allowed but moving it out of hierarchy, setting it and then
moving it back would work, which doesn't make much sense.

> So, considering we drop in sane_behavior the allow + exceptions case,
> the interface in sane_behavior mode would look like:
> - policy: {allow_all,deny}
> 	writing either will clear the active aw
> - active_whitelist
> 	list of in effect rules, read only
> - whitelist
> 	list of locally set rules, read only
> - whitelist_add
> 	write only, adds rule to the local list and active lists
> - whitelist_remove
> 	write only, removes rule from the local and active lists
> 
> What you think?

Yeah, I think that should work although you might also need
active_policy and "effective" might be a better choice as prefix.
Kay, Lennart, what do you guys think?

Thanks.

-- 
tejun

Re: [PATCH 0/4] devcg: Store local settings for each device cgroup

[Li Zefan]

Cc: Serge

On 2013/8/17 0:09, Tejun Heo wrote:
> Hello,
> 
> On Fri, Aug 16, 2013 at 12:02:04PM -0400, Aristeu Rozanski wrote:
>>> Yeah, that's the correct behavior, if I'm not misunderstanding you,
>>> but to be consistent we also need to allow creating rules which allow
>>> devices which aren't allowed by ancestors.  It won't be applicable at
>>> rule creation but may later become effective later on.
>>
>> Oh, I see, it's just matter of allowing to set the desired set or rules
>> locally even if they're not possible at the moment.
> 
> Yeah, otherwise, we'd get into situation where setting rules in place
> isn't allowed but moving it out of hierarchy, setting it and then
> moving it back would work, which doesn't make much sense.
> 
>> So, considering we drop in sane_behavior the allow + exceptions case,
>> the interface in sane_behavior mode would look like:
>> - policy: {allow_all,deny}
>> 	writing either will clear the active aw
>> - active_whitelist
>> 	list of in effect rules, read only
>> - whitelist
>> 	list of locally set rules, read only
>> - whitelist_add
>> 	write only, adds rule to the local list and active lists
>> - whitelist_remove
>> 	write only, removes rule from the local and active lists
>>
>> What you think?
> 
> Yeah, I think that should work although you might also need
> active_policy and "effective" might be a better choice as prefix.

As I've started to work on cpuset side, I'm also thinking about
adding cpuset.effective_cpus and cpuset.effective_mems.

> Kay, Lennart, what do you guys think?
> 

As Serge is the original author of devcg, let's also see how
Serge feel about the new interfaces?

Re: [PATCH 0/4] devcg: Store local settings for each device cgroup

[Aristeu Rozanski]

On Mon, Aug 19, 2013 at 10:53:06AM +0800, Li Zefan wrote:
> Cc: Serge

ugh, my apologies for not Cc'ing you on the patches Serge.

-- 
Aristeu

Re: [PATCH 0/4] devcg: Store local settings for each device cgroup

[Serge E. Hallyn]

Quoting Li Zefan (lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org):
> Cc: Serge
> 
> On 2013/8/17 0:09, Tejun Heo wrote:
> > Hello,
> > 
> > On Fri, Aug 16, 2013 at 12:02:04PM -0400, Aristeu Rozanski wrote:
> >>> Yeah, that's the correct behavior, if I'm not misunderstanding you,
> >>> but to be consistent we also need to allow creating rules which allow
> >>> devices which aren't allowed by ancestors.  It won't be applicable at
> >>> rule creation but may later become effective later on.
> >>
> >> Oh, I see, it's just matter of allowing to set the desired set or rules
> >> locally even if they're not possible at the moment.
> > 
> > Yeah, otherwise, we'd get into situation where setting rules in place
> > isn't allowed but moving it out of hierarchy, setting it and then
> > moving it back would work, which doesn't make much sense.
> > 
> >> So, considering we drop in sane_behavior the allow + exceptions case,
> >> the interface in sane_behavior mode would look like:
> >> - policy: {allow_all,deny}
> >> 	writing either will clear the active aw
> >> - active_whitelist
> >> 	list of in effect rules, read only
> >> - whitelist
> >> 	list of locally set rules, read only
> >> - whitelist_add
> >> 	write only, adds rule to the local list and active lists
> >> - whitelist_remove
> >> 	write only, removes rule from the local and active lists
> >>
> >> What you think?
> > 
> > Yeah, I think that should work although you might also need
> > active_policy and "effective" might be a better choice as prefix.
> 
> As I've started to work on cpuset side, I'm also thinking about
> adding cpuset.effective_cpus and cpuset.effective_mems.
> 
> > Kay, Lennart, what do you guys think?
> > 
> 
> As Serge is the original author of devcg, let's also see how
> Serge feel about the new interfaces?

(Hm, I thought I was subscribed to cgroup list, but I'm not seeing any
of these messages, thanks for the cc)

So long as there is a clear way to tell which interface we have,
(and hopefully it won't be changing again) it seems good.

thanks,
-serge