[PATCH] audit: wait_for_auditd() should use TASK_UNINTERRUPTIBLE

[PATCH] audit: wait_for_auditd() should use TASK_UNINTERRUPTIBLE

[Oleg Nesterov]

audit_log_start() does wait_for_auditd() in a loop until
audit_backlog_wait_time passes or audit_skb_queue has a room.

If signal_pending() is true this becomes a busy-wait loop,
schedule() in TASK_INTERRUPTIBLE won't block.

Reported-by: Guy Streeter <streeter@redhat.com>
Signed-off-by: Oleg Nesterov <oleg@redhat.com>

--- x/kernel/audit.c
+++ x/kernel/audit.c
@@ -1056,7 +1056,7 @@ static inline void audit_get_stamp(struc
 static void wait_for_auditd(unsigned long sleep_time)
 {
 	DECLARE_WAITQUEUE(wait, current);
-	set_current_state(TASK_INTERRUPTIBLE);
+	set_current_state(TASK_UNINTERRUPTIBLE);
 	add_wait_queue(&audit_backlog_wait, &wait);
 
 	if (audit_backlog_limit &&

Re: [PATCH] audit: wait_for_auditd() should use TASK_UNINTERRUPTIBLE

[Oleg Nesterov]

On 05/24, Oleg Nesterov wrote:
>
> audit_log_start() does wait_for_auditd() in a loop until
> audit_backlog_wait_time passes or audit_skb_queue has a room.
>
> If signal_pending() is true this becomes a busy-wait loop,
> schedule() in TASK_INTERRUPTIBLE won't block.

And the code looks strange imho. I think it should be cleanuped,
something like below. But I do not know how can I test this change.

Oleg.

--- x/kernel/audit.c
+++ x/kernel/audit.c
@@ -1053,18 +1053,19 @@ static inline void audit_get_stamp(struc
 /*
  * Wait for auditd to drain the queue a little
  */
-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_INTERRUPTIBLE);
-	add_wait_queue(&audit_backlog_wait, &wait);
-
-	if (audit_backlog_limit &&
-	    skb_queue_len(&audit_skb_queue) > audit_backlog_limit)
-		schedule_timeout(sleep_time);
+	bool can_wait = gfp_mask & __GFP_WAIT;
+	int reserve = can_wait ? 0 : 5;
 
-	__set_current_state(TASK_RUNNING);
-	remove_wait_queue(&audit_backlog_wait, &wait);
+	if (!audit_backlog_limit ||
+	    skb_queue_len(&audit_skb_queue) <= audit_backlog_limit + reserve)
+		return true;
+
+	return can_wait && 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
@@ -1095,8 +1096,6 @@ struct audit_buffer *audit_log_start(str
 	struct audit_buffer	*ab	= NULL;
 	struct timespec		t;
 	unsigned int		uninitialized_var(serial);
-	int reserve;
-	unsigned long timeout_start = jiffies;
 
 	if (audit_initialized != AUDIT_INITIALIZED)
 		return NULL;
@@ -1104,23 +1103,7 @@ struct audit_buffer *audit_log_start(str
 	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 (!wait_for_auditd(gfp_mask)) {
 		if (audit_rate_check() && printk_ratelimit())
 			printk(KERN_WARNING
 			       "audit: audit_backlog=%d > "

Re: [PATCH] audit: wait_for_auditd() should use TASK_UNINTERRUPTIBLE

[Andrew Morton]

On Fri, 24 May 2013 19:39:25 +0200 Oleg Nesterov <oleg@redhat.com> wrote:

> audit_log_start() does wait_for_auditd() in a loop until
> audit_backlog_wait_time passes or audit_skb_queue has a room.
> 
> If signal_pending() is true this becomes a busy-wait loop,
> schedule() in TASK_INTERRUPTIBLE won't block.

And that's game over for a uniprocessor non-preempt machine, yes?

> Reported-by: Guy Streeter <streeter@redhat.com>

And what did Guy report?  "that looks screwy"?  "my machine locked up"?

> @@ -1056,7 +1056,7 @@ static inline void audit_get_stamp(struc
>  static void wait_for_auditd(unsigned long sleep_time)
>  {
>  	DECLARE_WAITQUEUE(wait, current);
> -	set_current_state(TASK_INTERRUPTIBLE);
> +	set_current_state(TASK_UNINTERRUPTIBLE);
>  	add_wait_queue(&audit_backlog_wait, &wait);
>  
>  	if (audit_backlog_limit &&

Re: [PATCH] audit: wait_for_auditd() should use TASK_UNINTERRUPTIBLE

[Guy Streeter]

On 05/29/2013 04:59 PM, Andrew Morton wrote:
> On Fri, 24 May 2013 19:39:25 +0200 Oleg Nesterov <oleg@redhat.com> wrote:
> 
>> audit_log_start() does wait_for_auditd() in a loop until
>> audit_backlog_wait_time passes or audit_skb_queue has a room.
>>
>> If signal_pending() is true this becomes a busy-wait loop,
>> schedule() in TASK_INTERRUPTIBLE won't block.
> 
> And that's game over for a uniprocessor non-preempt machine, yes?
> 
>> Reported-by: Guy Streeter <streeter@redhat.com>
> 
> And what did Guy report?  "that looks screwy"?  "my machine locked up"?
>

Our customer was in fact running a uniprocessor machine, and they reported a
system hang.
--Guy

Re: [PATCH] audit: wait_for_auditd() should use TASK_UNINTERRUPTIBLE

[Oleg Nesterov]

On 05/29, Andrew Morton wrote:
>
> On Fri, 24 May 2013 19:39:25 +0200 Oleg Nesterov <oleg@redhat.com> wrote:
>
> > audit_log_start() does wait_for_auditd() in a loop until
> > audit_backlog_wait_time passes or audit_skb_queue has a room.
> >
> > If signal_pending() is true this becomes a busy-wait loop,
> > schedule() in TASK_INTERRUPTIBLE won't block.
>
> And that's game over for a uniprocessor non-preempt machine, yes?

If this task is rt, yes. Otherwise schedule() still does pick_next_task()
but this is obviously bad anyway. So I fully agree with "Cc: stable" you
added.

> > Reported-by: Guy Streeter <streeter@redhat.com>
>
> And what did Guy report?  "that looks screwy"?  "my machine locked up"?

He also investigated the problem and provided the detailed explanation ;)

Oleg.