[RFC PATCH] audit: reduce the number of kauditd_thread wakeups

[Paul Moore]

On Mon, Jun 7, 2021 at 2:40 PM Richard Guy Briggs <rgb at redhat.com> wrote:
> On 2021-06-05 23:23, Paul Moore wrote:
> > [NOTE: As this is an RFC patch, I wanted to add some commentary at
> >  the top of the patch description explaining where this patch came
> >  from and what testing has been done.  This patch is a derivative
> >  of another unreleased patch that removed all of the wake up calls
> >  from the audit_log_start() and audit_log_end() functions; while
> >  that patch worked well, there we some record losees with high
> >  volume burst traffic in the case of `auditctl -a task,never` or
> >  CONFIG_AUDITSYSCALL=n.  As this patch doesn't completely remove
> >  the wake up calls these two cases should behave similarly to how
> >  they do today.  As far as testing is concerned, this patch passes
> >  both the audit-testsuite and selinux-testsuite without problem and
> >  with expected audit queue losses (no losses outside of the tests
> >  which attempt to generate losses).  This patch also preserves the
> >  ability for the system to continue to function, in the
> >  `auditctl -a exit,always -S all` case, albeit very slowly.]
> >
> > When audit is enabled, the kauditd_thread() function runs in its own
> > kernel thread, emptying the audit record queue and sending the
> > entries to userspace via different means.  As part of its normal
> > processing it goes to sleep after emptying the queue and waits for
> > the audit functions to wake it.
> >
> > The current audit kernel code attempts to wake the kauditd thread
> > after each entry is added to the queue.  Considering that a single
> > syscall can have multiple audit records, with each wake attempt
> > involving locking and additional processing, this can be rather
> > wasteful.  In an effort to limit the number of wake attempts without
> > unnecessarily risking the audit queue size this patch does the
> > following:
> >
> > * In the case of syscall auditing the wake up call is moved from
> >   audit_log_end() to audit_log_exit() meaning that the kauditd
> >   thread is only woken once, at the end of the syscall, after all of
> >   the syscall's audit records have been added to the queue.
> >
> > * In the case where audit records are generated outside of a syscall
> >   context, the wake up call in audit_log_end() is preserved in order
> >   to ensure that these records do not suffer any additional latency
> >   or put unnecessary pressure on the queue.  This is done through
> >   some additional checking in audit_log_start() and an additional
> >   flag in the audit_buffer struct.
> >
> > * The audit_log_start() function never attempts to wake the kauditd
> >   thread.  In the current code this is only done when the queue is
> >   under pressure, but at that point it is extremely likely that the
> >   thread is already active or scheduled, do we should be able to
> >   safely remove this wake attempt.
> >
> > * Always wake the kauditd thread after processing management and
> >   user records in audit_receive_msg().  This should be relatively
> >   low frequency compared to the other audit sources, and doing a
> >   wake call here helps keep record latency low and the queue size
> >   in check.
> >
> > * The kauditd thread itself is now a bit better at handling high
> >   volume audit record bursts.  Previously after emptying the queue
> >   the thread would wake every process that was blocked and then go
> >   to sleep; possibly going to sleep just a flood of new records
> >   are added to the queue.  The new kauditd thread approach wakes all
> >   of the blocked processes and then reschedules itself in
> >   anticipation of new records.  When the thread returns to execution
> >   it checks the queue and if there are any records present it
> >   immediately starts processing them, if the queue is empty the
> >   kauditd thread goes back to sleep.
> >
> > Signed-off-by: Paul Moore <paul at paul-moore.com>
>
> This looks good to me.  Thank you for the thorough description.  I can
> see how this work was provoked given some of the other work in progress
> and some of the minor changes that will be needed to those other works
> as a result.
>
> Acked-by: Richard Guy Briggs <rgb at redhat.com>

Thanks for taking a look.  As a FYI, I'm going to sit on this until
the io_uring patches are merged as there is some overlap, and I want
to possibly play with this a bit more as well.  I wanted to post this
to see if people had any strong feelings about this, and of course
just to get it "out there" so I wouldn't forget about it :)

-- 
paul moore
www.paul-moore.com