selinux: uninline unlikely parts of avc_has_perm_noaudit()

This is based on earlier patch posted to the list by Linus, his
commit description read:

 "avc_has_perm_noaudit()is one of those hot functions that end up
  being used by almost all filesystem operations (through
  "avc_has_perm()") and it's intended to be cheap enough to inline.

  However, it turns out that the unlikely parts of it (where it
  doesn't find an existing avc node) need a fair amount of stack
  space for the automatic replacement node, so if it were to be
  inlined (at least clang does not) it would just use stack space
  unnecessarily.

  So split the unlikely part out of it, and mark that part noinline.
  That improves the actual likely part."

The basic idea behind the patch was reasonable, but there were minor
nits (double indenting, etc.) and the RCU read lock unlock/re-lock in
avc_compute_av() began to look even more ugly.  This patch builds on
Linus' first effort by cleaning things up a bit and removing the RCU
unlock/lock dance in avc_compute_av().

Removing the RCU lock dance in avc_compute_av() is safe as there are
currently two callers of avc_compute_av(): avc_has_perm_noaudit() and
avc_has_extended_perms().  The first caller in avc_has_perm_noaudit()
does not require a RCU lock as there is no avc_node to protect so the
RCU lock can be dropped before calling avc_compute_av().  The second
caller, avc_has_extended_perms(), is similar in that there is no
avc_node that requires RCU protection, but the code is simplified by
holding the RCU look around the avc_compute_av() call, and given that
we enter a RCU critical section in security_compute_av() (called from
av_compute_av()) the impact will likely be unnoticeable.  It is also
worth noting that avc_has_extended_perms() is only called from the
SELinux ioctl() access control hook at the moment.

Acked-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Paul Moore <paul@paul-moore.com>

1 files changed, 57 insertions, 28 deletions

diff --git a/security/selinux/avc.c b/security/selinux/avc.c
index 9a43af0ebd7d..5ce3ad451665 100644
--- a/security/selinux/avc.c
+++ b/security/selinux/avc.c
@@ -988,25 +988,26 @@ int avc_ss_reset(struct selinux_avc *avc, u32 seqno)
 	return rc;
 }
 
-/*
- * Slow-path helper function for avc_has_perm_noaudit,
- * when the avc_node lookup fails. We get called with
- * the RCU read lock held, and need to return with it
- * still held, but drop if for the security compute.
+/**
+ * avc_compute_av - Add an entry to the AVC based on the security policy
+ * @state: SELinux state pointer
+ * @ssid: subject
+ * @tsid: object/target
+ * @tclass: object class
+ * @avd: access vector decision
+ * @xp_node: AVC extended permissions node
  *
- * Don't inline this, since it's the slow-path and just
- * results in a bigger stack frame.
+ * Slow-path helper function for avc_has_perm_noaudit, when the avc_node lookup
+ * fails.  Don't inline this, since it's the slow-path and just results in a
+ * bigger stack frame.
  */
-static noinline
-struct avc_node *avc_compute_av(struct selinux_state *state,
-				u32 ssid, u32 tsid,
-				u16 tclass, struct av_decision *avd,
-				struct avc_xperms_node *xp_node)
+static noinline struct avc_node *avc_compute_av(struct selinux_state *state,
+						u32 ssid, u32 tsid, u16 tclass,
+						struct av_decision *avd,
+						struct avc_xperms_node *xp_node)
 {
-	rcu_read_unlock();
 	INIT_LIST_HEAD(&xp_node->xpd_head);
 	security_compute_av(state, ssid, tsid, tclass, avd, &xp_node->xp);
-	rcu_read_lock();
 	return avc_insert(state->avc, ssid, tsid, tclass, avd, xp_node);
 }
 
@@ -1113,6 +1114,36 @@ decision:
 }
 
 /**
+ * avc_perm_nonode - Add an entry to the AVC
+ * @state: SELinux state pointer
+ * @ssid: subject
+ * @tsid: object/target
+ * @tclass: object class
+ * @requested: requested permissions
+ * @flags: AVC flags
+ * @avd: access vector decision
+ *
+ * This is the "we have no node" part of avc_has_perm_noaudit(), which is
+ * unlikely and needs extra stack space for the new node that we generate, so
+ * don't inline it.
+ */
+static noinline int avc_perm_nonode(struct selinux_state *state,
+				    u32 ssid, u32 tsid, u16 tclass,
+				    u32 requested, unsigned int flags,
+				    struct av_decision *avd)
+{
+	u32 denied;
+	struct avc_xperms_node xp_node;
+
+	avc_compute_av(state, ssid, tsid, tclass, avd, &xp_node);
+	denied = requested & ~(avd->allowed);
+	if (unlikely(denied))
+		return avc_denied(state, ssid, tsid, tclass, requested, 0, 0,
+				  flags, avd);
+	return 0;
+}
+
+/**
  * avc_has_perm_noaudit - Check permissions but perform no auditing.
  * @state: SELinux state
  * @ssid: source security identifier
@@ -1139,29 +1170,27 @@ inline int avc_has_perm_noaudit(struct selinux_state *state,
 				unsigned int flags,
 				struct av_decision *avd)
 {
-	struct avc_node *node;
-	struct avc_xperms_node xp_node;
-	int rc = 0;
 	u32 denied;
+	struct avc_node *node;
 
 	if (WARN_ON(!requested))
 		return -EACCES;
 
 	rcu_read_lock();
-
 	node = avc_lookup(state->avc, ssid, tsid, tclass);
-	if (unlikely(!node))
-		avc_compute_av(state, ssid, tsid, tclass, avd, &xp_node);
-	else
-		memcpy(avd, &node->ae.avd, sizeof(*avd));
+	if (unlikely(!node)) {
+		rcu_read_unlock();
+		return avc_perm_nonode(state, ssid, tsid, tclass, requested,
+				       flags, avd);
+	}
+	denied = requested & ~node->ae.avd.allowed;
+	memcpy(avd, &node->ae.avd, sizeof(*avd));
+	rcu_read_unlock();
 
-	denied = requested & ~(avd->allowed);
 	if (unlikely(denied))
-		rc = avc_denied(state, ssid, tsid, tclass, requested, 0, 0,
-				flags, avd);
-
-	rcu_read_unlock();
-	return rc;
+		return avc_denied(state, ssid, tsid, tclass, requested, 0, 0,
+				  flags, avd);
+	return 0;
 }
 
 /**