[PATCH 0/5] Clean up SELinux global variables

[PATCH 0/5] Clean up SELinux global variables

[Ondrej Mosnacek]

Minor cleanups related to SELinux global variables, plus one fix for a
potential TCP SYN flood performance regression on SMP systems.

Ondrej Mosnacek (5):
  selinux: remove unused global variables
  selinux: drop the unnecessary aurule_callback variable
  selinux: make selinuxfs_mount static
  selinux: mark some global variables __ro_after_init
  selinux: mark selinux_xfrm_refcount as __read_mostly

 security/selinux/avc.c              | 10 +++++-----
 security/selinux/ibpkey.c           |  1 -
 security/selinux/include/security.h |  1 -
 security/selinux/netif.c            |  1 -
 security/selinux/netlink.c          |  2 +-
 security/selinux/netnode.c          |  1 -
 security/selinux/netport.c          |  1 -
 security/selinux/selinuxfs.c        |  4 ++--
 security/selinux/ss/avtab.c         |  4 ++--
 security/selinux/ss/ebitmap.c       |  2 +-
 security/selinux/ss/hashtab.c       |  2 +-
 security/selinux/ss/services.c      | 10 +++-------
 security/selinux/xfrm.c             |  2 +-
 13 files changed, 16 insertions(+), 25 deletions(-)

-- 
2.29.2

[PATCH 1/5] selinux: remove unused global variables

[Ondrej Mosnacek]

All of sel_ib_pkey_list, sel_netif_list, sel_netnode_list, and
sel_netport_list are declared but never used. Remove them.

Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 security/selinux/ibpkey.c  | 1 -
 security/selinux/netif.c   | 1 -
 security/selinux/netnode.c | 1 -
 security/selinux/netport.c | 1 -
 4 files changed, 4 deletions(-)

diff --git a/security/selinux/ibpkey.c b/security/selinux/ibpkey.c
index f68a7617cfb95..dbd8fe028b3f2 100644
--- a/security/selinux/ibpkey.c
+++ b/security/selinux/ibpkey.c
@@ -40,7 +40,6 @@ struct sel_ib_pkey {
 	struct rcu_head rcu;
 };
 
-static LIST_HEAD(sel_ib_pkey_list);
 static DEFINE_SPINLOCK(sel_ib_pkey_lock);
 static struct sel_ib_pkey_bkt sel_ib_pkey_hash[SEL_PKEY_HASH_SIZE];
 
diff --git a/security/selinux/netif.c b/security/selinux/netif.c
index 86813b46fad5f..1ab03efe74947 100644
--- a/security/selinux/netif.c
+++ b/security/selinux/netif.c
@@ -36,7 +36,6 @@ struct sel_netif {
 };
 
 static u32 sel_netif_total;
-static LIST_HEAD(sel_netif_list);
 static DEFINE_SPINLOCK(sel_netif_lock);
 static struct list_head sel_netif_hash[SEL_NETIF_HASH_SIZE];
 
diff --git a/security/selinux/netnode.c b/security/selinux/netnode.c
index 461fb548453ab..4a7d2ab5b9609 100644
--- a/security/selinux/netnode.c
+++ b/security/selinux/netnode.c
@@ -54,7 +54,6 @@ struct sel_netnode {
  * if this becomes a problem we can always add a hash table for each address
  * family later */
 
-static LIST_HEAD(sel_netnode_list);
 static DEFINE_SPINLOCK(sel_netnode_lock);
 static struct sel_netnode_bkt sel_netnode_hash[SEL_NETNODE_HASH_SIZE];
 
diff --git a/security/selinux/netport.c b/security/selinux/netport.c
index d340f4dcdf5f0..b8bc3897891d9 100644
--- a/security/selinux/netport.c
+++ b/security/selinux/netport.c
@@ -53,7 +53,6 @@ struct sel_netport {
  * if this becomes a problem we can always add a hash table for each address
  * family later */
 
-static LIST_HEAD(sel_netport_list);
 static DEFINE_SPINLOCK(sel_netport_lock);
 static struct sel_netport_bkt sel_netport_hash[SEL_NETPORT_HASH_SIZE];
 
-- 
2.29.2

[PATCH 2/5] selinux: drop the unnecessary aurule_callback variable

[Ondrej Mosnacek]

Its value is actually not changed anywhere, so it can be substituted for
a direct call to audit_update_lsm_rules().

Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 security/selinux/ss/services.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index 597b79703584e..5e08ce2c5994a 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -3693,15 +3693,11 @@ out:
 	return match;
 }
 
-static int (*aurule_callback)(void) = audit_update_lsm_rules;
-
 static int aurule_avc_callback(u32 event)
 {
-	int err = 0;
-
-	if (event == AVC_CALLBACK_RESET && aurule_callback)
-		err = aurule_callback();
-	return err;
+	if (event == AVC_CALLBACK_RESET)
+		return audit_update_lsm_rules();
+	return 0;
 }
 
 static int __init aurule_init(void)
-- 
2.29.2

[PATCH 3/5] selinux: make selinuxfs_mount static

[Ondrej Mosnacek]

It is not referenced outside selinuxfs.c, so remove its extern header
declaration and make it static.

Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 security/selinux/include/security.h | 1 -
 security/selinux/selinuxfs.c        | 2 +-
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
index 3cc8bab31ea85..765a258a899ef 100644
--- a/security/selinux/include/security.h
+++ b/security/selinux/include/security.h
@@ -436,7 +436,6 @@ extern void selinux_complete_init(void);
 extern int selinux_disable(struct selinux_state *state);
 extern void exit_sel_fs(void);
 extern struct path selinux_null;
-extern struct vfsmount *selinuxfs_mount;
 extern void selnl_notify_setenforce(int val);
 extern void selnl_notify_policyload(u32 seqno);
 extern int selinux_nlmsg_lookup(u16 sclass, u16 nlmsg_type, u32 *perm);
diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
index 4bde570d56a2c..4fdfe7b67df89 100644
--- a/security/selinux/selinuxfs.c
+++ b/security/selinux/selinuxfs.c
@@ -2204,7 +2204,7 @@ static struct file_system_type sel_fs_type = {
 	.kill_sb	= sel_kill_sb,
 };
 
-struct vfsmount *selinuxfs_mount;
+static struct vfsmount *selinuxfs_mount;
 struct path selinux_null;
 
 static int __init init_sel_fs(void)
-- 
2.29.2

[PATCH 4/5] selinux: mark some global variables __ro_after_init

[Ondrej Mosnacek]

All of these are never modified outside initcalls, so they can be
__ro_after_init.

Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 security/selinux/avc.c        | 10 +++++-----
 security/selinux/netlink.c    |  2 +-
 security/selinux/selinuxfs.c  |  4 ++--
 security/selinux/ss/avtab.c   |  4 ++--
 security/selinux/ss/ebitmap.c |  2 +-
 security/selinux/ss/hashtab.c |  2 +-
 6 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/security/selinux/avc.c b/security/selinux/avc.c
index 3c05827608b6a..ad451cf9375e4 100644
--- a/security/selinux/avc.c
+++ b/security/selinux/avc.c
@@ -118,11 +118,11 @@ void avc_set_cache_threshold(struct selinux_avc *avc,
 	avc->avc_cache_threshold = cache_threshold;
 }
 
-static struct avc_callback_node *avc_callbacks;
-static struct kmem_cache *avc_node_cachep;
-static struct kmem_cache *avc_xperms_data_cachep;
-static struct kmem_cache *avc_xperms_decision_cachep;
-static struct kmem_cache *avc_xperms_cachep;
+static struct avc_callback_node *avc_callbacks __ro_after_init;
+static struct kmem_cache *avc_node_cachep __ro_after_init;
+static struct kmem_cache *avc_xperms_data_cachep __ro_after_init;
+static struct kmem_cache *avc_xperms_decision_cachep __ro_after_init;
+static struct kmem_cache *avc_xperms_cachep __ro_after_init;
 
 static inline int avc_hash(u32 ssid, u32 tsid, u16 tclass)
 {
diff --git a/security/selinux/netlink.c b/security/selinux/netlink.c
index 621e2e9cd6a1d..1760aee712fd2 100644
--- a/security/selinux/netlink.c
+++ b/security/selinux/netlink.c
@@ -19,7 +19,7 @@
 
 #include "security.h"
 
-static struct sock *selnl;
+static struct sock *selnl __ro_after_init;
 
 static int selnl_msglen(int msgtype)
 {
diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
index 4fdfe7b67df89..01a7d50ed39b8 100644
--- a/security/selinux/selinuxfs.c
+++ b/security/selinux/selinuxfs.c
@@ -2204,8 +2204,8 @@ static struct file_system_type sel_fs_type = {
 	.kill_sb	= sel_kill_sb,
 };
 
-static struct vfsmount *selinuxfs_mount;
-struct path selinux_null;
+static struct vfsmount *selinuxfs_mount __ro_after_init;
+struct path selinux_null __ro_after_init;
 
 static int __init init_sel_fs(void)
 {
diff --git a/security/selinux/ss/avtab.c b/security/selinux/ss/avtab.c
index 0172d87e2b9ae..6dcb6aa4db7f0 100644
--- a/security/selinux/ss/avtab.c
+++ b/security/selinux/ss/avtab.c
@@ -23,8 +23,8 @@
 #include "avtab.h"
 #include "policydb.h"
 
-static struct kmem_cache *avtab_node_cachep;
-static struct kmem_cache *avtab_xperms_cachep;
+static struct kmem_cache *avtab_node_cachep __ro_after_init;
+static struct kmem_cache *avtab_xperms_cachep __ro_after_init;
 
 /* Based on MurmurHash3, written by Austin Appleby and placed in the
  * public domain.
diff --git a/security/selinux/ss/ebitmap.c b/security/selinux/ss/ebitmap.c
index 14bedc95c6dcf..61fcbb8d0f880 100644
--- a/security/selinux/ss/ebitmap.c
+++ b/security/selinux/ss/ebitmap.c
@@ -26,7 +26,7 @@
 
 #define BITS_PER_U64	(sizeof(u64) * 8)
 
-static struct kmem_cache *ebitmap_node_cachep;
+static struct kmem_cache *ebitmap_node_cachep __ro_after_init;
 
 int ebitmap_cmp(struct ebitmap *e1, struct ebitmap *e2)
 {
diff --git a/security/selinux/ss/hashtab.c b/security/selinux/ss/hashtab.c
index dab8c25c739b9..3881787ce492c 100644
--- a/security/selinux/ss/hashtab.c
+++ b/security/selinux/ss/hashtab.c
@@ -9,7 +9,7 @@
 #include <linux/errno.h>
 #include "hashtab.h"
 
-static struct kmem_cache *hashtab_node_cachep;
+static struct kmem_cache *hashtab_node_cachep __ro_after_init;
 
 /*
  * Here we simply round the number of elements up to the nearest power of two.
-- 
2.29.2

[PATCH 5/5] selinux: mark selinux_xfrm_refcount as __read_mostly

[Ondrej Mosnacek]

This is motivated by a perfomance regression of selinux_xfrm_enabled()
that happened on a RHEL kernel due to false sharing between
selinux_xfrm_refcount and (the late) selinux_ss.policy_rwlock (i.e. the
.bss section memory layout changed such that they happened to share the
same cacheline). Since the policy rwlock's memory region was modified
upon each read-side critical section, the readers of
selinux_xfrm_refcount had frequent cache misses, eventually leading to a
significant performance degradation under a TCP SYN flood on a system
with many cores (32 in this case, but it's detectable on less cores as
well).

While upstream has since switched to RCU locking, so the same can no
longer happen here, selinux_xfrm_refcount could still share a cacheline
with another frequently written region, thus marking it __read_mostly
still makes sense. __read_mostly helps, because it will put the symbol
in a separate section along with other read-mostly variables, so there
should never be a clash with frequently written data.

Since selinux_xfrm_refcount is modified only in case of an explicit
action, it should be safe to do this (i.e. it shouldn't disrupt other
read-mostly variables too much).

Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 security/selinux/xfrm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/selinux/xfrm.c b/security/selinux/xfrm.c
index c367d36965d4f..634f3db24da67 100644
--- a/security/selinux/xfrm.c
+++ b/security/selinux/xfrm.c
@@ -47,7 +47,7 @@
 #include "xfrm.h"
 
 /* Labeled XFRM instance counter */
-atomic_t selinux_xfrm_refcount = ATOMIC_INIT(0);
+atomic_t selinux_xfrm_refcount __read_mostly = ATOMIC_INIT(0);
 
 /*
  * Returns true if the context is an LSM/SELinux context.
-- 
2.29.2

Re: [PATCH 1/5] selinux: remove unused global variables

[Paul Moore]

On Wed, Jan 6, 2021 at 8:26 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> All of sel_ib_pkey_list, sel_netif_list, sel_netnode_list, and
> sel_netport_list are declared but never used. Remove them.
>
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
>  security/selinux/ibpkey.c  | 1 -
>  security/selinux/netif.c   | 1 -
>  security/selinux/netnode.c | 1 -
>  security/selinux/netport.c | 1 -
>  4 files changed, 4 deletions(-)

Merged into selinux/next, thanks.

-- 
paul moore
www.paul-moore.com

Re: [PATCH 2/5] selinux: drop the unnecessary aurule_callback variable

[Paul Moore]

On Wed, Jan 6, 2021 at 8:26 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> Its value is actually not changed anywhere, so it can be substituted for
> a direct call to audit_update_lsm_rules().
>
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
>  security/selinux/ss/services.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)

Merged into selinux/next, thanks.

-- 
paul moore
www.paul-moore.com

Re: [PATCH 3/5] selinux: make selinuxfs_mount static

[Paul Moore]

On Wed, Jan 6, 2021 at 8:26 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> It is not referenced outside selinuxfs.c, so remove its extern header
> declaration and make it static.
>
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
>  security/selinux/include/security.h | 1 -
>  security/selinux/selinuxfs.c        | 2 +-
>  2 files changed, 1 insertion(+), 2 deletions(-)

Merged into selinux/next, thanks.

-- 
paul moore
www.paul-moore.com

Re: [PATCH 4/5] selinux: mark some global variables __ro_after_init

[Paul Moore]

On Wed, Jan 6, 2021 at 8:26 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> All of these are never modified outside initcalls, so they can be
> __ro_after_init.
>
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
>  security/selinux/avc.c        | 10 +++++-----
>  security/selinux/netlink.c    |  2 +-
>  security/selinux/selinuxfs.c  |  4 ++--
>  security/selinux/ss/avtab.c   |  4 ++--
>  security/selinux/ss/ebitmap.c |  2 +-
>  security/selinux/ss/hashtab.c |  2 +-
>  6 files changed, 12 insertions(+), 12 deletions(-)

Merged into selinux/next, thanks.

-- 
paul moore
www.paul-moore.com

Re: [PATCH 5/5] selinux: mark selinux_xfrm_refcount as __read_mostly

[Paul Moore]

On Wed, Jan 6, 2021 at 8:26 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> This is motivated by a perfomance regression of selinux_xfrm_enabled()
> that happened on a RHEL kernel due to false sharing between
> selinux_xfrm_refcount and (the late) selinux_ss.policy_rwlock (i.e. the
> .bss section memory layout changed such that they happened to share the
> same cacheline). Since the policy rwlock's memory region was modified
> upon each read-side critical section, the readers of
> selinux_xfrm_refcount had frequent cache misses, eventually leading to a
> significant performance degradation under a TCP SYN flood on a system
> with many cores (32 in this case, but it's detectable on less cores as
> well).
>
> While upstream has since switched to RCU locking, so the same can no
> longer happen here, selinux_xfrm_refcount could still share a cacheline
> with another frequently written region, thus marking it __read_mostly
> still makes sense. __read_mostly helps, because it will put the symbol
> in a separate section along with other read-mostly variables, so there
> should never be a clash with frequently written data.
>
> Since selinux_xfrm_refcount is modified only in case of an explicit
> action, it should be safe to do this (i.e. it shouldn't disrupt other
> read-mostly variables too much).
>
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
>  security/selinux/xfrm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Even on a system that is using labeled IPsec, which is likely very
rare, the refcount should really only be changing when the SPD or SAD
changes which should be measured in minutes or seconds on a loaded
system.  If the __read_mostly tag proves to be a problem we can always
look into other solutions.

Merged into selinux/next, thanks.

-- 
paul moore
www.paul-moore.com