Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1533694 > unrolled thread

Re: [PATCH] audit: remove the audit freelist

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] audit: remove the audit freelist Paul Moore <paul@paul-moore.com> - 2016-12-01 01:10 +0100
    Re: [PATCH] audit: remove the audit freelist Florian Westphal <fw@strlen.de> - 2016-12-01 02:50 +0100
      Re: [PATCH] audit: remove the audit freelist Paul Moore <paul@paul-moore.com> - 2016-12-02 01:10 +0100
        Re: [PATCH] audit: remove the audit freelist Florian Westphal <fw@strlen.de> - 2016-12-02 01:20 +0100
          Re: [PATCH] audit: remove the audit freelist Paul Moore <paul@paul-moore.com> - 2016-12-02 23:00 +0100

#1533694 — Re: [PATCH] audit: remove the audit freelist

On Tue, Nov 15, 2016 at 8:16 AM, Florian Westphal <fw@strlen.de> 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@strlen.de>
> ---
>  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?

-- 
paul moore
www.paul-moore.com

[toc] | [next] | [standalone]

#1533740

Paul Moore <paul@paul-moore.com> wrote:
> On Tue, Nov 15, 2016 at 8:16 AM, Florian Westphal <fw@strlen.de> 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@strlen.de>
> > ---
> >  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 don't think it makes sense to keep this DIY cache on top of slub
cache. 

[toc] | [prev] | [next] | [standalone]

#1534561

On Wed, Nov 30, 2016 at 8:44 PM, Florian Westphal <fw@strlen.de> wrote:
> Paul Moore <paul@paul-moore.com> wrote:
>> On Tue, Nov 15, 2016 at 8:16 AM, Florian Westphal <fw@strlen.de> 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@strlen.de>
>> > ---
>> >  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

[toc] | [prev] | [next] | [standalone]

#1534562

Paul Moore <paul@paul-moore.com> wrote:
> On Wed, Nov 30, 2016 at 8:44 PM, Florian Westphal <fw@strlen.de> wrote:
> > Paul Moore <paul@paul-moore.com> wrote:
> >> On Tue, Nov 15, 2016 at 8:16 AM, Florian Westphal <fw@strlen.de> 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@strlen.de>
> >> > ---
> >> >  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.

It does convert to a kmem_cache, indirectly.

kmalloc() uses builtin_constant_size() magic to resolve the kmalloc to
kmem_cache_alloc, using the precreated kmalloc_caches[] in slab_common.c .

[toc] | [prev] | [next] | [standalone]

#1535247

On Thu, Dec 1, 2016 at 7:09 PM, Florian Westphal <fw@strlen.de> wrote:
> Paul Moore <paul@paul-moore.com> wrote:
>> On Wed, Nov 30, 2016 at 8:44 PM, Florian Westphal <fw@strlen.de> wrote:
>> > Paul Moore <paul@paul-moore.com> wrote:
>> >> On Tue, Nov 15, 2016 at 8:16 AM, Florian Westphal <fw@strlen.de> 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@strlen.de>
>> >> > ---
>> >> >  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.
>
> It does convert to a kmem_cache, indirectly.
>
> kmalloc() uses builtin_constant_size() magic to resolve the kmalloc to
> kmem_cache_alloc, using the precreated kmalloc_caches[] in slab_common.c .

Yes, understood, I just think there may be some advantages (tracking,
etc.) to using a dedicated audit_buffer kmem_cache rather than the
system wide bucket.

-- 
paul moore
www.paul-moore.com

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web