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

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

On 2017-04-11 16:07, Paul Moore wrote:
> On Mon, Apr 10, 2017 at 12:04 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > 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?
> 
> Check my math, but in the patched code if the nlmsg_put() call fails
> then we jump to "err" which calls audit_buffer_free() which in turn
> calls kfree_skb() on ab->skb so I don't believe we have a memory leak
> on error ... I'll hold off on merging this in case I'm missing
> something, but I'm pretty sure we're okay here.

Ok, yes, you're right.  This is ringing a bell...  I think there was
another place recently that the extra free_skb() was dropped and I had
missed audit_buffer_free() doing the right thing then.

> > Otherwise, I like the intent of this simplification.

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

> >> +     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
> 
> paul moore

- 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