| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20161129161233.GG6897@madcap2.tricolour.ca>
Date: Tue, 29 Nov 2016 11:12:33 -0500
From: Richard Guy Briggs <rgb@...hat.com>
To: Florian Westphal <fw@...len.de>
Cc: linux-kernel@...r.kernel.org, linux-audit@...hat.com
Subject: Re: [PATCH] audit: remove the audit freelist
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 <fw@...len.de>
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 <rgb@...hat.com>
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635
Powered by blists - more mailing lists