Re: proposing [PATCH] audit: get rid of *NO* daemon at audit_pid=0 message

[Eric Paris <eparis@redhat.com>]

On Mon, 2013-11-11 at 14:20 -0500, Steve Grubb wrote:
> On Monday, November 11, 2013 12:21:33 PM Eric Paris wrote:
> > On Wed, 2013-10-30 at 00:05 +0100, Mateusz Guzik wrote:
> > > Hello,
> > > 
> > > I wrote a trivial patch for what I believe is a subsystem you maintain.
> > > 
> > > I'm sending it privately first to ensure it looks ok at has proper
> > > recipients (I'm new to linux world, sorry :>).
> > > 
> > > 'To' would be: linux-audit@redhat.com
> > > 
> > > The rest is:
> > > 
> > > From: Mateusz Guzik <mguzik@redhat.com>
> > > Date: Tue, 29 Oct 2013 23:51:52 +0100
> > > Subject: [PATCH] audit: get rid of *NO* daemon at audit_pid=0 message
> > > 
> > > kauditd_send_skb is called after audit_pid was checked to be non-zero.
> > > 
> > > However, it can be set to 0 due to auditd exiting while kauditd_send_skb
> > > is still executed and this can result in a spurious warning about missing
> > > auditd.
> 
> Is it really spurious? Meaning an admin at a site making heavy use of the 
> audit subsystem would never want to know this? Are the events always put back 
> with no chance of loss?
> 
> -Steve

auidt_pid will only get set to 0 in 2 ways.  1) auditd died, in which
case we will record the first lost message (because the pid won't be 0,
we set it to 0 after we record that message).  2) auditd/auditctl
explicitly set audit_pid to 0 (aka on shutdown)   In which case, we
don't expect such a printk.  It is normal that messages which race with
auditd shutting down may get 'lost'.  If the message came 1 millisecond
later it would never have gotten on the queue since audit_pid would be
0.

-Eric
> 
>  
> > > Re-check audit_pid before printing the message.
> > > 
> > > Signed-off-by: Mateusz Guzik <mguzik@redhat.com>
> > > Cc: Eric Paris <eparis@redhat.com>
> > > Cc: linux-kernel@vger.kernel.org
> > 
> > Acked-by: Eric Paris <eparis@redhat.com>
> > 
> > > ---
> > > 
> > >  kernel/audit.c | 8 +++++---
> > >  1 file changed, 5 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/kernel/audit.c b/kernel/audit.c
> > > index 7b0e23a..a91a965 100644
> > > --- a/kernel/audit.c
> > > +++ b/kernel/audit.c
> > > @@ -388,9 +388,11 @@ static void kauditd_send_skb(struct sk_buff *skb)
> > > 
> > >  	err = netlink_unicast(audit_sock, skb, audit_nlk_portid, 0);
> > >  	if (err < 0) {
> > >  	
> > >  		BUG_ON(err != -ECONNREFUSED); /* Shouldn't happen */
> > > 
> > > -		printk(KERN_ERR "audit: *NO* daemon at audit_pid=%d\n", 
> audit_pid);
> > > -		audit_log_lost("auditd disappeared\n");
> > > -		audit_pid = 0;
> > > +		if (audit_pid) {
> > > +			printk(KERN_ERR "audit: *NO* daemon at audit_pid=%d\n", 
> audit_pid);
> > > +			audit_log_lost("auditd disappeared\n");
> > > +			audit_pid = 0;
> > > +		}
> > > 
> > >  		/* we might get lucky and get this in the next auditd */
> > >  		audit_hold_skb(skb);
> > >  	
> > >  	} else
> > 
> > --
> > Linux-audit mailing list
> > Linux-audit@redhat.com
> > https://www.redhat.com/mailman/listinfo/linux-audit
>