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

[John Johansen]

On 02/14/2013 12:31 AM, Seth Arnold wrote:
> On Fri, Feb 08, 2013 at 01:00:55PM -0800, John Johansen wrote:
>> signed-offby: John Johansen <john.johansen at canonical.com>
>> ---
>>  security/apparmor/domain.c           |   15 ++-
>>  security/apparmor/include/apparmor.h |    6 ++
>>  security/apparmor/include/policy.h   |   44 +++++++-
>>  security/apparmor/policy.c           |  195 ++++++++++++++++++----------------
>>  security/apparmor/policy_unpack.c    |    5 +
>>  5 files changed, 164 insertions(+), 101 deletions(-)
> 
> I'm sorry that I haven't reviewed this entire patch yet -- it's heavier
> lifting than I expected. But I'll give you what I've got now, with the
> hopes that it may unblock you sooner.
> 
>> @@ -641,7 +641,10 @@ int aa_change_hat(const char *hats[], int count, u64 token, bool permtest)
>>  	if (count) {
>>  		/* attempting to change into a new hat or switch to a sibling */
>>  		struct aa_profile *root;
>> -		root = PROFILE_IS_HAT(profile) ? profile->parent : profile;
>> +		if (PROFILE_IS_HAT(profile))
>> +			root = aa_get_profile_rcu(&profile->parent);
>> +		else
>> +			root = aa_get_profile(profile);
>>  
>>  		/* find first matching hat */
>>  		for (i = 0; i < count && !hat; i++)
> 
> 'root' is get here but there's a branch where it isn't put:
> 
> 		} else {
> 			target = hat->base.hname;
> 			if (!PROFILE_IS_HAT(hat)) {
> 				info = "target not hat";
> 				error = -EPERM;
> 				goto audit;
> 			}
> 		}
> 
yep thanks

>> @@ -42,6 +42,7 @@ extern const char *const profile_mode_names[];
>>  
>>  #define PROFILE_IS_HAT(_profile) ((_profile)->flags & PFLAG_HAT)
>>  
>> +#define list_empty_rcu(X) (list_empty(X) || (X)->prev == LIST_POISON2)
> 
> This feels brittle. I haven't put the time into fully understanding the
> uses of it yet, but one at least feels like an opportunity to just return
> worse information to userspace (whether there are zero hats or just an
> invalid hat was selected). Besides, in that case, it is racing against
> replacement, perhaps just using list_empty() alone would be fine --
> it might not be true by the next sync(), but it _might have been true_
> at some point in the past. Learning mode is never going to be perfect.
> 
> But perhaps the other two uses are more compelling...
> 
I will see what I can do to rework, but rcu heads once in use get poisoned
and list_empty() does not detect that the "list" is empty.

At this point its a matter of whether the head has been poisoned for through
the quiescent period or not

>> +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;
>> +}
> 
> How many iterations of this are possible, if two or more cores each
> trying to aa_get_profile_rcu() the same profile... Would they step on
> each other's toes in the kref_get_not0() loop, bouncing among them and
> never one 'owning' it?
> 
> How long could that go on?
> 
it could theoretically go on for ever, but the writer is slow, has to
unpack profiles, do memory allocations, may sleep, etc.
While the reader is fairly fast.

So with our reference counting once a count goes to 0 it stays there, it
is now into the free cycle. Competing readers won't cause a problem,
it racing the last put or a writer.

Currently IIRC this is just being used to guard list accesses, because its
possible to find the profile as the writer replaces/removes it, and the
writers put beats the readers get.

Basically our profile access is refcount protected, its only the list
access that is actually rcu

> 
> Wow, it seems so little after so much hair-pulling..
> 
sorry about that