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

[John Johansen <john.johansen@canonical.com>]

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;
>   }