Patchwork [09/11] AppArmor: address performance regression of replaced profile

login
register
mail settings
Submitter John Johansen
Date April 13, 2010, 7:09 a.m.
Message ID <1271142580-26555-10-git-send-email-john.johansen@canonical.com>
Download mbox | patch
Permalink /patch/50043/
State Accepted
Delegated to: Andy Whitcroft
Headers show

Comments

John Johansen - April 13, 2010, 7:09 a.m.
From: John Johansen <john.johansen@canonical.com>

OriginalAuthor: John Johansen <john.johansen@canonical.com>
OriginalLocation: git://git.kernel.org/pub/scm/linux/kernel/git/jj/linux-apparm$
commit: 5b2ed5984571ca59570240f505dc90810bb56842
BugLink: http://bugs.launchpad.net/bugs/549428

If a file has been opened under an old version of a profile (one that
has been replaced) it is labeled with the original profile and the
labeling is used to avoid performing revalidation (name lookup + permission
check) on every file access.

Replacement changes the profile pointer so that the labeling check fails
and revalidation must be performed.  This can cause a performance regression
that is noticable on files that are accessed frequently.

Make sure to get the newest version of the cached file profile before
comparing to current confinement profile.

Also, the permissions that were granted on open were not being stored
in the file->cxt forcing a revalidation because the check to avoid
revalidation also checks that the requested permissions are a subset of
cached granted permissions.

So Ensure that the granted permissions are stored.

Signed-off-by: John Johansen <john.johansen@canonical.com>
---
 security/apparmor/lsm.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
Andy Whitcroft - April 13, 2010, 9:19 a.m.
On Tue, Apr 13, 2010 at 12:09:38AM -0700, john.johansen@canonical.com wrote:
> From: John Johansen <john.johansen@canonical.com>
> 
> OriginalAuthor: John Johansen <john.johansen@canonical.com>
> OriginalLocation: git://git.kernel.org/pub/scm/linux/kernel/git/jj/linux-apparm$
> commit: 5b2ed5984571ca59570240f505dc90810bb56842
> BugLink: http://bugs.launchpad.net/bugs/549428
> 
> If a file has been opened under an old version of a profile (one that
> has been replaced) it is labeled with the original profile and the
> labeling is used to avoid performing revalidation (name lookup + permission
> check) on every file access.
> 
> Replacement changes the profile pointer so that the labeling check fails
> and revalidation must be performed.  This can cause a performance regression
> that is noticable on files that are accessed frequently.
> 
> Make sure to get the newest version of the cached file profile before
> comparing to current confinement profile.
> 
> Also, the permissions that were granted on open were not being stored
> in the file->cxt forcing a revalidation because the check to avoid
> revalidation also checks that the requested permissions are a subset of
> cached granted permissions.
> 
> So Ensure that the granted permissions are stored.
> 
> Signed-off-by: John Johansen <john.johansen@canonical.com>
> ---
>  security/apparmor/lsm.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index d1c1be0..56509ce 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -419,7 +419,7 @@ static int apparmor_dentry_open(struct file *file, const struct cred *cred)
>  		/* released by aa_free_file_context */
>  		fcxt->profile = aa_get_profile(profile);
>  		/* todo cache actual allowed permissions */
> -		fcxt->allowed = 0;
> +		fcxt->allowed = aa_map_file_to_perms(file);
>  	}
>  
>  	return error;
> @@ -448,7 +448,7 @@ static int apparmor_file_permission(struct file *file, int mask)
>  	 * TODO: cache profiles that have revalidated?
>  	 */
>  	struct aa_file_cxt *fcxt = file->f_security;
> -	struct aa_profile *profile, *fprofile = fcxt->profile;
> +	struct aa_profile *profile, *fprofile = aa_newest_version(fcxt->profile);
>  	int error = 0;
>  
>  	if (!fprofile || !file->f_path.mnt ||

Ok.  Seems to do what is claimed:

Acked-by: Andy Whitcroft <apw@canonical.com>

-apw

Patch

diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index d1c1be0..56509ce 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -419,7 +419,7 @@  static int apparmor_dentry_open(struct file *file, const struct cred *cred)
 		/* released by aa_free_file_context */
 		fcxt->profile = aa_get_profile(profile);
 		/* todo cache actual allowed permissions */
-		fcxt->allowed = 0;
+		fcxt->allowed = aa_map_file_to_perms(file);
 	}
 
 	return error;
@@ -448,7 +448,7 @@  static int apparmor_file_permission(struct file *file, int mask)
 	 * TODO: cache profiles that have revalidated?
 	 */
 	struct aa_file_cxt *fcxt = file->f_security;
-	struct aa_profile *profile, *fprofile = fcxt->profile;
+	struct aa_profile *profile, *fprofile = aa_newest_version(fcxt->profile);
 	int error = 0;
 
 	if (!fprofile || !file->f_path.mnt ||