On 2017-03-21 14:59, Paul Moore wrote:
> From: Paul Moore <p...@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 <f...@strlen.de>
> Signed-off-by: Paul Moore <p...@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?
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 <r...@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
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
