[apparmor] [PATCH 19/43] apparmor: convert profile lists to RCU based locking

[John Johansen]

On 02/20/2013 08:01 PM, Seth Arnold wrote:
> On Fri, Feb 08, 2013 at 01:00:55PM -0800, John Johansen wrote:
>> +/**
>> + * aa_get_profile_rcu - increment a refcount profile that can be replaced
>> + * @p: pointer to profile that can be replaced (NOT NULL)
>> + *
>> + * Returns: pointer to a refcounted profile.
>> + *     else NULL if no profile
>> + */
>> +static inline struct aa_profile *aa_get_profile_rcu(struct aa_profile **p)
>> +{
>> +	struct aa_profile *c;
>> +
>> +	rcu_read_lock();
>> +	do {
>> +		c = rcu_dereference(*p);
>> +	} while (c && !kref_get_not0(&c->base.count));
>> +	rcu_read_unlock();
>> +
>> +	return c;
>> +}
> 
> The only use of this function is to get profile->parent in change_hat.
> Does profile->parent need this extra protection in the other places it
> is used? Or is it sufficient to just deref parent without getting its
> reference count in the other cases? (filesystem, freeing, new null
> profile).
> 

once a profile is live, parent profile dereferences must be within
an rcu_read_lock critical section, or protected by a write lock.
Once a valid refcount to the parent is obtained the reference can
be dereferenced as normal.  Also note this only affect profile->parent
not ns->parent, because only profile parent can be live updated.

We coult simplify this by taking the write lock in change_hat but
this fn will be used later for replacedby accesses.


so lets walk through the uses
filesystem doesn't use profile->parent

the use in __replace_profile is protected by locking so it isn't needed

free_profile only after their are no more refcounts, and a quiescent
period.  So it should not be possible to reference it

in the new null, etc cases the profile is not yet live, so there is
no chance racing with other tasks/processors


> 
>> @@ -447,7 +433,7 @@ out:
>>  static void __list_add_profile(struct list_head *list,
>>  			       struct aa_profile *profile)
>>  {
>> -	list_add(&profile->base.list, list);
>> +	list_add_rcu(&profile->base.list, list);
>>  	/* get list reference */
>>  	aa_get_profile(profile);
>>  }
> 
> Two of the three callers of this function have an aa_get_profile(profile);
err, I only count 2 uses of __list_add_profile, can you clarify

> equivalent line very close by. Are the callers mistaken? Is this routine
> mistaken? Or are they weird because the _third_ case, replacement, is
> very weird? :)
>