| Message ID | 20200826135906.1912186-4-omosnace@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | selinux: RCU conversion follow-ups | expand |
On Wed, Aug 26, 2020 at 9:59 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > After the RCU conversion, it is possible to simply check the policy > pointer against NULL instead. > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com>
On Wed, Aug 26, 2020 at 10:15 AM Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > > On Wed, Aug 26, 2020 at 9:59 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > > > After the RCU conversion, it is possible to simply check the policy > > pointer against NULL instead. > > > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> > > Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com> Looks fine to me as well, but it doesn't merge cleanly and since you are already respinning patch 1/3 I figured I would just bail on this patch and let you take care ot it. I would suggest dropping patch 2/3 from the patchset, respinning patches 1/3 and 3/3, and reposting them for inclusion into selinux/next. That's likely the quickest way forward on these.
On Thu, Aug 27, 2020 at 10:05 AM Paul Moore <paul@paul-moore.com> wrote: > > On Wed, Aug 26, 2020 at 10:15 AM Stephen Smalley > <stephen.smalley.work@gmail.com> wrote: > > > > On Wed, Aug 26, 2020 at 9:59 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > > > > > After the RCU conversion, it is possible to simply check the policy > > > pointer against NULL instead. > > > > > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> > > > > Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com> > > Looks fine to me as well, but it doesn't merge cleanly and since you > are already respinning patch 1/3 I figured I would just bail on this > patch and let you take care ot it. > > I would suggest dropping patch 2/3 from the patchset, respinning > patches 1/3 and 3/3, and reposting them for inclusion into > selinux/next. That's likely the quickest way forward on these. Technically 3/3 isn't safe without 2/3 (or some replacement for it, e.g. moving the policycaps array into struct selinux_policy as suggested by Ondrej).
On Thu, Aug 27, 2020 at 10:17 AM Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > > On Thu, Aug 27, 2020 at 10:05 AM Paul Moore <paul@paul-moore.com> wrote: > > > > On Wed, Aug 26, 2020 at 10:15 AM Stephen Smalley > > <stephen.smalley.work@gmail.com> wrote: > > > > > > On Wed, Aug 26, 2020 at 9:59 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > > > > > > > After the RCU conversion, it is possible to simply check the policy > > > > pointer against NULL instead. > > > > > > > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> > > > > > > Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com> > > > > Looks fine to me as well, but it doesn't merge cleanly and since you > > are already respinning patch 1/3 I figured I would just bail on this > > patch and let you take care ot it. > > > > I would suggest dropping patch 2/3 from the patchset, respinning > > patches 1/3 and 3/3, and reposting them for inclusion into > > selinux/next. That's likely the quickest way forward on these. > > Technically 3/3 isn't safe without 2/3 (or some replacement for it, > e.g. moving the policycaps array into struct selinux_policy as > suggested by Ondrej). Then just respin 1/3 and we'll deal with the other bits later.
diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h index 9ab8f8da47812..714c389cc72a0 100644 --- a/security/selinux/include/security.h +++ b/security/selinux/include/security.h @@ -95,7 +95,6 @@ struct selinux_state { bool enforcing; #endif bool checkreqprot; - bool initialized; struct page *status_page; struct mutex status_lock; @@ -110,14 +109,7 @@ extern struct selinux_state selinux_state; static inline bool selinux_initialized(const struct selinux_state *state) { - /* do a synchronized load to avoid race conditions */ - return smp_load_acquire(&state->initialized); -} - -static inline void selinux_mark_initialized(struct selinux_state *state) -{ - /* do a synchronized write to avoid race conditions */ - smp_store_release(&state->initialized, true); + return rcu_access_pointer(state->policy) != NULL; } #ifdef CONFIG_SECURITY_SELINUX_DEVELOP diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c index e82a2cfe171f3..112ca3d9834d7 100644 --- a/security/selinux/ss/services.c +++ b/security/selinux/ss/services.c @@ -2118,9 +2118,6 @@ static int security_preserve_bools(struct selinux_policy *oldpolicy, static void selinux_policy_free(struct selinux_policy *policy) { - if (!policy) - return; - policydb_destroy(&policy->policydb); sidtab_destroy(policy->sidtab); kfree(policy->sidtab); @@ -2194,20 +2191,19 @@ void selinux_policy_commit(struct selinux_state *state, /* Install the new policy. */ rcu_assign_pointer(state->policy, newpolicy); - if (!selinux_initialized(state)) { + if (!oldpolicy) { /* * After first policy load, the security server is * marked as initialized and ready to handle requests and * any objects created prior to policy load are then labeled. */ - selinux_mark_initialized(state); selinux_complete_init(); + } else { + /* Free the old policy */ + synchronize_rcu(); + selinux_policy_free(oldpolicy); } - /* Free the old policy */ - synchronize_rcu(); - selinux_policy_free(oldpolicy); - /* Notify others of the policy change */ selinux_notify_policy_change(state, seqno); } @@ -2255,13 +2251,6 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len, goto err; } - - if (!selinux_initialized(state)) { - /* First policy load, so no need to preserve state from old policy */ - *newpolicyp = newpolicy; - return 0; - } - /* * NOTE: We do not need to take the rcu read lock * around the code below because other policy-modifying @@ -2269,6 +2258,11 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len, * fsi->mutex. */ oldpolicy = rcu_dereference_check(state->policy, 1); + if (!oldpolicy) { + /* First policy load, so no need to preserve state from old policy */ + *newpolicyp = newpolicy; + return 0; + } /* Preserve active boolean values from the old policy */ rc = security_preserve_bools(oldpolicy, newpolicy);
After the RCU conversion, it is possible to simply check the policy pointer against NULL instead. Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> --- security/selinux/include/security.h | 10 +--------- security/selinux/ss/services.c | 26 ++++++++++---------------- 2 files changed, 11 insertions(+), 25 deletions(-)