Re: [RFC PATCH 6/7] audit: wake up audit_backlog_wait queue when auditd goes away.

[Paul Moore <paul@paul-moore.com>]

On Thursday, October 22, 2015 02:53:19 PM Richard Guy Briggs wrote:
> When auditd goes away (died, killed or shutdown, or net namespace shut
> down), there is no point in sleeping waiting for auditd to drain the
> queue since that message would be distined for the hold queue after the
> timeout anyways.  This will needlessly have those processes wait the
> full default timeout of 60 seconds (audit_backlog_wait_time).
> 
> Wake up the processes caught in the audit_backlog_wait queue when auditd
> is no longer present so they can be sent instead to the hold queue.
> 
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
>  kernel/audit.c |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 34411af..688fa1e 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -425,6 +425,7 @@ restart:
>  				audit_log_lost(s);
>  				audit_pid = 0;
>  				audit_sock = NULL;
> +				wake_up(&audit_backlog_wait);
>  			} else {
>  				pr_warn("re-scheduling(#%d) write to audit_pid=%d\n",
>  					attempts, audit_pid);
> @@ -882,6 +883,8 @@ static int audit_receive_msg(struct sk_buff *skb, struct
> nlmsghdr *nlh) audit_pid = new_pid;
>  			audit_nlk_portid = NETLINK_CB(skb).portid;
>  			audit_sock = skb->sk;
> +			if (!audit_pid)
> +				wake_up(&audit_backlog_wait);
>  		}
>  		if (s.mask & AUDIT_STATUS_RATE_LIMIT) {
>  			err = audit_set_rate_limit(s.rate_limit);

I'm thinking it might be time for two small, static helper functions, 
auditd_register() and auditd_unregister() (or similar, feel free to suggest 
other names), that set/reset the various auditd state variables and handle the 
wake_up() call.  We're duplicating some code that is starting to get non-
trivial.

I'd also add a comment about why you are calling wake_up() in the unregister 
function.

> @@ -1154,6 +1157,7 @@ static void __net_exit audit_net_exit(struct net *net)
> if (sock == audit_sock) {
>  		audit_pid = 0;
>  		audit_sock = NULL;
> +		wake_up(&audit_backlog_wait);
>  	}
> 
>  	RCU_INIT_POINTER(aunet->nlsk, NULL);
> @@ -1393,7 +1397,7 @@ struct audit_buffer *audit_log_start(struct
> audit_context *ctx, gfp_t gfp_mask, sleep_time = timeout_start +
> audit_backlog_wait_time - jiffies; if (sleep_time > 0) {
>  				sleep_time = wait_for_auditd(sleep_time);
> -				if (sleep_time > 0)
> +				if (audit_pid && sleep_time > 0)
>  					continue;

Perhaps handle this in wait_for_auditd()?  Right now this is the only caller, 
but if we use it elsewhere it seems like we would want the same logic.

-- 
paul moore
www.paul-moore.com