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

[John Johansen]

On 01/26/2013 12:20 AM, Seth Arnold wrote:
> 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"
> 
thanks

>> +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.
> 
heh, the unlikely penalty is minor compared to all work being done to
free the mem. However the
  if (r)
is needed to protect against the r->profile dereference, at which point
we might as well wrap the whole thing

>> +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?
> 
With some compiler flags yes, but not with the kernel's standard set
and this is the common pattern of how kref_put is used in the kernel

>> @@ -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...
> 
hehe alright,

first when we allocate new, a new empty replacedby struct is also created. This
could have been delayed to be done here, and I messed with that but in the end
I decided it was cleaner to allocate and discard if not needed, instead of
wait and determine if it is needed.

so
>> +		if (old) {
>>  			__replace_profile(old, new);
>> +			aa_put_replacedby(new->replacedby);
>> +			new->replacedby = aa_get_replacedby(old->replacedby);

is replacing the preallocated replacedby struct with the one that is now shared
with old.  Basically all replacements of profile X will share a replacedby struct
so they can all forward to the newest version.

We do this because different kernel object may be holding references to different
versions. We could use the replacedby struct as a mux and just reference to it
off of kernel objects but that has its own tradeoffs. We actually do do this
in the fs patch later in the sequence so that fs objects can always magically
reference the new version.

We don't do that with profiles because of how permission caching on objects
will work, Basically in the labeling patches we will need access to the
version of the profile granted it.

Note: old->replacedby has the profile it points to updated in
>>  			__replace_profile(old, new);


The new profile case

>> +		} else
>>  			__list_add_profile(&policy->profiles, new);

just uses the replacedby struct that was originally allocated with the profile,
so it doesn't need to do anything special


The rename case is ugly (the actual ugliness will show up later in the fs
and labeling patches), and is something that will likely be broken for the
3.0 release, as nothing is actually using it yet.

Rename is replacing a profile of name X with one of name Y. This could
be used by the tools to replace null-XXX to the actual profile name.

It breaks down into two cases
  rename X (rename) to Y (new) where no Y already exists

>> +		} else if (rename) {
>> +			__replace_profile(rename, new);
>> +			aa_put_replacedby(new->replacedby);
>> +			new->replacedby = aa_get_replacedby(rename->replacedby);

we just reuse the existing (rename) profile replacedby struct in this case
and treat it like a regular replacement

It was originally split like this because of the fs patch. The new profile gets
a different fs entry than rename so it can not be treated exactly the same
as replace later on.  The split is just showing up here


the other case is
  rename X (rename) to Y (new) when Y (old) already exists

>> +			if (rename)
>> +				__replace_profile(rename, new);

This is effectively merging 2 profiles into 1 new one. In this case we have
2 existing replacedby structs. We keep both of them
  new shares old's replacedby struct (and fs entry in the future)
  rename has its replacedby struct updated to point to new, its the last
  in the profile chain and will result in it disappearing when the last
  reference to rename is dropped.

The
>   foo
> 
>   if old
>       foo
>   if rename
>       foo
>   
>   foo

pattern would work here but doesn't once the fs patches are encountered. So
its just a matter of where the logic gets introduced. I tend to work patches
going up and down the stack pushing and popping as needed, so often you will
hit things like this that hint at things to come