[RFC PATCH 6/9] audit: rework audit_log_start()

[Paul Moore]

On Fri, Nov 25, 2016 at 11:41 AM, Richard Guy Briggs <rgb at redhat.com> wrote:
> On 2016-11-25 11:38, Paul Moore wrote:
>> On Thu, Nov 24, 2016 at 1:41 AM, Richard Guy Briggs <rgb at redhat.com> wrote:
>> > On 2016-11-23 20:42, Paul Moore wrote:
>> >> From: Paul Moore <paul at paul-moore.com>
>> >>
>> >> The backlog queue handling in audit_log_start() is a little odd with
>> >> some questionable design decisions, this patch attempts to rectify
>> >> this with the following changes:
>> >>
>> >> * Never make auditd wait, ignore any backlog limits as we need auditd
>> >> awake so it can drain the backlog queue.
>> >>
>> >> * When we hit a backlog limit and start dropping records, don't wake
>> >> all the tasks sleeping on the backlog, that's silly.  Instead, let
>> >> kauditd_thread() take care of waking everyone once it has had a chance
>> >> to drain the backlog queue.
>> >>
>> >> * Don't keep a global backlog timeout countdown, make it per-task.  A
>> >> per-task timer means we won't have all the sleeping tasks waking at
>> >> the same time and hammering on an already stressed backlog queue.
>> >>
>> >> Signed-off-by: Paul Moore <paul at paul-moore.com>
>> >> ---
>> >>  kernel/audit.c |   92 ++++++++++++++++++++++----------------------------------
>>
>> ...
>>
>> >>  1 file changed, 36 insertions(+), 56 deletions(-)
>> >> +     /* don't ever fail/sleep on auditd since we need auditd to drain the
>> >> +      * queue; also, when we are checking for auditd, compare PIDs using
>> >> +      * task_tgid_vnr() since auditd_pid is set in audit_receive_msg() using
>> >> +      * a PID anchored in the caller's namespace */
>> >> +     if (!(audit_pid && audit_pid == task_tgid_vnr(current))) {
>> >
>> > Could the change from task_tgid() [should be same as current->tgid] to
>> > task_tgid_vnr() be pulled out into a seperate patch to make the
>> > namespace behaviour change implicaiton much more clear?
>>
>> Considering the comment above the if-conditional I don't think there
>> is much to be gained by splitting it out to a separate patch.
>
> Except the ability to find it when someone goes looking for things that
> change namespace behaviour, which this patch objective should not
> include.
>
> I agree it is well explained in the comment, but that change is
> unrelated to the goal of the rest of the patch.

It is very related to the goals of the patch, look at the title,
"rework audit_log_start()", as well as the description where I
specifically call out not making auditd wait on the backlog.

-- 
paul moore
www.paul-moore.com