Re: [RFC]selinux: Improving SELinux read/write performance

[Stephen Smalley]

On Wed, 2007-09-12 at 17:51 +0900, Yuichi Nakamura wrote:
> Hi. 
> 
> Stephen Smalley pointed out possibility of race condition
> in off-list discussion.
> Stephen Smalley said:
> > One other observation about the patch:  it presently leaves open a
> > (small) race window in which the file could get relabeled or policy gets
> > reloaded between the time of the normal permission check (from
> > selinux_inode_permission) and the time you copy the inode SID and policy
> > seqno to the file security struct.  In which case you might end up never
> > revalidating access upon read/write even though conditions changed since
> > the open-time permission check.  Not sure how to cleanly fix in a
> > lock-free manner, and adding locks here will only make matters worse.
> 
> To fix that, permission has to be checked in selinux_dentry_open.
> Therefore, in open, number of permission checks increased.
> As shown in benchmark result below, it does not affect open/close 
> performance so much.
> 
> Following is benchmark result.
> * Benchmark
> lmbench simple read,write,open/close.
> 
> * Before tuning
> 1) Result for x86(Pentium 4 2.6Ghz), kernel 2.6.22
>                 Base  SELinux  Overhead(%)
> Simple read     1.10  1.24     12.3
> Simple write    1.02  1.14     14.0
> open/close      5.97  7.45     24.9
> * Base: kernel compiled without SELinux support
> 
> 2) Result for SH(SH4, SH7751R), kernel 2.6.22
>                 Base    SELinux   Overhead(%)
> Simple read     2.39    5.49      130.5
> Simple write    2.07    5.10      146.6
> open/close      32.6    62.8      93.0
> 
> * After tuning
> 1) Result for x86(Pentium 4 2.6Ghz), kernel 2.6.22
>                 Base  SELinux  Overhead(%)
> Simple read     1.10  1.13     2.3(Before 12.3)
> Simple write    1.02  1.024    0.6(Before 14.0)
> open/close      5.97  7.48     25.3(Before 24.9)
> * Base: kernel compiled without SELinux support
> 
> 2) Result for SH(SH4, SH7751R), kernel 2.6.22
>                 Base    SELinux   Overhead(%)
> Simple read     2.39    2.63      10.4(Before 130.5)
> Simple write    2.07    2.34      13.1(Before 146.6)
> open/close      32.6    58.7      80.2(before 93.0)
> 
> Next is a patch.

Thanks, a few comments below.

> 
> * Description of patch
> This patch improves performance of read/write in SELinux.
> It improves performance by skipping permission check in 
> selinux_file_permission. Permission is only checked when 
> sid change or policy load is detected after file open.
> To detect sid change, new LSM hook securiy_dentry_open is added.

I think I'd reword this a little, e.g.

It reduces the selinux overhead on read/write by only revalidating
permissions in selinux_file_permission if the task or inode labels have
changed or the policy has changed since the open-time check.  A new LSM
hook, security_dentry_open, is added to capture the necessary state at
open time to allow this optimization.

> 
> Signed-off-by: Yuichi Nakamura<ynakam@xxxxxxxxxxxxxx>
> ---
>  fs/open.c                         |    5 ++++
>  include/linux/security.h          |   16 ++++++++++++++
>  security/dummy.c                  |    6 +++++
>  security/selinux/avc.c            |    5 ++++
>  security/selinux/hooks.c          |   43 +++++++++++++++++++++++++++++++++++++-
>  security/selinux/include/avc.h    |    2 +
>  security/selinux/include/objsec.h |    2 +
>  7 files changed, 78 insertions(+), 1 deletion(-)
<snip>
> diff -purN -X linux-2.6.22/Documentation/dontdiff linux-2.6.22.orig/security/selinux/hooks.c linux-2.6.22/security/selinux/hooks.c
> --- linux-2.6.22.orig/security/selinux/hooks.c	2007-07-09 08:32:17.000000000 +0900
> +++ linux-2.6.22/security/selinux/hooks.c	2007-09-12 08:42:49.000000000 +0900
> @@ -80,6 +82,7 @@
>  
>  #define XATTR_SELINUX_SUFFIX "selinux"
>  #define XATTR_NAME_SELINUX XATTR_SECURITY_PREFIX XATTR_SELINUX_SUFFIX
> +#define ACC_MODE(x) ("\000\004\002\006"[(x)&O_ACCMODE])

Leftover from prior version of the patch, no longer needed.

<snip>
> @@ -2715,6 +2737,23 @@ static int selinux_file_receive(struct f
>  	return file_has_perm(current, file, file_to_av(file));
>  }
>  
> +static int selinux_dentry_open(struct file *file)
> +{
> +	struct file_security_struct *fsec;
> +	struct inode *inode;
> +	struct inode_security_struct *isec;
> +	inode = file->f_path.dentry->d_inode;
> +	fsec = file->f_security;
> +	isec = inode->i_security;

I'd add a comment here, e.g.
	  /* 
	   * Save inode label and policy sequence number
	   * at open-time so that selinux_file_permission
	   * can determine whether revalidation is necessary.
	   * Task label is already saved in the file security
           * struct as its SID.
	   */

> +	fsec->isid = isec->sid;
> +	fsec->pseqno = avc_policy_seqno();
> +
> +	/*Permission has to be rechecked here.
> +	  Policy load of inode sid can happen between
> +	  may_open and selinux_dentry_open.*/

Typo in the comment (s/of/or/), coding style isn't right for a
multi-line comment, and likely needs clarification, e.g.
	/*
	 * Since the inode label or policy seqno may have changed
	 * between the selinux_inode_permission check and the saving
	 * of state above, recheck that access is still permitted.
	 * Otherwise, access might never be revalidated against the
	 * new inode label or new policy.
	 * This check is not redundant - do not remove. 
	 */

> +	return inode_has_perm(current, inode, file_to_av(file), NULL);
> +}
> +
>  /* task security operations */
>  
>  static int selinux_task_create(unsigned long clone_flags)

> diff -purN -X linux-2.6.22/Documentation/dontdiff linux-2.6.22.orig/fs/open.c linux-2.6.22/fs/open.c
> --- linux-2.6.22.orig/fs/open.c	2007-07-09 08:32:17.000000000 +0900
> +++ linux-2.6.22/fs/open.c	2007-09-12 08:31:24.000000000 +0900
> @@ -696,8 +696,13 @@ static struct file *__dentry_open(struct
>  	f->f_op = fops_get(inode->i_fop);
>  	file_move(f, &inode->i_sb->s_files);
>  
> +	error = security_dentry_open(f);
> +	if (error)
> +		goto cleanup_all;
> +
>  	if (!open && f->f_op)
>  		open = f->f_op->open;
> +

Extraneous whitespace leftover from prior version of the patch.

>  	if (open) {
>  		error = open(inode, f);
>  		if (error)
> diff -purN -X linux-2.6.22/Documentation/dontdiff linux-2.6.22.orig/include/linux/security.h linux-2.6.22/include/linux/security.h
> --- linux-2.6.22.orig/include/linux/security.h	2007-07-09 08:32:17.000000000 +0900
> +++ linux-2.6.22/include/linux/security.h	2007-09-12 08:30:16.000000000 +0900
> @@ -503,6 +503,11 @@ struct request_sock;
>   *	@file contains the file structure being received.
>   *	Return 0 if permission is granted.
>   *
> + * Security hook for dentry
> + *
> + * @dentry_open
> + *   Check permission or get additional information before opening dentry.
> + *

More precisely, "Save open-time permission checking state for later use
upon file_permission, and recheck access if anything has changed since
inode_permission."
  
-- 
Stephen Smalley
National Security Agency

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/