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

[John Johansen]

On 02/21/2013 10:16 AM, Seth Arnold wrote:
> Sorry this might be confusing, I had been looking at the top of your
> tree for my code inspections while reading through the details of the
> patch. So some of the references to profile->parent or __list_add_profile
> might have been added by _future_ patches -- I was afraid I'd forget
> the details if I waited until getting to those future patches.
> 
ah, ouch

> On Wed, Feb 20, 2013 at 11:30:27PM -0800, John Johansen wrote:
>>>> +static inline struct aa_profile *aa_get_profile_rcu(struct aa_profile **p)
>>>
>>> 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
>>
>> so lets walk through the uses
>> filesystem doesn't use profile->parent
> 
> int __aa_fs_profile_mkdir(struct aa_profile *profile, struct dentry *parent)
> {
>         struct aa_profile *child;
>         struct dentry *dent = NULL, *dir;
>         int error;
> 
>         if (!parent) {
>                 dent = prof_dir(profile->parent);
>                 /* adding to parent that previously didn't have children */
>                 dent = securityfs_create_dir("profiles", dent);
>                 if (IS_ERR(dent))
>                         goto fail;
>                 prof_child_dir(profile->parent) = parent = dent;
>         }
> 	/* ... */
> 
this use is fine because the profile isn't live yet. The file is being created
before the profile is added to the list.

> 
>> 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
> 
> Excellent :) Thanks
> 
>>>> @@ -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
> 
> The first two increment the use count:
> 
> struct aa_profile *aa_new_null_profile(struct aa_profile *parent, int hat)
> {
> 	/* ... */
>         mutex_lock(&profile->ns->lock);
>         /* add list ref */
>         aa_get_profile(profile);
>         __list_add_profile(&parent->base.profiles, profile);
>         mutex_unlock(&profile->ns->lock);
> 
> 
> struct aa_label *aa_setup_default_label(void)
> {
> 	/* ... */
>         aa_get_profile(profile);
>         /* replacedby being set needed by fs interface */
>         __list_add_profile(&root_ns->base.profiles, profile);
> 
> ssize_t aa_replace_profiles(void *udata, size_t size, bool noreplace)
> {
> 	/* ... */
>                         rcu_assign_pointer(new->label.replacedby->label,
>                                            aa_get_label(&new->label));
>                         __list_add_profile(&policy->profiles, new);
>                         l = aa_label_insert(&ns->labels, &new->label);
>                         aa_put_label(l);
> 
> 
Indeed this is odd, something isn't right.

thanks
Seth