public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [RFC]selinux: Improving SELinux read/write performance
@ 2007-09-06  7:27 Yuichi Nakamura
  2007-09-06 13:47 ` Stephen Smalley
  0 siblings, 1 reply; 7+ messages in thread
From: Yuichi Nakamura @ 2007-09-06  7:27 UTC (permalink / raw)
  To: selinux
  Cc: ynakam, Stephen Smalley, busybox, James Morris, Eric Paris,
	kaigai, linux-security-module, linux-kernel

Hello.

As I posted before in selinux list,
I found big overhead of SELinux in read/write on some CPUs,
and trying tuning.
There were discussion in previous threads.
Part 1:
http://marc.info/?t=118845343400001&r=1&w=2
Part 2:
http://marc.info/?t=118880749800004&r=1&w=2

I would like to RFC again about this topic.

1. Background
Look at benchmark result below.
lmbench simple read/write.
Big overhead exists especially on SH(SuperH) arch.

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.00  1.14     14.0
* 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
# This result is a little different from previous threads, 
# because I changed some kernel configs.

Overhead more than 100%
I also found about 70-90% overhead in ARM.

2. About patch
I found a overhead in selinux_file_permission function.
This is a function that is called in read/write calls, 
and does SELinux permission check.
SELinux checks permission both in open and read/write time.
Stephen Smalley sugessted that we can usually skip permission check 
in selinux_file_permission.
By this patch, 
permission check in selinux_file_permssion is done only when
- sid of task has changed after file open
- sid of inode has changed after file open
- policy load or boolean change happen after file open

To detect these changes,
I added entries in file_security struct and saving these values at file open.

And to save sid of inode at the time of file open,
I had to add new LSM hook in __dentry_open function.

3. Benchmark after applying this patch
1) Result for x86(Pentium 4 2.6Ghz), kernel 2.6.22
                Base  SELinux  Overhead(%)
Simple read     1.10  1.12     1.6(Before 12.3)
Simple write    1.00  1.03     3.6(Before 14.0)

2) Result for SH(SH4, SH7751R), kernel 2.6.22
                Base    SELinux   Overhead(%)
Simple read     2.39    2.65      11.1(Before 130.5)
Simple write    2.07    2.28      10.5(Before 146.6)

Performance has improved a lot.
I want comments from community.

Signed-off-by: Yuichi Nakamura<ynakam@hitachisoft.jp>
---
 fs/open.c                         |    5 +++
 include/linux/security.h          |   11 +++++++
 security/selinux/avc.c            |    5 ++-
 security/selinux/hooks.c          |   54 +++++++++++++++++++++++++++++++++++++-
 security/selinux/include/objsec.h |    3 ++
 5 files changed, 76 insertions(+), 2 deletions(-)
diff -purN -X linux-2.6.22/Documentation/dontdiff linux-2.6.22.orig/security/selinux/avc.c linux-2.6.22/security/selinux/avc.c
--- linux-2.6.22.orig/security/selinux/avc.c	2007-07-09 08:32:17.000000000 +0900
+++ linux-2.6.22/security/selinux/avc.c	2007-09-06 14:33:35.000000000 +0900
@@ -123,6 +123,7 @@ DEFINE_PER_CPU(struct avc_cache_stats, a
 #endif
 
 static struct avc_cache avc_cache;
+u32 policy_seqno = 0;
 static struct avc_callback_node *avc_callbacks;
 static struct kmem_cache *avc_node_cachep;
 
@@ -431,8 +432,10 @@ static int avc_latest_notif_update(int s
 			ret = -EAGAIN;
 		}
 	} else {
-		if (seqno > avc_cache.latest_notif)
+		if (seqno > avc_cache.latest_notif) {
 			avc_cache.latest_notif = seqno;
+			policy_seqno = seqno;
+		}
 	}
 	spin_unlock_irqrestore(&notif_lock, flag);
 
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-06 16:08:36.000000000 +0900
@@ -84,6 +84,7 @@
 extern unsigned int policydb_loaded_version;
 extern int selinux_nlmsg_lookup(u16 sclass, u16 nlmsg_type, u32 *perm);
 extern int selinux_compat_net;
+extern u32 policy_seqno;
 
 #ifdef CONFIG_SECURITY_SELINUX_DEVELOP
 int selinux_enforcing = 0;
@@ -220,6 +221,8 @@ static int file_alloc_security(struct fi
 
 	fsec->file = file;
 	fsec->sid = tsec->sid;
+	fsec->tsid = tsec->sid;
+	fsec->pseqno = policy_seqno;
 	fsec->fown_sid = tsec->sid;
 	file->f_security = fsec;
 
@@ -2458,7 +2461,7 @@ static int selinux_inode_listsecurity(st
 
 /* file security operations */
 
-static int selinux_file_permission(struct file *file, int mask)
+static int do_selinux_file_permission(struct file *file, int mask)
 {
 	int rc;
 	struct inode *inode = file->f_path.dentry->d_inode;
@@ -2480,6 +2483,43 @@ static int selinux_file_permission(struc
 	return selinux_netlbl_inode_permission(inode, mask);
 }
 
+static int selinux_file_permission(struct file *file, int mask)
+{
+	struct inode *inode = file->f_path.dentry->d_inode;
+	struct task_security_struct *tsec = current->security;
+	struct file_security_struct *fsec = file->f_security;
+	struct inode_security_struct *isec = inode->i_security;
+
+	if (!mask) {
+		/* No permission to check.  Existence test. */
+		return 0;
+	}
+
+	if (tsec->sid != fsec->tsid) {
+		if (tsec->sid != fsec->sid) {
+			struct vfsmount *mnt = file->f_path.mnt;
+			struct dentry *dentry = file->f_path.dentry;
+			struct avc_audit_data ad;
+			int rc;
+			AVC_AUDIT_DATA_INIT(&ad, FS);
+			ad.u.fs.mnt = mnt;
+			ad.u.fs.dentry = dentry;
+			rc = avc_has_perm(tsec->sid, fsec->sid,
+					  SECCLASS_FD,
+					  FD__USE,
+					  &ad);
+			if (rc)
+				return rc;
+		}
+		return do_selinux_file_permission(file, mask);
+	}
+
+	if (fsec->isid == isec->sid && fsec->pseqno == policy_seqno)
+		return selinux_netlbl_inode_permission(inode, mask);
+
+	return do_selinux_file_permission(file, mask);
+}
+
 static int selinux_file_alloc_security(struct file *file)
 {
 	return file_alloc_security(file);
@@ -2715,6 +2755,16 @@ 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, int flags)
+{
+	struct file_security_struct *fsec;
+	struct inode_security_struct *isec;
+	fsec = file->f_security;
+	isec = file->f_path.dentry->d_inode->i_security;
+	fsec->isid = isec->sid;
+	return 0;
+}
+
 /* task security operations */
 
 static int selinux_task_create(unsigned long clone_flags)
@@ -4780,6 +4830,8 @@ static struct security_operations selinu
 	.file_send_sigiotask =		selinux_file_send_sigiotask,
 	.file_receive =			selinux_file_receive,
 
+	.dentry_open =                  selinux_dentry_open,
+
 	.task_create =			selinux_task_create,
 	.task_alloc_security =		selinux_task_alloc_security,
 	.task_free_security =		selinux_task_free_security,
diff -purN -X linux-2.6.22/Documentation/dontdiff linux-2.6.22.orig/security/selinux/include/objsec.h linux-2.6.22/security/selinux/include/objsec.h
--- linux-2.6.22.orig/security/selinux/include/objsec.h	2007-07-09 08:32:17.000000000 +0900
+++ linux-2.6.22/security/selinux/include/objsec.h	2007-09-06 14:58:11.000000000 +0900
@@ -53,6 +53,9 @@ struct file_security_struct {
 	struct file *file;              /* back pointer to file object */
 	u32 sid;              /* SID of open file description */
 	u32 fown_sid;         /* SID of file owner (for SIGIO) */
+	u32 tsid;             /* SID of task at the time of file open*/
+	u32 isid;             /* SID of inode at the time of file open */
+	u32 pseqno;           /* Policy seqno at the time of file open */
 };
 
 struct superblock_security_struct {
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-06 15:12:29.000000000 +0900
@@ -698,6 +698,11 @@ static struct file *__dentry_open(struct
 
 	if (!open && f->f_op)
 		open = f->f_op->open;
+
+	error = security_dentry_open(f, flags);
+	if (error)
+		goto cleanup_all;
+
 	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-06 15:22:39.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.
+ *
  * Security hooks for task operations.
  *
  * @task_create:
@@ -1253,6 +1258,7 @@ struct security_operations {
 	int (*file_send_sigiotask) (struct task_struct * tsk,
 				    struct fown_struct * fown, int sig);
 	int (*file_receive) (struct file * file);
+	int (*dentry_open)  (struct file *file, int flags);
 
 	int (*task_create) (unsigned long clone_flags);
 	int (*task_alloc_security) (struct task_struct * p);
@@ -1854,6 +1860,11 @@ static inline int security_file_receive 
 	return security_ops->file_receive (file);
 }
 
+static inline int security_dentry_open (struct file *file, int flags)
+{
+	return security_ops->dentry_open (file, flags);
+}
+
 static inline int security_task_create (unsigned long clone_flags)
 {
 	return security_ops->task_create (clone_flags);

Regards,
-- 
Yuichi Nakamura
Hitachi Software Engineering Co., Ltd.
Japan SELinux Users Group(JSELUG): http://www.selinux.gr.jp/
SELinux Policy Editor: http://seedit.sourceforge.net/


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC]selinux: Improving SELinux read/write performance
  2007-09-06  7:27 [RFC]selinux: Improving SELinux read/write performance Yuichi Nakamura
@ 2007-09-06 13:47 ` Stephen Smalley
  2007-09-10  1:31   ` Yuichi Nakamura
  0 siblings, 1 reply; 7+ messages in thread
From: Stephen Smalley @ 2007-09-06 13:47 UTC (permalink / raw)
  To: Yuichi Nakamura
  Cc: selinux, busybox, James Morris, Eric Paris, kaigai,
	linux-security-module, linux-kernel

On Thu, 2007-09-06 at 16:27 +0900, Yuichi Nakamura wrote:
> Hello.
> 
> As I posted before in selinux list,
> I found big overhead of SELinux in read/write on some CPUs,
> and trying tuning.
> There were discussion in previous threads.
> Part 1:
> http://marc.info/?t=118845343400001&r=1&w=2
> Part 2:
> http://marc.info/?t=118880749800004&r=1&w=2
> 
> I would like to RFC again about this topic.

Thanks for your work on this, as this is clearly an important area to
improve.

> 1. Background
> Look at benchmark result below.
> lmbench simple read/write.
> Big overhead exists especially on SH(SuperH) arch.
> 
> 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.00  1.14     14.0
> * 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
> # This result is a little different from previous threads, 
> # because I changed some kernel configs.
> 
> Overhead more than 100%
> I also found about 70-90% overhead in ARM.
> 
> 2. About patch
> I found a overhead in selinux_file_permission function.
> This is a function that is called in read/write calls, 
> and does SELinux permission check.
> SELinux checks permission both in open and read/write time.
> Stephen Smalley sugessted that we can usually skip permission check 
> in selinux_file_permission.
> By this patch, 
> permission check in selinux_file_permssion is done only when
> - sid of task has changed after file open
> - sid of inode has changed after file open
> - policy load or boolean change happen after file open
> 
> To detect these changes,
> I added entries in file_security struct and saving these values at file open.
> 
> And to save sid of inode at the time of file open,
> I had to add new LSM hook in __dentry_open function.
> 
> 3. Benchmark after applying this patch
> 1) Result for x86(Pentium 4 2.6Ghz), kernel 2.6.22
>                 Base  SELinux  Overhead(%)
> Simple read     1.10  1.12     1.6(Before 12.3)
> Simple write    1.00  1.03     3.6(Before 14.0)
> 
> 2) Result for SH(SH4, SH7751R), kernel 2.6.22
>                 Base    SELinux   Overhead(%)
> Simple read     2.39    2.65      11.1(Before 130.5)
> Simple write    2.07    2.28      10.5(Before 146.6)
> 
> Performance has improved a lot.
> I want comments from community.
> 
> Signed-off-by: Yuichi Nakamura<ynakam@hitachisoft.jp>
> ---
>  fs/open.c                         |    5 +++
>  include/linux/security.h          |   11 +++++++
>  security/selinux/avc.c            |    5 ++-
>  security/selinux/hooks.c          |   54 +++++++++++++++++++++++++++++++++++++-
>  security/selinux/include/objsec.h |    3 ++
>  5 files changed, 76 insertions(+), 2 deletions(-)
> diff -purN -X linux-2.6.22/Documentation/dontdiff linux-2.6.22.orig/security/selinux/avc.c linux-2.6.22/security/selinux/avc.c
> --- linux-2.6.22.orig/security/selinux/avc.c	2007-07-09 08:32:17.000000000 +0900
> +++ linux-2.6.22/security/selinux/avc.c	2007-09-06 14:33:35.000000000 +0900
> @@ -123,6 +123,7 @@ DEFINE_PER_CPU(struct avc_cache_stats, a
>  #endif
>  
>  static struct avc_cache avc_cache;
> +u32 policy_seqno = 0;
>  static struct avc_callback_node *avc_callbacks;
>  static struct kmem_cache *avc_node_cachep;
>  
> @@ -431,8 +432,10 @@ static int avc_latest_notif_update(int s
>  			ret = -EAGAIN;
>  		}
>  	} else {
> -		if (seqno > avc_cache.latest_notif)
> +		if (seqno > avc_cache.latest_notif) {
>  			avc_cache.latest_notif = seqno;
> +			policy_seqno = seqno;
> +		}

I would have just provided an avc interface for obtaining the seqno,
e.g.
u32 avc_policy_seqno(void)
{
	return avc_cache.latest_notif;
}

>  	}
>  	spin_unlock_irqrestore(&notif_lock, flag);
>  
> 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-06 16:08:36.000000000 +0900
> @@ -84,6 +84,7 @@
>  extern unsigned int policydb_loaded_version;
>  extern int selinux_nlmsg_lookup(u16 sclass, u16 nlmsg_type, u32 *perm);
>  extern int selinux_compat_net;
> +extern u32 policy_seqno;

I think that they frown upon extern declarations in .c files (versus
in .h files), so we don't want to add more - we ultimately should clean
up what we presently have.

>  
>  #ifdef CONFIG_SECURITY_SELINUX_DEVELOP
>  int selinux_enforcing = 0;
> @@ -220,6 +221,8 @@ static int file_alloc_security(struct fi
>  
>  	fsec->file = file;
>  	fsec->sid = tsec->sid;
> +	fsec->tsid = tsec->sid;

I'm not sure why we need the separate field here, as fsec->sid already
holds the allocating task SID and doesn't change.

> +	fsec->pseqno = policy_seqno;

Not sure that you want to set the seqno here versus from your new hook,
as it could conceivably change in between.

>  	fsec->fown_sid = tsec->sid;
>  	file->f_security = fsec;
>  
> @@ -2458,7 +2461,7 @@ static int selinux_inode_listsecurity(st
>  
>  /* file security operations */
>  
> -static int selinux_file_permission(struct file *file, int mask)
> +static int do_selinux_file_permission(struct file *file, int mask)

Might want to rename for clarity, e.g. 
selinux_revalidate_file_permission or
selinux_file_permission_slow (slow path) or
something similar.

>  {
>  	int rc;
>  	struct inode *inode = file->f_path.dentry->d_inode;
> @@ -2480,6 +2483,43 @@ static int selinux_file_permission(struc
>  	return selinux_netlbl_inode_permission(inode, mask);
>  }
>  
> +static int selinux_file_permission(struct file *file, int mask)
> +{
> +	struct inode *inode = file->f_path.dentry->d_inode;
> +	struct task_security_struct *tsec = current->security;
> +	struct file_security_struct *fsec = file->f_security;
> +	struct inode_security_struct *isec = inode->i_security;
> +
> +	if (!mask) {
> +		/* No permission to check.  Existence test. */
> +		return 0;
> +	}
> +
> +	if (tsec->sid != fsec->tsid) {
> +		if (tsec->sid != fsec->sid) {
> +			struct vfsmount *mnt = file->f_path.mnt;
> +			struct dentry *dentry = file->f_path.dentry;
> +			struct avc_audit_data ad;
> +			int rc;
> +			AVC_AUDIT_DATA_INIT(&ad, FS);
> +			ad.u.fs.mnt = mnt;
> +			ad.u.fs.dentry = dentry;
> +			rc = avc_has_perm(tsec->sid, fsec->sid,
> +					  SECCLASS_FD,
> +					  FD__USE,
> +					  &ad);
> +			if (rc)
> +				return rc;
> +		}

Why inline the FD_USE check here given that you are falling back to the
full function call anyway?  I also don't see why you separate this from
the rest of the comparison - I'd just make it something like:
	if (tsec->sid == fsec->sid && isec->sid == fsec->isid &&
	    avc_seqno() == fsec->seqno)
		return selinux_netlbl_inode_permission(inode, mask);
	return selinux_revalidate_file_permission(file, mask);

>  static int selinux_file_alloc_security(struct file *file)
>  {
>  	return file_alloc_security(file);
> @@ -2715,6 +2755,16 @@ 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, int flags)
> +{
> +	struct file_security_struct *fsec;
> +	struct inode_security_struct *isec;
> +	fsec = file->f_security;
> +	isec = file->f_path.dentry->d_inode->i_security;
> +	fsec->isid = isec->sid;

Set the seqno here too.  Ideally, it would come straight from the AVC
entry that was used for the open-time check, but that is a bit more
invasive and there will always be a small window there.

> diff -purN -X linux-2.6.22/Documentation/dontdiff linux-2.6.22.orig/security/selinux/include/objsec.h linux-2.6.22/security/selinux/include/objsec.h
> --- linux-2.6.22.orig/security/selinux/include/objsec.h	2007-07-09 08:32:17.000000000 +0900
> +++ linux-2.6.22/security/selinux/include/objsec.h	2007-09-06 14:58:11.000000000 +0900
> @@ -53,6 +53,9 @@ struct file_security_struct {
>  	struct file *file;              /* back pointer to file object */
>  	u32 sid;              /* SID of open file description */
>  	u32 fown_sid;         /* SID of file owner (for SIGIO) */
> +	u32 tsid;             /* SID of task at the time of file open*/
> +	u32 isid;             /* SID of inode at the time of file open */
> +	u32 pseqno;           /* Policy seqno at the time of file open */

No need for tsid above I think.

> 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-06 15:22:39.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.
> + *
>   * Security hooks for task operations.
>   *
>   * @task_create:
> @@ -1253,6 +1258,7 @@ struct security_operations {
>  	int (*file_send_sigiotask) (struct task_struct * tsk,
>  				    struct fown_struct * fown, int sig);
>  	int (*file_receive) (struct file * file);
> +	int (*dentry_open)  (struct file *file, int flags);
>  
>  	int (*task_create) (unsigned long clone_flags);
>  	int (*task_alloc_security) (struct task_struct * p);
> @@ -1854,6 +1860,11 @@ static inline int security_file_receive 
>  	return security_ops->file_receive (file);
>  }
>  
> +static inline int security_dentry_open (struct file *file, int flags)
> +{
> +	return security_ops->dentry_open (file, flags);
> +}
> +
>  static inline int security_task_create (unsigned long clone_flags)
>  {
>  	return security_ops->task_create (clone_flags);

Need to also provide the stub definition in the #else case (SECURITY=n)
and a stub function for the dummy security module.

Thanks.

-- 
Stephen Smalley
National Security Agency


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC]selinux: Improving SELinux read/write performance
  2007-09-06 13:47 ` Stephen Smalley
@ 2007-09-10  1:31   ` Yuichi Nakamura
  2007-09-10 13:02     ` Stephen Smalley
  0 siblings, 1 reply; 7+ messages in thread
From: Yuichi Nakamura @ 2007-09-10  1:31 UTC (permalink / raw)
  To: Stephen Smalley
  Cc: ynakam, selinux, busybox, James Morris, Eric Paris, kaigai,
	linux-security-module, linux-kernel

On Thu, 06 Sep 2007 09:47:15 -0400
Stephen Smalley  wrote:
<snip>
> >  
> > @@ -431,8 +432,10 @@ static int avc_latest_notif_update(int s
> >  			ret = -EAGAIN;
> >  		}
> >  	} else {
> > -		if (seqno > avc_cache.latest_notif)
> > +		if (seqno > avc_cache.latest_notif) {
> >  			avc_cache.latest_notif = seqno;
> > +			policy_seqno = seqno;
> > +		}
> 
> I would have just provided an avc interface for obtaining the seqno,
> e.g.
> u32 avc_policy_seqno(void)
> {
> 	return avc_cache.latest_notif;
> }
Fixed.


> 
> >  	}
> >  	spin_unlock_irqrestore(&notif_lock, flag);
> >  
> > 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-06 16:08:36.000000000 +0900
> > @@ -84,6 +84,7 @@
> >  extern unsigned int policydb_loaded_version;
> >  extern int selinux_nlmsg_lookup(u16 sclass, u16 nlmsg_type, u32 *perm);
> >  extern int selinux_compat_net;
> > +extern u32 policy_seqno;
> 
> I think that they frown upon extern declarations in .c files (versus
> in .h files), so we don't want to add more - we ultimately should clean
> up what we presently have.
Removed policy_seqno.

> 
> >  
> >  #ifdef CONFIG_SECURITY_SELINUX_DEVELOP
> >  int selinux_enforcing = 0;
> > @@ -220,6 +221,8 @@ static int file_alloc_security(struct fi
> >  
> >  	fsec->file = file;
> >  	fsec->sid = tsec->sid;
> > +	fsec->tsid = tsec->sid;
> 
> I'm not sure why we need the separate field here, as fsec->sid already
> holds the allocating task SID and doesn't change.
I see.
removed fsec->tsid.

> > +	fsec->pseqno = policy_seqno;
> 
> Not sure that you want to set the seqno here versus from your new hook,
> as it could conceivably change in between.
Fixed, pseqno is set in selinux_dentry_open.


> >  	fsec->fown_sid = tsec->sid;
> >  	file->f_security = fsec;
> >  
> > @@ -2458,7 +2461,7 @@ static int selinux_inode_listsecurity(st
> >  
> >  /* file security operations */
> >  
> > -static int selinux_file_permission(struct file *file, int mask)
> > +static int do_selinux_file_permission(struct file *file, int mask)
> 
> Might want to rename for clarity, e.g. 
> selinux_revalidate_file_permission or
> selinux_file_permission_slow (slow path) or
> something similar.
Renamed to selinux_revalidate_file_permission.


> 
> >  {
> >  	int rc;
> >  	struct inode *inode = file->f_path.dentry->d_inode;
> > @@ -2480,6 +2483,43 @@ static int selinux_file_permission(struc
> >  	return selinux_netlbl_inode_permission(inode, mask);
> >  }
> >  
> > +static int selinux_file_permission(struct file *file, int mask)
> > +{
> > +	struct inode *inode = file->f_path.dentry->d_inode;
> > +	struct task_security_struct *tsec = current->security;
> > +	struct file_security_struct *fsec = file->f_security;
> > +	struct inode_security_struct *isec = inode->i_security;
> > +
> > +	if (!mask) {
> > +		/* No permission to check.  Existence test. */
> > +		return 0;
> > +	}
> > +
> > +	if (tsec->sid != fsec->tsid) {
> > +		if (tsec->sid != fsec->sid) {
> > +			struct vfsmount *mnt = file->f_path.mnt;
> > +			struct dentry *dentry = file->f_path.dentry;
> > +			struct avc_audit_data ad;
> > +			int rc;
> > +			AVC_AUDIT_DATA_INIT(&ad, FS);
> > +			ad.u.fs.mnt = mnt;
> > +			ad.u.fs.dentry = dentry;
> > +			rc = avc_has_perm(tsec->sid, fsec->sid,
> > +					  SECCLASS_FD,
> > +					  FD__USE,
> > +					  &ad);
> > +			if (rc)
> > +				return rc;
> > +		}
> 
> Why inline the FD_USE check here given that you are falling back to the
> full function call anyway?  I also don't see why you separate this from
> the rest of the comparison - I'd just make it something like:
> 	if (tsec->sid == fsec->sid && isec->sid == fsec->isid &&
> 	    avc_seqno() == fsec->seqno)
> 		return selinux_netlbl_inode_permission(inode, mask);
> 	return selinux_revalidate_file_permission(file, mask);
Thanks, I missed that.
FD_USE check is called in file_permission..
Fixed like you pointed out.


> >  static int selinux_file_alloc_security(struct file *file)
> >  {
> >  	return file_alloc_security(file);
> > @@ -2715,6 +2755,16 @@ 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, int flags)
> > +{
> > +	struct file_security_struct *fsec;
> > +	struct inode_security_struct *isec;
> > +	fsec = file->f_security;
> > +	isec = file->f_path.dentry->d_inode->i_security;
> > +	fsec->isid = isec->sid;
> 
> Set the seqno here too.  Ideally, it would come straight from the AVC
> entry that was used for the open-time check, but that is a bit more
> invasive and there will always be a small window there.
Fixed, setting pseqno now.


> > diff -purN -X linux-2.6.22/Documentation/dontdiff linux-2.6.22.orig/security/selinux/include/objsec.h linux-2.6.22/security/selinux/include/objsec.h
> > --- linux-2.6.22.orig/security/selinux/include/objsec.h	2007-07-09 08:32:17.000000000 +0900
> > +++ linux-2.6.22/security/selinux/include/objsec.h	2007-09-06 14:58:11.000000000 +0900
> > @@ -53,6 +53,9 @@ struct file_security_struct {
> >  	struct file *file;              /* back pointer to file object */
> >  	u32 sid;              /* SID of open file description */
> >  	u32 fown_sid;         /* SID of file owner (for SIGIO) */
> > +	u32 tsid;             /* SID of task at the time of file open*/
> > +	u32 isid;             /* SID of inode at the time of file open */
> > +	u32 pseqno;           /* Policy seqno at the time of file open */
> 
> No need for tsid above I think.
Removed tsid.


> > 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-06 15:22:39.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.
> > + *
> >   * Security hooks for task operations.
> >   *
> >   * @task_create:
> > @@ -1253,6 +1258,7 @@ struct security_operations {
> >  	int (*file_send_sigiotask) (struct task_struct * tsk,
> >  				    struct fown_struct * fown, int sig);
> >  	int (*file_receive) (struct file * file);
> > +	int (*dentry_open)  (struct file *file, int flags);
> >  
> >  	int (*task_create) (unsigned long clone_flags);
> >  	int (*task_alloc_security) (struct task_struct * p);
> > @@ -1854,6 +1860,11 @@ static inline int security_file_receive 
> >  	return security_ops->file_receive (file);
> >  }
> >  
> > +static inline int security_dentry_open (struct file *file, int flags)
> > +{
> > +	return security_ops->dentry_open (file, flags);
> > +}
> > +
> >  static inline int security_task_create (unsigned long clone_flags)
> >  {
> >  	return security_ops->task_create (clone_flags);
> 
> Need to also provide the stub definition in the #else case (SECURITY=n)
> and a stub function for the dummy security module.
Fixed.


> 
> Thanks.
> 
> -- 
> Stephen Smalley
> National Security Agency

Next is updated patch.

Signed-off-by: Yuichi Nakamura<ynakam@hitachisoft.jp>
---
 fs/open.c                         |    5 +++++
 include/linux/security.h          |   16 ++++++++++++++++
 security/selinux/avc.c            |    5 +++++
 security/selinux/hooks.c          |   36 +++++++++++++++++++++++++++++++++++-
 security/selinux/include/avc.h    |    2 ++
 security/selinux/include/objsec.h |    2 ++
 6 files changed, 65 insertions(+), 1 deletion(-)
diff -purN -X linux-2.6.22/Documentation/dontdiff linux-2.6.22.orig/security/selinux/avc.c linux-2.6.22/security/selinux/avc.c
--- linux-2.6.22.orig/security/selinux/avc.c	2007-07-09 08:32:17.000000000 +0900
+++ linux-2.6.22/security/selinux/avc.c	2007-09-10 09:56:22.000000000 +0900
@@ -913,3 +913,8 @@ int avc_has_perm(u32 ssid, u32 tsid, u16
 	avc_audit(ssid, tsid, tclass, requested, &avd, rc, auditdata);
 	return rc;
 }
+
+u32 avc_policy_seqno(void)
+{
+	return avc_cache.latest_notif;
+}
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-10 10:11:13.000000000 +0900
@@ -14,6 +14,8 @@
  *                          <dgoeddel@trustedcs.com>
  *  Copyright (C) 2006 Hewlett-Packard Development Company, L.P.
  *                     Paul Moore, <paul.moore@hp.com>
+ *  Copyright (C) 2007 Hitachi Software Engineering Co., Ltd.
+ *                     Yuichi Nakamura <ynakam@hitachisoft.jp>
  *
  *	This program is free software; you can redistribute it and/or modify
  *	it under the terms of the GNU General Public License version 2,
@@ -2458,7 +2460,7 @@ static int selinux_inode_listsecurity(st
 
 /* file security operations */
 
-static int selinux_file_permission(struct file *file, int mask)
+static int selinux_revalidate_file_permission(struct file *file, int mask)
 {
 	int rc;
 	struct inode *inode = file->f_path.dentry->d_inode;
@@ -2480,6 +2482,25 @@ static int selinux_file_permission(struc
 	return selinux_netlbl_inode_permission(inode, mask);
 }
 
+static int selinux_file_permission(struct file *file, int mask)
+{
+	struct inode *inode = file->f_path.dentry->d_inode;
+	struct task_security_struct *tsec = current->security;
+	struct file_security_struct *fsec = file->f_security;
+	struct inode_security_struct *isec = inode->i_security;
+
+	if (!mask) {
+		/* No permission to check.  Existence test. */
+		return 0;
+	}
+
+	if (tsec->sid == fsec->sid && fsec->isid == isec->sid
+	    && fsec->pseqno == avc_policy_seqno())
+		return selinux_netlbl_inode_permission(inode, mask);
+
+	return selinux_revalidate_file_permission(file, mask);
+}
+
 static int selinux_file_alloc_security(struct file *file)
 {
 	return file_alloc_security(file);
@@ -2715,6 +2736,17 @@ 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, int flags)
+{
+	struct file_security_struct *fsec;
+	struct inode_security_struct *isec;
+	fsec = file->f_security;
+	isec = file->f_path.dentry->d_inode->i_security;
+	fsec->isid = isec->sid;
+	fsec->pseqno = avc_policy_seqno();
+	return 0;
+}
+
 /* task security operations */
 
 static int selinux_task_create(unsigned long clone_flags)
@@ -4780,6 +4812,8 @@ static struct security_operations selinu
 	.file_send_sigiotask =		selinux_file_send_sigiotask,
 	.file_receive =			selinux_file_receive,
 
+	.dentry_open =                  selinux_dentry_open,
+
 	.task_create =			selinux_task_create,
 	.task_alloc_security =		selinux_task_alloc_security,
 	.task_free_security =		selinux_task_free_security,
diff -purN -X linux-2.6.22/Documentation/dontdiff linux-2.6.22.orig/security/selinux/include/avc.h linux-2.6.22/security/selinux/include/avc.h
--- linux-2.6.22.orig/security/selinux/include/avc.h	2007-07-09 08:32:17.000000000 +0900
+++ linux-2.6.22/security/selinux/include/avc.h	2007-09-10 09:56:22.000000000 +0900
@@ -110,6 +110,8 @@ int avc_has_perm(u32 ssid, u32 tsid,
                  u16 tclass, u32 requested,
                  struct avc_audit_data *auditdata);
 
+u32 avc_policy_seqno(void);
+
 #define AVC_CALLBACK_GRANT		1
 #define AVC_CALLBACK_TRY_REVOKE		2
 #define AVC_CALLBACK_REVOKE		4
diff -purN -X linux-2.6.22/Documentation/dontdiff linux-2.6.22.orig/security/selinux/include/objsec.h linux-2.6.22/security/selinux/include/objsec.h
--- linux-2.6.22.orig/security/selinux/include/objsec.h	2007-07-09 08:32:17.000000000 +0900
+++ linux-2.6.22/security/selinux/include/objsec.h	2007-09-10 09:56:22.000000000 +0900
@@ -53,6 +53,8 @@ struct file_security_struct {
 	struct file *file;              /* back pointer to file object */
 	u32 sid;              /* SID of open file description */
 	u32 fown_sid;         /* SID of file owner (for SIGIO) */
+	u32 isid;             /* SID of inode at the time of file open */
+	u32 pseqno;           /* Policy seqno at the time of file open */
 };
 
 struct superblock_security_struct {
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-10 09:56:22.000000000 +0900
@@ -698,6 +698,11 @@ static struct file *__dentry_open(struct
 
 	if (!open && f->f_op)
 		open = f->f_op->open;
+
+	error = security_dentry_open(f, flags);
+	if (error)
+		goto cleanup_all;
+
 	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-10 09:56:22.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.
+ *
  * Security hooks for task operations.
  *
  * @task_create:
@@ -1253,6 +1258,7 @@ struct security_operations {
 	int (*file_send_sigiotask) (struct task_struct * tsk,
 				    struct fown_struct * fown, int sig);
 	int (*file_receive) (struct file * file);
+	int (*dentry_open)  (struct file *file, int flags);
 
 	int (*task_create) (unsigned long clone_flags);
 	int (*task_alloc_security) (struct task_struct * p);
@@ -1854,6 +1860,11 @@ static inline int security_file_receive 
 	return security_ops->file_receive (file);
 }
 
+static inline int security_dentry_open (struct file *file, int flags)
+{
+	return security_ops->dentry_open (file, flags);
+}
+
 static inline int security_task_create (unsigned long clone_flags)
 {
 	return security_ops->task_create (clone_flags);
@@ -2529,6 +2540,11 @@ static inline int security_file_receive 
 	return 0;
 }
 
+static inline int security_dentry_open (struct file *file, int flags)
+{
+	return 0;
+}
+
 static inline int security_task_create (unsigned long clone_flags)
 {
 	return 0;

Regards,
-- 
Yuichi Nakamura
Hitachi Software Engineering Co., Ltd.
Japan SELinux Users Group(JSELUG): http://www.selinux.gr.jp/
SELinux Policy Editor: http://seedit.sourceforge.net/


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC]selinux: Improving SELinux read/write performance
  2007-09-10  1:31   ` Yuichi Nakamura
@ 2007-09-10 13:02     ` Stephen Smalley
  2007-09-12  8:51       ` Yuichi Nakamura
  0 siblings, 1 reply; 7+ messages in thread
From: Stephen Smalley @ 2007-09-10 13:02 UTC (permalink / raw)
  To: Yuichi Nakamura
  Cc: selinux, busybox, James Morris, Eric Paris, kaigai,
	linux-security-module, linux-kernel

On Mon, 2007-09-10 at 10:31 +0900, Yuichi Nakamura wrote:
> Next is updated patch.

Thanks.
Please include the short description of the patch though when
re-submitting.

> Signed-off-by: Yuichi Nakamura<ynakam@hitachisoft.jp>
> ---
>  fs/open.c                         |    5 +++++
>  include/linux/security.h          |   16 ++++++++++++++++
>  security/selinux/avc.c            |    5 +++++
>  security/selinux/hooks.c          |   36 +++++++++++++++++++++++++++++++++++-
>  security/selinux/include/avc.h    |    2 ++
>  security/selinux/include/objsec.h |    2 ++
>  6 files changed, 65 insertions(+), 1 deletion(-)

Still missing the necessary changes to security/dummy.c (add
dummy_dentry_open() and update security_fixup_ops()).  For
CONFIG_SECURITY=y but SELinux disabled.

Also, have you re-run your benchmarks with this version of the patch?
   
> diff -purN -X linux-2.6.22/Documentation/dontdiff linux-2.6.22.orig/security/selinux/avc.c linux-2.6.22/security/selinux/avc.c
> --- linux-2.6.22.orig/security/selinux/avc.c	2007-07-09 08:32:17.000000000 +0900
> +++ linux-2.6.22/security/selinux/avc.c	2007-09-10 09:56:22.000000000 +0900
> @@ -913,3 +913,8 @@ int avc_has_perm(u32 ssid, u32 tsid, u16
>  	avc_audit(ssid, tsid, tclass, requested, &avd, rc, auditdata);
>  	return rc;
>  }
> +
> +u32 avc_policy_seqno(void)
> +{
> +	return avc_cache.latest_notif;
> +}
> 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-10 10:11:13.000000000 +0900
> @@ -14,6 +14,8 @@
>   *                          <dgoeddel@trustedcs.com>
>   *  Copyright (C) 2006 Hewlett-Packard Development Company, L.P.
>   *                     Paul Moore, <paul.moore@hp.com>
> + *  Copyright (C) 2007 Hitachi Software Engineering Co., Ltd.
> + *                     Yuichi Nakamura <ynakam@hitachisoft.jp>
>   *
>   *	This program is free software; you can redistribute it and/or modify
>   *	it under the terms of the GNU General Public License version 2,
> @@ -2458,7 +2460,7 @@ static int selinux_inode_listsecurity(st
>  
>  /* file security operations */
>  
> -static int selinux_file_permission(struct file *file, int mask)
> +static int selinux_revalidate_file_permission(struct file *file, int mask)
>  {
>  	int rc;
>  	struct inode *inode = file->f_path.dentry->d_inode;
> @@ -2480,6 +2482,25 @@ static int selinux_file_permission(struc
>  	return selinux_netlbl_inode_permission(inode, mask);
>  }
>  
> +static int selinux_file_permission(struct file *file, int mask)
> +{
> +	struct inode *inode = file->f_path.dentry->d_inode;
> +	struct task_security_struct *tsec = current->security;
> +	struct file_security_struct *fsec = file->f_security;
> +	struct inode_security_struct *isec = inode->i_security;
> +
> +	if (!mask) {
> +		/* No permission to check.  Existence test. */
> +		return 0;
> +	}
> +
> +	if (tsec->sid == fsec->sid && fsec->isid == isec->sid
> +	    && fsec->pseqno == avc_policy_seqno())
> +		return selinux_netlbl_inode_permission(inode, mask);
> +
> +	return selinux_revalidate_file_permission(file, mask);
> +}
> +
>  static int selinux_file_alloc_security(struct file *file)
>  {
>  	return file_alloc_security(file);
> @@ -2715,6 +2736,17 @@ 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, int flags)
> +{
> +	struct file_security_struct *fsec;
> +	struct inode_security_struct *isec;
> +	fsec = file->f_security;
> +	isec = file->f_path.dentry->d_inode->i_security;
> +	fsec->isid = isec->sid;
> +	fsec->pseqno = avc_policy_seqno();
> +	return 0;
> +}
> +
>  /* task security operations */
>  
>  static int selinux_task_create(unsigned long clone_flags)
> @@ -4780,6 +4812,8 @@ static struct security_operations selinu
>  	.file_send_sigiotask =		selinux_file_send_sigiotask,
>  	.file_receive =			selinux_file_receive,
>  
> +	.dentry_open =                  selinux_dentry_open,
> +
>  	.task_create =			selinux_task_create,
>  	.task_alloc_security =		selinux_task_alloc_security,
>  	.task_free_security =		selinux_task_free_security,
> diff -purN -X linux-2.6.22/Documentation/dontdiff linux-2.6.22.orig/security/selinux/include/avc.h linux-2.6.22/security/selinux/include/avc.h
> --- linux-2.6.22.orig/security/selinux/include/avc.h	2007-07-09 08:32:17.000000000 +0900
> +++ linux-2.6.22/security/selinux/include/avc.h	2007-09-10 09:56:22.000000000 +0900
> @@ -110,6 +110,8 @@ int avc_has_perm(u32 ssid, u32 tsid,
>                   u16 tclass, u32 requested,
>                   struct avc_audit_data *auditdata);
>  
> +u32 avc_policy_seqno(void);
> +
>  #define AVC_CALLBACK_GRANT		1
>  #define AVC_CALLBACK_TRY_REVOKE		2
>  #define AVC_CALLBACK_REVOKE		4
> diff -purN -X linux-2.6.22/Documentation/dontdiff linux-2.6.22.orig/security/selinux/include/objsec.h linux-2.6.22/security/selinux/include/objsec.h
> --- linux-2.6.22.orig/security/selinux/include/objsec.h	2007-07-09 08:32:17.000000000 +0900
> +++ linux-2.6.22/security/selinux/include/objsec.h	2007-09-10 09:56:22.000000000 +0900
> @@ -53,6 +53,8 @@ struct file_security_struct {
>  	struct file *file;              /* back pointer to file object */
>  	u32 sid;              /* SID of open file description */
>  	u32 fown_sid;         /* SID of file owner (for SIGIO) */
> +	u32 isid;             /* SID of inode at the time of file open */
> +	u32 pseqno;           /* Policy seqno at the time of file open */
>  };
>  
>  struct superblock_security_struct {
> 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-10 09:56:22.000000000 +0900
> @@ -698,6 +698,11 @@ static struct file *__dentry_open(struct
>  
>  	if (!open && f->f_op)
>  		open = f->f_op->open;
> +
> +	error = security_dentry_open(f, flags);
> +	if (error)
> +		goto cleanup_all;
> +
>  	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-10 09:56:22.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.
> + *
>   * Security hooks for task operations.
>   *
>   * @task_create:
> @@ -1253,6 +1258,7 @@ struct security_operations {
>  	int (*file_send_sigiotask) (struct task_struct * tsk,
>  				    struct fown_struct * fown, int sig);
>  	int (*file_receive) (struct file * file);
> +	int (*dentry_open)  (struct file *file, int flags);
>  
>  	int (*task_create) (unsigned long clone_flags);
>  	int (*task_alloc_security) (struct task_struct * p);
> @@ -1854,6 +1860,11 @@ static inline int security_file_receive 
>  	return security_ops->file_receive (file);
>  }
>  
> +static inline int security_dentry_open (struct file *file, int flags)
> +{
> +	return security_ops->dentry_open (file, flags);
> +}
> +
>  static inline int security_task_create (unsigned long clone_flags)
>  {
>  	return security_ops->task_create (clone_flags);
> @@ -2529,6 +2540,11 @@ static inline int security_file_receive 
>  	return 0;
>  }
>  
> +static inline int security_dentry_open (struct file *file, int flags)
> +{
> +	return 0;
> +}
> +
>  static inline int security_task_create (unsigned long clone_flags)
>  {
>  	return 0;
> 
> Regards,
-- 
Stephen Smalley
National Security Agency


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC]selinux: Improving SELinux read/write performance
  2007-09-10 13:02     ` Stephen Smalley
@ 2007-09-12  8:51       ` Yuichi Nakamura
  2007-09-13 12:58         ` Stephen Smalley
  0 siblings, 1 reply; 7+ messages in thread
From: Yuichi Nakamura @ 2007-09-12  8:51 UTC (permalink / raw)
  To: Stephen Smalley
  Cc: ynakam, selinux, busybox, James Morris, Eric Paris, kaigai,
	linux-security-module, linux-kernel

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.

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

Signed-off-by: Yuichi Nakamura<ynakam@hitachisoft.jp>
---
 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(-)
diff -purN -X linux-2.6.22/Documentation/dontdiff linux-2.6.22.orig/security/selinux/avc.c linux-2.6.22/security/selinux/avc.c
--- linux-2.6.22.orig/security/selinux/avc.c	2007-07-09 08:32:17.000000000 +0900
+++ linux-2.6.22/security/selinux/avc.c	2007-09-12 08:24:27.000000000 +0900
@@ -913,3 +913,8 @@ int avc_has_perm(u32 ssid, u32 tsid, u16
 	avc_audit(ssid, tsid, tclass, requested, &avd, rc, auditdata);
 	return rc;
 }
+
+u32 avc_policy_seqno(void)
+{
+	return avc_cache.latest_notif;
+}
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
@@ -14,6 +14,8 @@
  *                          <dgoeddel@trustedcs.com>
  *  Copyright (C) 2006 Hewlett-Packard Development Company, L.P.
  *                     Paul Moore, <paul.moore@hp.com>
+ *  Copyright (C) 2007 Hitachi Software Engineering Co., Ltd.
+ *                     Yuichi Nakamura <ynakam@hitachisoft.jp>
  *
  *	This program is free software; you can redistribute it and/or modify
  *	it under the terms of the GNU General Public License version 2,
@@ -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])
 
 extern unsigned int policydb_loaded_version;
 extern int selinux_nlmsg_lookup(u16 sclass, u16 nlmsg_type, u32 *perm);
@@ -2458,7 +2461,7 @@ static int selinux_inode_listsecurity(st
 
 /* file security operations */
 
-static int selinux_file_permission(struct file *file, int mask)
+static int selinux_revalidate_file_permission(struct file *file, int mask)
 {
 	int rc;
 	struct inode *inode = file->f_path.dentry->d_inode;
@@ -2480,6 +2483,25 @@ static int selinux_file_permission(struc
 	return selinux_netlbl_inode_permission(inode, mask);
 }
 
+static int selinux_file_permission(struct file *file, int mask)
+{
+	struct inode *inode = file->f_path.dentry->d_inode;
+	struct task_security_struct *tsec = current->security;
+	struct file_security_struct *fsec = file->f_security;
+	struct inode_security_struct *isec = inode->i_security;
+
+	if (!mask) {
+		/* No permission to check.  Existence test. */
+		return 0;
+	}
+
+	if (tsec->sid == fsec->sid && fsec->isid == isec->sid
+	    && fsec->pseqno == avc_policy_seqno())
+		return selinux_netlbl_inode_permission(inode, mask);
+
+	return selinux_revalidate_file_permission(file, mask);
+}
+
 static int selinux_file_alloc_security(struct file *file)
 {
 	return file_alloc_security(file);
@@ -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;
+	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.*/
+	return inode_has_perm(current, inode, file_to_av(file), NULL);
+}
+
 /* task security operations */
 
 static int selinux_task_create(unsigned long clone_flags)
@@ -4780,6 +4819,8 @@ static struct security_operations selinu
 	.file_send_sigiotask =		selinux_file_send_sigiotask,
 	.file_receive =			selinux_file_receive,
 
+	.dentry_open =                  selinux_dentry_open,
+
 	.task_create =			selinux_task_create,
 	.task_alloc_security =		selinux_task_alloc_security,
 	.task_free_security =		selinux_task_free_security,
diff -purN -X linux-2.6.22/Documentation/dontdiff linux-2.6.22.orig/security/selinux/include/avc.h linux-2.6.22/security/selinux/include/avc.h
--- linux-2.6.22.orig/security/selinux/include/avc.h	2007-07-09 08:32:17.000000000 +0900
+++ linux-2.6.22/security/selinux/include/avc.h	2007-09-12 08:24:27.000000000 +0900
@@ -110,6 +110,8 @@ int avc_has_perm(u32 ssid, u32 tsid,
                  u16 tclass, u32 requested,
                  struct avc_audit_data *auditdata);
 
+u32 avc_policy_seqno(void);
+
 #define AVC_CALLBACK_GRANT		1
 #define AVC_CALLBACK_TRY_REVOKE		2
 #define AVC_CALLBACK_REVOKE		4
diff -purN -X linux-2.6.22/Documentation/dontdiff linux-2.6.22.orig/security/selinux/include/objsec.h linux-2.6.22/security/selinux/include/objsec.h
--- linux-2.6.22.orig/security/selinux/include/objsec.h	2007-07-09 08:32:17.000000000 +0900
+++ linux-2.6.22/security/selinux/include/objsec.h	2007-09-12 08:24:27.000000000 +0900
@@ -53,6 +53,8 @@ struct file_security_struct {
 	struct file *file;              /* back pointer to file object */
 	u32 sid;              /* SID of open file description */
 	u32 fown_sid;         /* SID of file owner (for SIGIO) */
+	u32 isid;             /* SID of inode at the time of file open */
+	u32 pseqno;           /* Policy seqno at the time of file open */
 };
 
 struct superblock_security_struct {
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;
+
 	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.
+ *
  * Security hooks for task operations.
  *
  * @task_create:
@@ -1253,6 +1258,7 @@ struct security_operations {
 	int (*file_send_sigiotask) (struct task_struct * tsk,
 				    struct fown_struct * fown, int sig);
 	int (*file_receive) (struct file * file);
+	int (*dentry_open)  (struct file *file);
 
 	int (*task_create) (unsigned long clone_flags);
 	int (*task_alloc_security) (struct task_struct * p);
@@ -1854,6 +1860,11 @@ static inline int security_file_receive 
 	return security_ops->file_receive (file);
 }
 
+static inline int security_dentry_open (struct file *file)
+{
+	return security_ops->dentry_open (file);
+}
+
 static inline int security_task_create (unsigned long clone_flags)
 {
 	return security_ops->task_create (clone_flags);
@@ -2529,6 +2540,11 @@ static inline int security_file_receive 
 	return 0;
 }
 
+static inline int security_dentry_open (struct file *file)
+{
+	return 0;
+}
+
 static inline int security_task_create (unsigned long clone_flags)
 {
 	return 0;
--- linux-2.6.22.orig/security/dummy.c	2007-07-09 08:32:17.000000000 +0900
+++ linux-2.6.22/security/dummy.c	2007-09-12 08:29:45.000000000 +0900
@@ -459,6 +459,11 @@ static int dummy_file_receive (struct fi
 	return 0;
 }
 
+static int dummy_dentry_open (struct file *file)
+{
+	return 0;
+}
+
 static int dummy_task_create (unsigned long clone_flags)
 {
 	return 0;
@@ -1029,6 +1034,7 @@ void security_fixup_ops (struct security
 	set_to_dummy_if_null(ops, file_set_fowner);
 	set_to_dummy_if_null(ops, file_send_sigiotask);
 	set_to_dummy_if_null(ops, file_receive);
+	set_to_dummy_if_null(ops, dentry_open);
 	set_to_dummy_if_null(ops, task_create);
 	set_to_dummy_if_null(ops, task_alloc_security);
 	set_to_dummy_if_null(ops, task_free_security);

Regards,
-- 
Yuichi Nakamura
Hitachi Software Engineering Co., Ltd.
Japan SELinux Users Group(JSELUG): http://www.selinux.gr.jp/
SELinux Policy Editor: http://seedit.sourceforge.net/


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC]selinux: Improving SELinux read/write performance
  2007-09-12  8:51       ` Yuichi Nakamura
@ 2007-09-13 12:58         ` Stephen Smalley
  2007-09-14  0:10           ` Yuichi Nakamura
  0 siblings, 1 reply; 7+ messages in thread
From: Stephen Smalley @ 2007-09-13 12:58 UTC (permalink / raw)
  To: Yuichi Nakamura
  Cc: selinux, busybox, James Morris, Eric Paris, kaigai,
	linux-security-module, linux-kernel

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@hitachisoft.jp>
> ---
>  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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC]selinux: Improving SELinux read/write performance
  2007-09-13 12:58         ` Stephen Smalley
@ 2007-09-14  0:10           ` Yuichi Nakamura
  0 siblings, 0 replies; 7+ messages in thread
From: Yuichi Nakamura @ 2007-09-14  0:10 UTC (permalink / raw)
  To: Stephen Smalley
  Cc: ynakam, selinux, busybox, James Morris, Eric Paris, kaigai,
	linux-security-module, linux-kernel


On Thu, 13 Sep 2007 08:58:32 -0400
Stephen Smalley wrote:
> On Wed, 2007-09-12 at 17:51 +0900, Yuichi Nakamura wrote:
<snip>
> Thanks, a few comments below.
Thanks for comments!

> > 
> > * 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.
I see, I will use that.

> > 
> > Signed-off-by: Yuichi Nakamura<ynakam@hitachisoft.jp>
> > ---
> >  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.
Fixed.

> 
> <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.
> 	   */
Fixed.

> 
> > +	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. 
> 	 */
Fixed.


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

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

> -- 
> Stephen Smalley
> National Security Agency

I would like to send patch in next e-mail in new thread.

Regards,
-- 
Yuichi Nakamura
Hitachi Software Engineering Co., Ltd.
Japan SELinux Users Group(JSELUG): http://www.selinux.gr.jp/
SELinux Policy Editor: http://seedit.sourceforge.net/


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2007-09-14  0:11 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2007-09-06  7:27 [RFC]selinux: Improving SELinux read/write performance Yuichi Nakamura
2007-09-06 13:47 ` Stephen Smalley
2007-09-10  1:31   ` Yuichi Nakamura
2007-09-10 13:02     ` Stephen Smalley
2007-09-12  8:51       ` Yuichi Nakamura
2007-09-13 12:58         ` Stephen Smalley
2007-09-14  0:10           ` Yuichi Nakamura

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox