Re: [RFC PATCH 3/3] selinux: track policy lifetime with refcount

[Lakshmi Ramasubramanian <nramas@linux.microsoft.com>]

On 9/7/20 12:47 AM, Ondrej Mosnacek wrote:
> On Sat, Sep 5, 2020 at 11:33 PM Lakshmi Ramasubramanian
> <nramas@linux.microsoft.com> wrote:
>> On 8/25/20 8:20 AM, Ondrej Mosnacek wrote:
>>
>> Hi Ondrej,
>>
>> I am just trying understand the expected behavior w.r.t the usage of
>> rcu_dereference_protected() for accessing SELinux policy. Could you
>> please clarify?
>>
>>> Instead of holding the RCU read lock the whole time while accessing the
>>> policy, add a simple refcount mechanism to track its lifetime. After
>>> this, the RCU read lock is held only for a brief time when fetching the
>>> policy pointer and incrementing the refcount. The policy struct is then
>>> guaranteed to stay alive until the refcount is decremented.
>>>
>>> Freeing of the policy remains the responsibility of the task that does
>>> the policy reload. In case the refcount drops to zero in a different
>>> task, the policy load task is notified via a completion.
>>>
>>> The advantage of this change is that the operations that access the
>>> policy can now do sleeping allocations, since they don't need to hold
>>> the RCU read lock anymore. This patch so far only leverages this in
>>> security_read_policy() for the vmalloc_user() allocation (although this
>>> function is always called under fsi->mutex and could just access the
>>> policy pointer directly). The conversion of affected GFP_ATOMIC
>>> allocations to GFP_KERNEL is left for a later patch, since auditing
>>> which code paths may still need GFP_ATOMIC is not very easy.
>>>
>>> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
>>> ---
>>>    security/selinux/ss/services.c | 286 ++++++++++++++++-----------------
>>>    security/selinux/ss/services.h |   6 +
>>>    2 files changed, 147 insertions(+), 145 deletions(-)
>>
>> int security_read_policy(struct selinux_state *state,
>>                           void **data, size_t *len)
>> {
>>          struct selinux_policy *policy;
>>
>>          policy = rcu_dereference_protected(
>>                          state->policy,
>>                           lockdep_is_held(&state->policy_mutex));
>>          if (!policy)
>>                  return -EINVAL;
>> ...
>> }
>>
>> If "policy_mutex" is not held by the caller of security_read_policy() I
>> was expecting the above rcu_dereference_protected() call to return NULL,
> 
> No, that's not how rcu_dereference_protected() works. You should only
> call it when you know for sure that you are in a section that is
> mutually exclusive with anything that might change the pointer. The
> check argument is only used for sanity checking that this is indeed
> true, but other than triggering a warning when RCU debugging is
> enabled the result of the check is ignored.
> 
> If the returned pointer is NULL, it just means that the pointer hasn't
> been assigned yet, i.e. that no policy has been loaded yet (this was
> checked using selinux_initialized() before, but when we're under
> policy_mutex, checking the pointer for NULL is possible and simpler).
> 
> BTW, you should've replied to [1], which is the merged patch that
> introduced the code you are referencing :)

Thanks for clarifying Ondrej.

  -lakshmi