On Wed, May 01, 2013 at 02:30:47PM -0700, John Johansen wrote: I just noticed that these changes have swapped the order of two operations. Previously, ns->unconfined was updated to ns->parent->unconfined before all the profiles in the namespace are released. With this change, all the profiles in the namespace are released (which means they take ns->unconfined->label for themselves), and then ns->unconfined is replaced with ns->parent->unconfined.
While destroy_namespace() in newer iterations populates the replacedby
struct, I'm worried that the aa_put_profile(unconfined) means the refcount
is dropped despite processes actively using the original ns->unconfined.
Here's the parts that worry me:
> @@ -510,17 +494,40 @@ static void __ns_list_release(struct list_head *head);
> */
> static void destroy_namespace(struct aa_namespace *ns)
> {
> + struct aa_profile *unconfined;
> +
> if (!ns)
> return;
>
> - write_lock(&ns->lock);
> + mutex_lock(&ns->lock);
> /* release all profiles in this namespace */
> __profile_list_release(&ns->base.profiles);
>
> /* release all sub namespaces */
> __ns_list_release(&ns->sub_ns);
>
> - write_unlock(&ns->lock);
> + unconfined = ns->unconfined;
> + /*
> + * break the ns, unconfined profile cyclic reference and forward
> + * all new unconfined profiles requests to the parent namespace
> + * This will result in all confined tasks that have a profile
> + * being removed, inheriting the parent->unconfined profile.
> + */
> + if (ns->parent)
> + ns->unconfined = aa_get_profile(ns->parent->unconfined);
> +
> + /* release original ns->unconfined ref */
> + aa_put_profile(unconfined);
> +
> + mutex_unlock(&ns->lock);
> +}
> +
> +static void aa_put_ns_rcu(struct rcu_head *head)
> +{
> + struct aa_namespace *ns = container_of(head, struct aa_namespace,
> + base.rcu);
> + /* release ns->base.list ref */
> + aa_put_namespace(ns);
> }
>
> /**
> @@ -531,26 +538,12 @@ static void destroy_namespace(struct aa_namespace *ns)
> */
> static void __remove_namespace(struct aa_namespace *ns)
> {
> - struct aa_profile *unconfined = ns->unconfined;
> -
> /* remove ns from namespace list */
> - list_del_init(&ns->base.list);
> -
> - /*
> - * break the ns, unconfined profile cyclic reference and forward
> - * all new unconfined profiles requests to the parent namespace
> - * This will result in all confined tasks that have a profile
> - * being removed, inheriting the parent->unconfined profile.
> - */
> - if (ns->parent)
> - ns->unconfined = aa_get_profile(ns->parent->unconfined);
> + list_del_rcu(&ns->base.list);
>
> destroy_namespace(ns);
>
> - /* release original ns->unconfined ref */
> - aa_put_profile(unconfined);
> - /* release ns->base.list ref, from removal above */
> - aa_put_namespace(ns);
> + call_rcu(&ns->base.rcu, aa_put_ns_rcu);
> }
>
> /**
Thanks
signature.asc
Description: Digital signature
-- AppArmor mailing list [email protected] Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
