| Message ID | 20200825152045.1719298-3-omosnace@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | selinux: RCU conversion follow-ups | expand |
On Tue, Aug 25, 2020 at 11:20 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> > --- > diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c > index ec4570d6c38f7..d863b23f2a670 100644 > --- a/security/selinux/ss/services.c > +++ b/security/selinux/ss/services.c > @@ -2221,20 +2218,19 @@ void selinux_policy_commit(struct selinux_state *state, > /* Load the policycaps from the new policy */ > security_load_policycaps(state, 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(); This isn't quite equivalent. The difference is whether security_load_policycaps() has completed. Not sure of the implications but could yield different behavior.
On Tue, Aug 25, 2020 at 6:07 PM Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > On Tue, Aug 25, 2020 at 11:20 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> > > --- > > > diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c > > index ec4570d6c38f7..d863b23f2a670 100644 > > --- a/security/selinux/ss/services.c > > +++ b/security/selinux/ss/services.c > > @@ -2221,20 +2218,19 @@ void selinux_policy_commit(struct selinux_state *state, > > /* Load the policycaps from the new policy */ > > security_load_policycaps(state, 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(); > > This isn't quite equivalent. The difference is whether > security_load_policycaps() has completed. Not sure of the > implications but could yield different behavior. Good point... And you just reminded me of my plan to eliminate the selinux_state::policycap[] array and replace it with calls to security_policycap_supported(). That should have no more race conditions than the current code at least... I'll try to splice such a patch below this one for the next revision. Or is there some compelling reason to keep the array?
On Tue, Aug 25, 2020 at 1:21 PM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > On Tue, Aug 25, 2020 at 6:07 PM Stephen Smalley > <stephen.smalley.work@gmail.com> wrote: > > On Tue, Aug 25, 2020 at 11:20 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> > > > --- > > > > > diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c > > > index ec4570d6c38f7..d863b23f2a670 100644 > > > --- a/security/selinux/ss/services.c > > > +++ b/security/selinux/ss/services.c > > > @@ -2221,20 +2218,19 @@ void selinux_policy_commit(struct selinux_state *state, > > > /* Load the policycaps from the new policy */ > > > security_load_policycaps(state, 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(); > > > > This isn't quite equivalent. The difference is whether > > security_load_policycaps() has completed. Not sure of the > > implications but could yield different behavior. > > Good point... And you just reminded me of my plan to eliminate the > selinux_state::policycap[] array and replace it with calls to > security_policycap_supported(). That should have no more race > conditions than the current code at least... I'll try to splice such a > patch below this one for the next revision. Or is there some > compelling reason to keep the array? Only reason I can see would potentially be performance overhead of ebitmap_get_bit(). Probably not significant.
diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h index 839774929a10d..64aafda76f9ae 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; bool policycap[__POLICYDB_CAPABILITY_MAX]; struct page *status_page; @@ -111,14 +110,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 ec4570d6c38f7..d863b23f2a670 100644 --- a/security/selinux/ss/services.c +++ b/security/selinux/ss/services.c @@ -2142,9 +2142,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); @@ -2221,20 +2218,19 @@ void selinux_policy_commit(struct selinux_state *state, /* Load the policycaps from the new policy */ security_load_policycaps(state, 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); } @@ -2282,13 +2278,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 @@ -2296,6 +2285,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(-)