[apparmor] [PATCH 21/32] apparmor: change how profile replacement update is done

[Seth Arnold]

On Wed, Jan 16, 2013 at 01:28:50PM -0800, 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.

A few questions further on..

>  /**
> + * aa_get_newest_profile - find the newest version of @profile
> + * @profile: the profile to check for newer versions of
> + *
> + * Returns: refcounted newest version of @profile taking into account
> + *          replacement, renamces and removals

"renamces"

> +static void free_replacedby(struct aa_replacedby *r)
> +{
> +	if (r) {
> +		aa_put_profile(r->profile);
> +		kzfree(r);
> +	}
> +}

Both aa_put_profile() and kzfree() are prepared to handle NULL pointers;
you could remove the if (r) check if you don't think that will trip the
unlikely() penalty in kzfree() too often.

> +static inline void aa_put_replacedby(struct aa_replacedby *p)
> +{
> +       if (p)
> +               kref_put(&p->count, aa_free_replacedby_kref);
> +}
> +

Do you need a (void) here to silence a warning?

> @@ -1128,11 +1129,18 @@ ssize_t aa_replace_profiles(void *udata, size_t size, bool noreplace)
>  
>  		audit_policy(op, GFP_ATOMIC, new->base.name, NULL, error);
>  
> +		if (old) {
>  			__replace_profile(old, new);
> +			aa_put_replacedby(new->replacedby);
> +			new->replacedby = aa_get_replacedby(old->replacedby);
> +
> +			if (rename)
> +				__replace_profile(rename, new);
> +		} else if (rename) {
> +			__replace_profile(rename, new);
> +			aa_put_replacedby(new->replacedby);
> +			new->replacedby = aa_get_replacedby(rename->replacedby);
> +		} else
>  			__list_add_profile(&policy->profiles, new);
>  
>  		aa_put_profile(rename);

Could you explain this a bit? I'm seriously confused about the
aa_put_replacedby(new->replacedby) -- after all, this is replacing an
old profile with a newer profile. How is there a 'replacedby' already
on new? Why 'put' the new one and 'get' the old one?

In most other places in the code, old and rename are .. not entirely
independant .. but also not intertwingled quite like this, either. I
have to wonder if there's a more linear re-writing of this block that
wouldn't run like this:

  if old
      foo
      if rename
          foo
  elseif rename
      foo
  else
      foo

but more like the pattern seen elsewhere:

  foo

  if old
      foo
  if rename
      foo
  
  foo

But of course, I haven't got a clue why this block works the way it does :)
so I can't really offer a re-work...

Thanks
-------------- 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/20130126/4f2980ed/attachment.pgp>