linux-kernel - Re: [PATCH] apparmor: try to avoid refing the label in apparmor_file

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <c9f55357-029b-4799-8072-f5c96216d60c@canonical.com>
Date: Thu, 20 Jun 2024 10:08:58 -0700
From: John Johansen <john.johansen@...onical.com>
To: Mateusz Guzik <mjguzik@...il.com>
Cc: paul@...l-moore.com, jmorris@...ei.org, serge@...lyn.com,
 linux-kernel@...r.kernel.org, apparmor@...ts.ubuntu.com,
 linux-security-module@...r.kernel.org, Neeraj.Upadhyay@....com
Subject: Re: [PATCH] apparmor: try to avoid refing the label in
 apparmor_file_open

On 6/20/24 09:41, Mateusz Guzik wrote:
> On Thu, Jun 20, 2024 at 09:26:00AM -0700, John Johansen wrote:
>> On 6/20/24 06:15, Mateusz Guzik wrote:
>>> It can be done in the common case.
>>>> A 24-way open1_processes from will-it-scale (separate file open) shows:
>>>     29.37%  [kernel]           [k] apparmor_file_open
>>>     26.84%  [kernel]           [k] apparmor_file_alloc_security
>>>     26.62%  [kernel]           [k] apparmor_file_free_security
>>>      1.32%  [kernel]           [k] clear_bhb_loop
>>>
>>> apparmor_file_open is eliminated from the profile with the patch.
>>>
>>> Throughput (ops/s):
>>> before:	6092196
>>> after:	8309726 (+36%)
>>>
>>> Signed-off-by: Mateusz Guzik <mjguzik@...il.com>
>> can you cleanup the commit message and I will pull this in
>>
> 
> First of all thanks for a timely review.
> 
> I thought that's a decent commit message though. ;)
> 
> Would something like this work:
> <cm>
> apparmor: try to avoid refing the label in apparmor_file_open
> 
> In the common case it can be avoided, which in turn reduces the
> performance impact apparmor on parallel open() invocations.
> 
> When benchmarking on 24-core vm using will-it-scale's open1_process
> ("Separate file open"), the results are (ops/s):
> before: 6092196
> after:  8309726 (+36%)
> </cm>
> 
> If this is fine I'll send a v2.
> 
it will do, largely, I was just looking for something that explains
a little more than. "It can be done in the common case"


> If you are looking for something fundamentally different I would say it
> will be the fastest if you write your own commit message while borrowing
> the numbers and denoting all the wording is yours. I'm trying to reduce
> back and forth over email here.
> 
>>> Am I missing something which makes the approach below not work to begin
>>> with?
>>>
>> no this will work in the short term. Long term there is work that will
>> break this. Both replacing unconfined and the object delegation work
>> will cause a performance regression as I am not sure we will be able
>> to conditionally get the label but that is something for those patch
>> series to work out. My biggest concern being people objecting to necessary
>> changes that regress performance, if it can't be worked out, but
>> that really isn't a reason to stop this now.
>>
> 
> hrm. I was looking at going a step further, now I'm going to have to
> poke around.