[PATCH v2 0/3] selinux: RCU conversion follow-ups

[PATCH v2 0/3] selinux: RCU conversion follow-ups

[Ondrej Mosnacek]

This series contains some follow-up patches for the policy rwlock to RCU
conversion that has been merged recently.

Passed selinux-testsuite under both release and debug Fedora kernel
configs.

v2:
- just dereference the policy pointer directly in security_read_policy()
- add a patch to remove the redundant policycap array from selinux_state
- drop the refcount patch for now and remove the RFC keyword
  - I'd like to check the performance impact before going further with it
  - also, it might be better to continue holding read_lock() in existing
    functions and only use the refcount approach in the IMA policy export
    function (the standard access pattern will work the same also after
    adding the refcount support)

Ondrej Mosnacek (3):
  selinux: simplify away security_policydb_len()
  selinux: eliminate the redundant policycap array
  selinux: remove the 'initialized' flag from selinux_state

 security/selinux/include/security.h | 53 ++++++------------
 security/selinux/selinuxfs.c        | 12 ++--
 security/selinux/ss/services.c      | 85 +++++++----------------------
 3 files changed, 43 insertions(+), 107 deletions(-)

-- 
2.26.2

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

[Ondrej Mosnacek]

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>
---
 security/selinux/include/security.h |  1 -
 security/selinux/selinuxfs.c        | 12 +++++------
 security/selinux/ss/services.c      | 32 ++++++++---------------------
 3 files changed, 15 insertions(+), 30 deletions(-)

diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
index 505e51264d51c..839774929a10d 100644
--- a/security/selinux/include/security.h
+++ b/security/selinux/include/security.h
@@ -218,7 +218,6 @@ void selinux_policy_cancel(struct selinux_state *state,
 			struct selinux_policy *policy);
 int security_read_policy(struct selinux_state *state,
 			 void **data, size_t *len);
-size_t security_policydb_len(struct selinux_state *state);
 
 int security_policycap_supported(struct selinux_state *state,
 				 unsigned int req_cap);
diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
index d1872adf0c474..fc44c4b8c0692 100644
--- a/security/selinux/selinuxfs.c
+++ b/security/selinux/selinuxfs.c
@@ -417,16 +417,16 @@ static int sel_open_policy(struct inode *inode, struct file *filp)
 	if (!plm)
 		goto err;
 
-	if (i_size_read(inode) != security_policydb_len(state)) {
-		inode_lock(inode);
-		i_size_write(inode, security_policydb_len(state));
-		inode_unlock(inode);
-	}
-
 	rc = security_read_policy(state, &plm->data, &plm->len);
 	if (rc)
 		goto err;
 
+	if ((size_t)i_size_read(inode) != plm->len) {
+		inode_lock(inode);
+		i_size_write(inode, plm->len);
+		inode_unlock(inode);
+	}
+
 	fsi->policy_opened = 1;
 
 	filp->private_data = plm;
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
@@ -2331,22 +2331,6 @@ err:
 	return rc;
 }
 
-size_t security_policydb_len(struct selinux_state *state)
-{
-	struct selinux_policy *policy;
-	size_t len;
-
-	if (!selinux_initialized(state))
-		return 0;
-
-	rcu_read_lock();
-	policy = rcu_dereference(state->policy);
-	len = policy->policydb.len;
-	rcu_read_unlock();
-
-	return len;
-}
-
 /**
  * security_port_sid - Obtain the SID for a port.
  * @protocol: protocol number
@@ -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;
 
-	*len = security_policydb_len(state);
-
+	*len = policy->policydb.len;
 	*data = vmalloc_user(*len);
 	if (!*data)
 		return -ENOMEM;
@@ -3924,11 +3914,7 @@ int security_read_policy(struct selinux_state *state,
 	fp.data = *data;
 	fp.len = *len;
 
-	rcu_read_lock();
-	policy = rcu_dereference(state->policy);
 	rc = policydb_write(&policy->policydb, &fp);
-	rcu_read_unlock();
-
 	if (rc)
 		return rc;
 
-- 
2.26.2

[PATCH v2 2/3] selinux: eliminate the redundant policycap array

[Ondrej Mosnacek]

The policycap array in struct selinux_state is redundant and can be
substituted by calling security_policycap_supported().

Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 security/selinux/include/security.h | 42 ++++++++++++-----------------
 security/selinux/ss/services.c      | 27 -------------------
 2 files changed, 17 insertions(+), 52 deletions(-)

diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
index 839774929a10d..9ab8f8da47812 100644
--- a/security/selinux/include/security.h
+++ b/security/selinux/include/security.h
@@ -96,7 +96,6 @@ struct selinux_state {
 #endif
 	bool checkreqprot;
 	bool initialized;
-	bool policycap[__POLICYDB_CAPABILITY_MAX];
 
 	struct page *status_page;
 	struct mutex status_lock;
@@ -159,53 +158,49 @@ static inline bool selinux_disabled(struct selinux_state *state)
 }
 #endif
 
+int security_policycap_supported(struct selinux_state *state,
+				 unsigned int req_cap);
+
 static inline bool selinux_policycap_netpeer(void)
 {
-	struct selinux_state *state = &selinux_state;
-
-	return state->policycap[POLICYDB_CAPABILITY_NETPEER];
+	return security_policycap_supported(&selinux_state,
+					    POLICYDB_CAPABILITY_NETPEER);
 }
 
 static inline bool selinux_policycap_openperm(void)
 {
-	struct selinux_state *state = &selinux_state;
-
-	return state->policycap[POLICYDB_CAPABILITY_OPENPERM];
+	return security_policycap_supported(&selinux_state,
+					    POLICYDB_CAPABILITY_OPENPERM);
 }
 
 static inline bool selinux_policycap_extsockclass(void)
 {
-	struct selinux_state *state = &selinux_state;
-
-	return state->policycap[POLICYDB_CAPABILITY_EXTSOCKCLASS];
+	return security_policycap_supported(&selinux_state,
+					    POLICYDB_CAPABILITY_EXTSOCKCLASS);
 }
 
 static inline bool selinux_policycap_alwaysnetwork(void)
 {
-	struct selinux_state *state = &selinux_state;
-
-	return state->policycap[POLICYDB_CAPABILITY_ALWAYSNETWORK];
+	return security_policycap_supported(&selinux_state,
+					    POLICYDB_CAPABILITY_ALWAYSNETWORK);
 }
 
 static inline bool selinux_policycap_cgroupseclabel(void)
 {
-	struct selinux_state *state = &selinux_state;
-
-	return state->policycap[POLICYDB_CAPABILITY_CGROUPSECLABEL];
+	return security_policycap_supported(&selinux_state,
+					    POLICYDB_CAPABILITY_CGROUPSECLABEL);
 }
 
 static inline bool selinux_policycap_nnp_nosuid_transition(void)
 {
-	struct selinux_state *state = &selinux_state;
-
-	return state->policycap[POLICYDB_CAPABILITY_NNP_NOSUID_TRANSITION];
+	return security_policycap_supported(&selinux_state,
+					    POLICYDB_CAPABILITY_NNP_NOSUID_TRANSITION);
 }
 
 static inline bool selinux_policycap_genfs_seclabel_symlinks(void)
 {
-	struct selinux_state *state = &selinux_state;
-
-	return state->policycap[POLICYDB_CAPABILITY_GENFS_SECLABEL_SYMLINKS];
+	return security_policycap_supported(&selinux_state,
+					    POLICYDB_CAPABILITY_GENFS_SECLABEL_SYMLINKS);
 }
 
 int security_mls_enabled(struct selinux_state *state);
@@ -219,9 +214,6 @@ void selinux_policy_cancel(struct selinux_state *state,
 int security_read_policy(struct selinux_state *state,
 			 void **data, size_t *len);
 
-int security_policycap_supported(struct selinux_state *state,
-				 unsigned int req_cap);
-
 #define SEL_VEC_MAX 32
 struct av_decision {
 	u32 allowed;
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index 7cc2f7486c18f..e82a2cfe171f3 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -2113,30 +2113,6 @@ bad:
 	return 0;
 }
 
-static void security_load_policycaps(struct selinux_state *state,
-				struct selinux_policy *policy)
-{
-	struct policydb *p;
-	unsigned int i;
-	struct ebitmap_node *node;
-
-	p = &policy->policydb;
-
-	for (i = 0; i < ARRAY_SIZE(state->policycap); i++)
-		state->policycap[i] = ebitmap_get_bit(&p->policycaps, i);
-
-	for (i = 0; i < ARRAY_SIZE(selinux_policycap_names); i++)
-		pr_info("SELinux:  policy capability %s=%d\n",
-			selinux_policycap_names[i],
-			ebitmap_get_bit(&p->policycaps, i));
-
-	ebitmap_for_each_positive_bit(&p->policycaps, node, i) {
-		if (i >= ARRAY_SIZE(selinux_policycap_names))
-			pr_info("SELinux:  unknown policy capability %u\n",
-				i);
-	}
-}
-
 static int security_preserve_bools(struct selinux_policy *oldpolicy,
 				struct selinux_policy *newpolicy);
 
@@ -2218,9 +2194,6 @@ void selinux_policy_commit(struct selinux_state *state,
 	/* Install the new policy. */
 	rcu_assign_pointer(state->policy, newpolicy);
 
-	/* Load the policycaps from the new policy */
-	security_load_policycaps(state, newpolicy);
-
 	if (!selinux_initialized(state)) {
 		/*
 		 * After first policy load, the security server is
-- 
2.26.2

[PATCH v2 3/3] selinux: remove the 'initialized' flag from selinux_state

[Ondrej Mosnacek]

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(-)

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);
-- 
2.26.2

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

[Stephen Smalley]

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.

Re: [PATCH v2 2/3] selinux: eliminate the redundant policycap array

[Stephen Smalley]

On Wed, Aug 26, 2020 at 9:59 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> The policycap array in struct selinux_state is redundant and can be
> substituted by calling security_policycap_supported().
>
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---

> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index 7cc2f7486c18f..e82a2cfe171f3 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -2113,30 +2113,6 @@ bad:
>         return 0;
>  }
>
> -static void security_load_policycaps(struct selinux_state *state,
> -                               struct selinux_policy *policy)
> -{
> -       struct policydb *p;
> -       unsigned int i;
> -       struct ebitmap_node *node;
> -
> -       p = &policy->policydb;
> -
> -       for (i = 0; i < ARRAY_SIZE(state->policycap); i++)
> -               state->policycap[i] = ebitmap_get_bit(&p->policycaps, i);
> -
> -       for (i = 0; i < ARRAY_SIZE(selinux_policycap_names); i++)
> -               pr_info("SELinux:  policy capability %s=%d\n",
> -                       selinux_policycap_names[i],
> -                       ebitmap_get_bit(&p->policycaps, i));
> -
> -       ebitmap_for_each_positive_bit(&p->policycaps, node, i) {
> -               if (i >= ARRAY_SIZE(selinux_policycap_names))
> -                       pr_info("SELinux:  unknown policy capability %u\n",
> -                               i);
> -       }
> -}
> -

Two requests:
1. Can you do a little benchmarking to confirm that calling
security_policycap_supported() each time doesn't cause any significant
overheads?  Networking benchmark might be of interest.

2. Can you retain the logging of the policy capability values?  Just
drop the first part of the function and rename it e.g.
security_log_policycaps().

Re: [PATCH v2 3/3] selinux: remove the 'initialized' flag from selinux_state

[Stephen Smalley]

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>

Re: [PATCH v2 2/3] selinux: eliminate the redundant policycap array

[Ondrej Mosnacek]

On Wed, Aug 26, 2020 at 4:12 PM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
> On Wed, Aug 26, 2020 at 9:59 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> >
> > The policycap array in struct selinux_state is redundant and can be
> > substituted by calling security_policycap_supported().
> >
> > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > ---
>
> > diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> > index 7cc2f7486c18f..e82a2cfe171f3 100644
> > --- a/security/selinux/ss/services.c
> > +++ b/security/selinux/ss/services.c
> > @@ -2113,30 +2113,6 @@ bad:
> >         return 0;
> >  }
> >
> > -static void security_load_policycaps(struct selinux_state *state,
> > -                               struct selinux_policy *policy)
> > -{
> > -       struct policydb *p;
> > -       unsigned int i;
> > -       struct ebitmap_node *node;
> > -
> > -       p = &policy->policydb;
> > -
> > -       for (i = 0; i < ARRAY_SIZE(state->policycap); i++)
> > -               state->policycap[i] = ebitmap_get_bit(&p->policycaps, i);
> > -
> > -       for (i = 0; i < ARRAY_SIZE(selinux_policycap_names); i++)
> > -               pr_info("SELinux:  policy capability %s=%d\n",
> > -                       selinux_policycap_names[i],
> > -                       ebitmap_get_bit(&p->policycaps, i));
> > -
> > -       ebitmap_for_each_positive_bit(&p->policycaps, node, i) {
> > -               if (i >= ARRAY_SIZE(selinux_policycap_names))
> > -                       pr_info("SELinux:  unknown policy capability %u\n",
> > -                               i);
> > -       }
> > -}
> > -
>
> Two requests:
> 1. Can you do a little benchmarking to confirm that calling
> security_policycap_supported() each time doesn't cause any significant
> overheads?  Networking benchmark might be of interest.

I tried to sample a simple `ping -f -i 0 -c 5000000 127.0.0.1` with
perf and indeed security_policycap_supported() now makes up about half
the time spent in some hooks (selinux_socket_sock_rcv_skb(),
selinux_ip_postroute()), mainly because of ebitmap_get_bit() it seems.
I'll try moving the array to policydb and using it in
security_policycap_supported() instead of the bitmap.

>
> 2. Can you retain the logging of the policy capability values?  Just
> drop the first part of the function and rename it e.g.
> security_log_policycaps().

Sure, somehow I failed to notice those prints...

-- 
Ondrej Mosnacek
Software Engineer, Platform Security - SELinux kernel
Red Hat, Inc.

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

[Paul Moore]

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

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

[Paul Moore]

On Thu, Aug 27, 2020 at 9:56 AM Paul Moore <paul@paul-moore.com> wrote:
>
> 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?

Oh, just in case it wasn't clear from my comments above, I think this
patch is fine :)

-- 
paul moore
www.paul-moore.com

Re: [PATCH v2 3/3] selinux: remove the 'initialized' flag from selinux_state

[Paul Moore]

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.

-- 
paul moore
www.paul-moore.com

Re: [PATCH v2 3/3] selinux: remove the 'initialized' flag from selinux_state

[Stephen Smalley]

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).

Re: [PATCH v2 3/3] selinux: remove the 'initialized' flag from selinux_state

[Paul Moore]

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.

-- 
paul moore
www.paul-moore.com