Re: [PATCH 3/3] Use conditional

[Kurt Garloff <garloff@suse.de>]

Hi Tony,

On Sun, Jul 03, 2005 at 12:00:07PM -0700, Tony Jones wrote:
> Agree with James, pls resend to linux-security-module@wirex.com.

Done.

> The topic of replacing dummy (with capability) was discussed there
> last week, in the context of stacker, but a common solution for both
> cases would be needed.

Both cases?

> Also, I was going to ask where 4/5 and 5/5 were :-)

They were part of my previous submission attempts as indicated in the
first message [Patch 0/3]. Last time I sent them people were discussing
whether or not we should have this likely/unlikely, so I left this out
this time, as I'd rather prefer discussing the other patches.

Still, the 3% perf. increase measurement that has been done was with
patches 4 and 5, so claiming otherwise would not be right.

> If you are claiming a perf increase it would be nice to get an idea
> what these patches were even though you believe most of the gain was
> in patch #3.

For completeness, find both attached.

Best,
-- 
Kurt Garloff, Director SUSE Labs, Novell Inc.

From: Kurt Garloff <garloff@suse.de>
Subject: Consider the capability case the likely one
References: SUSE40217, SUSE39439

The case that security_ops points to the default capability_
security_ops is the fast path and arguably the more likely one
on most systems. So mark it likely to tell the compiler to
optimize accordingly and increase the chances of having this
predicted correctly by the CPU.

This is patch 4/5 of the LSM overhaul.

 security.h |    2 +-
 1 files changed, 1 insertion(+), 1 deletion(-)

Signed-off-by: Kurt Garloff <garloff@suse.de>

Index: linux-2.6.12/include/linux/security.h
===================================================================
--- linux-2.6.12.orig/include/linux/security.h
+++ linux-2.6.12/include/linux/security.h
@@ -1258,9 +1258,9 @@ extern int mod_reg_security	(const char 
 extern int mod_unreg_security	(const char *name, struct security_operations *ops);
 
 /* Condition for invocation of non-default security_op */
 #  define COND_SECURITY(seop, def) 	\
-	(security_ops == &capability_security_ops)? def: security_ops->seop
+	(likely(security_ops == &capability_security_ops))? def: security_ops->seop
 
 # else /* CONFIG_SECURITY */
 static inline int security_init(void)
 {

From: Kurt Garloff <garloff@suse.de>
Subject: Test security_enabled var rather than security_ops pointer
References: SUSE40217, SUSE39439

Rather than doing a pointer comparison, test an integer var
for being null. Should be slightly faster.
I consider this patch as optional.

Note that it does not introduce a (new) race, as we still set
the security_ops pointer to the capability_security_ops. So no
wmb() is needed for the module unload case.

Sidenote: A wmb() in the module load and unload cases might
actually be useful to ensure that the other CPUs start using the
new LSM pointer ASAP. It's still racy, but that's by design of
LSM. Introducing locks would hurt performance tremendously.
One could do RCU ... but that's not for this patchset.

This is patch 5/5 of the LSM overhaul.

 include/linux/security.h |    7 ++++---
 security/security.c      |    7 +++++++
 2 files changed, 11 insertions(+), 3 deletions(-)

Signed-off-by: Kurt Garloff <garloff@suse.de>

Index: linux-2.6.12/include/linux/security.h
===================================================================
--- linux-2.6.12.orig/include/linux/security.h
+++ linux-2.6.12/include/linux/security.h
@@ -1246,10 +1246,10 @@ struct security_operations {
 };
 
 /* global variables */
 extern struct security_operations *security_ops;
-/* default security ops */
-extern struct security_operations capability_security_ops;
+/* Security enabled? */
+extern int security_enabled;
 
 /* prototypes */
 extern int security_init	(void);
 extern int register_security	(struct security_operations *ops);
@@ -1258,16 +1258,17 @@ extern int mod_reg_security	(const char 
 extern int mod_unreg_security	(const char *name, struct security_operations *ops);
 
 /* Condition for invocation of non-default security_op */
 #  define COND_SECURITY(seop, def) 	\
-	(likely(security_ops == &capability_security_ops))? def: security_ops->seop
+	(unlikely(security_enabled))? security_ops->seop: def
 
 # else /* CONFIG_SECURITY */
 static inline int security_init(void)
 {
 	return 0;
 }
 
+#  define security_enabled 0
 #  define COND_SECURITY(seop, def) def
 # endif
 
 # ifdef CONFIG_SECURITY_NETWORK
Index: linux-2.6.12/security/security.c
===================================================================
--- linux-2.6.12.orig/security/security.c
+++ linux-2.6.12/security/security.c
@@ -21,10 +21,14 @@
 #define SECURITY_FRAMEWORK_VERSION	"1.0.0"
 
 /* things that live in dummy.c */
 extern void security_fixup_ops (struct security_operations *ops);
+/* default security ops */
+extern struct security_operations capability_security_ops;
 
 struct security_operations *security_ops;	/* Initialized to NULL */
+int security_enabled;				/* ditto */
+EXPORT_SYMBOL(security_enabled);
 
 static inline int verify(struct security_operations *ops)
 {
 	/* verify the security_operations structure exists */
@@ -59,8 +63,9 @@ int __init security_init(void)
 		       "capability_security_ops structure.\n", __FUNCTION__);
 		return -EIO;
 	}
 
+	security_enabled = 0;
 	security_ops = &capability_security_ops;
 
 	/* Initialize compiled-in security modules */
 	do_security_initcalls();
@@ -91,8 +96,9 @@ int register_security(struct security_op
 	if (security_ops != &capability_security_ops)
 		return -EAGAIN;
 
 	security_ops = ops;
+	security_enabled = 1;
 
 	return 0;
 }
 
@@ -115,8 +121,9 @@ int unregister_security(struct security_
 		       "registered, failing.\n", __FUNCTION__);
 		return -EINVAL;
 	}
 
+	security_enabled = 0;
 	security_ops = &capability_security_ops;
 
 	return 0;
 }

Attachment: pgp0cFWqtoUR9.pgp
Description: PGP signature