* [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(¬if_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(¬if_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(¬if_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
Powered by JetHome