[RFC PATCH 5/9] audit: rework the audit queue handling

[Paul Moore]

On Thu, Nov 24, 2016 at 1:35 AM, Richard Guy Briggs <rgb at redhat.com> wrote:
> On 2016-11-23 20:41, Paul Moore wrote:
>> From: Paul Moore <paul at paul-moore.com>
>>
>> The audit record backlog queue has always been a bit of a mess, and
>> the moving the multicast send into kauditd_thread() from
>> audit_log_end() only makes things worse.  This patch attempts to fix
>> the backlog queue with a better design that should hold up better
>> under load and have less of a performance impact at syscall
>> invocation time.
>>
>> While it looks like there is a log going on in this patch, the main
>> change is the move from a single backlog queue to three queues:
>>
>> * A queue for holding records generated from audit_log_end() that
>> haven't been consumed by kauditd_thread() (audit_queue).
>>
>> * A queue for holding records that have been sent via multicast but
>> had a temporary failure when sending via unicast and need a resend
>> (audit_retry_queue).
>>
>> * A queue for holding records that haven't been sent via unicast
>> because no one is listening (audit_hold_queue).
>>
>> Special care is taken in this patch to ensure that the proper
>> record ordering is preserved, e.g. we send everything in the hold
>> queue first, then the retry queue, and finally the main queue.
>>
>> Signed-off-by: Paul Moore <paul at paul-moore.com>
>> ---
>>  kernel/audit.c |  347 ++++++++++++++++++++++++++++++++++++--------------------
>>  1 file changed, 226 insertions(+), 121 deletions(-)

...

>> +/**
>> + * kauditd_retry_skb - Queue an audit record, attempt to send again to auditd
>> + * @skb: audit record
>> + *
>> + * Description:
>> + * Not as serious as kauditd_hold_skb() as we still have a connected auditd,
>> + * but for some reason we are having problems sending it audit records so
>> + * queue the given record and attempt to resend.
>> + */
>> +static void kauditd_retry_skb(struct sk_buff *skb)
>>  {
>> -     int err;
>> -     int attempts = 0;
>> -#define AUDITD_RETRIES 5
>> +     /* NOTE: because records should only live in the retry queue for a
>> +      * short period of time, before either being sent or moved to the hold
>> +      * queue, we don't currently enforce a limit on this queue */
>> +     skb_queue_tail(&audit_retry_queue, skb);
>> +}
>
> Can kauditd_retry_skb() be eliminated entirely and just call
> skb_queue_tail(&audit_retry_queue, skb) directly?  The comment is
> helpful, but found the code harder to follow in kauditd_thread() as a
> result.

It could, yes, and I waffled on that quite a bit while cleaning up
these patches before posting.  Ultimately I decided to keep it
separate both to mimic the similar hold function as well as draw more
attention to the comment inside the function.  There should be zero
performance impact, especially since this is a bit of a corner case,
and the compiler is most likely going to inline this function anyway.

-- 
paul moore
www.paul-moore.com