On Sat, Sep 5, 2020 at 11:33 PM Lakshmi Ramasubramanian
<nramas@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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 :)
[1] https://patchwork.kernel.org/patch/11741025/
[2] https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git/commit/?h=next&id=9ff9abc4c6be27ff27b6df625501a46711730520
[3] https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git/commit/?h=next&id=66ccd2560affc6e653ef7372ea36fb825743d186
> but policy is non-NULL and I see the following messages in "dmesg" log.
>
> Is this expected?
Yes, that's expected. The caller of security_read_policy() in this
case needs to ensure that state->policy_mutex is being held.
--
Ondrej Mosnacek
Software Engineer, Platform Security - SELinux kernel
Red Hat, Inc.
