audit: cleanup audit_log_start/wait_for_auditd mess

PATCH In review Oleg Nesterov 2 participants 4 messages Jun 3, 2013 – Jun 7, 2013 Audit

Share & tools

Oleg Nesterov

author

2013-06-03 20:50 UTC #0 lore raw

cover audit: cleanup audit_log_start/wait_for_auditd mess

Really, audit_log_start/wait_for_auditd look so ugly imho, I simply can't resist. Compile-tested only, I do not know how to use audit, but I think the code is really simple after this cleanup.

On top of audit-wait_for_auditd-should-use-task_uninterruptible.patch

Oleg.

Oleg Nesterov

author

2013-06-03 20:50 UTC #1 lore raw

PATCH 1/1 audit: cleanup audit_log_start/wait_for_auditd mess

audit_log_start() tries to obfusticate the code/logic and reimplements wait_event_timeout() in a very confusing way.

Just change wait_for_auditd() to use wait_event_timeout() and return bool.

Also remove the unnecessary "ab = NULL" initialization.

Signed-off-by Oleg Nesterov <[email protected]>

---
 kernel/audit.c |   45 +++++++++++++--------------------------------
 1 files changed, 13 insertions(+), 32 deletions(-)

kernel/audit.c +13 ~~-32~~

index 91e53d0..af0a04d 100644
@@ -1051,20 +1051,19 @@ static inline void audit_get_stamp(struct audit_context *ctx,
 }
 
 /*
- * Wait for auditd to drain the queue a little
+ * Wait for auditd to drain audit_skb_queue if it is full
  */
-static void wait_for_auditd(unsigned long sleep_time)
+static bool wait_for_auditd(gfp_t gfp_mask)
 {
-	DECLARE_WAITQUEUE(wait, current);
-	set_current_state(TASK_UNINTERRUPTIBLE);
-	add_wait_queue(&audit_backlog_wait, &wait);
-
-	if (audit_backlog_limit &&
-	    skb_queue_len(&audit_skb_queue) > audit_backlog_limit)
-		schedule_timeout(sleep_time);
-
-	__set_current_state(TASK_RUNNING);
-	remove_wait_queue(&audit_backlog_wait, &wait);
+	/* Atomic callers use can 5 more entries over the limit */
+	if (!(gfp_mask & __GFP_WAIT))
+		return skb_queue_len(&audit_skb_queue) <=
+					audit_backlog_limit + 5;
+
+	return wait_event_timeout(audit_backlog_wait,
+		!audit_backlog_limit ||
+			skb_queue_len(&audit_skb_queue) <= audit_backlog_limit,
+		audit_backlog_wait_time);
 }
 
 /* Obtain an audit buffer.  This routine does locking to obtain the

@@ -1092,11 +1091,9 @@ static void wait_for_auditd(unsigned long sleep_time)
 struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask,
 				     int type)
 {
-	struct audit_buffer	*ab	= NULL;
+	struct audit_buffer	*ab;
 	struct timespec		t;
 	unsigned int		uninitialized_var(serial);
-	int reserve;
-	unsigned long timeout_start = jiffies;
 
 	if (audit_initialized != AUDIT_INITIALIZED)
 		return NULL;

@@ -1104,23 +1101,7 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask,
 	if (unlikely(audit_filter_type(type)))
 		return NULL;
 
-	if (gfp_mask & __GFP_WAIT)
-		reserve = 0;
-	else
-		reserve = 5; /* Allow atomic callers to go up to five
-				entries over the normal backlog limit */
-
-	while (audit_backlog_limit
-	       && skb_queue_len(&audit_skb_queue) > audit_backlog_limit + reserve) {
-		if (gfp_mask & __GFP_WAIT && audit_backlog_wait_time) {
-			unsigned long sleep_time;
-
-			sleep_time = timeout_start + audit_backlog_wait_time -
-					jiffies;
-			if ((long)sleep_time > 0)
-				wait_for_auditd(sleep_time);
-			continue;
-		}
+	if (audit_backlog_limit && !wait_for_auditd(gfp_mask)) {
 		if (audit_rate_check() && printk_ratelimit())
 			printk(KERN_WARNING
 			       "audit: audit_backlog=%d > "

Signature · 1 line

1.5.5.1

Andrew Morton

2013-06-06 20:55 UTC #2 lore raw

Quoted 7 lines · On Mon, 3 Jun 2013 22:46:20 +0200 Oleg Nesterov <[email protected]> wrote:

audit_log_start() tries to obfusticate the code/logic and reimplements wait_event_timeout() in a very confusing way.
Just change wait_for_auditd() to use wait_event_timeout() and return bool.
Also remove the unnecessary "ab = NULL" initialization.

I would prefer that this be nicely tested - for a cosmetic thing I don't think it's worth the risk as-is.

Quoted 13 lines

kernel/audit.c +2 -3

@@ -1051,20 +1051,19 @@ static inline void audit_get_stamp(struct audit_context *ctx,
 }
 
 /*
- * Wait for auditd to drain the queue a little
+ * Wait for auditd to drain audit_skb_queue if it is full
  */
-static void wait_for_auditd(unsigned long sleep_time)
+static bool wait_for_auditd(gfp_t gfp_mask)
 {
-	DECLARE_WAITQUEUE(wait, current);

kernel/audit.c static inline void audit_get_stamp(struct audit_context *ctx, · line 1058

Quoted 10 lines

kernel/audit.c +1 -9

-	set_current_state(TASK_UNINTERRUPTIBLE);
-	add_wait_queue(&audit_backlog_wait, &wait);
-
-	if (audit_backlog_limit &&
-	    skb_queue_len(&audit_skb_queue) > audit_backlog_limit)
-		schedule_timeout(sleep_time);
-
-	__set_current_state(TASK_RUNNING);
-	remove_wait_queue(&audit_backlog_wait, &wait);
+	/* Atomic callers use can 5 more entries over the limit */

"can use"

Quoted 12 lines

kernel/audit.c +8 -0

+	if (!(gfp_mask & __GFP_WAIT))
+		return skb_queue_len(&audit_skb_queue) <=
+					audit_backlog_limit + 5;
+
+	return wait_event_timeout(audit_backlog_wait,
+		!audit_backlog_limit ||
+			skb_queue_len(&audit_skb_queue) <= audit_backlog_limit,
+		audit_backlog_wait_time);
 }
 
...

Oleg Nesterov

author

2013-06-07 11:56 UTC #3 lore raw

Quoted 6 lines · Andrew Morton

Quoted 3 lines · On Mon, 3 Jun 2013 22:46:20 +0200 Oleg Nesterov <[email protected]> wrote:
audit_log_start() tries to obfusticate the code/logic and reimplements wait_event_timeout() in a very confusing way.

Quoted 7 lines

Quoted 4 lines
Just change wait_for_auditd() to use wait_event_timeout() and return bool.
Also remove the unnecessary "ab = NULL" initialization.
I would prefer that this be nicely tested - for a cosmetic thing I don't think it's worth the risk as-is.

OK, agreed. Please ignore.

Oleg.