8:19 a.m.
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);
8:27 a.m.
New subject: Audit issue
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
8:32 a.m.
New subject: Audit issue
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.
8:47 a.m.
New subject: Audit issue
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
8:56 a.m.
New subject: Audit issue
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.
8:59 a.m.
New subject: Audit issue
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
6:28 p.m.
New subject: Audit issue
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
8:35 a.m.
New subject: Audit issue
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
6252
days inactive
6253
days old
linux-audit@lists.linux-audit.osci.io
7 comments
3 participants
participants (3)
-
Alexander Viro -
Eric Paris -
Steve Grubb