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

[Serge E. Hallyn]

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

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

So do you need

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

here?

> +		if (devcg->local.behavior == devcg->behavior)
> +			rc = revalidate_exceptions(devcg);
> +		if (rc)
> +			break;
> +		list_del_init(&devcg->propagate_pending);
> +	}
> +
> +	return rc;
> +}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/