[PATCH] apparmor: try to avoid refing the label in apparmor_file_open

[PATCH] apparmor: try to avoid refing the label in apparmor_file_open

[Mateusz Guzik]

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@gmail.com>
---

I think this is a worthwhile touch up regardless of what happens with
label refcouting in the long run. It does not of course does not fully
fix the problem.

I concede the naming is not consistent with other stuff in the file and
I'm not going to argue about it -- happy to name it whatever as long as
the problem is sorted out.

Am I missing something which makes the approach below not work to begin
with?

 security/apparmor/include/cred.h | 20 ++++++++++++++++++++
 security/apparmor/lsm.c          |  5 +++--
 2 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/security/apparmor/include/cred.h b/security/apparmor/include/cred.h
index 58fdc72af664..7265d2f81dd5 100644
--- a/security/apparmor/include/cred.h
+++ b/security/apparmor/include/cred.h
@@ -63,6 +63,26 @@ static inline struct aa_label *aa_get_newest_cred_label(const struct cred *cred)
 	return aa_get_newest_label(aa_cred_raw_label(cred));
 }
 
+static inline struct aa_label *aa_get_newest_cred_label_condref(const struct cred *cred,
+								bool *needput)
+{
+	struct aa_label *l = aa_cred_raw_label(cred);
+
+	if (unlikely(label_is_stale(l))) {
+		*needput = true;
+		return aa_get_newest_label(l);
+	}
+
+	*needput = false;
+	return l;
+}
+
+static inline void aa_put_label_condref(struct aa_label *l, bool needput)
+{
+	if (unlikely(needput))
+		aa_put_label(l);
+}
+
 /**
  * aa_current_raw_label - find the current tasks confining label
  *
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index 2cea34657a47..4bf87eac4a56 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -461,6 +461,7 @@ static int apparmor_file_open(struct file *file)
 	struct aa_file_ctx *fctx = file_ctx(file);
 	struct aa_label *label;
 	int error = 0;
+	bool needput;
 
 	if (!path_mediated_fs(file->f_path.dentry))
 		return 0;
@@ -477,7 +478,7 @@ static int apparmor_file_open(struct file *file)
 		return 0;
 	}
 
-	label = aa_get_newest_cred_label(file->f_cred);
+	label = aa_get_newest_cred_label_condref(file->f_cred, &needput);
 	if (!unconfined(label)) {
 		struct mnt_idmap *idmap = file_mnt_idmap(file);
 		struct inode *inode = file_inode(file);
@@ -494,7 +495,7 @@ static int apparmor_file_open(struct file *file)
 		/* todo cache full allowed permissions set and state */
 		fctx->allow = aa_map_file_to_perms(file);
 	}
-	aa_put_label(label);
+	aa_put_label_condref(label, needput);
 
 	return error;
 }
-- 
2.43.0

Re: [PATCH] apparmor: try to avoid refing the label in apparmor_file_open

[John Johansen]

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@gmail.com>
can you cleanup the commit message and I will pull this in

> ---
> 
> I think this is a worthwhile touch up regardless of what happens with
> label refcouting in the long run. It does not of course does not fully
> fix the problem.
> 
I have no objections to incremental improvements.

> I concede the naming is not consistent with other stuff in the file and
> I'm not going to argue about it -- happy to name it whatever as long as
> the problem is sorted out.
> 
its fine, we could use crit_section here like with the current_label but
I don't think we really gain anything by doing so.

> 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.



>   security/apparmor/include/cred.h | 20 ++++++++++++++++++++
>   security/apparmor/lsm.c          |  5 +++--
>   2 files changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/security/apparmor/include/cred.h b/security/apparmor/include/cred.h
> index 58fdc72af664..7265d2f81dd5 100644
> --- a/security/apparmor/include/cred.h
> +++ b/security/apparmor/include/cred.h
> @@ -63,6 +63,26 @@ static inline struct aa_label *aa_get_newest_cred_label(const struct cred *cred)
>   	return aa_get_newest_label(aa_cred_raw_label(cred));
>   }
>   
> +static inline struct aa_label *aa_get_newest_cred_label_condref(const struct cred *cred,
> +								bool *needput)
> +{
> +	struct aa_label *l = aa_cred_raw_label(cred);
> +
> +	if (unlikely(label_is_stale(l))) {
> +		*needput = true;
> +		return aa_get_newest_label(l);
> +	}
> +
> +	*needput = false;
> +	return l;
> +}
> +
> +static inline void aa_put_label_condref(struct aa_label *l, bool needput)
> +{
> +	if (unlikely(needput))
> +		aa_put_label(l);
> +}
> +
>   /**
>    * aa_current_raw_label - find the current tasks confining label
>    *
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index 2cea34657a47..4bf87eac4a56 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -461,6 +461,7 @@ static int apparmor_file_open(struct file *file)
>   	struct aa_file_ctx *fctx = file_ctx(file);
>   	struct aa_label *label;
>   	int error = 0;
> +	bool needput;
>   
>   	if (!path_mediated_fs(file->f_path.dentry))
>   		return 0;
> @@ -477,7 +478,7 @@ static int apparmor_file_open(struct file *file)
>   		return 0;
>   	}
>   
> -	label = aa_get_newest_cred_label(file->f_cred);
> +	label = aa_get_newest_cred_label_condref(file->f_cred, &needput);
>   	if (!unconfined(label)) {
>   		struct mnt_idmap *idmap = file_mnt_idmap(file);
>   		struct inode *inode = file_inode(file);
> @@ -494,7 +495,7 @@ static int apparmor_file_open(struct file *file)
>   		/* todo cache full allowed permissions set and state */
>   		fctx->allow = aa_map_file_to_perms(file);
>   	}
> -	aa_put_label(label);
> +	aa_put_label_condref(label, needput);
>   
>   	return error;
>   }

Re: [PATCH] apparmor: try to avoid refing the label in apparmor_file_open

[Mateusz Guzik]

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@gmail.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.

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.

Re: [PATCH] apparmor: try to avoid refing the label in apparmor_file_open

[John Johansen]

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@gmail.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.