On Mon, Jun 7, 2021 at 2:40 PM Richard Guy Briggs <rgb(a)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(a)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(a)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