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

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