Re: Audit issue

Re: Audit issue

[Alexander Viro]

On Wed, Oct 31, 2007 at 05:40:19PM -0400, Steve Grubb wrote:
> On Tuesday 30 October 2007 07:15:25 pm Alexander Viro wrote:
> > On Tue, Oct 30, 2007 at 07:07:29PM -0400, Steve Grubb wrote:
> > > On Tuesday 30 October 2007 03:04:54 pm Eric Paris wrote:
> > > > why is it acceptable to mandate audit=1 in gurb but not to mandate
> > > > 'don't use auditctl -e 0' ?? ????
> > >
> > > Its not that audit=1 is mandated. Its recommended. In the other case,
> > > temporarily taking the audit system offline should in no way impair the
> > > ability to start auditing again. It is required that an admin be able to
> > > track any users in the system if they are accessing files or attempting
> > > to make privileged calls.
> >
> > Ahem...  If you have it disabled for a while, what's going to do the
> > tracking until you reenable it?

Have fun...

diff --git a/kernel/audit.c b/kernel/audit.c
index f93c271..83227f8 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -70,6 +70,7 @@ static int	audit_initialized;
  * 1 - auditing enabled
  * 2 - auditing enabled and configuration is locked/unchangeable. */
 int		audit_enabled;
+int		audit_ever_enabled;
 
 /* Default state when kernel boots without any parameters. */
 static int	audit_default;
@@ -340,8 +341,10 @@ static int audit_set_enabled(int state, uid_t loginuid, u32 sid)
 		state, old, loginuid, res);
 
 	/* If we are allowed, make the change */
-	if (res == 1)
+	if (res == 1) {
 		audit_enabled = state;
+		audit_ever_enabled |= !!state;
+	}
 	/* Not allowed, update reason */
 	else if (rc == 0)
 		rc = -EPERM;
@@ -965,6 +968,7 @@ static int __init audit_init(void)
 	skb_queue_head_init(&audit_skb_queue);
 	audit_initialized = 1;
 	audit_enabled = audit_default;
+	audit_ever_enabled |= !!audit_default;
 
 	/* Register the callback with selinux.  This callback will be invoked
 	 * when a new policy is loaded. */
@@ -992,8 +996,10 @@ static int __init audit_enable(char *str)
 	printk(KERN_INFO "audit: %s%s\n",
 	       audit_default ? "enabled" : "disabled",
 	       audit_initialized ? "" : " (after initialization)");
-	if (audit_initialized)
+	if (audit_initialized) {
 		audit_enabled = audit_default;
+		audit_ever_enabled |= !!audit_default;
+	}
 	return 1;
 }
 
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index bce9ecd..250f00f 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -70,6 +70,7 @@
 #include "audit.h"
 
 extern struct list_head audit_filter_list[];
+extern int audit_ever_enabled;
 
 /* AUDIT_NAMES is the number of slots we reserve in the audit_context
  * for saving names from getname(). */
@@ -814,7 +815,7 @@ int audit_alloc(struct task_struct *tsk)
 	struct audit_context *context;
 	enum audit_state     state;
 
-	if (likely(!audit_enabled))
+	if (likely(!audit_ever_enabled))
 		return 0; /* Return if not auditing. */
 
 	state = audit_filter_task(tsk);

Re: Audit issue

[Steve Grubb]

On Thursday 08 November 2007 09:19:26 Alexander Viro wrote:
> Have fun...

Thanks for posting this patch. Is it impossible to "repair " processes by 
simply adding a context if the pointer is NULL?


> diff --git a/kernel/audit.c b/kernel/audit.c
> index f93c271..83227f8 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -70,6 +70,7 @@ static int	audit_initialized;
>   * 1 - auditing enabled
>   * 2 - auditing enabled and configuration is locked/unchangeable. */
>  int		audit_enabled;
> +int		audit_ever_enabled;
>
>  /* Default state when kernel boots without any parameters. */
>  static int	audit_default;
> @@ -965,6 +968,7 @@ static int __init audit_init(void)
>  	skb_queue_head_init(&audit_skb_queue);
>  	audit_initialized = 1;
>  	audit_enabled = audit_default;
> +	audit_ever_enabled |= !!audit_default;

Should the declaration of audit_ever_enabled set a default value like 0 since 
this is being or'ed in? Or should this just be an assignment?

-Steve

Re: Audit issue

[Alexander Viro]

On Thu, Nov 08, 2007 at 09:27:13AM -0500, Steve Grubb wrote:
> On Thursday 08 November 2007 09:19:26 Alexander Viro wrote:
> > Have fun...
> 
> Thanks for posting this patch. Is it impossible to "repair " processes by 
> simply adding a context if the pointer is NULL?

At which point would you do that?  I'd rather not try to play with locking,
etc., when we set audit_enabled to non-zero...  Especially when there's
a trivially non-intrusive patch.

Re: Audit issue

[Steve Grubb]

On Thursday 08 November 2007 09:32:18 Alexander Viro wrote:
> > Thanks for posting this patch. Is it impossible to "repair " processes by
> > simply adding a context if the pointer is NULL?
>
> At which point would you do that?

Possibly on syscall exit? Shouldn't the kernel have released all locks by that 
point? And what about syscall entry...isn't that before any locking starts to 
occur?

> I'd rather not try to play with locking, etc., when we set audit_enabled to
> non-zero...

Sure.

> Especially when there's a trivially non-intrusive patch.

True, but I'm thinking this will cause performance to go down if the audit 
system was ever enabled. It doesn't look as bad as the audit system actually 
being on, but it may be doing unnecessary allocations I think.

-Steve

Re: Audit issue

[Alexander Viro]

On Thu, Nov 08, 2007 at 09:47:40AM -0500, Steve Grubb wrote:
> On Thursday 08 November 2007 09:32:18 Alexander Viro wrote:
> > > Thanks for posting this patch. Is it impossible to "repair " processes by
> > > simply adding a context if the pointer is NULL?
> >
> > At which point would you do that?
> 
> Possibly on syscall exit? Shouldn't the kernel have released all locks by that 
> point? And what about syscall entry...isn't that before any locking starts to 
> occur?

You do not get there unless you have ->audit_context != NULL.  And if
you remove that check, you are in for more overhead.

> True, but I'm thinking this will cause performance to go down if the audit 
> system was ever enabled. It doesn't look as bad as the audit system actually 
> being on, but it may be doing unnecessary allocations I think.

*shrug*

Easy enough to test - boot with audit disabled, run benchmarks, enable
it, flush all caches (e.g. by memory pressure), rerun the benchmarks,
compare...  I don't think it will be serious problem, but if it will
we can always look for trickier solutions.

Re: Audit issue

[Steve Grubb]

On Thursday 08 November 2007 09:56:51 Alexander Viro wrote:
> Easy enough to test - boot with audit disabled, run benchmarks, enable
> it, flush all caches (e.g. by memory pressure), rerun the benchmarks,
> compare...  I don't think it will be serious problem, but if it will
> we can always look for trickier solutions.

OK. I'll try to build a kernel and check this out. Might have some results 
later.

-Steve

Re: Audit issue

[Steve Grubb]

On Thursday 08 November 2007 09:59:30 Steve Grubb wrote:
> On Thursday 08 November 2007 09:56:51 Alexander Viro wrote:
> > Easy enough to test - boot with audit disabled, run benchmarks, enable
> > it, flush all caches (e.g. by memory pressure), rerun the benchmarks,
> > compare...  I don't think it will be serious problem, but if it will
> > we can always look for trickier solutions.
>
> OK. I'll try to build a kernel and check this out. Might have some results
> later.

OK, had a chance to do testing. First, the patch works. It solves the problem 
that was reported. Here's some performance numbers using the performance test 
I have at http://people.redhat.com/sgrubb/files/lspp-perf.tar.gz

without patch boot with audit=0
audit disabled: 38.9
audit enabled:  42.3

without patch boot with audit=1
audit disabled: 41.4
audit enabled: 42.9

with patch boot with audit=0
audit disabled: 38.6
audit enabled: 43.8

with patch boot with audit=1
audit disabled: 44.2
audit enabled: 44.6


So, when audit is enabled at boot. There is virtually no performance 
difference between enabled and not. The old way, we had a 4% performance 
improvement when audit was disabled. Looking at the audit=0 case, there is 
about a 3.5% performance hit when audit is enabled with the new patch. The 
old way, audit disabled was always significantly faster ~ %4. With the patch 
its only about 1% faster.

-Steve

Re: Audit issue

[Eric Paris]

On Thu, 2007-11-08 at 09:27 -0500, Steve Grubb wrote:
> On Thursday 08 November 2007 09:19:26 Alexander Viro wrote:
> > Have fun...
> 
> Thanks for posting this patch. Is it impossible to "repair " processes by 
> simply adding a context if the pointer is NULL?
> 
> 
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index f93c271..83227f8 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -70,6 +70,7 @@ static int	audit_initialized;
> >   * 1 - auditing enabled
> >   * 2 - auditing enabled and configuration is locked/unchangeable. */
> >  int		audit_enabled;
> > +int		audit_ever_enabled;
> >
> >  /* Default state when kernel boots without any parameters. */
> >  static int	audit_default;
> > @@ -965,6 +968,7 @@ static int __init audit_init(void)
> >  	skb_queue_head_init(&audit_skb_queue);
> >  	audit_initialized = 1;
> >  	audit_enabled = audit_default;
> > +	audit_ever_enabled |= !!audit_default;
> 
> Should the declaration of audit_ever_enabled set a default value like 0 since 
> this is being or'ed in? Or should this just be an assignment?

No, global defined variables like this are initialized to 0 in the
kernel.  So this already does what you are thinking.

-Eric