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:

quoted

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

quoted

+/**
+ * __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

quoted

+ * @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

quoted

+/**
+ * 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

quoted

+/**
+ * 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.

quoted

+		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