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

On Mon, Apr 10, 2017 at 12:04 AM, Richard Guy Briggs <rgb(a)redhat.com&gt; wrote: > 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? 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.

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