[apparmor] [PATCH 02/36] apparmor: convert profile lists to RCU based locking

[Seth Arnold]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 490 bytes
Desc: Digital signature
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20130508/256f0311/attachment.pgp>