linux-kernel - Re: [BUG][PATCH] audit: audit_log_start running on auditd should not stop

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 24 Oct 2013 13:55:37 +0800
From:	Gao feng <gaofeng@...fujitsu.com>
To:	Richard Guy Briggs <rgb@...hat.com>
CC:	linux-audit@...hat.com, linux-kernel@...r.kernel.org,
	Toshiyuki Okajima <toshi.okajima@...fujitsu.com>,
	eparis@...hat.com
Subject: Re: [BUG][PATCH] audit: audit_log_start running on auditd should
 not stop

On 10/24/2013 03:55 AM, Richard Guy Briggs wrote:
> 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, 

include or not include?

The problem is, if the audit_backlog_limit is 3, but there are 5 tasks
calling audit_log_start, so 2 tasks will wait auditd to consume
audit_skb_queue. if before auditd consumes skbs, somebody want to kill
auditd, and auditd will set the audit_pid to zero, this will triger an
audit message. so auditd will wait for himself. and this waiting is endless,
since auditd cann't consume audit_skb_queue any more.

the commit 8ac1c8d5 prevent this problem happening. because if once a task is
blocked over the audit_backlog_wait_time. the audit_backlog_wait_time will
be set to zero(audit_backlog_wait_overflow which is zero). so the other tasks
will not wait anymore. but I'm confused if this is what we expected? these
audit messages will lost once any task is blocked over audit_backlog_wait_time.


So, AFAIK if commit 8ac1c8d5 exist, this patch is not necessary, but
we still do something to fix the problem commit 8ac1c8d5 brings.

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@...hat.com>
> 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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/