Messages in this thread | | | From | Eiichi Tsukata <> | Subject | Re: [PATCH v2 5/5] audit: do not use exclusive wait in audit_receive() | Date | Tue, 23 May 2023 03:58:30 +0000 |
| |
> On May 22, 2023, at 13:44, Eiichi Tsukata <eiichi.tsukata@nutanix.com> wrote: > > > >> On May 20, 2023, at 5:54, Paul Moore <paul@paul-moore.com> wrote: >> >> On May 11, 2023 Eiichi Tsukata <eiichi.tsukata@nutanix.com> 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@nutanix.com> >>> --- >>> 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
| |