From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753768AbcK2QMq (ORCPT ); Tue, 29 Nov 2016 11:12:46 -0500 Received: from mx1.redhat.com ([209.132.183.28]:41354 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751072AbcK2QMh (ORCPT ); Tue, 29 Nov 2016 11:12:37 -0500 Date: Tue, 29 Nov 2016 11:12:33 -0500 From: Richard Guy Briggs To: Florian Westphal Cc: linux-kernel@vger.kernel.org, linux-audit@redhat.com Subject: Re: [PATCH] audit: remove the audit freelist Message-ID: <20161129161233.GG6897@madcap2.tricolour.ca> References: <1479215774-29810-1-git-send-email-fw@strlen.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1479215774-29810-1-git-send-email-fw@strlen.de> User-Agent: Mutt/1.5.21 (2010-09-15) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.26]); Tue, 29 Nov 2016 16:12:36 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 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 Kernel Security Engineering, Base Operating Systems, Red Hat Remote, Ottawa, Canada Voice: +1.647.777.2635, Internal: (81) 32635