[RFC]Tuning selinux_file_permission

[RFC]Tuning selinux_file_permission

[Yuichi Nakamura]

Hi.

As I posted before, 
I found big overhead in read/write on some CPUs.
http://marc.info/?t=118845343400001&r=1&w=2

I tried tuning based on manual inlining of selinux_file_permission, 
and it works to some extent,
but Stephen suggested better idea.

Stephen Smalley wrote:
> I'd also like to separately explore a different approach for improving
> the overhead on read/write that has come up previously, namely don't
> recheck at all unless one of the following conditions is met:
> 1) process SID has changed since open-time check (i.e. exec with SID
> transition or setcon),
> 2) file SID has changed since open-time check (i.e. setxattr) ,
> 3) policy seqno has changed since open-time check (i.e. policy reload or
> boolean change).

I tried tuning of selinux_file_permission based on this idea.
I wrote a patch that shows the concept, and it can reduce much overhead.
I want comments from community.

1. Detail of patch
1) Prepared global serial number, u32 sid_serial.
 It is initialized as 1, 
 is incremented when sid is changed in the system:
   - policy is reloaded
   - boolean is changed
   - domain transition/setcon happend 
   - setxattr happend

2) Added "sid_serial" member to struct file_secuirty.

3) In file open, file_security->sid_serial is set as global sid_serial.

4) In file read/write, selinux_file_permission is called. 
   file_security->sid_serial and global sid_serial is compared 
   before permission check.
   If sid_serial is not changed, permission check is skipped.
   If it is different, this means some relabel could happen after file open,
   then call do_selinux_file_permission and permission recheck happen.

5) If sid_serial is incremented too much, 
   and returned to zero.
   permission recheck in selinux_file_permission is forced by notify_sid_serial_end().
   This is to avoid following situation.
   * sid_serial = 10 at file open
   * sid_serial is incremented 2^32+10 times, then sid_serial returns to 10
   * In file read, selinux_file_permission is called.
     sid_serial is unchanged(=10), then permission check is skipped.
   * sid may be changed, but check is skipped

2. Benchmark
lmbench simple read/write.

1) Result for x86(Pentium 4)
             Base     SELinux   Overhead(%)
Simple read  1.1034   1.116    1.16(before 12.3)
Simple write 0.9989   1.018    1.97(before 14.0)
 * Base = SELinux disabled kernel

2) Result for SH(SuperH, processor for embedded devices)
                Base   SELinux  Overhead(%)
Simple read     2.6781  2.67    -0.37(before 141.5)
Simple write    2.0781  2.34     12.5(before 155.9)

Overhead is reduced a lot in both PC and embedded devices.


Below is a patch.

---
 security/selinux/avc.c            |   49 +++++++++++++++++++++++++
 security/selinux/hooks.c          |   74 +++++++++++++++++++++++++++++++++-----
 security/selinux/include/avc.h    |    4 ++
 security/selinux/include/objsec.h |    2 +
 security/selinux/selinuxfs.c      |    3 +
 5 files changed, 124 insertions(+), 8 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-03 14:22:22.000000000 +0900
@@ -29,8 +29,10 @@
 #include <linux/audit.h>
 #include <linux/ipv6.h>
 #include <net/ipv6.h>
+#include <linux/file.h>
 #include "avc.h"
 #include "avc_ss.h"
+#include "objsec.h"
 
 static const struct av_perm_to_string av_perm_to_string[] = {
 #define S_(c, v, s) { c, v, s },
@@ -126,6 +128,15 @@ static struct avc_cache avc_cache;
 static struct avc_callback_node *avc_callbacks;
 static struct kmem_cache *avc_node_cachep;
 
+/*This number is incremented when sids are changed
+ - policy reload
+ - boolean change
+ - domain transition
+ - setxattr
+*/
+static u32 sid_serial = 1;
+
+
 static inline int avc_hash(u32 ssid, u32 tsid, u16 tclass)
 {
 	return (ssid ^ (tsid<<2) ^ (tclass<<4)) & (AVC_CACHE_SLOTS - 1);
@@ -913,3 +924,41 @@ int avc_has_perm(u32 ssid, u32 tsid, u16
 	avc_audit(ssid, tsid, tclass, requested, &avd, rc, auditdata);
 	return rc;
 }
+
+/*
+  Notify all processes that sid_serial returned to zero.
+*/
+void notify_sid_serial_end()
+{
+	struct task_struct *p;
+	struct files_struct *files;
+	struct file *file;
+	struct file_security_struct *fsec;
+	int i;
+
+	/*Mutex is not considered yet!*/
+	for_each_process(p) {
+		files = p->files;
+		for (i = 0; i < atomic_read(&(files->count)); i++) {
+			file = files->fd_array[i];
+			if (file) {
+				fsec = file->f_security;
+				if (fsec)
+					fsec->force_file_check = 1;
+			}
+		}
+	}
+}
+
+void sid_serial_incr()
+{
+	sid_serial++;
+	if (sid_serial == 0)
+		notify_sid_serial_end();
+
+}
+
+u32 read_sid_serial()
+{
+	return sid_serial;
+}
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-03 14:31:54.000000000 +0900
@@ -220,7 +220,9 @@ static int file_alloc_security(struct fi
 
 	fsec->file = file;
 	fsec->sid = tsec->sid;
+	fsec->force_file_check = 0;
 	fsec->fown_sid = tsec->sid;
+	fsec->sid_serial = read_sid_serial();
 	file->f_security = fsec;
 
 	return 0;
@@ -991,6 +993,7 @@ out_unlock:
 out:
 	if (isec->sclass == SECCLASS_FILE)
 		isec->sclass = inode_mode_to_security_class(inode->i_mode);
+
 	return rc;
 }
 
@@ -1691,6 +1694,8 @@ static int selinux_bprm_set_security(str
 
 		/* Set the security field to the new SID. */
 		bsec->sid = newsid;
+
+		sid_serial_incr();
 	}
 
 	bsec->set = 1;
@@ -2289,7 +2294,7 @@ static int selinux_inode_getattr(struct 
 	return dentry_has_perm(current, mnt, dentry, FILE__GETATTR);
 }
 
-static int selinux_inode_setxattr(struct dentry *dentry, char *name, void *value, size_t size, int flags)
+static inline int do_selinux_inode_setxattr(struct dentry *dentry, char *name, void *value, size_t size, int flags)
 {
 	struct task_security_struct *tsec = current->security;
 	struct inode *inode = dentry->d_inode;
@@ -2348,6 +2353,17 @@ static int selinux_inode_setxattr(struct
 			    FILESYSTEM__ASSOCIATE,
 			    &ad);
 }
+static int selinux_inode_setxattr(struct dentry *dentry, char *name, void *value, size_t size, int flags)
+{
+	int rc;
+
+	rc = do_selinux_inode_setxattr(dentry, name, value, size, flags);
+	if (rc)
+		return rc;
+
+	sid_serial_incr();
+	return 0;
+}
 
 static void selinux_inode_post_setxattr(struct dentry *dentry, char *name,
                                         void *value, size_t size, int flags)
@@ -2457,17 +2473,11 @@ 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;
 
-	if (!mask) {
-		/* No permission to check.  Existence test. */
-		return 0;
-	}
-
 	/* file_mask_to_av won't add FILE__WRITE if MAY_APPEND is set */
 	if ((file->f_flags & O_APPEND) && (mask & MAY_WRITE))
 		mask |= MAY_APPEND;
@@ -2480,6 +2490,53 @@ static int selinux_file_permission(struc
 	return selinux_netlbl_inode_permission(inode, mask);
 }
 
+static int selinux_file_permission(struct file *file, int mask)
+{
+
+	struct task_security_struct *tsec = current->security;
+	struct file_security_struct *fsec = file->f_security;
+	int rc;
+	u32 current_sid_serial;
+
+	if (!mask) {
+		/* No permission to check.  Existence test. */
+		return 0;
+	}
+
+	/*Check FS__USE*/
+	if (tsec->sid != fsec->sid) {
+		struct vfsmount *mnt = file->f_path.mnt;
+		struct dentry *dentry = file->f_path.dentry;
+		struct avc_audit_data ad;
+		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;
+	}
+
+	/*Skip permission check
+	  when sids are not changed after open*/
+	current_sid_serial = read_sid_serial();
+	if (fsec->sid_serial == current_sid_serial &&
+	    !(fsec->force_file_check))
+		return 0;
+
+	rc = do_selinux_file_permission(file, mask);
+	if (rc)
+		return rc;
+
+	fsec->sid_serial = current_sid_serial;
+	fsec->force_file_check = 0;
+
+	return 0;
+}
+
+
 static int selinux_file_alloc_security(struct file *file)
 {
 	return file_alloc_security(file);
@@ -4642,6 +4699,7 @@ static int selinux_setprocattr(struct ta
 	else
 		return -EINVAL;
 
+	sid_serial_incr();
 	return size;
 }
 
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-03 09:05:47.000000000 +0900
@@ -127,6 +127,10 @@ int avc_add_callback(int (*callback)(u32
 
 /* Exported to selinuxfs */
 int avc_get_hash_stats(char *page);
+
+void sid_serial_incr();
+u32 read_sid_serial();
+
 extern unsigned int avc_cache_threshold;
 
 #ifdef CONFIG_SECURITY_SELINUX_AVC_STATS
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-03 14:28:18.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 sid_serial;       /* sid_serial at open time*/
+	bool force_file_check; /* It is set when sid_serial returns zero*/
 };
 
 struct superblock_security_struct {
diff -purN -X linux-2.6.22/Documentation/dontdiff linux-2.6.22.orig/security/selinux/selinuxfs.c linux-2.6.22/security/selinux/selinuxfs.c
--- linux-2.6.22.orig/security/selinux/selinuxfs.c	2007-07-09 08:32:17.000000000 +0900
+++ linux-2.6.22/security/selinux/selinuxfs.c	2007-09-03 09:10:38.000000000 +0900
@@ -294,6 +294,7 @@ static ssize_t sel_write_load(struct fil
 	audit_log(current->audit_context, GFP_KERNEL, AUDIT_MAC_POLICY_LOAD,
 		"policy loaded auid=%u",
 		audit_get_loginuid(current->audit_context));
+	sid_serial_incr();
 out:
 	mutex_unlock(&sel_mutex);
 	vfree(data);
@@ -872,6 +873,8 @@ static ssize_t sel_write_bool(struct fil
 	bool_pending_values[inode->i_ino&SEL_INO_MASK] = new_value;
 	length = count;
 
+	sid_serial_incr();
+
 out:
 	mutex_unlock(&sel_mutex);
 	if (page)

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/


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [RFC]Tuning selinux_file_permission

[Stephen Smalley]

On Mon, 2007-09-03 at 17:04 +0900, Yuichi Nakamura wrote:
> Hi.
> 
> As I posted before, 
> I found big overhead in read/write on some CPUs.
> http://marc.info/?t=118845343400001&r=1&w=2
> 
> I tried tuning based on manual inlining of selinux_file_permission, 
> and it works to some extent,
> but Stephen suggested better idea.
> 
> Stephen Smalley wrote:
> > I'd also like to separately explore a different approach for improving
> > the overhead on read/write that has come up previously, namely don't
> > recheck at all unless one of the following conditions is met:
> > 1) process SID has changed since open-time check (i.e. exec with SID
> > transition or setcon),
> > 2) file SID has changed since open-time check (i.e. setxattr) ,
> > 3) policy seqno has changed since open-time check (i.e. policy reload or
> > boolean change).
> 
> I tried tuning of selinux_file_permission based on this idea.
> I wrote a patch that shows the concept, and it can reduce much overhead.
> I want comments from community.
> 
> 1. Detail of patch
> 1) Prepared global serial number, u32 sid_serial.

Perhaps this should be an atomic_t?

>  It is initialized as 1, 
>  is incremented when sid is changed in the system:
>    - policy is reloaded
>    - boolean is changed
>    - domain transition/setcon happend 
>    - setxattr happend

This is very coarse-grained; while a policy reload or boolean change may
affect many processes and files (thus a global counter like the policy
seqno is appropriate), a process or inode SID change can be localized to
that process or inode and doesn't need to affect all read/write
operations.  Why should a domain transition in some unrelated process
cause all of my process' file accesses to get rechecked on subsequent
read/write calls?

Did you consider or try the alternative of saving the (process SID,
inode SID, policy seqno) triple in the file security struct at open time
and comparing with the current values at read/write time, only
rechecking access upon changes to any one of those three values?  That
would require a new hook in the open code path at a point where we have
the struct file and the inode, likely from __dentry_open, so that we
could extract the inode SID and save it to a new field in the the file
security struct.  The task SID is already effectively saved in the file
security struct since fsec->sid = tsec->sid upon file_alloc_security.
Extracting the policy seqno would require a new interface to the AVC.

On the other hand, your approach is simpler.

> 
> 2) Added "sid_serial" member to struct file_secuirty.
> 
> 3) In file open, file_security->sid_serial is set as global sid_serial.
> 
> 4) In file read/write, selinux_file_permission is called. 
>    file_security->sid_serial and global sid_serial is compared 
>    before permission check.
>    If sid_serial is not changed, permission check is skipped.
>    If it is different, this means some relabel could happen after file open,
>    then call do_selinux_file_permission and permission recheck happen.
> 
> 5) If sid_serial is incremented too much, 
>    and returned to zero.
>    permission recheck in selinux_file_permission is forced by notify_sid_serial_end().
>    This is to avoid following situation.
>    * sid_serial = 10 at file open
>    * sid_serial is incremented 2^32+10 times, then sid_serial returns to 10
>    * In file read, selinux_file_permission is called.
>      sid_serial is unchanged(=10), then permission check is skipped.
>    * sid may be changed, but check is skipped

This would be less of a concern if you weren't using a single global
counter for changes to any process or file SID.  But can't you avoid the
need for this notify_sid_serial_end() stuff just by ceasing to increment
the counter if it ever overflows to zero, and not skipping checks if it
is zero?  Just means that system performance will degrade if/when the
counter overflows.

The AVC will discard entries from the security server if the seqno is
less than its last revocation notification seqno, so if we have rollover
there, we'll end up not caching anything further.  But that is much less
likely too, as policy seqno only changes on policy reload or boolean
change.

> 2. Benchmark
> lmbench simple read/write.
> 
> 1) Result for x86(Pentium 4)
>              Base     SELinux   Overhead(%)
> Simple read  1.1034   1.116    1.16(before 12.3)
> Simple write 0.9989   1.018    1.97(before 14.0)
>  * Base = SELinux disabled kernel
> 
> 2) Result for SH(SuperH, processor for embedded devices)
>                 Base   SELinux  Overhead(%)
> Simple read     2.6781  2.67    -0.37(before 141.5)
> Simple write    2.0781  2.34     12.5(before 155.9)
> 
> Overhead is reduced a lot in both PC and embedded devices.

That is quite a win.

> 
> Below is a patch.
> 
> ---
>  security/selinux/avc.c            |   49 +++++++++++++++++++++++++
>  security/selinux/hooks.c          |   74 +++++++++++++++++++++++++++++++++-----
>  security/selinux/include/avc.h    |    4 ++
>  security/selinux/include/objsec.h |    2 +
>  security/selinux/selinuxfs.c      |    3 +
>  5 files changed, 124 insertions(+), 8 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-03 14:22:22.000000000 +0900
> @@ -29,8 +29,10 @@
>  #include <linux/audit.h>
>  #include <linux/ipv6.h>
>  #include <net/ipv6.h>
> +#include <linux/file.h>
>  #include "avc.h"
>  #include "avc_ss.h"
> +#include "objsec.h"
>  
>  static const struct av_perm_to_string av_perm_to_string[] = {
>  #define S_(c, v, s) { c, v, s },
> @@ -126,6 +128,15 @@ static struct avc_cache avc_cache;
>  static struct avc_callback_node *avc_callbacks;
>  static struct kmem_cache *avc_node_cachep;
>  
> +/*This number is incremented when sids are changed
> + - policy reload
> + - boolean change
> + - domain transition
> + - setxattr
> +*/
> +static u32 sid_serial = 1;

atomic_t?

> +
> +
>  static inline int avc_hash(u32 ssid, u32 tsid, u16 tclass)
>  {
>  	return (ssid ^ (tsid<<2) ^ (tclass<<4)) & (AVC_CACHE_SLOTS - 1);
> @@ -913,3 +924,41 @@ int avc_has_perm(u32 ssid, u32 tsid, u16
>  	avc_audit(ssid, tsid, tclass, requested, &avd, rc, auditdata);
>  	return rc;
>  }
> +
> +/*
> +  Notify all processes that sid_serial returned to zero.
> +*/
> +void notify_sid_serial_end()
> +{
> +	struct task_struct *p;
> +	struct files_struct *files;
> +	struct file *file;
> +	struct file_security_struct *fsec;
> +	int i;
> +
> +	/*Mutex is not considered yet!*/

Well, that's pretty important to consider, and you might have a look at
the revoke code as an example.  But I'd rather eliminate the need for
this kind of global walk of all open files altogether.  If you are
initializing it to 1, then you know that a value of 0 is invalid, so you
can make sure that you don't increment it again if it ever hits 0 and
don't skip checks if it equals 0.  Right?

> +	for_each_process(p) {
> +		files = p->files;
> +		for (i = 0; i < atomic_read(&(files->count)); i++) {
> +			file = files->fd_array[i];
> +			if (file) {
> +				fsec = file->f_security;
> +				if (fsec)
> +					fsec->force_file_check = 1;
> +			}
> +		}
> +	}
> +}
> +
> +void sid_serial_incr()
> +{
> +	sid_serial++;
> +	if (sid_serial == 0)
> +		notify_sid_serial_end();

atomic_inc?

> +
> +}
> +
> +u32 read_sid_serial()
> +{
> +	return sid_serial;
> +}

atomic_read?

> 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-03 14:31:54.000000000 +0900
> @@ -220,7 +220,9 @@ static int file_alloc_security(struct fi
>  
>  	fsec->file = file;
>  	fsec->sid = tsec->sid;
> +	fsec->force_file_check = 0;
>  	fsec->fown_sid = tsec->sid;
> +	fsec->sid_serial = read_sid_serial();
>  	file->f_security = fsec;
>  
>  	return 0;
> @@ -991,6 +993,7 @@ out_unlock:
>  out:
>  	if (isec->sclass == SECCLASS_FILE)
>  		isec->sclass = inode_mode_to_security_class(inode->i_mode);
> +

Extraneous to this patch.

>  	return rc;
>  }
>  
> @@ -1691,6 +1694,8 @@ static int selinux_bprm_set_security(str
>  
>  		/* Set the security field to the new SID. */
>  		bsec->sid = newsid;
> +
> +		sid_serial_incr();
>  	}
>  
>  	bsec->set = 1;
> @@ -2289,7 +2294,7 @@ static int selinux_inode_getattr(struct 
>  	return dentry_has_perm(current, mnt, dentry, FILE__GETATTR);
>  }
>  
> -static int selinux_inode_setxattr(struct dentry *dentry, char *name, void *value, size_t size, int flags)
> +static inline int do_selinux_inode_setxattr(struct dentry *dentry, char *name, void *value, size_t size, int flags)
>  {
>  	struct task_security_struct *tsec = current->security;
>  	struct inode *inode = dentry->d_inode;
> @@ -2348,6 +2353,17 @@ static int selinux_inode_setxattr(struct
>  			    FILESYSTEM__ASSOCIATE,
>  			    &ad);
>  }
> +static int selinux_inode_setxattr(struct dentry *dentry, char *name, void *value, size_t size, int flags)
> +{
> +	int rc;
> +
> +	rc = do_selinux_inode_setxattr(dentry, name, value, size, flags);
> +	if (rc)
> +		return rc;
> +
> +	sid_serial_incr();

That would increment it on all setxattr calls, not just setxattr of
SELinux attributes.

> +	return 0;
> +}
>  
>  static void selinux_inode_post_setxattr(struct dentry *dentry, char *name,
>                                          void *value, size_t size, int flags)
> @@ -2457,17 +2473,11 @@ 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;
>  
> -	if (!mask) {
> -		/* No permission to check.  Existence test. */
> -		return 0;
> -	}
> -
>  	/* file_mask_to_av won't add FILE__WRITE if MAY_APPEND is set */
>  	if ((file->f_flags & O_APPEND) && (mask & MAY_WRITE))
>  		mask |= MAY_APPEND;
> @@ -2480,6 +2490,53 @@ static int selinux_file_permission(struc
>  	return selinux_netlbl_inode_permission(inode, mask);
>  }
>  
> +static int selinux_file_permission(struct file *file, int mask)
> +{
> +
> +	struct task_security_struct *tsec = current->security;
> +	struct file_security_struct *fsec = file->f_security;
> +	int rc;
> +	u32 current_sid_serial;
> +
> +	if (!mask) {
> +		/* No permission to check.  Existence test. */
> +		return 0;
> +	}
> +
> +	/*Check FS__USE*/

Useless comment (and not quite right - FD__USE).

> +	if (tsec->sid != fsec->sid) {
> +		struct vfsmount *mnt = file->f_path.mnt;
> +		struct dentry *dentry = file->f_path.dentry;
> +		struct avc_audit_data ad;
> +		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;
> +	}

Seems like this would follow the decision of whether or not to skip the
check, as you don't need to recheck FD__USE either if the task SID
hasn't changed.

> +
> +	/*Skip permission check
> +	  when sids are not changed after open*/
> +	current_sid_serial = read_sid_serial();
> +	if (fsec->sid_serial == current_sid_serial &&
> +	    !(fsec->force_file_check))
> +		return 0;
> +
> +	rc = do_selinux_file_permission(file, mask);
> +	if (rc)
> +		return rc;
> +
> +	fsec->sid_serial = current_sid_serial;
> +	fsec->force_file_check = 0;
> +
> +	return 0;
> +}
> +
> +
>  static int selinux_file_alloc_security(struct file *file)
>  {
>  	return file_alloc_security(file);
> @@ -4642,6 +4699,7 @@ static int selinux_setprocattr(struct ta
>  	else
>  		return -EINVAL;
>  
> +	sid_serial_incr();
>  	return size;
>  }
>  
> 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-03 09:05:47.000000000 +0900
> @@ -127,6 +127,10 @@ int avc_add_callback(int (*callback)(u32
>  
>  /* Exported to selinuxfs */
>  int avc_get_hash_stats(char *page);
> +
> +void sid_serial_incr();
> +u32 read_sid_serial();
> +
>  extern unsigned int avc_cache_threshold;
>  
>  #ifdef CONFIG_SECURITY_SELINUX_AVC_STATS
> 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-03 14:28:18.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 sid_serial;       /* sid_serial at open time*/
> +	bool force_file_check; /* It is set when sid_serial returns zero*/
>  };
>  
>  struct superblock_security_struct {
> diff -purN -X linux-2.6.22/Documentation/dontdiff linux-2.6.22.orig/security/selinux/selinuxfs.c linux-2.6.22/security/selinux/selinuxfs.c
> --- linux-2.6.22.orig/security/selinux/selinuxfs.c	2007-07-09 08:32:17.000000000 +0900
> +++ linux-2.6.22/security/selinux/selinuxfs.c	2007-09-03 09:10:38.000000000 +0900
> @@ -294,6 +294,7 @@ static ssize_t sel_write_load(struct fil
>  	audit_log(current->audit_context, GFP_KERNEL, AUDIT_MAC_POLICY_LOAD,
>  		"policy loaded auid=%u",
>  		audit_get_loginuid(current->audit_context));
> +	sid_serial_incr();
>  out:
>  	mutex_unlock(&sel_mutex);
>  	vfree(data);
> @@ -872,6 +873,8 @@ static ssize_t sel_write_bool(struct fil
>  	bool_pending_values[inode->i_ino&SEL_INO_MASK] = new_value;
>  	length = count;
>  
> +	sid_serial_incr();
> +
>  out:
>  	mutex_unlock(&sel_mutex);
>  	if (page)
> 
> Regards,
-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [RFC]Tuning selinux_file_permission

[Yuichi Nakamura]

Hi.

At first, thanks for useful comments and advice.

On Tue, 04 Sep 2007 09:26:16 -0400
Stephen Smalley wrote:
> On Mon, 2007-09-03 at 17:04 +0900, Yuichi Nakamura wrote:
> > Hi.
> > 
> > As I posted before, 
> > I found big overhead in read/write on some CPUs.
> > http://marc.info/?t=118845343400001&r=1&w=2
> > 
> > I tried tuning based on manual inlining of selinux_file_permission, 
> > and it works to some extent,
> > but Stephen suggested better idea.
> > 
> > Stephen Smalley wrote:
> > > I'd also like to separately explore a different approach for improving
> > > the overhead on read/write that has come up previously, namely don't
> > > recheck at all unless one of the following conditions is met:
> > > 1) process SID has changed since open-time check (i.e. exec with SID
> > > transition or setcon),
> > > 2) file SID has changed since open-time check (i.e. setxattr) ,
> > > 3) policy seqno has changed since open-time check (i.e. policy reload or
> > > boolean change).
> > 
> > I tried tuning of selinux_file_permission based on this idea.
> > I wrote a patch that shows the concept, and it can reduce much overhead.
> > I want comments from community.
> > 
> > 1. Detail of patch
> > 1) Prepared global serial number, u32 sid_serial.
> 
> Perhaps this should be an atomic_t?
Thanks, I will use atomic_t in future patch,
if I use such counters.

> 
> >  It is initialized as 1, 
> >  is incremented when sid is changed in the system:
> >    - policy is reloaded
> >    - boolean is changed
> >    - domain transition/setcon happend 
> >    - setxattr happend
> 
> This is very coarse-grained; while a policy reload or boolean change may
> affect many processes and files (thus a global counter like the policy
> seqno is appropriate), a process or inode SID change can be localized to
> that process or inode and doesn't need to affect all read/write
> operations.  Why should a domain transition in some unrelated process
> cause all of my process' file accesses to get rechecked on subsequent
> read/write calls?
> 
> Did you consider or try the alternative of saving the (process SID,
> inode SID, policy seqno) triple in the file security struct at open time
> and comparing with the current values at read/write time, only
> rechecking access upon changes to any one of those three values?  That
> would require a new hook in the open code path at a point where we have
> the struct file and the inode, likely from __dentry_open, so that we
> could extract the inode SID and save it to a new field in the the file
> security struct.  The task SID is already effectively saved in the file
> security struct since fsec->sid = tsec->sid upon file_alloc_security.
> Extracting the policy seqno would require a new interface to the AVC.
Yes, at first I thought to change  LSM, 
but I was afraid to touch LSM, 
because I also have to obtain agreements from LSM community.

After looked at your advice, 
I think code may become simpler.
I would like to try this approach and compare code.


> On the other hand, your approach is simpler.
> 
> > 
> > 2) Added "sid_serial" member to struct file_secuirty.
> > 
> > 3) In file open, file_security->sid_serial is set as global sid_serial.
> > 
> > 4) In file read/write, selinux_file_permission is called. 
> >    file_security->sid_serial and global sid_serial is compared 
> >    before permission check.
> >    If sid_serial is not changed, permission check is skipped.
> >    If it is different, this means some relabel could happen after file open,
> >    then call do_selinux_file_permission and permission recheck happen.
> > 
> > 5) If sid_serial is incremented too much, 
> >    and returned to zero.
> >    permission recheck in selinux_file_permission is forced by notify_sid_serial_end().
> >    This is to avoid following situation.
> >    * sid_serial = 10 at file open
> >    * sid_serial is incremented 2^32+10 times, then sid_serial returns to 10
> >    * In file read, selinux_file_permission is called.
> >      sid_serial is unchanged(=10), then permission check is skipped.
> >    * sid may be changed, but check is skipped
> 
> This would be less of a concern if you weren't using a single global
> counter for changes to any process or file SID.  But can't you avoid the
> need for this notify_sid_serial_end() stuff just by ceasing to increment
> the counter if it ever overflows to zero, and not skipping checks if it
> is zero?  Just means that system performance will degrade if/when the
> counter overflows.
> 
> The AVC will discard entries from the security server if the seqno is
> less than its last revocation notification seqno, so if we have rollover
> there, we'll end up not caching anything further.  But that is much less
> likely too, as policy seqno only changes on policy reload or boolean
> change.
> 
> > 2. Benchmark
> > lmbench simple read/write.
> > 
> > 1) Result for x86(Pentium 4)
> >              Base     SELinux   Overhead(%)
> > Simple read  1.1034   1.116    1.16(before 12.3)
> > Simple write 0.9989   1.018    1.97(before 14.0)
> >  * Base = SELinux disabled kernel
> > 
> > 2) Result for SH(SuperH, processor for embedded devices)
> >                 Base   SELinux  Overhead(%)
> > Simple read     2.6781  2.67    -0.37(before 141.5)
> > Simple write    2.0781  2.34     12.5(before 155.9)
> > 
> > Overhead is reduced a lot in both PC and embedded devices.
> 
> That is quite a win.
> 
> > 
> > Below is a patch.
> > 
> > ---
> >  security/selinux/avc.c            |   49 +++++++++++++++++++++++++
> >  security/selinux/hooks.c          |   74 +++++++++++++++++++++++++++++++++-----
> >  security/selinux/include/avc.h    |    4 ++
> >  security/selinux/include/objsec.h |    2 +
> >  security/selinux/selinuxfs.c      |    3 +
> >  5 files changed, 124 insertions(+), 8 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-03 14:22:22.000000000 +0900
> > @@ -29,8 +29,10 @@
> >  #include <linux/audit.h>
> >  #include <linux/ipv6.h>
> >  #include <net/ipv6.h>
> > +#include <linux/file.h>
> >  #include "avc.h"
> >  #include "avc_ss.h"
> > +#include "objsec.h"
> >  
> >  static const struct av_perm_to_string av_perm_to_string[] = {
> >  #define S_(c, v, s) { c, v, s },
> > @@ -126,6 +128,15 @@ static struct avc_cache avc_cache;
> >  static struct avc_callback_node *avc_callbacks;
> >  static struct kmem_cache *avc_node_cachep;
> >  
> > +/*This number is incremented when sids are changed
> > + - policy reload
> > + - boolean change
> > + - domain transition
> > + - setxattr
> > +*/
> > +static u32 sid_serial = 1;
> 
> atomic_t?
> 
> > +
> > +
> >  static inline int avc_hash(u32 ssid, u32 tsid, u16 tclass)
> >  {
> >  	return (ssid ^ (tsid<<2) ^ (tclass<<4)) & (AVC_CACHE_SLOTS - 1);
> > @@ -913,3 +924,41 @@ int avc_has_perm(u32 ssid, u32 tsid, u16
> >  	avc_audit(ssid, tsid, tclass, requested, &avd, rc, auditdata);
> >  	return rc;
> >  }
> > +
> > +/*
> > +  Notify all processes that sid_serial returned to zero.
> > +*/
> > +void notify_sid_serial_end()
> > +{
> > +	struct task_struct *p;
> > +	struct files_struct *files;
> > +	struct file *file;
> > +	struct file_security_struct *fsec;
> > +	int i;
> > +
> > +	/*Mutex is not considered yet!*/
> 
> Well, that's pretty important to consider, and you might have a look at
> the revoke code as an example.  But I'd rather eliminate the need for
> this kind of global walk of all open files altogether.  If you are
> initializing it to 1, then you know that a value of 0 is invalid, so you
> can make sure that you don't increment it again if it ever hits 0 and
> don't skip checks if it equals 0.  Right?
I see. 
If we adopted approach that add new LSM hook,
this logic will be OK.
because it is less likely to overflow counter as you say.
People will rarely load_policy 2^32 times.



> 
> > +	for_each_process(p) {
> > +		files = p->files;
> > +		for (i = 0; i < atomic_read(&(files->count)); i++) {
> > +			file = files->fd_array[i];
> > +			if (file) {
> > +				fsec = file->f_security;
> > +				if (fsec)
> > +					fsec->force_file_check = 1;
> > +			}
> > +		}
> > +	}
> > +}
> > +
> > +void sid_serial_incr()
> > +{
> > +	sid_serial++;
> > +	if (sid_serial == 0)
> > +		notify_sid_serial_end();
> 
> atomic_inc?
> 
> > +
> > +}
> > +
> > +u32 read_sid_serial()
> > +{
> > +	return sid_serial;
> > +}
> 
> atomic_read?
> 
> > 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-03 14:31:54.000000000 +0900
> > @@ -220,7 +220,9 @@ static int file_alloc_security(struct fi
> >  
> >  	fsec->file = file;
> >  	fsec->sid = tsec->sid;
> > +	fsec->force_file_check = 0;
> >  	fsec->fown_sid = tsec->sid;
> > +	fsec->sid_serial = read_sid_serial();
> >  	file->f_security = fsec;
> >  
> >  	return 0;
> > @@ -991,6 +993,7 @@ out_unlock:
> >  out:
> >  	if (isec->sclass == SECCLASS_FILE)
> >  		isec->sclass = inode_mode_to_security_class(inode->i_mode);
> > +
> 
> Extraneous to this patch.
> 
> >  	return rc;
> >  }
> >  
> > @@ -1691,6 +1694,8 @@ static int selinux_bprm_set_security(str
> >  
> >  		/* Set the security field to the new SID. */
> >  		bsec->sid = newsid;
> > +
> > +		sid_serial_incr();
> >  	}
> >  
> >  	bsec->set = 1;
> > @@ -2289,7 +2294,7 @@ static int selinux_inode_getattr(struct 
> >  	return dentry_has_perm(current, mnt, dentry, FILE__GETATTR);
> >  }
> >  
> > -static int selinux_inode_setxattr(struct dentry *dentry, char *name, void *value, size_t size, int flags)
> > +static inline int do_selinux_inode_setxattr(struct dentry *dentry, char *name, void *value, size_t size, int flags)
> >  {
> >  	struct task_security_struct *tsec = current->security;
> >  	struct inode *inode = dentry->d_inode;
> > @@ -2348,6 +2353,17 @@ static int selinux_inode_setxattr(struct
> >  			    FILESYSTEM__ASSOCIATE,
> >  			    &ad);
> >  }
> > +static int selinux_inode_setxattr(struct dentry *dentry, char *name, void *value, size_t size, int flags)
> > +{
> > +	int rc;
> > +
> > +	rc = do_selinux_inode_setxattr(dentry, name, value, size, flags);
> > +	if (rc)
> > +		return rc;
> > +
> > +	sid_serial_incr();
> 
> That would increment it on all setxattr calls, not just setxattr of
> SELinux attributes.
> 
> > +	return 0;
> > +}
> >  
> >  static void selinux_inode_post_setxattr(struct dentry *dentry, char *name,
> >                                          void *value, size_t size, int flags)
> > @@ -2457,17 +2473,11 @@ 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;
> >  
> > -	if (!mask) {
> > -		/* No permission to check.  Existence test. */
> > -		return 0;
> > -	}
> > -
> >  	/* file_mask_to_av won't add FILE__WRITE if MAY_APPEND is set */
> >  	if ((file->f_flags & O_APPEND) && (mask & MAY_WRITE))
> >  		mask |= MAY_APPEND;
> > @@ -2480,6 +2490,53 @@ static int selinux_file_permission(struc
> >  	return selinux_netlbl_inode_permission(inode, mask);
> >  }
> >  
> > +static int selinux_file_permission(struct file *file, int mask)
> > +{
> > +
> > +	struct task_security_struct *tsec = current->security;
> > +	struct file_security_struct *fsec = file->f_security;
> > +	int rc;
> > +	u32 current_sid_serial;
> > +
> > +	if (!mask) {
> > +		/* No permission to check.  Existence test. */
> > +		return 0;
> > +	}
> > +
> > +	/*Check FS__USE*/
> 
> Useless comment (and not quite right - FD__USE).
> 
> > +	if (tsec->sid != fsec->sid) {
> > +		struct vfsmount *mnt = file->f_path.mnt;
> > +		struct dentry *dentry = file->f_path.dentry;
> > +		struct avc_audit_data ad;
> > +		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;
> > +	}
> 
> Seems like this would follow the decision of whether or not to skip the
> check, as you don't need to recheck FD__USE either if the task SID
> hasn't changed.
I will fix it.


> 
> > +
> > +	/*Skip permission check
> > +	  when sids are not changed after open*/
> > +	current_sid_serial = read_sid_serial();
> > +	if (fsec->sid_serial == current_sid_serial &&
> > +	    !(fsec->force_file_check))
> > +		return 0;
> > +
> > +	rc = do_selinux_file_permission(file, mask);
> > +	if (rc)
> > +		return rc;
> > +
> > +	fsec->sid_serial = current_sid_serial;
> > +	fsec->force_file_check = 0;
> > +
> > +	return 0;
> > +}
> > +
> > +
> >  static int selinux_file_alloc_security(struct file *file)
> >  {
> >  	return file_alloc_security(file);
> > @@ -4642,6 +4699,7 @@ static int selinux_setprocattr(struct ta
> >  	else
> >  		return -EINVAL;
> >  
> > +	sid_serial_incr();
> >  	return size;
> >  }
> >  
> > 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-03 09:05:47.000000000 +0900
> > @@ -127,6 +127,10 @@ int avc_add_callback(int (*callback)(u32
> >  
> >  /* Exported to selinuxfs */
> >  int avc_get_hash_stats(char *page);
> > +
> > +void sid_serial_incr();
> > +u32 read_sid_serial();
> > +
> >  extern unsigned int avc_cache_threshold;
> >  
> >  #ifdef CONFIG_SECURITY_SELINUX_AVC_STATS
> > 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-03 14:28:18.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 sid_serial;       /* sid_serial at open time*/
> > +	bool force_file_check; /* It is set when sid_serial returns zero*/
> >  };
> >  
> >  struct superblock_security_struct {
> > diff -purN -X linux-2.6.22/Documentation/dontdiff linux-2.6.22.orig/security/selinux/selinuxfs.c linux-2.6.22/security/selinux/selinuxfs.c
> > --- linux-2.6.22.orig/security/selinux/selinuxfs.c	2007-07-09 08:32:17.000000000 +0900
> > +++ linux-2.6.22/security/selinux/selinuxfs.c	2007-09-03 09:10:38.000000000 +0900
> > @@ -294,6 +294,7 @@ static ssize_t sel_write_load(struct fil
> >  	audit_log(current->audit_context, GFP_KERNEL, AUDIT_MAC_POLICY_LOAD,
> >  		"policy loaded auid=%u",
> >  		audit_get_loginuid(current->audit_context));
> > +	sid_serial_incr();
> >  out:
> >  	mutex_unlock(&sel_mutex);
> >  	vfree(data);
> > @@ -872,6 +873,8 @@ static ssize_t sel_write_bool(struct fil
> >  	bool_pending_values[inode->i_ino&SEL_INO_MASK] = new_value;
> >  	length = count;
> >  
> > +	sid_serial_incr();
> > +
> >  out:
> >  	mutex_unlock(&sel_mutex);
> >  	if (page)
> > 
> > Regards,
> -- 
> Stephen Smalley
> National Security Agency
> 
> 
> --
> This message was distributed to subscribers of the selinux mailing list.
> If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
> the words "unsubscribe selinux" without quotes as the message.

Regards,
Yuichi Nakamura

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [RFC]Tuning selinux_file_permission

[Stephen Smalley]

On Mon, 2007-09-03 at 17:04 +0900, Yuichi Nakamura wrote:
> 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-03 14:31:54.000000000 +0900
> @@ -4642,6 +4699,7 @@ static int selinux_setprocattr(struct ta
>  	else
>  		return -EINVAL;
>  
> +	sid_serial_incr();
>  	return size;
>  }

Also, here you only want to increment if setting 'current', and only if
successful, so you want to move this up to where tsec->sid is updated
(two locations).

-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [RFC]Tuning selinux_file_permission

[Paul Moore]

On Monday, September 3 2007 4:04:46 am Yuichi Nakamura wrote:
> +static int selinux_file_permission(struct file *file, int mask)
> +{
> +
> +	struct task_security_struct *tsec = current->security;
> +	struct file_security_struct *fsec = file->f_security;
> +	int rc;
> +	u32 current_sid_serial;
> +
> +	if (!mask) {
> +		/* No permission to check.  Existence test. */
> +		return 0;
> +	}
> +
> +	/*Check FS__USE*/
> +	if (tsec->sid != fsec->sid) {
> +		struct vfsmount *mnt = file->f_path.mnt;
> +		struct dentry *dentry = file->f_path.dentry;
> +		struct avc_audit_data ad;
> +		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;
> +	}
> +
> +	/*Skip permission check
> +	  when sids are not changed after open*/
> +	current_sid_serial = read_sid_serial();
> +	if (fsec->sid_serial == current_sid_serial &&
> +	    !(fsec->force_file_check))
> +		return 0;

Instead of simply returning 0 here, you should return the return value from 
selinux_netlbl_inode_permission just like you are doing in your 
do_selinux_file_permission() function above.

This NetLabel call is required to ensure that the on-the-wire label is set 
correctly for connected stream sockets initiated by a remote host.  It may be 
possible to do away with this call at some point but it requires additional 
functionality which we do not have at present.

> +	rc = do_selinux_file_permission(file, mask);
> +	if (rc)
> +		return rc;
> +
> +	fsec->sid_serial = current_sid_serial;
> +	fsec->force_file_check = 0;
> +
> +	return 0;
> +}

-- 
paul moore
linux security @ hp

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [RFC]Tuning selinux_file_permission

[Yuichi Nakamura]

On Wed, 5 Sep 2007 10:19:44 -0400
Paul Moore wrote:
> On Monday, September 3 2007 4:04:46 am Yuichi Nakamura wrote:
> > +static int selinux_file_permission(struct file *file, int mask)
> > +{
> > +
> > +	struct task_security_struct *tsec = current->security;
> > +	struct file_security_struct *fsec = file->f_security;
> > +	int rc;
> > +	u32 current_sid_serial;
> > +
> > +	if (!mask) {
> > +		/* No permission to check.  Existence test. */
> > +		return 0;
> > +	}
> > +
> > +	/*Check FS__USE*/
> > +	if (tsec->sid != fsec->sid) {
> > +		struct vfsmount *mnt = file->f_path.mnt;
> > +		struct dentry *dentry = file->f_path.dentry;
> > +		struct avc_audit_data ad;
> > +		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;
> > +	}
> > +
> > +	/*Skip permission check
> > +	  when sids are not changed after open*/
> > +	current_sid_serial = read_sid_serial();
> > +	if (fsec->sid_serial == current_sid_serial &&
> > +	    !(fsec->force_file_check))
> > +		return 0;
> 
> Instead of simply returning 0 here, you should return the return value from 
> selinux_netlbl_inode_permission just like you are doing in your 
> do_selinux_file_permission() function above.
> 
> This NetLabel call is required to ensure that the on-the-wire label is set 
> correctly for connected stream sockets initiated by a remote host.  It may be 
> possible to do away with this call at some point but it requires additional 
> functionality which we do not have at present.

Thanks,
I forgot about that.
I will fix it in next patch.

> 
> > +	rc = do_selinux_file_permission(file, mask);
> > +	if (rc)
> > +		return rc;
> > +
> > +	fsec->sid_serial = current_sid_serial;
> > +	fsec->force_file_check = 0;
> > +
> > +	return 0;
> > +}
> 
> -- 
> paul moore
> linux security @ hp
> 
> --
> This message was distributed to subscribers of the selinux mailing list.
> If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
> the words "unsubscribe selinux" without quotes as the message.

Regards,
Yuichi Nakamura




--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.