Audit issue

DISCUSSION Alexander Viro 3 participants 8 messages Nov 8, 2007 – Nov 9, 2007 4 messages not in the archive Audit

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Alexander Viro

author

2007-11-08 14:19 UTC #4 lore raw

Quoted 6 lines · Steve Grubb

Quoted 5 lines · Alexander Viro
Quoted 4 lines · Steve Grubb
Quoted 2 lines · Eric Paris
why is it acceptable to mandate audit=1 in gurb but not to mandate 'don't use auditctl -e 0' ?? ????

Quoted 8 lines

Quoted 8 lines
Quoted 5 lines
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...

kernel/audit.c +8 -2

index f93c271..83227f8 100644
@@ -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;
 }

kernel/auditsc.c +2 -1

index bce9ecd..250f00f 100644
@@ -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);

Steve Grubb

2007-11-08 14:27 UTC #5 lore raw

Quoted 1 line · 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?

Quoted 13 lines

kernel/audit.c +1 -0

index f93c271..83227f8 100644
@@ -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;

kernel/audit.c static int __init audit_init(void) · line 971

Quoted 4 lines

kernel/audit.c +1 -0

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

Alexander Viro

author

2007-11-08 14:32 UTC #6 lore raw

Quoted 5 lines · Steve Grubb

Quoted 1 line · 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.

Steve Grubb

2007-11-08 14:47 UTC #7 lore raw

Quoted 4 lines · On Thursday 08 November 2007 09:32:18 Alexander Viro wrote:

Quoted 2 lines
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?

Quoted 2 lines

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

Sure.

Quoted 1 line

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

Alexander Viro

author

2007-11-08 14:56 UTC #8 lore raw

Quoted 9 lines · Steve Grubb

Quoted 4 lines · On Thursday 08 November 2007 09:32:18 Alexander Viro wrote:
Quoted 2 lines
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.

Quoted 3 lines

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.

Steve Grubb

2007-11-08 14:59 UTC #9 lore raw

Quoted 4 lines · 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

Steve Grubb

2007-11-09 00:28 UTC #10 lore raw

Quoted 8 lines · On Thursday 08 November 2007 09:59:30 Steve Grubb wrote:

Quoted 4 lines · 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

Eric Paris

maintainer

2007-11-08 14:35 UTC #11 lore raw

In reply to Steve Grubb

Quoted 17 lines · Steve Grubb

Quoted 1 line · 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?
Quoted 10 lines
kernel/audit.c +1 -0
index f93c271..83227f8 100644
@@ -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;
 

Quoted 10 lines

kernel/audit.c static int __init audit_init(void) · line 971
Quoted 7 lines
kernel/audit.c +1 -0
 /* 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