I also tried to extend Cong Wang's idea to attempt to proactively respond to a NETLINK_URELEASE on the audit_sock and reset it, but ran into a locking error stack dump using mutex_lock(&audit_cmd_mutex) in the notifier callback. Eliminating the lock since the sock is dead anways eliminates the error. Is it safe? I'll resubmit if this looks remotely sane. Meanwhile I'll try to get the test case to compile.

@@ -1167,10 +1190,14 @@ static void __net_exit audit_net_exit(struct net *net) { struct audit_net *aunet = net_generic(net, audit_net_id); struct sock *sock = aunet->nlsk; + + mutex_lock(&audit_cmd_mutex); if (sock == audit_sock) { audit_pid = 0; + audit_nlk_portid = 0; audit_sock = NULL; } + mutex_unlock(&audit_cmd_mutex);

On 2016-12-09 23:40, Cong Wang wrote: > On Fri, Dec 9, 2016 at 8:13 PM, Cong Wang <xiyou.wangcong(a)gmail.com&gt; wrote: > > On Fri, Dec 9, 2016 at 3:01 AM, Richard Guy Briggs <rgb(a)redhat.com&gt; wrote: > >> On 2016-12-08 22:57, Cong Wang wrote: > >>> On Thu, Dec 8, 2016 at 10:02 PM, Richard Guy Briggs <rgb(a)redhat.com&gt; wrote: > >>> > I also tried to extend Cong Wang's idea to attempt to proactively respond to a > >>> > NETLINK_URELEASE on the audit_sock and reset it, but ran into a locking error > >>> > stack dump using mutex_lock(&audit_cmd_mutex) in the notifier callback. > >>> > Eliminating the lock since the sock is dead anways eliminates the error. > >>> > > >>> > Is it safe? I'll resubmit if this looks remotely sane. Meanwhile I'll try to > >>> > get the test case to compile. > >>> > >>> It doesn't look safe, because 'audit_sock', 'audit_nlk_portid' and 'audit_pid' > >>> are updated as a whole and race between audit_receive_msg() and > >>> NETLINK_URELEASE. > >> > >> This is what I expected and why I originally added the mutex lock in the > >> callback... The dumps I got were bare with no wrapper identifying the > >> process context or specific error, so I'm at a bit of a loss how to > >> solve this (without thinking more about it) other than instinctively > >> removing the mutex. > > > > Netlink notifier can safely be converted to blocking one, I will send > > a patch. > > > > But I seriously doubt you really need NETLINK_URELEASE here, > > it adds nothing but overhead, b/c the netlink notifier is called on > > every netlink socket in the system, but for net exit path, that is > > relatively a slow path. > > > > Also, kauditd_send_skb() needs audit_cmd_mutex too. > > Please let me know what you think about the attached patch? > > Thanks! > commit a12b43ee814625933ff155c20dc863c59cfcf240 > Author: Cong Wang <xiyou.wangcong(a)gmail.com&gt; > Date: Fri Dec 9 17:56:42 2016 -0800 > > audit: close a race condition on audit_sock > > Signed-off-by: Cong Wang <xiyou.wangcong(a)gmail.com&gt; > > diff --git a/kernel/audit.c b/kernel/audit.c > index f1ca116..ab947d8 100644 > --- a/kernel/audit.c > +++ b/kernel/audit.c > @@ -423,6 +423,8 @@ static void kauditd_send_skb(struct sk_buff *skb) > snprintf(s, sizeof(s), "audit_pid=%d reset", audit_pid); > audit_log_lost(s); > audit_pid = 0; > + audit_nlk_portid = 0; > + sock_put(audit_sock); > audit_sock = NULL; > } else { > pr_warn("re-scheduling(#%d) write to audit_pid=%d\n", > @@ -899,6 +901,9 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh) > audit_log_config_change("audit_pid", new_pid, audit_pid, 1); > audit_pid = new_pid; > audit_nlk_portid = NETLINK_CB(skb).portid; > + sock_hold(skb->sk); > + if (audit_sock) > + sock_put(audit_sock); > audit_sock = skb->sk; > } > if (s.mask & AUDIT_STATUS_RATE_LIMIT) { > @@ -1167,10 +1172,6 @@ static void __net_exit audit_net_exit(struct net *net) > { > struct audit_net *aunet = net_generic(net, audit_net_id); > struct sock *sock = aunet->nlsk; > - if (sock == audit_sock) { > - audit_pid = 0; > - audit_sock = NULL; > - } So how does this not leak memory leaving the sock refcount incremented by the registered audit daemon when that daemon shuts down normally?

- RGB

On Mon, Dec 12, 2016 at 5:03 AM, Richard Guy Briggs <rgb(a)redhat.com&gt; wrote: > Resetting audit_sock appears to be racy. > > audit_sock was being copied and dereferenced without using a refcount on > the source sock. > > Bump the refcount on the underlying sock when we store a refrence in > audit_sock and release it when we reset audit_sock. audit_sock > modification needs the audit_cmd_mutex. > > See: https://lkml.org/lkml/2016/11/26/232 > > Thanks to Eric Dumazet <edumazet(a)google.com&gt; and Cong Wang > <xiyou.wangcong(a)gmail.com&gt; on ideas how to fix it. > > Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com&gt; > --- > There has been a lot of change in the audit code that is about to go > upstream to address audit queue issues. This patch is based on the > source tree: git://git.infradead.org/users/pcmoore/audit#next > --- > kernel/audit.c | 34 ++++++++++++++++++++++++++++------ > 1 files changed, 28 insertions(+), 6 deletions(-) This is coming in pretty late for the v4.10 merge window, much later than I would usually take things, but this is arguably important, and (at first glance) relatively low risk - what testing have you done on this?

> diff --git a/kernel/audit.c b/kernel/audit.c > index f20eee0..439f7f3 100644 > --- a/kernel/audit.c > +++ b/kernel/audit.c > @@ -452,7 +452,9 @@ static void auditd_reset(void) > struct sk_buff *skb; > > /* break the connection */ > + sock_put(audit_sock); > audit_pid = 0; > + audit_nlk_portid = 0; > audit_sock = NULL; > > /* flush all of the retry queue to the hold queue */ > @@ -478,6 +480,12 @@ static int kauditd_send_unicast_skb(struct sk_buff *skb) > if (rc >= 0) { > consume_skb(skb); > rc = 0; > + } else { > + if (rc & (-ENOMEM|-EPERM|-ECONNREFUSED)) { > + mutex_lock(&audit_cmd_mutex); > + auditd_reset(); > + mutex_unlock(&audit_cmd_mutex); > + } > } > > return rc; > @@ -579,7 +587,9 @@ static int kauditd_thread(void *dummy) > > auditd = 0; > if (AUDITD_BAD(rc, reschedule)) { > + mutex_lock(&audit_cmd_mutex); > auditd_reset(); > + mutex_unlock(&audit_cmd_mutex); > reschedule = 0; > } > } else > @@ -594,7 +604,9 @@ static int kauditd_thread(void *dummy) > auditd = 0; > if (AUDITD_BAD(rc, reschedule)) { > kauditd_hold_skb(skb); > + mutex_lock(&audit_cmd_mutex); > auditd_reset(); > + mutex_unlock(&audit_cmd_mutex); > reschedule = 0; > } else > /* temporary problem (we hope), queue > @@ -623,7 +635,9 @@ quick_loop: > if (rc) { > auditd = 0; > if (AUDITD_BAD(rc, reschedule)) { > + mutex_lock(&audit_cmd_mutex); > auditd_reset(); > + mutex_unlock(&audit_cmd_mutex); > reschedule = 0; > } > > @@ -1004,17 +1018,22 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh) > return -EACCES; > } > if (audit_pid && new_pid && > - audit_replace(requesting_pid) != -ECONNREFUSED) { > + (audit_replace(requesting_pid) & (-ECONNREFUSED|-EPERM|-ENOMEM))) { > audit_log_config_change("audit_pid", new_pid, audit_pid, 0); > return -EEXIST; > } > if (audit_enabled != AUDIT_OFF) > audit_log_config_change("audit_pid", new_pid, audit_pid, 1); > - audit_pid = new_pid; > - audit_nlk_portid = NETLINK_CB(skb).portid; > - audit_sock = skb->sk; > - if (!new_pid) > + if (new_pid) { > + if (audit_sock) > + sock_put(audit_sock); > + audit_pid = new_pid; > + audit_nlk_portid = NETLINK_CB(skb).portid; > + sock_hold(skb->sk); > + audit_sock = skb->sk; > + } else { > auditd_reset(); > + } > wake_up_interruptible(&kauditd_wait); > } > if (s.mask & AUDIT_STATUS_RATE_LIMIT) { > @@ -1283,8 +1302,11 @@ static void __net_exit audit_net_exit(struct net *net) > { > struct audit_net *aunet = net_generic(net, audit_net_id); > struct sock *sock = aunet->nlsk; > - if (sock == audit_sock) > + if (sock == audit_sock) { > + mutex_lock(&audit_cmd_mutex); > auditd_reset(); > + mutex_unlock(&audit_cmd_mutex); > + } > > RCU_INIT_POINTER(aunet->nlsk, NULL); > synchronize_net(); > -- > 1.7.1 > > -- > Linux-audit mailing list > Linux-audit(a)redhat.com > https://www.redhat.com/mailman/listinfo/linux-audit -- paul moore www.paul-moore.com

On Mon, Dec 12, 2016 at 5:03 AM, Richard Guy Briggs <rgb(a)redhat.com&gt; wrote: > Resetting audit_sock appears to be racy. > > audit_sock was being copied and dereferenced without using a refcount on > the source sock. > > Bump the refcount on the underlying sock when we store a refrence in > audit_sock and release it when we reset audit_sock. audit_sock > modification needs the audit_cmd_mutex. > > See: https://lkml.org/lkml/2016/11/26/232 > > Thanks to Eric Dumazet <edumazet(a)google.com&gt; and Cong Wang > <xiyou.wangcong(a)gmail.com&gt; on ideas how to fix it. > > Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com&gt; > --- > There has been a lot of change in the audit code that is about to go > upstream to address audit queue issues. This patch is based on the > source tree: git://git.infradead.org/users/pcmoore/audit#next > --- > kernel/audit.c | 34 ++++++++++++++++++++++++++++------ > 1 files changed, 28 insertions(+), 6 deletions(-) > diff --git a/kernel/audit.c b/kernel/audit.c > index f20eee0..439f7f3 100644 > --- a/kernel/audit.c > +++ b/kernel/audit.c > @@ -452,7 +452,9 @@ static void auditd_reset(void) > struct sk_buff *skb; > > /* break the connection */ > + sock_put(audit_sock); > audit_pid = 0; > + audit_nlk_portid = 0; > audit_sock = NULL; > > /* flush all of the retry queue to the hold queue */ > @@ -478,6 +480,12 @@ static int kauditd_send_unicast_skb(struct sk_buff *skb) > if (rc >= 0) { > consume_skb(skb); > rc = 0; > + } else { > + if (rc & (-ENOMEM|-EPERM|-ECONNREFUSED)) { I dislike the way you wrote this because instead of simply looking at this to see if it correct I need to sort out all the bits and find out if there are other error codes that could run afoul of this check ... make it simple, e.g. (rc == -ENOMEM || rc == -EPERM || ...). Actually, since EPERM is 1, -EPERM (-1 in two's compliment is 0xffffffff) is going to cause this to be true for pretty much any value of rc, yes?

> + mutex_lock(&audit_cmd_mutex); > + auditd_reset(); > + mutex_unlock(&audit_cmd_mutex); > + } The code in audit#next handles netlink_unicast() errors in kauditd_thread() and you are adding error handling code here in kauditd_send_unicast_skb() ... that's messy. I don't care too much where the auditd_reset() call is made, but let's only do it in one function; FWIW, I originally put the error handling code in kauditd_thread() because there was other error handling code that needed to done in that scope so it resulted in cleaner code.

Related, I see you are now considering ENOMEM to be a fatal condition, that differs from the AUDITD_BAD macro in kauditd_thread(); this difference needs to be reconciled.

Finally, you should update the comment header block for auditd_reset() that it needs to be called with the audit_cmd_mutex held.

> @@ -1004,17 +1018,22 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh) > return -EACCES; > } > if (audit_pid && new_pid && > - audit_replace(requesting_pid) != -ECONNREFUSED) { > + (audit_replace(requesting_pid) & (-ECONNREFUSED|-EPERM|-ENOMEM))) { Do we simply want to treat any error here as fatal, and not just ECONN/EPERM/ENOMEM? If not, let's come up with a single macro to handle the fatal netlink_unicast() return codes so we have some chance to keep things consistent in the future.

paul moore

Resetting audit_sock appears to be racy. audit_sock was being copied and dereferenced without using a refcount on the source sock. Bump the refcount on the underlying sock when we store a refrence in audit_sock and release it when we reset audit_sock. audit_sock modification needs the audit_cmd_mutex. See: https://lkml.org/lkml/2016/11/26/232 Thanks to Eric Dumazet <edumazet(a)google.com&gt; and Cong Wang <xiyou.wangcong(a)gmail.com&gt; on ideas how to fix it. Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com&gt; --- There has been a lot of change in the audit code that is about to go upstream to address audit queue issues. This patch is based on the source tree: git://git.infradead.org/users/pcmoore/audit#next --- kernel/audit.c | 34 ++++++++++++++++++++++++++++------ 1 files changed, 28 insertions(+), 6 deletions(-) diff --git a/kernel/audit.c b/kernel/audit.c index f20eee0..439f7f3 100644 --- a/kernel/audit.c +++ b/kernel/audit.c @@ -452,7 +452,9 @@ static void auditd_reset(void) struct sk_buff *skb; /* break the connection */ + sock_put(audit_sock);

audit_pid = 0; + audit_nlk_portid = 0; audit_sock = NULL; /* flush all of the retry queue to the hold queue */ @@ -478,6 +480,12 @@ static int kauditd_send_unicast_skb(struct sk_buff *skb) if (rc >= 0) { consume_skb(skb); rc = 0; + } else { + if (rc & (-ENOMEM|-EPERM|-ECONNREFUSED)) {

+ mutex_lock(&audit_cmd_mutex); + auditd_reset(); + mutex_unlock(&audit_cmd_mutex); + } } return rc; @@ -579,7 +587,9 @@ static int kauditd_thread(void *dummy) auditd = 0; if (AUDITD_BAD(rc, reschedule)) { + mutex_lock(&audit_cmd_mutex); auditd_reset(); + mutex_unlock(&audit_cmd_mutex); reschedule = 0; } } else @@ -594,7 +604,9 @@ static int kauditd_thread(void *dummy) auditd = 0; if (AUDITD_BAD(rc, reschedule)) { kauditd_hold_skb(skb); + mutex_lock(&audit_cmd_mutex); auditd_reset(); + mutex_unlock(&audit_cmd_mutex); reschedule = 0; } else /* temporary problem (we hope), queue @@ -623,7 +635,9 @@ quick_loop: if (rc) { auditd = 0; if (AUDITD_BAD(rc, reschedule)) { + mutex_lock(&audit_cmd_mutex); auditd_reset(); + mutex_unlock(&audit_cmd_mutex); reschedule = 0; } @@ -1004,17 +1018,22 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh) return -EACCES; } if (audit_pid && new_pid && - audit_replace(requesting_pid) != -ECONNREFUSED) { + (audit_replace(requesting_pid) & (-ECONNREFUSED|-EPERM|-ENOMEM))) { audit_log_config_change("audit_pid", new_pid, audit_pid, 0); return -EEXIST; } if (audit_enabled != AUDIT_OFF) audit_log_config_change("audit_pid", new_pid, audit_pid, 1); - audit_pid = new_pid; - audit_nlk_portid = NETLINK_CB(skb).portid; - audit_sock = skb->sk; - if (!new_pid) + if (new_pid) { + if (audit_sock) + sock_put(audit_sock); + audit_pid = new_pid; + audit_nlk_portid = NETLINK_CB(skb).portid; + sock_hold(skb->sk);

+ audit_sock = skb->sk; + } else { auditd_reset(); + } wake_up_interruptible(&kauditd_wait); } if (s.mask & AUDIT_STATUS_RATE_LIMIT) { @@ -1283,8 +1302,11 @@ static void __net_exit audit_net_exit(struct net *net) { struct audit_net *aunet = net_generic(net, audit_net_id); struct sock *sock = aunet->nlsk; - if (sock == audit_sock) + if (sock == audit_sock) { + mutex_lock(&audit_cmd_mutex);

auditd_reset(); + mutex_unlock(&audit_cmd_mutex); + } RCU_INIT_POINTER(aunet->nlsk, NULL); synchronize_net(); -- 1.7.1

@@ -1283,8 +1299,10 @@ static void __net_exit audit_net_exit(struct net *net) { struct audit_net *aunet = net_generic(net, audit_net_id); struct sock *sock = aunet->nlsk; + mutex_lock(&audit_cmd_mutex); if (sock == audit_sock) auditd_reset(); + mutex_unlock(&audit_cmd_mutex);

On 2016-12-13 16:19, Cong Wang wrote: > On Tue, Dec 13, 2016 at 7:03 AM, Richard Guy Briggs <rgb(a)redhat.com&gt; wrote: > > @@ -1283,8 +1299,10 @@ static void __net_exit audit_net_exit(struct net *net) > > { > > struct audit_net *aunet = net_generic(net, audit_net_id); > > struct sock *sock = aunet->nlsk; > > + mutex_lock(&audit_cmd_mutex); > > if (sock == audit_sock) > > auditd_reset(); > > + mutex_unlock(&audit_cmd_mutex); > > This still doesn't look correct to me, b/c here we release the audit_sock > refcnt twice: > > 1) inside audit_reset() The audit_reset() refcount decrement corresponds to a setting of audit_sock only if audit_sock is still non-NULL.