[PATCH] audit: remove the audit freelist

[PATCH] audit: remove the audit freelist

[Florian Westphal]

allows better debugging as freeing audit buffers now always honors slub
debug hooks (e.g. object poisoning) and leak checker can detect the
free operation.

Removal also results in a small speedup (using
single rule 'iptables -A INPUT -i lo -j AUDIT --type drop'):

super_netperf 4 -H 127.0.0.1 -l 360 -t UDP_RR -- -R 1 -m 64
Before:
294953
After:
298013

(alloc/free no longer serializes on spinlock, allocator can use percpu
 pool).

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 kernel/audit.c | 53 ++++++++---------------------------------------------
 1 file changed, 8 insertions(+), 45 deletions(-)

diff --git a/kernel/audit.c b/kernel/audit.c
index f1ca11613379..396868dc523a 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -131,13 +131,6 @@ static int audit_net_id;
 /* Hash for inode-based rules */
 struct list_head audit_inode_hash[AUDIT_INODE_BUCKETS];
 
-/* The audit_freelist is a list of pre-allocated audit buffers (if more
- * than AUDIT_MAXFREE are in use, the audit buffer is freed instead of
- * being placed on the freelist). */
-static DEFINE_SPINLOCK(audit_freelist_lock);
-static int	   audit_freelist_count;
-static LIST_HEAD(audit_freelist);
-
 static struct sk_buff_head audit_skb_queue;
 /* queue of skbs to send to auditd when/if it comes back */
 static struct sk_buff_head audit_skb_hold_queue;
@@ -164,17 +157,11 @@ DEFINE_MUTEX(audit_cmd_mutex);
  * should be at least that large. */
 #define AUDIT_BUFSIZ 1024
 
-/* AUDIT_MAXFREE is the number of empty audit_buffers we keep on the
- * audit_freelist.  Doing so eliminates many kmalloc/kfree calls. */
-#define AUDIT_MAXFREE  (2*NR_CPUS)
-
-/* The audit_buffer is used when formatting an audit record.  The caller
- * locks briefly to get the record off the freelist or to allocate the
- * buffer, and locks briefly to send the buffer to the netlink layer or
+/* The audit_buffer is used when formatting an audit record.
+ * The caller locks briefly to send the buffer to the netlink layer or
  * to place it on a transmit queue.  Multiple audit_buffers can be in
  * use simultaneously. */
 struct audit_buffer {
-	struct list_head     list;
 	struct sk_buff       *skb;	/* formatted skb ready to send */
 	struct audit_context *ctx;	/* NULL or associated context */
 	gfp_t		     gfp_mask;
@@ -1247,43 +1234,22 @@ __setup("audit_backlog_limit=", audit_backlog_limit_set);
 
 static void audit_buffer_free(struct audit_buffer *ab)
 {
-	unsigned long flags;
-
 	if (!ab)
 		return;
 
 	kfree_skb(ab->skb);
-	spin_lock_irqsave(&audit_freelist_lock, flags);
-	if (audit_freelist_count > AUDIT_MAXFREE)
-		kfree(ab);
-	else {
-		audit_freelist_count++;
-		list_add(&ab->list, &audit_freelist);
-	}
-	spin_unlock_irqrestore(&audit_freelist_lock, flags);
+	kfree(ab);
 }
 
 static struct audit_buffer * audit_buffer_alloc(struct audit_context *ctx,
 						gfp_t gfp_mask, int type)
 {
-	unsigned long flags;
-	struct audit_buffer *ab = NULL;
+	struct audit_buffer *ab;
 	struct nlmsghdr *nlh;
 
-	spin_lock_irqsave(&audit_freelist_lock, flags);
-	if (!list_empty(&audit_freelist)) {
-		ab = list_entry(audit_freelist.next,
-				struct audit_buffer, list);
-		list_del(&ab->list);
-		--audit_freelist_count;
-	}
-	spin_unlock_irqrestore(&audit_freelist_lock, flags);
-
-	if (!ab) {
-		ab = kmalloc(sizeof(*ab), gfp_mask);
-		if (!ab)
-			goto err;
-	}
+	ab = kmalloc(sizeof(*ab), gfp_mask);
+	if (!ab)
+		return NULL;
 
 	ab->ctx = ctx;
 	ab->gfp_mask = gfp_mask;
@@ -1294,13 +1260,10 @@ static struct audit_buffer * audit_buffer_alloc(struct audit_context *ctx,
 
 	nlh = nlmsg_put(ab->skb, 0, 0, type, 0, 0);
 	if (!nlh)
-		goto out_kfree_skb;
+		goto err;
 
 	return ab;
 
-out_kfree_skb:
-	kfree_skb(ab->skb);
-	ab->skb = NULL;
 err:
 	audit_buffer_free(ab);
 	return NULL;
-- 
2.7.3

Re: [PATCH] audit: remove the audit freelist

[Richard Guy Briggs]

On 2016-11-15 14:16, Florian Westphal wrote:
> allows better debugging as freeing audit buffers now always honors slub
> debug hooks (e.g. object poisoning) and leak checker can detect the
> free operation.
> 
> Removal also results in a small speedup (using
> single rule 'iptables -A INPUT -i lo -j AUDIT --type drop'):
> 
> super_netperf 4 -H 127.0.0.1 -l 360 -t UDP_RR -- -R 1 -m 64
> Before:
> 294953
> After:
> 298013
> 
> (alloc/free no longer serializes on spinlock, allocator can use percpu
>  pool).
> 
> Signed-off-by: Florian Westphal <fw@strlen.de>

I've got a minor concern about a skipped skb_free below, but other than
that, I like this simplification and in particular the patch stats.  :)

> ---
>  kernel/audit.c | 53 ++++++++---------------------------------------------
>  1 file changed, 8 insertions(+), 45 deletions(-)
> 
> diff --git a/kernel/audit.c b/kernel/audit.c
> index f1ca11613379..396868dc523a 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -131,13 +131,6 @@ static int audit_net_id;
>  /* Hash for inode-based rules */
>  struct list_head audit_inode_hash[AUDIT_INODE_BUCKETS];
>  
> -/* The audit_freelist is a list of pre-allocated audit buffers (if more
> - * than AUDIT_MAXFREE are in use, the audit buffer is freed instead of
> - * being placed on the freelist). */
> -static DEFINE_SPINLOCK(audit_freelist_lock);
> -static int	   audit_freelist_count;
> -static LIST_HEAD(audit_freelist);
> -
>  static struct sk_buff_head audit_skb_queue;
>  /* queue of skbs to send to auditd when/if it comes back */
>  static struct sk_buff_head audit_skb_hold_queue;
> @@ -164,17 +157,11 @@ DEFINE_MUTEX(audit_cmd_mutex);
>   * should be at least that large. */
>  #define AUDIT_BUFSIZ 1024
>  
> -/* AUDIT_MAXFREE is the number of empty audit_buffers we keep on the
> - * audit_freelist.  Doing so eliminates many kmalloc/kfree calls. */
> -#define AUDIT_MAXFREE  (2*NR_CPUS)
> -
> -/* The audit_buffer is used when formatting an audit record.  The caller
> - * locks briefly to get the record off the freelist or to allocate the
> - * buffer, and locks briefly to send the buffer to the netlink layer or
> +/* The audit_buffer is used when formatting an audit record.
> + * The caller locks briefly to send the buffer to the netlink layer or
>   * to place it on a transmit queue.  Multiple audit_buffers can be in
>   * use simultaneously. */
>  struct audit_buffer {
> -	struct list_head     list;
>  	struct sk_buff       *skb;	/* formatted skb ready to send */
>  	struct audit_context *ctx;	/* NULL or associated context */
>  	gfp_t		     gfp_mask;
> @@ -1247,43 +1234,22 @@ __setup("audit_backlog_limit=", audit_backlog_limit_set);
>  
>  static void audit_buffer_free(struct audit_buffer *ab)
>  {
> -	unsigned long flags;
> -
>  	if (!ab)
>  		return;
>  
>  	kfree_skb(ab->skb);
> -	spin_lock_irqsave(&audit_freelist_lock, flags);
> -	if (audit_freelist_count > AUDIT_MAXFREE)
> -		kfree(ab);
> -	else {
> -		audit_freelist_count++;
> -		list_add(&ab->list, &audit_freelist);
> -	}
> -	spin_unlock_irqrestore(&audit_freelist_lock, flags);
> +	kfree(ab);
>  }
>  
>  static struct audit_buffer * audit_buffer_alloc(struct audit_context *ctx,
>  						gfp_t gfp_mask, int type)
>  {
> -	unsigned long flags;
> -	struct audit_buffer *ab = NULL;
> +	struct audit_buffer *ab;
>  	struct nlmsghdr *nlh;
>  
> -	spin_lock_irqsave(&audit_freelist_lock, flags);
> -	if (!list_empty(&audit_freelist)) {
> -		ab = list_entry(audit_freelist.next,
> -				struct audit_buffer, list);
> -		list_del(&ab->list);
> -		--audit_freelist_count;
> -	}
> -	spin_unlock_irqrestore(&audit_freelist_lock, flags);
> -
> -	if (!ab) {
> -		ab = kmalloc(sizeof(*ab), gfp_mask);
> -		if (!ab)
> -			goto err;
> -	}
> +	ab = kmalloc(sizeof(*ab), gfp_mask);
> +	if (!ab)
> +		return NULL;
>  
>  	ab->ctx = ctx;
>  	ab->gfp_mask = gfp_mask;
> @@ -1294,13 +1260,10 @@ static struct audit_buffer * audit_buffer_alloc(struct audit_context *ctx,
>  
>  	nlh = nlmsg_put(ab->skb, 0, 0, type, 0, 0);
>  	if (!nlh)
> -		goto out_kfree_skb;
> +		goto err;
>  
>  	return ab;
>  
> -out_kfree_skb:
> -	kfree_skb(ab->skb);
> -	ab->skb = NULL;

Why is the kfree_skb() skipped on error from nlmsg_put()?  I don't see
much risk in nlmsg_put() failing considering the very simple arguments,
however the code path is not trivial either.

>  err:
>  	audit_buffer_free(ab);
>  	return NULL;

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635

Re: [PATCH] audit: remove the audit freelist

[Florian Westphal]

Richard Guy Briggs <rgb@redhat.com> wrote:
> >  static void audit_buffer_free(struct audit_buffer *ab)
> >  {
> > -	unsigned long flags;
> > -
> >  	if (!ab)
> >  		return;
> >  
> >  	kfree_skb(ab->skb);
> > -	spin_lock_irqsave(&audit_freelist_lock, flags);
> > -	if (audit_freelist_count > AUDIT_MAXFREE)
> > -		kfree(ab);
> > -	else {
> > -		audit_freelist_count++;
> > -		list_add(&ab->list, &audit_freelist);
> > -	}
> > -	spin_unlock_irqrestore(&audit_freelist_lock, flags);
> > +	kfree(ab);
> >  }
[..]

> >  	nlh = nlmsg_put(ab->skb, 0, 0, type, 0, 0);
> >  	if (!nlh)
> > -		goto out_kfree_skb;
> > +		goto err;
> >  
> >  	return ab;
> >  
> > -out_kfree_skb:
> > -	kfree_skb(ab->skb);
> > -	ab->skb = NULL;
> 
> Why is the kfree_skb() skipped on error from nlmsg_put()?  I don't see
> much risk in nlmsg_put() failing considering the very simple arguments,
> however the code path is not trivial either.

if nlmsg_put fails we jump to err and ...

> >  err:
> >  	audit_buffer_free(ab);
> >  	return NULL;

... ab->skb gets free'd by audit_buffer_free() here.

Re: [PATCH] audit: remove the audit freelist

[Richard Guy Briggs]

On 2016-11-29 18:24, Florian Westphal wrote:
> Richard Guy Briggs <rgb@redhat.com> wrote:
> > >  static void audit_buffer_free(struct audit_buffer *ab)
> > >  {
> > > -	unsigned long flags;
> > > -
> > >  	if (!ab)
> > >  		return;
> > >  
> > >  	kfree_skb(ab->skb);
> > > -	spin_lock_irqsave(&audit_freelist_lock, flags);
> > > -	if (audit_freelist_count > AUDIT_MAXFREE)
> > > -		kfree(ab);
> > > -	else {
> > > -		audit_freelist_count++;
> > > -		list_add(&ab->list, &audit_freelist);
> > > -	}
> > > -	spin_unlock_irqrestore(&audit_freelist_lock, flags);
> > > +	kfree(ab);
> > >  }
> [..]
> 
> > >  	nlh = nlmsg_put(ab->skb, 0, 0, type, 0, 0);
> > >  	if (!nlh)
> > > -		goto out_kfree_skb;
> > > +		goto err;
> > >  
> > >  	return ab;
> > >  
> > > -out_kfree_skb:
> > > -	kfree_skb(ab->skb);
> > > -	ab->skb = NULL;
> > 
> > Why is the kfree_skb() skipped on error from nlmsg_put()?  I don't see
> > much risk in nlmsg_put() failing considering the very simple arguments,
> > however the code path is not trivial either.
> 
> if nlmsg_put fails we jump to err and ...
> 
> > >  err:
> > >  	audit_buffer_free(ab);
> > >  	return NULL;
> 
> ... ab->skb gets free'd by audit_buffer_free() here.

Duh, thank you!  It was already redundant in plain sight in your patch.
Sorry for the brain fart.  :)

Reviewed-by: Richard Guy Briggs <rgb@redhat.com>

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635

Re: [PATCH] audit: remove the audit freelist

[Paul Moore]

On Tue, Nov 15, 2016 at 8:16 AM, Florian Westphal <fw@strlen.de> wrote:
> allows better debugging as freeing audit buffers now always honors slub
> debug hooks (e.g. object poisoning) and leak checker can detect the
> free operation.
>
> Removal also results in a small speedup (using
> single rule 'iptables -A INPUT -i lo -j AUDIT --type drop'):
>
> super_netperf 4 -H 127.0.0.1 -l 360 -t UDP_RR -- -R 1 -m 64
> Before:
> 294953
> After:
> 298013
>
> (alloc/free no longer serializes on spinlock, allocator can use percpu
>  pool).
>
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  kernel/audit.c | 53 ++++++++---------------------------------------------
>  1 file changed, 8 insertions(+), 45 deletions(-)

Sorry for the delay, I was hoping to have some time to play around
with this and offer a more meaningful comment ... I've often wondered
about converting audit_buffer, and audit_context for that matter, over
to their own kmem_cache; have you considered that?  Or was this
proposed due to simplicity?

-- 
paul moore
www.paul-moore.com

Re: [PATCH] audit: remove the audit freelist

[Florian Westphal]

Paul Moore <paul@paul-moore.com> wrote:
> On Tue, Nov 15, 2016 at 8:16 AM, Florian Westphal <fw@strlen.de> wrote:
> > allows better debugging as freeing audit buffers now always honors slub
> > debug hooks (e.g. object poisoning) and leak checker can detect the
> > free operation.
> >
> > Removal also results in a small speedup (using
> > single rule 'iptables -A INPUT -i lo -j AUDIT --type drop'):
> >
> > super_netperf 4 -H 127.0.0.1 -l 360 -t UDP_RR -- -R 1 -m 64
> > Before:
> > 294953
> > After:
> > 298013
> >
> > (alloc/free no longer serializes on spinlock, allocator can use percpu
> >  pool).
> >
> > Signed-off-by: Florian Westphal <fw@strlen.de>
> > ---
> >  kernel/audit.c | 53 ++++++++---------------------------------------------
> >  1 file changed, 8 insertions(+), 45 deletions(-)
> 
> Sorry for the delay, I was hoping to have some time to play around
> with this and offer a more meaningful comment ... I've often wondered
> about converting audit_buffer, and audit_context for that matter, over
> to their own kmem_cache; have you considered that?  Or was this
> proposed due to simplicity?

Not sure I understand, you could still convert it on top of this.
(Although audit_buffer is just 24 bytes after this patch so it will
 come from 32byte kmalloc slab).

I don't think it makes sense to keep this DIY cache on top of slub
cache. 

Re: [PATCH] audit: remove the audit freelist

[Paul Moore]

On Wed, Nov 30, 2016 at 8:44 PM, Florian Westphal <fw@strlen.de> wrote:
> Paul Moore <paul@paul-moore.com> wrote:
>> On Tue, Nov 15, 2016 at 8:16 AM, Florian Westphal <fw@strlen.de> wrote:
>> > allows better debugging as freeing audit buffers now always honors slub
>> > debug hooks (e.g. object poisoning) and leak checker can detect the
>> > free operation.
>> >
>> > Removal also results in a small speedup (using
>> > single rule 'iptables -A INPUT -i lo -j AUDIT --type drop'):
>> >
>> > super_netperf 4 -H 127.0.0.1 -l 360 -t UDP_RR -- -R 1 -m 64
>> > Before:
>> > 294953
>> > After:
>> > 298013
>> >
>> > (alloc/free no longer serializes on spinlock, allocator can use percpu
>> >  pool).
>> >
>> > Signed-off-by: Florian Westphal <fw@strlen.de>
>> > ---
>> >  kernel/audit.c | 53 ++++++++---------------------------------------------
>> >  1 file changed, 8 insertions(+), 45 deletions(-)
>>
>> Sorry for the delay, I was hoping to have some time to play around
>> with this and offer a more meaningful comment ... I've often wondered
>> about converting audit_buffer, and audit_context for that matter, over
>> to their own kmem_cache; have you considered that?  Or was this
>> proposed due to simplicity?
>
> Not sure I understand, you could still convert it on top of this.
> (Although audit_buffer is just 24 bytes after this patch so it will
>  come from 32byte kmalloc slab).

I'm not arguing against this patch, partly just musing out loud,
partly just seeing if you had experimented with creating a
audit_buffer specific kmem_cache (I'm guessing the answer here is
"no").  If we do convert to a kmem_cache this patch would be the
obvious first step.  I'd also want to cobble together some tests we
can use to measure performance.  Using netperf is good, but I'd also
like to exercise the syscall records as it is probably easier to
isolate the audit subsystem that way.

> I don't think it makes sense to keep this DIY cache on top of slub
> cache.

I agree, there probably isn't much sense in keeping this around.  In
case you're interested, I started tracking this on GitHub at the link
below:

 * https://github.com/linux-audit/audit-kernel/issues/29

-- 
paul moore
www.paul-moore.com

Re: [PATCH] audit: remove the audit freelist

[Florian Westphal]

Paul Moore <paul@paul-moore.com> wrote:
> On Wed, Nov 30, 2016 at 8:44 PM, Florian Westphal <fw@strlen.de> wrote:
> > Paul Moore <paul@paul-moore.com> wrote:
> >> On Tue, Nov 15, 2016 at 8:16 AM, Florian Westphal <fw@strlen.de> wrote:
> >> > allows better debugging as freeing audit buffers now always honors slub
> >> > debug hooks (e.g. object poisoning) and leak checker can detect the
> >> > free operation.
> >> >
> >> > Removal also results in a small speedup (using
> >> > single rule 'iptables -A INPUT -i lo -j AUDIT --type drop'):
> >> >
> >> > super_netperf 4 -H 127.0.0.1 -l 360 -t UDP_RR -- -R 1 -m 64
> >> > Before:
> >> > 294953
> >> > After:
> >> > 298013
> >> >
> >> > (alloc/free no longer serializes on spinlock, allocator can use percpu
> >> >  pool).
> >> >
> >> > Signed-off-by: Florian Westphal <fw@strlen.de>
> >> > ---
> >> >  kernel/audit.c | 53 ++++++++---------------------------------------------
> >> >  1 file changed, 8 insertions(+), 45 deletions(-)
> >>
> >> Sorry for the delay, I was hoping to have some time to play around
> >> with this and offer a more meaningful comment ... I've often wondered
> >> about converting audit_buffer, and audit_context for that matter, over
> >> to their own kmem_cache; have you considered that?  Or was this
> >> proposed due to simplicity?
> >
> > Not sure I understand, you could still convert it on top of this.
> > (Although audit_buffer is just 24 bytes after this patch so it will
> >  come from 32byte kmalloc slab).
> 
> I'm not arguing against this patch, partly just musing out loud,
> partly just seeing if you had experimented with creating a
> audit_buffer specific kmem_cache (I'm guessing the answer here is
> "no").  If we do convert to a kmem_cache this patch would be the
> obvious first step.

It does convert to a kmem_cache, indirectly.

kmalloc() uses builtin_constant_size() magic to resolve the kmalloc to
kmem_cache_alloc, using the precreated kmalloc_caches[] in slab_common.c .

Re: [PATCH] audit: remove the audit freelist

[Paul Moore]

On Thu, Dec 1, 2016 at 7:09 PM, Florian Westphal <fw@strlen.de> wrote:
> Paul Moore <paul@paul-moore.com> wrote:
>> On Wed, Nov 30, 2016 at 8:44 PM, Florian Westphal <fw@strlen.de> wrote:
>> > Paul Moore <paul@paul-moore.com> wrote:
>> >> On Tue, Nov 15, 2016 at 8:16 AM, Florian Westphal <fw@strlen.de> wrote:
>> >> > allows better debugging as freeing audit buffers now always honors slub
>> >> > debug hooks (e.g. object poisoning) and leak checker can detect the
>> >> > free operation.
>> >> >
>> >> > Removal also results in a small speedup (using
>> >> > single rule 'iptables -A INPUT -i lo -j AUDIT --type drop'):
>> >> >
>> >> > super_netperf 4 -H 127.0.0.1 -l 360 -t UDP_RR -- -R 1 -m 64
>> >> > Before:
>> >> > 294953
>> >> > After:
>> >> > 298013
>> >> >
>> >> > (alloc/free no longer serializes on spinlock, allocator can use percpu
>> >> >  pool).
>> >> >
>> >> > Signed-off-by: Florian Westphal <fw@strlen.de>
>> >> > ---
>> >> >  kernel/audit.c | 53 ++++++++---------------------------------------------
>> >> >  1 file changed, 8 insertions(+), 45 deletions(-)
>> >>
>> >> Sorry for the delay, I was hoping to have some time to play around
>> >> with this and offer a more meaningful comment ... I've often wondered
>> >> about converting audit_buffer, and audit_context for that matter, over
>> >> to their own kmem_cache; have you considered that?  Or was this
>> >> proposed due to simplicity?
>> >
>> > Not sure I understand, you could still convert it on top of this.
>> > (Although audit_buffer is just 24 bytes after this patch so it will
>> >  come from 32byte kmalloc slab).
>>
>> I'm not arguing against this patch, partly just musing out loud,
>> partly just seeing if you had experimented with creating a
>> audit_buffer specific kmem_cache (I'm guessing the answer here is
>> "no").  If we do convert to a kmem_cache this patch would be the
>> obvious first step.
>
> It does convert to a kmem_cache, indirectly.
>
> kmalloc() uses builtin_constant_size() magic to resolve the kmalloc to
> kmem_cache_alloc, using the precreated kmalloc_caches[] in slab_common.c .

Yes, understood, I just think there may be some advantages (tracking,
etc.) to using a dedicated audit_buffer kmem_cache rather than the
system wide bucket.

-- 
paul moore
www.paul-moore.com