On Wed, Apr 12, 2017 at 12:59 AM, Richard Guy Briggs <r...@redhat.com> wrote:
> On 2017-04-11 16:07, Paul Moore wrote:
>> On Mon, Apr 10, 2017 at 12:04 AM, Richard Guy Briggs <r...@redhat.com> wrote:
>> > 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?
>>
>> 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.
>
> Ok, yes, you're right. This is ringing a bell... I think there was
> another place recently that the extra free_skb() was dropped and I had
> missed audit_buffer_free() doing the right thing then.
>
>> > Otherwise, I like the intent of this simplification.
>
> Looks good,
> Reviewed-by: Richard Guy Briggs <r...@redhat.com>
Merged.
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
