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

On 2017-03-21 14:59, Paul Moore wrote: > From: Paul Moore <paul(a)paul-moore.com&gt; > 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(a)strlen.de&gt; > Signed-off-by: Paul Moore <paul(a)paul-moore.com&gt; > --- > 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(a)redhat.com&gt; 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 -- Linux-audit mailing list Linux-audit(a)redhat.com https://www.redhat.com/mailman/listinfo/linux-audit