Re: [PATCH] audit: remove the audit freelist

From: Paul Moore
Date: Thu Dec 01 2016 - 19:03:04 EST


On Wed, Nov 30, 2016 at 8:44 PM, Florian Westphal <fw@xxxxxxxxx> wrote:
> Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
>> On Tue, Nov 15, 2016 at 8:16 AM, Florian Westphal <fw@xxxxxxxxx> 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@xxxxxxxxx>
>> > ---
>> > kernel/audit.c | 53 ++++++++---------------------------------------------
>> > 1 file changed, 8 insertions(+), 45 deletions(-)
>>
>> Sorry for the delay, I was hoping to have some time to play around
>> with this and offer a more meaningful comment ... I've often wondered
>> about converting audit_buffer, and audit_context for that matter, over
>> to their own kmem_cache; have you considered that? Or was this
>> proposed due to simplicity?
>
> Not sure I understand, you could still convert it on top of this.
> (Although audit_buffer is just 24 bytes after this patch so it will
> come from 32byte kmalloc slab).

I'm not arguing against this patch, partly just musing out loud,
partly just seeing if you had experimented with creating a
audit_buffer specific kmem_cache (I'm guessing the answer here is
"no"). If we do convert to a kmem_cache this patch would be the
obvious first step. I'd also want to cobble together some tests we
can use to measure performance. Using netperf is good, but I'd also
like to exercise the syscall records as it is probably easier to
isolate the audit subsystem that way.

> I don't think it makes sense to keep this DIY cache on top of slub
> cache.

I agree, there probably isn't much sense in keeping this around. In
case you're interested, I started tracking this on GitHub at the link
below:

* https://github.com/linux-audit/audit-kernel/issues/29

--
paul moore
www.paul-moore.com