Re: [PATCH V1] audit: try harder to send to auditd upon netlink failure

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

On Friday, September 04, 2015 05:14:54 AM Richard Guy Briggs wrote:
> There are several reports of the kernel losing contact with auditd ...

Even if this doesn't completely solve the problem, I like the extra reporting 
and robustness of this change.  Some comments inline ...

> diff --git a/kernel/audit.c b/kernel/audit.c
> index 1c13e42..4ee114a 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -404,19 +404,54 @@ static void audit_printk_skb(struct sk_buff *skb)
>  	audit_hold_skb(skb);
>  }
> 
> +static char *audit_strerror(int err)
> +{
> +	switch (err) {
> +	case -ECONNREFUSED:
> +		return "ECONNREFUSED";
> +	case -EPERM:
> +		return "EPERM";
> +	case -ENOMEM:
> +		return "ENOMEM";
> +	case -EAGAIN:
> +		return "EAGAIN";
> +	case -ERESTARTSYS:
> +		return "ERESTARTSYS";
> +	case -EINTR:
> +		return "EINTR";
> +	default:
> +		return "(other)";
> +	}
> +}

See comments below.

>  static void kauditd_send_skb(struct sk_buff *skb)
>  {
>  	int err;
> +	int attempts = 0;
> +#define AUDITD_RETRIES 5
> +
> +restart:
>  	/* take a reference in case we can't send it and we want to hold it */
>  	skb_get(skb);

Should the restart label go after the skb_get() call?  It seems like if we 
ever jump to restart we could end up needlessly bumping the skb's refcnt.

>  	err = netlink_unicast(audit_sock, skb, audit_nlk_portid, 0);
>  	if (err < 0) {
>  		BUG_ON(err != -ECONNREFUSED); /* Shouldn't happen */
> +		pr_err("netlink_unicast sending to audit_pid=%d returned error: %d, 
%s\n"
> +		       , audit_pid, err, audit_strerror(err));

This is a style nit, but please put the comma on the preceding line.  I know 
why you did it, but it bothers me.

I'm also debating if audit_strerror() is worth it.  I agree with you that it 
is a good idea to indicate the specific error code, I'm just not sure we need 
to bother translating that into a proper errno name.  Can you think of some 
reason why we would need the errno name as opposed to the integer?

>  		if (audit_pid) {
> -			pr_err("*NO* daemon at audit_pid=%d\n", audit_pid);
> -			audit_log_lost("auditd disappeared");
> -			audit_pid = 0;
> -			audit_sock = NULL;
> +			if (err == -ECONNREFUSED || err == -EPERM
> +			    || ++attempts >= AUDITD_RETRIES) {
> +				audit_log_lost("audit_pid=%d reset");
> +				audit_pid = 0;
> +				audit_sock = NULL;
> +			} else {
> +				pr_warn("re-scheduling(#%d) write to audit_pid=%d\n"
> +					, attempts, audit_pid);

Same thing with the comma.

> +				set_current_state(TASK_INTERRUPTIBLE);
> +				schedule();
> +				__set_current_state(TASK_RUNNING);
> +				goto restart;

See my comment above about the skb's reference count.

> +			}
>  		}
>  		/* we might get lucky and get this in the next auditd */
>  		audit_hold_skb(skb);

-- 
paul moore
www.paul-moore.com