Re: [RFC PATCH 4/4] audit: use kmem_cache to manage the audit_buffer cache

[Richard Guy Briggs <rgb@redhat.com>]

On 2017-03-21 14:59, Paul Moore wrote:
> From: Paul Moore <paul@paul-moore.com>
> The audit subsystem implemented its own buffer cache mechanism which
> is a bit silly these days when we could use the kmem_cache construct.
> 
> Some credit is due to Florian Westphal for originally proposing that
> we remove the audit cache implementation in favor of simple
> kmalloc()/kfree() calls, but I would rather have a dedicated slab
> cache to ease debugging and future stats/performance work.
> 
> Cc: Florian Westphal <fw@strlen.de>
> Signed-off-by: Paul Moore <paul@paul-moore.com>
> ---
>  kernel/audit.c |   66 ++++++++++++++------------------------------------------
>  1 file changed, 17 insertions(+), 49 deletions(-)
> 
> diff --git a/kernel/audit.c b/kernel/audit.c
> index b718bf3a73f8..f78cdd75a4d2 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -59,6 +59,7 @@
>  #include <linux/mutex.h>
>  #include <linux/gfp.h>
>  #include <linux/pid.h>
> +#include <linux/slab.h>
>  
>  #include <linux/audit.h>
>  
> @@ -152,12 +153,7 @@ static atomic_t	audit_lost = ATOMIC_INIT(0);
>  /* 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 kmem_cache *audit_buffer_cache;
>  
>  /* queue msgs to send via kauditd_task */
>  static struct sk_buff_head audit_queue;
> @@ -193,17 +189,12 @@ 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
>   * 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;
> @@ -1489,6 +1480,10 @@ static int __init audit_init(void)
>  	if (audit_initialized == AUDIT_DISABLED)
>  		return 0;
>  
> +	audit_buffer_cache = kmem_cache_create("audit_buffer",
> +					       sizeof(struct audit_buffer),
> +					       0, SLAB_PANIC, NULL);
> +
>  	memset(&auditd_conn, 0, sizeof(auditd_conn));
>  	spin_lock_init(&auditd_conn.lock);
>  
> @@ -1557,60 +1552,33 @@ __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);
> +	kmem_cache_free(audit_buffer_cache, ab);
>  }
>  
> -static struct audit_buffer * audit_buffer_alloc(struct audit_context *ctx,
> -						gfp_t gfp_mask, int type)
> +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 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;
> -	}
> +	struct audit_buffer *ab;
>  
> -	ab->ctx = ctx;
> -	ab->gfp_mask = gfp_mask;
> +	ab = kmem_cache_alloc(audit_buffer_cache, gfp_mask);
> +	if (!ab)
> +		return NULL;
>  
>  	ab->skb = nlmsg_new(AUDIT_BUFSIZ, gfp_mask);
>  	if (!ab->skb)
>  		goto err;
> +	if (!nlmsg_put(ab->skb, 0, 0, type, 0, 0))
> +		goto err;
>  
> -	nlh = nlmsg_put(ab->skb, 0, 0, type, 0, 0);
> -	if (!nlh)
> -		goto out_kfree_skb;

Is there a reason to care about an error returned from nlmsg_put() if
you aren't going to free the skb that was allocated?  If you think
nlmsg_put() can't fail due to extremely simple calling arguments then
there is no need to check its return code.

If nlmsg_new() succeeds, it has allocated an skb.  If nlmsg_put() fails,
you free the audit_buffer and the skb is now a memory leak.

Have I read this correctly?

Otherwise, I like the intent of this simplification.

> +	ab->ctx = ctx;
> +	ab->gfp_mask = gfp_mask;
>  
>  	return ab;
>  
> -out_kfree_skb:
> -	kfree_skb(ab->skb);
> -	ab->skb = NULL;
>  err:
>  	audit_buffer_free(ab);
>  	return NULL;

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635