[apparmor] [PATCH 03/36] apparmor: change how profile replacement update is done

[John Johansen]

On 05/07/2013 06:09 PM, Seth Arnold wrote:
> On Wed, May 01, 2013 at 02:30:48PM -0700, John Johansen wrote:
>> remove the use of replaced by chaining and move to profile invalidation
>> and lookup to handle task replacement.
>>
>> Replacement chaining can result in large chains of profiles being pinned
>> in memory when one profile in the chain is use. With implicit labeling
>> this will be even more of a problem, so move to a direct lookup method.
> 
> Could you explain the old_replacedby in the following few chunks? I just
> can't make heads or tails of why it works the way it does. Thanks.

Sure its a boolean to say use the old_replacedby and not the new one. In the
case of a straight replacement the old replacedby struct is shared between
the old profile and the new profile.

This prevents building up long chains of replacedby's to follow/free when
lots of replacements happen.

What we used to do was embed a direct pointer to profile that replaced you

  A1 -> A2 -> A3 -> A4

the chain would then be walked to find the most recent replacement. So say
you have a process or file that is long lived and hasn't been accessed for
a while and is labeled with A1.

now when it gets used it has to walk all the way to A4 to get updated.
Even worse as long as a reference to A1 exists A2, and A3 are pinned by
ref counting and can not be freed (this actually happens a lot because
we can't update creds).

The 3rd problem is when A1 finally does free it causes A2, and A3 to drop
and be freed as well. If the chain is long this can actually blow the
stack with how we used to do it.

Now we have a separate replacedby struct that is shared and protected by
rcu and refcounting (its a bit weird)

Now
  A1 -> R1 -> A4
        ^
        A2

A2 and A3 are no longer pinned and can be freed when their reference count
drops. Also A1 can update it self with a single hope instead of following
a long chain.

I suppose I should modify the code to use a bool instead of int and come
up with a better name for old_replacedby, perhaps use_old_replacedby

> 
>> @@ -992,6 +995,7 @@ static struct aa_profile *__list_lookup_parent(struct list_head *lh,
>>   * __replace_profile - replace @old with @new on a list
>>   * @old: profile to be replaced  (NOT NULL)
>>   * @new: profile to replace @old with  (NOT NULL)
>> + * @old_replacedby: transfer @old->replacedby to @new
>>   *
>>   * Will duplicate and refcount elements that @new inherits from @old
>>   * and will inherit @old children.
>> @@ -1000,7 +1004,8 @@ static struct aa_profile *__list_lookup_parent(struct list_head *lh,
>>   *
>>   * Requires: namespace list lock be held, or list not be shared
>>   */
>> -static void __replace_profile(struct aa_profile *old, struct aa_profile *new)
>> +static void __replace_profile(struct aa_profile *old, struct aa_profile *new,
>> +			      int old_replacedby)
>>  {
>>  	struct aa_profile *child, *tmp;
>>  
>> @@ -1015,7 +1020,7 @@ static void __replace_profile(struct aa_profile *old, struct aa_profile *new)
>>  			p = __find_child(&new->base.profiles, child->base.name);
>>  			if (p) {
>>  				/* @p replaces @child  */
>> -				__replace_profile(child, p);
>> +				__replace_profile(child, p, old_replacedby);
>>  				continue;
>>  			}
>>  
>> @@ -1034,8 +1039,11 @@ static void __replace_profile(struct aa_profile *old, struct aa_profile *new)
>>  		struct aa_profile *parent = rcu_dereference(old->parent);
>>  		rcu_assign_pointer(new->parent, aa_get_profile(parent));
>>  	}
>> -	/* released by free_profile */
>> -	old->replacedby = aa_get_profile(new);
>> +	__aa_update_replacedby(old, new);
>> +	if (old_replacedby) {
>> +		aa_put_replacedby(new->replacedby);
>> +		new->replacedby = aa_get_replacedby(old->replacedby);
>> +	}
>>  
>>  	if (list_empty_rcu(&new->base.list)) {
>>  		/* new is not on a list already */
>> @@ -1159,23 +1167,24 @@ ssize_t aa_replace_profiles(void *udata, size_t size, bool noreplace)
>>  		audit_policy(op, GFP_ATOMIC, ent->new->base.name, NULL, error);
>>  
>>  		if (ent->old) {
>> -			__replace_profile(ent->old, ent->new);
>> +			__replace_profile(ent->old, ent->new, 1);
>>  			if (ent->rename)
>> -				__replace_profile(ent->rename, ent->new);
>> +				__replace_profile(ent->rename, ent->new, 0);
>>  		} else if (ent->rename) {
>> -			__replace_profile(ent->rename, ent->new);
>> +			__replace_profile(ent->rename, ent->new, 0);
>>  		} else if (ent->new->parent) {
>>  			struct aa_profile *parent, *newest;
>>  			parent = rcu_dereference_protected(ent->new->parent,
>>  						     mutex_is_locked(&ns->lock));
>> -			newest = aa_newest_version(parent);
>> +			newest = aa_get_newest_profile(parent);
>>  
>>  			/* parent replaced in this atomic set? */
>>  			if (newest != parent) {
>>  				aa_get_profile(newest);
>>  				aa_put_profile(parent);
>>  				rcu_assign_pointer(ent->new->parent, newest);
>> -			}
>> +			} else
>> +				aa_put_profile(newest);
>>  			__list_add_profile(&parent->base.profiles, ent->new);
>>  		} else
>>  			__list_add_profile(&ns->base.profiles, ent->new);
>> -- 
>> 1.8.1.2
>>
>>
>> -- 
>> AppArmor mailing list
>> AppArmor at lists.ubuntu.com
>> Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
>>
>>
>>