Re: [BUG][PATCH] audit: audit_log_start running on auditd should notstop

[Richard Guy Briggs]

On Tue, Oct 15, 2013 at 02:30:34PM +0800, Gao feng wrote:
> Hi Toshiyuki-san,

Toshiuki and Gao,

> On 10/15/2013 12:43 PM, Toshiyuki Okajima wrote:
> > The backlog cannot be consumed when audit_log_start is running on auditd
> > even if audit_log_start calls wait_for_auditd to consume it.
> > The situation is a deadlock because only auditd can consume the backlog.
> > If the other process needs to send the backlog, it can be also stopped 
> > by the deadlock.
> > 
> > So, audit_log_start running on auditd should not stop.
> > 
> > You can see the deadlock with the following reproducer:
> >  # auditctl -a exit,always -S all
> >  # reboot

> Hmm, I see, There may be other code paths that auditd can call audit_log_start except
> audit_log_config_change. so it's better to handle this problem in audit_log_start.
> 
> but current task is only meaningful when gfp_mask & __GFP_WAIT is true.
> so maybe the below patch is what you want.

I have been following this thread with interest.  I like the general
evolution of this patch.  The first patch was a bit too abrupt, dropping
too much, but this one makes much more sense.  I would be tempted to
make the reserve even bigger.

I see that you should be using a kernel that has included commit
8ac1c8d5 (which made it into v3.12-rc3)
	audit: fix endless wait in audit_log_start()
That was an obvious bug, but I was still concerned about the cause of
the initial wait.  There are other fixes and ideas in the works that
should alleviate some of the pressure to make the service more usable.
	https://lkml.org/lkml/2013/9/18/453

I have tested with and without this v3 patch and I don't see any
significant difference with the reproducer provided above.  I'm also
testing with a reproducer of the endless wait bug (readahead-collector).

What are your expected results?  What are your actual results in each
case?  How are they different?

> diff --git a/kernel/audit.c b/kernel/audit.c
> index 7b0e23a..10b4545 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -1095,7 +1095,9 @@ struct audit_buffer *audit_log_start(struct audit_context
>         struct audit_buffer     *ab     = NULL;
>         struct timespec         t;
>         unsigned int            uninitialized_var(serial);
> -       int reserve;
> +       int reserve = 5; /* Allow atomic callers to go up to five
> +                           entries over the normal backlog limit */
> +
>         unsigned long timeout_start = jiffies;
> 
>         if (audit_initialized != AUDIT_INITIALIZED)
> @@ -1104,11 +1106,12 @@ struct audit_buffer *audit_log_start(struct audit_contex
>         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 */
> +       if (gfp_mask & __GFP_WAIT) {
> +               if (audit_pid && audit_pid == current->pid)
> +                       gfp_mask &= ~__GFP_WAIT;
> +               else
> +                       reserve = 0;
> +       }
> 
>         while (audit_backlog_limit
>                && skb_queue_len(&audit_skb_queue) > audit_backlog_limit + reserv

- RGB

--
Richard Guy Briggs <rbriggs@xxxxxxxxxx>
Senior Software Engineer
Kernel Security
AMER ENG Base Operating Systems
Remote, Ottawa, Canada
Voice: +1.647.777.2635
Internal: (81) 32635
Alt: +1.613.693.0684x3545
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/