Re: [PATCH v2 1/3] selinux: simplify away security_policydb_len()

From: Paul Moore <paul@paul-moore.com>
To: Stephen Smalley <stephen.smalley.work@gmail.com>
Cc: Ondrej Mosnacek <omosnace@redhat.com>,
	SElinux list <selinux@vger.kernel.org>
Subject: Re: [PATCH v2 1/3] selinux: simplify away security_policydb_len()
Date: Thu, 27 Aug 2020 09:56:38 -0400	[thread overview]
Message-ID: <CAHC9VhTOTuscseVrLEj17y2ULG9rWjLrxT+PorakT8EGk4dhTA@mail.gmail.com> (raw)
In-Reply-To: <CAEjxPJ4LL022dtKyqs+YB-gKVuJqRNQu0dafjgzZLqStqn-zSA@mail.gmail.com>

On Wed, Aug 26, 2020 at 10:05 AM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Wed, Aug 26, 2020 at 9:59 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> >
> > Remove the security_policydb_len() calls from sel_open_policy() and
> > instead update the inode size from the size returned from
> > security_read_policy().
> >
> > Since after this change security_policydb_len() is only called from
> > security_load_policy(), remove it entirely and just open-code it there.
> >
> > Also, since security_load_policy() is always called with fsi->mutex
> > held, make it dereference the policy pointer directly and drop the
> > unnecessary RCU locking.
> >
> > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
>
> One comment below but nonetheless:
> Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com>
>
> > diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> > index 8381614627569..7cc2f7486c18f 100644
> > --- a/security/selinux/ss/services.c
> > +++ b/security/selinux/ss/services.c
> > @@ -3912,11 +3896,17 @@ int security_read_policy(struct selinux_state *state,
> >         int rc;
> >         struct policy_file fp;
> >
> > -       if (!selinux_initialized(state))
> > +       /*
> > +        * NOTE: We do not need to take the rcu read lock
> > +        * around the code below because other policy-modifying
> > +        * operations are already excluded by selinuxfs via
> > +        * fsi->mutex.
> > +        */
> > +       policy = rcu_dereference_check(state->policy, 1);
> > +       if (!policy)
> >                 return -EINVAL;
>
> If/when my patch to move the mutex to selinux_state and use it in
> rcu_dereference_protected() lands, we'll want to change this one over
> too.

FWIW, I felt the mutex move was more significant than this patchset so
I merged it first.  Ondrej, would you mind rebasing this patch and
making the changes above?

Thanks.

-- 
paul moore
www.paul-moore.com