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

[John Johansen]

On 05/08/2013 02:37 PM, Seth Arnold wrote:
> 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
yes it is an interesting change, and I am having problems remembering
why I did it.

I think it comes down to you are moving processes that where confined
in that namespace into the parent namespace, and this could affect
the behavior of the children/execs.

Should those children continue to be confined in the namespace they
where in or should they pickup the confinement of the parent namespace.
Think of a newly unconfined app, now execing and picking up the
confinement specified in the parent namespace.

Note that this is in the case where the process was not stacked with
the parent, in that case the task remains being correctly confined.
This case is where the child was not confined by the parent, and now
is.

Arguments can be made in favor for either behavior. Currently the
code takes the approach that a removed namespace won't merge down.
Notice how the replacedby field for the unconfined profile is not
updated.

However I am still not sure this is the correct behavior


> 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.
> 
the profile references around your concern look good (that is refcounts are
taken on ns->unconfined where needed) however there is an issue that needs
to be fixed.
- __profile_list_release is setting replacedby without dealing with potential
  replacedby references that already exist

> 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
> 
> 
>