[PATCH 3/3] Use conditional

[PATCH 3/3] Use conditional

[Kurt Garloff]

[-- Attachment #1.1: Type: text/plain, Size: 364 bytes --]

Hi,

this optimizes the case where no LSM is loaded and the (new) default 
capablities is used. These are not called via indirect calls but 
called as hardcoded calls and might thus be inlined; the price for
this is a conditional -- benchmarks done by hp showed this to be
beneficial (on ia64).

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

[-- Attachment #1.2: security-avoid-indir-call --]
[-- Type: text/plain, Size: 2522 bytes --]

From: Kurt Garloff <garloff@suse.de>
Subject: Replace indirect calls by a branch
References: SUSE40217, SUSE39439

In the LSM stub collection, rather do a branch than an indirect
call. Many of the functions called do only return 0 or do nothing
for the default (capability) case.
This is a fast-path optimization; a branch is faster than an
indirect call, even more so if correctly predicted.
This shows a >3% perf. increase in netperf -t TCP_RR benchmark on IA64.
(More exactly: The benchmark was taken with the next two patches
 applied as well, but I attribute the main effect to this patch.)

This is patch 3/5 of the LSM overhaul.

 include/linux/security.h |    6 +++++-
 security/security.c      |    2 --
 2 files changed, 5 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,17 +1246,21 @@ struct security_operations {
 };
 
 /* global variables */
 extern struct security_operations *security_ops;
+/* default security ops */
+extern struct security_operations capability_security_ops;
 
 /* prototypes */
 extern int security_init	(void);
 extern int register_security	(struct security_operations *ops);
 extern int unregister_security	(struct security_operations *ops);
 extern int mod_reg_security	(const char *name, struct security_operations *ops);
 extern int mod_unreg_security	(const char *name, struct security_operations *ops);
 
-#  define COND_SECURITY(seop, def) security_ops->seop
+/* Condition for invocation of non-default security_op */
+#  define COND_SECURITY(seop, def) 	\
+	(security_ops == &capability_security_ops)? def: security_ops->seop
 
 # else /* CONFIG_SECURITY */
 static inline int security_init(void)
 {
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,8 @@
 #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 */
 
 static inline int verify(struct security_operations *ops)

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 3/3] Use conditional

[James Morris]

On Sun, 3 Jul 2005, Kurt Garloff wrote:

> capablities is used. These are not called via indirect calls but 
> called as hardcoded calls and might thus be inlined; the price for
> this is a conditional -- benchmarks done by hp showed this to be
> beneficial (on ia64).

What about on i386, x86_64 or ppc64?


- James
-- 
James Morris
<jmorris@redhat.com>

Re: [PATCH 3/3] Use conditional

[Kurt Garloff]

[-- Attachment #1: Type: text/plain, Size: 988 bytes --]

Hi James,

[added linux-security-module@wirex.com to Cc]

On Sun, Jul 03, 2005 at 12:51:20PM -0400, James Morris wrote:
> On Sun, 3 Jul 2005, Kurt Garloff wrote:
> 
> > capablities is used. These are not called via indirect calls but 
> > called as hardcoded calls and might thus be inlined; the price for
> > this is a conditional -- benchmarks done by hp showed this to be
> > beneficial (on ia64).
> 
> What about on i386, x86_64 or ppc64?

We tested on i386 as well at the time, and it looked like a tiny
improvement. But doing the statistics, it was in the noise. 
I have no numbers for x86_64 or ppc64.

If you have reason to believe that there could be regressions, we 
should indeed do the benchmarks.

Sidenote: The patches 1 -- 2b alone still make sense, so I would
vote not for delaying their inclusion until we can collect numbers
for all arches we care about to take a decision on patch 3.

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

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 3/3] Use conditional

[Tony Jones]

On Sun, Jul 03, 2005 at 05:44:05PM +0200, Kurt Garloff wrote:

Agree with James, pls resend to linux-security-module@wirex.com.

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.

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

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.

Thanks

> Hi,
> 
> this optimizes the case where no LSM is loaded and the (new) default 
> capablities is used. These are not called via indirect calls but 
> called as hardcoded calls and might thus be inlined; the price for
> this is a conditional -- benchmarks done by hp showed this to be
> beneficial (on ia64).
> 
> Enjoy,
> -- 
> Kurt Garloff, Director SUSE Labs, Novell Inc.

> From: Kurt Garloff <garloff@suse.de>
> Subject: Replace indirect calls by a branch
> References: SUSE40217, SUSE39439
> 
> In the LSM stub collection, rather do a branch than an indirect
> call. Many of the functions called do only return 0 or do nothing
> for the default (capability) case.
> This is a fast-path optimization; a branch is faster than an
> indirect call, even more so if correctly predicted.
> This shows a >3% perf. increase in netperf -t TCP_RR benchmark on IA64.
> (More exactly: The benchmark was taken with the next two patches
>  applied as well, but I attribute the main effect to this patch.)
> 
> This is patch 3/5 of the LSM overhaul.

Re: [PATCH 3/3] Use conditional

[Kurt Garloff]

[-- Attachment #1.1: Type: text/plain, Size: 1068 bytes --]

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.

[-- Attachment #1.2: security-likely-cap --]
[-- Type: text/plain, Size: 1262 bytes --]

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

[-- Attachment #1.3: security-se-enabled --]
[-- Type: text/plain, Size: 3669 bytes --]

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 #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 3/3] Use conditional

[Tony Jones]

On Mon, Jul 04, 2005 at 08:59:02AM +0200, Kurt Garloff wrote:

> > 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?

CONFIG_SECURITY_STACKER and !CONFIG_SECURITY_STACKER ;-)

http://mail.wirex.com/pipermail/linux-security-module/2005-June/6200.html

I was assuming (bad of me I know) that Serge's patch would nail both cases
with one stone.

Tony

Re: [PATCH 3/3] Use conditional

[serge]

Quoting Tony Jones (tonyj@suse.de):
> On Mon, Jul 04, 2005 at 08:59:02AM +0200, Kurt Garloff wrote:
> 
> > > 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?
> 
> CONFIG_SECURITY_STACKER and !CONFIG_SECURITY_STACKER ;-)
> 
> http://mail.wirex.com/pipermail/linux-security-module/2005-June/6200.html
> 
> I was assuming (bad of me I know) that Serge's patch would nail both cases
> with one stone.

Yes, sorry, I never got around to the replace-dummy-with-capability
patch.  There wasn't a single cry when Chris asked for anyone who'd
care about dummy being removed, so I do plan on switching that.

thanks,
-serge

Re: [PATCH 3/3] Use conditional

[Kurt Garloff]

[-- Attachment #1: Type: text/plain, Size: 1426 bytes --]

Hi Serge,

On Mon, Jul 04, 2005 at 07:01:05AM -0500, serge@hallyn.com wrote:
> Quoting Tony Jones (tonyj@suse.de):
> > On Mon, Jul 04, 2005 at 08:59:02AM +0200, Kurt Garloff wrote:
> > 
> > > > 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?
> > 
> > CONFIG_SECURITY_STACKER and !CONFIG_SECURITY_STACKER ;-)
> > 
> > http://mail.wirex.com/pipermail/linux-security-module/2005-June/6200.html
> > 
> > I was assuming (bad of me I know) that Serge's patch would nail both cases
> > with one stone.
> 
> Yes, sorry, I never got around to the replace-dummy-with-capability
> patch.  There wasn't a single cry when Chris asked for anyone who'd
> care about dummy being removed, so I do plan on switching that.

I was a bit careful: My patch did switch the default, but LSMs that
don't fill in all security_ops would still fall back to dummy, just
to make sure there is no possibly unintended change in behaviour.

Getting rid of dummy entirely would be better, I agree, but someone
needs to review that this won't break anything.

So how should we proceed?
You want to do the dummy removal first, then have stacker merged
and then what remains of my patches? Or should I start ... ?

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

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 3/3] Use conditional

[serge]

Hey,

Quoting Kurt Garloff (garloff@suse.de):
> Getting rid of dummy entirely would be better, I agree, but someone
> needs to review that this won't break anything.

Unfortunately I think it's way too soon for that.  Even if stacker is
accepted, it is still a module (for now at least) which can be compiled
out.  So we'll need dummy hooks for modules (like seclvl) to use.  I
just don't think it's possible to get rid of that yet.

> So how should we proceed?
> You want to do the dummy removal first, then have stacker merged
> and then what remains of my patches? Or should I start ... ?

I think your patches to make capability the default are the best
place to start.  Doing the same under stacker will be trivial, and
I'll do that in the next set I send out.

thanks,
-serge

Re: [PATCH 3/3] Use conditional

[Kurt Garloff]

[-- Attachment #1: Type: text/plain, Size: 1503 bytes --]

Hi Serge,

On Mon, Jul 04, 2005 at 07:37:21AM -0500, serge@hallyn.com wrote:
> Quoting Kurt Garloff (garloff@suse.de):
> > Getting rid of dummy entirely would be better, I agree, but someone
> > needs to review that this won't break anything.
> 
> Unfortunately I think it's way too soon for that.  Even if stacker is
> accepted, it is still a module (for now at least) which can be compiled
> out.  So we'll need dummy hooks for modules (like seclvl) to use.  I
> just don't think it's possible to get rid of that yet.

Hmmmm, getting rid of dummy would mean replacing it with capability.
- The differences between cap and dummy affect a relatively small
  subset of hooks
- If all of these hooks are implemented by all LSMs, we're done and
  can just remove dummy and replace it by capability.
- If not, we'd need to review for all of these LSMs, whether defaulting
  to capability rather than dummy could create a problem and whether 
  that can be addressed easily.

seclvl would probably need some changes, indeed.

root_plug could become shorter :-)

> > So how should we proceed?
> > You want to do the dummy removal first, then have stacker merged
> > and then what remains of my patches? Or should I start ... ?
> 
> I think your patches to make capability the default are the best
> place to start.  Doing the same under stacker will be trivial, and
> I'll do that in the next set I send out.

Sounds good!
-- 
Kurt Garloff, Director SUSE Labs, Novell Inc.

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]