linux-kernel - Re: [PATCH v2] 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <155a24f7-8059-49b0-93fa-94bcdc058621@amd.com>
Date: Thu, 20 Jun 2024 23:53:12 +0530
From: Neeraj Upadhyay <Neeraj.Upadhyay@....com>
To: Mateusz Guzik <mjguzik@...il.com>, john.johansen@...onical.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
Subject: Re: [PATCH v2] apparmor: try to avoid refing the label in
 apparmor_file_open



On 6/20/2024 10:45 PM, Mateusz Guzik wrote:
> apparmor: try to avoid refing the label in apparmor_file_open
> 
> If the label is not stale (which is the common case), the fact that the
> passed file object holds a reference can be leverged to avoid the

Minor: Typo 'leveraged'

> ref/unref cycle. Doing so reduces performance impact of apparmor on
> parallel open() invocations.
> 
> When benchmarking on a 24-core vm using will-it-scale's open1_process
> ("Separate file open"), the results are (ops/s):
> before: 6092196
> after:  8309726 (+36%)
> 
> Signed-off-by: Mateusz Guzik <mjguzik@...il.com>
> ---


Trying to understand the changes done here. So, while the file cred can be updated
async to the task (referring to the comment from John here [1]), the file cred label
cannot change during apparmor_file_open() execution?


Reviewed-by: Neeraj upadhyay <Neeraj.Upadhyay@....com>


Thanks
Neeraj

[1] https://lore.kernel.org/lkml/9bfaeec2-535d-4401-8244-7560f660a065@canonical.com/


> 
> v2:
> - reword the commit message
> 
> If you want any changes made to it can you just do them on your own
> accord? :) Will be faster for both of us than another mail trip.
> 
>  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;
>  }