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

[Seth Arnold]

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.

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;
        }
	/* ... */


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

-------------- 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/20130221/5d442ff6/attachment.pgp>