Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1532459 > unrolled thread

Re: [PATCH] audit: remove the audit freelist

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] audit: remove the audit freelist Richard Guy Briggs <rgb@redhat.com> - 2016-11-29 17:20 +0100
    Re: [PATCH] audit: remove the audit freelist Florian Westphal <fw@strlen.de> - 2016-11-29 18:30 +0100
      Re: [PATCH] audit: remove the audit freelist Richard Guy Briggs <rgb@redhat.com> - 2016-11-30 05:50 +0100

#1532459 — Re: [PATCH] audit: remove the audit freelist

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

[toc] | [next] | [standalone]

#1532598

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.

[toc] | [prev] | [next] | [standalone]

#1532949

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web