[PATCH -mm 0/1] audit: cleanup audit_log_start/wait_for_auditd mess

[PATCH -mm 0/1] audit: cleanup audit_log_start/wait_for_auditd mess

[Oleg Nesterov]

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.

[PATCH -mm 1/1] audit: cleanup audit_log_start/wait_for_auditd mess

[Oleg Nesterov]

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 <oleg@redhat.com>
---
 kernel/audit.c |   45 +++++++++++++--------------------------------
 1 files changed, 13 insertions(+), 32 deletions(-)

diff --git a/kernel/audit.c b/kernel/audit.c
index 91e53d0..af0a04d 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -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 > "
-- 
1.5.5.1

Re: [PATCH -mm 1/1] audit: cleanup audit_log_start/wait_for_auditd mess

[Andrew Morton]

On Mon, 3 Jun 2013 22:46:20 +0200 Oleg Nesterov <oleg@redhat.com> 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.

> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -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 */

"can use"

> +	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);
>  }
>
> ...
>

Re: [PATCH -mm 1/1] audit: cleanup audit_log_start/wait_for_auditd mess

[Oleg Nesterov]

On 06/06, Andrew Morton wrote:
>
> On Mon, 3 Jun 2013 22:46:20 +0200 Oleg Nesterov <oleg@redhat.com> 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.

OK, agreed. Please ignore.

Oleg.