Re: [PATCH v2 5/5] audit: do not use exclusive wait in audit_receive()

[Eiichi Tsukata]

> On May 22, 2023, at 13:44, Eiichi Tsukata <eiichi.tsukata@xxxxxxxxxxx> wrote:
> 
> 
> 
>> On May 20, 2023, at 5:54, Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
>> 
>> On May 11, 2023 Eiichi Tsukata <eiichi.tsukata@xxxxxxxxxxx> wrote:
>>> 
>>> kauditd thread issues wake_up() before it goes to sleep. The wake_up()
>>> call wakes up only one process as waiter side uses exclusive wait.
>>> This can be problematic when there are multiple processes (one is in
>>> audit_receive() and others are in audit_log_start()) waiting on
>>> audit_backlog_wait queue.
>>> 
>>> For example, if there are two processes waiting:
>>> 
>>> Process (A): in audit_receive()
>>> Process (B): in audit_log_start()
>>> 
>>> And (A) is at the head of the wait queue. Then kauditd's wake_up() only
>>> wakes up (A) leaving (B) as it is even if @audit_queue is drained. As a
>>> result, (B) can be blocked for up to backlog_wait_time.
>>> 
>>> To prevent the issue, use non-exclusive wait in audit_receive() so that
>>> kauditd can wake up all waiters in audit_receive().
>>> 
>>> Fixes: 8f110f530635 ("audit: ensure userspace is penalized the same as the kernel when under pressure")
>>> Signed-off-by: Eiichi Tsukata <eiichi.tsukata@xxxxxxxxxxx>
>>> ---
>>> kernel/audit.c | 17 +++++++++++------
>>> 1 file changed, 11 insertions(+), 6 deletions(-)
>> 
>> This was also discussed in the last patchset.
>> 
>> 
> 
> This bug is much easily reproducible on real environments and can cause problematic
> user space failure like SSH connection timeout.
> Let’s not keep the bug unfixed.
> (Of course we’ve already carefully tuned audit related params and user space auditd config so that our product won’t hit backlog full.)
> 
> Other ideas in my minds are:
> 
> (1) Use different wait queues in audit_receive() and audit_log_start() to guarantee kautid 
>  wake_up() tries to wake up a waiter in audit_log_start().
> 
> (2) Periodically (say in every 1 sec) check if @audit_queue is full in audit_receive() to prevent 
>  audit_receive() from unnecessarily waiting for so long time. 
> 
> BTW, the default backlog_wait_time is 60 * HZ which seems pretty large.
> I’d appreciate if you could tell me the reason behind that value.
> 
> Eiichi

I came up with a better idea:

(3) Move wait_for_kauditd() in audit_receive() *before* audit_ctl_lock() 
 and restrict penalty only for msg_type which can queue a new audit record. (AUDIT_USER, AUDIT_TRIM, AUDIT_MAKE_EQUIV, ..)

Originally, it’s not reasonable to give penalty for innocent operation
like AUDIT_GET.
This approach makes successive audit_log_end() wake up kauditd.
Also it prevents audit_log_end() from queueing skb ignoring backlog_limit.

Eiichi