Stupid VFS name lookup interface..

Stupid VFS name lookup interface..

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 2005 bytes --]

Ok, Al, please tell me why I'm wrong, but I was looking at the hot
code in fs/dcache.c again (__d_lookup_rcu() remains the hottest
function under pathname lookup heavy operations) and that "inode"
argument was mis-commented (it used to be an in-out argument long long
ago) and it just kept bugging me.

And it looks totally pointless anyway. Nobody sane actually wants it.

Yeah, several filesystems use "pinode->i_sb" to look up their
superblock, but they can use "dentry->d_sb" for that instead. You did
most of that long ago, I think.

The *one* insane exception is ncpfs, which actually wants to look at
the parent (ie directory) inode data in order to decide if it should
use a case sensitive hash or not. However, even in that case, I'd
argue that we could just optimistically do a
ACCESS_ONCE(dentry->d_inode) and do the compare using the information
we got from that.

Because we don't care if the dentry->d_inode is unstable: if we got
some stale inode, we would hit the dentry_rcuwalk_barrier() case for
that parent when we later check the sequence numbers. So then we'd
throw away the comparison result anyway.  We check both the dentry and
the parent sequence count in lookup_fast(), verifying that they've
been stable over the sequence.

So as far as I can tell, the only thing we should worry about might be
a NULL pointer due to a concurrent rmdir(), but the identity of the
inode itself we really don't care too much about. Take one or the
other, and don't crash on NULL.

There's a similat case going on in proc_sys_compare(). Same logic applies.

Hmm?

Getting rid of those annoying separate dentry inode pointers makes
__d_lookup_rcu() compile into noticeably better code on x86-64,
because there's no stack frame needed any more. And all the
filesystems end up cleaner too, and the crazy cases go where they
belong.

Untested patch attached. It compiles cleanly, looks sane, and most of
it is just making the function prototypes look much nicer. I think it
works.

              Linus

[-- Attachment #2: patch.diff --]
[-- Type: application/octet-stream, Size: 34655 bytes --]

 Documentation/filesystems/Locking |  6 ++---
 Documentation/filesystems/vfs.txt | 19 +++++++---------
 fs/adfs/dir.c                     |  6 ++---
 fs/affs/namei.c                   | 26 +++++++--------------
 fs/cifs/dir.c                     |  9 +++-----
 fs/dcache.c                       | 27 ++++++++--------------
 fs/efivarfs/super.c               |  9 ++++----
 fs/fat/namei_msdos.c              |  6 ++---
 fs/fat/namei_vfat.c               | 12 ++++------
 fs/gfs2/dentry.c                  |  3 +--
 fs/hfs/hfs_fs.h                   |  7 ++----
 fs/hfs/string.c                   |  6 ++---
 fs/hfsplus/hfsplus_fs.h           |  7 ++----
 fs/hfsplus/unicode.c              |  7 ++----
 fs/hpfs/dentry.c                  |  7 ++----
 fs/isofs/inode.c                  | 48 +++++++++++++--------------------------
 fs/isofs/namei.c                  |  3 +--
 fs/jfs/namei.c                    |  7 ++----
 fs/namei.c                        |  7 +++---
 fs/ncpfs/dir.c                    | 32 +++++++++++++++++++-------
 fs/proc/proc_sysctl.c             |  7 +++---
 fs/sysv/namei.c                   |  3 +--
 include/linux/dcache.h            |  9 +++-----
 23 files changed, 108 insertions(+), 165 deletions(-)

diff --git a/Documentation/filesystems/Locking b/Documentation/filesystems/Locking
index 0706d32a61e6..0c4a9bd2fc82 100644
--- a/Documentation/filesystems/Locking
+++ b/Documentation/filesystems/Locking
@@ -11,10 +11,8 @@ be able to use diff(1).
 prototypes:
 	int (*d_revalidate)(struct dentry *, unsigned int);
 	int (*d_weak_revalidate)(struct dentry *, unsigned int);
-	int (*d_hash)(const struct dentry *, const struct inode *,
-			struct qstr *);
-	int (*d_compare)(const struct dentry *, const struct inode *,
-			const struct dentry *, const struct inode *,
+	int (*d_hash)(const struct dentry *, struct qstr *);
+	int (*d_compare)(const struct dentry *, const struct dentry *,
 			unsigned int, const char *, const struct qstr *);
 	int (*d_delete)(struct dentry *);
 	void (*d_release)(struct dentry *);
diff --git a/Documentation/filesystems/vfs.txt b/Documentation/filesystems/vfs.txt
index bc4b06b3160a..94087989be0b 100644
--- a/Documentation/filesystems/vfs.txt
+++ b/Documentation/filesystems/vfs.txt
@@ -901,10 +901,8 @@ defined:
 struct dentry_operations {
 	int (*d_revalidate)(struct dentry *, unsigned int);
 	int (*d_weak_revalidate)(struct dentry *, unsigned int);
-	int (*d_hash)(const struct dentry *, const struct inode *,
-			struct qstr *);
-	int (*d_compare)(const struct dentry *, const struct inode *,
-			const struct dentry *, const struct inode *,
+	int (*d_hash)(const struct dentry *, struct qstr *);
+	int (*d_compare)(const struct dentry *, const struct dentry *,
 			unsigned int, const char *, const struct qstr *);
 	int (*d_delete)(const struct dentry *);
 	void (*d_release)(struct dentry *);
@@ -949,25 +947,24 @@ struct dentry_operations {
 
   d_hash: called when the VFS adds a dentry to the hash table. The first
 	dentry passed to d_hash is the parent directory that the name is
-	to be hashed into. The inode is the dentry's inode.
+	to be hashed into.
 
 	Same locking and synchronisation rules as d_compare regarding
 	what is safe to dereference etc.
 
   d_compare: called to compare a dentry name with a given name. The first
 	dentry is the parent of the dentry to be compared, the second is
-	the parent's inode, then the dentry and inode (may be NULL) of the
-	child dentry. len and name string are properties of the dentry to be
-	compared. qstr is the name to compare it with.
+	the child dentry. len and name string are properties of the dentry
+	to be compared. qstr is the name to compare it with.
 
 	Must be constant and idempotent, and should not take locks if
-	possible, and should not or store into the dentry or inodes.
-	Should not dereference pointers outside the dentry or inodes without
+	possible, and should not or store into the dentry.
+	Should not dereference pointers outside the dentry without
 	lots of care (eg.  d_parent, d_inode, d_name should not be used).
 
 	However, our vfsmount is pinned, and RCU held, so the dentries and
 	inodes won't disappear, neither will our sb or filesystem module.
-	->i_sb and ->d_sb may be used.
+	->d_sb may be used.
 
 	It is a tricky calling convention because it needs to be called under
 	"rcu-walk", ie. without any locks or references on things.
diff --git a/fs/adfs/dir.c b/fs/adfs/dir.c
index 9cf874ce8336..88acc312b886 100644
--- a/fs/adfs/dir.c
+++ b/fs/adfs/dir.c
@@ -197,8 +197,7 @@ const struct file_operations adfs_dir_operations = {
 };
 
 static int
-adfs_hash(const struct dentry *parent, const struct inode *inode,
-		struct qstr *qstr)
+adfs_hash(const struct dentry *parent, struct qstr *qstr)
 {
 	const unsigned int name_len = ADFS_SB(parent->d_sb)->s_namelen;
 	const unsigned char *name;
@@ -234,8 +233,7 @@ adfs_hash(const struct dentry *parent, const struct inode *inode,
  * requirements of the underlying filesystem.
  */
 static int
-adfs_compare(const struct dentry *parent, const struct inode *pinode,
-		const struct dentry *dentry, const struct inode *inode,
+adfs_compare(const struct dentry *parent, const struct dentry *dentry,
 		unsigned int len, const char *str, const struct qstr *name)
 {
 	int i;
diff --git a/fs/affs/namei.c b/fs/affs/namei.c
index ff65884a7839..c36cbb4537a2 100644
--- a/fs/affs/namei.c
+++ b/fs/affs/namei.c
@@ -13,18 +13,12 @@
 typedef int (*toupper_t)(int);
 
 static int	 affs_toupper(int ch);
-static int	 affs_hash_dentry(const struct dentry *,
-		const struct inode *, struct qstr *);
-static int       affs_compare_dentry(const struct dentry *parent,
-		const struct inode *pinode,
-		const struct dentry *dentry, const struct inode *inode,
+static int	 affs_hash_dentry(const struct dentry *, struct qstr *);
+static int       affs_compare_dentry(const struct dentry *parent, const struct dentry *dentry,
 		unsigned int len, const char *str, const struct qstr *name);
 static int	 affs_intl_toupper(int ch);
-static int	 affs_intl_hash_dentry(const struct dentry *,
-		const struct inode *, struct qstr *);
-static int       affs_intl_compare_dentry(const struct dentry *parent,
-		const struct inode *pinode,
-		const struct dentry *dentry, const struct inode *inode,
+static int	 affs_intl_hash_dentry(const struct dentry *, struct qstr *);
+static int       affs_intl_compare_dentry(const struct dentry *parent, const struct dentry *dentry,
 		unsigned int len, const char *str, const struct qstr *name);
 
 const struct dentry_operations affs_dentry_operations = {
@@ -86,14 +80,12 @@ __affs_hash_dentry(struct qstr *qstr, toupper_t toupper)
 }
 
 static int
-affs_hash_dentry(const struct dentry *dentry, const struct inode *inode,
-		struct qstr *qstr)
+affs_hash_dentry(const struct dentry *dentry, struct qstr *qstr)
 {
 	return __affs_hash_dentry(qstr, affs_toupper);
 }
 static int
-affs_intl_hash_dentry(const struct dentry *dentry, const struct inode *inode,
-		struct qstr *qstr)
+affs_intl_hash_dentry(const struct dentry *dentry, struct qstr *qstr)
 {
 	return __affs_hash_dentry(qstr, affs_intl_toupper);
 }
@@ -131,15 +123,13 @@ static inline int __affs_compare_dentry(unsigned int len,
 }
 
 static int
-affs_compare_dentry(const struct dentry *parent, const struct inode *pinode,
-		const struct dentry *dentry, const struct inode *inode,
+affs_compare_dentry(const struct dentry *parent, const struct dentry *dentry,
 		unsigned int len, const char *str, const struct qstr *name)
 {
 	return __affs_compare_dentry(len, str, name, affs_toupper);
 }
 static int
-affs_intl_compare_dentry(const struct dentry *parent,const struct inode *pinode,
-		const struct dentry *dentry, const struct inode *inode,
+affs_intl_compare_dentry(const struct dentry *parent, const struct dentry *dentry,
 		unsigned int len, const char *str, const struct qstr *name)
 {
 	return __affs_compare_dentry(len, str, name, affs_intl_toupper);
diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
index 5699b5036ed8..5175aebf6737 100644
--- a/fs/cifs/dir.c
+++ b/fs/cifs/dir.c
@@ -822,8 +822,7 @@ const struct dentry_operations cifs_dentry_ops = {
 /* d_delete:       cifs_d_delete,      */ /* not needed except for debugging */
 };
 
-static int cifs_ci_hash(const struct dentry *dentry, const struct inode *inode,
-		struct qstr *q)
+static int cifs_ci_hash(const struct dentry *dentry, struct qstr *q)
 {
 	struct nls_table *codepage = CIFS_SB(dentry->d_sb)->local_nls;
 	unsigned long hash;
@@ -838,12 +837,10 @@ static int cifs_ci_hash(const struct dentry *dentry, const struct inode *inode,
 	return 0;
 }
 
-static int cifs_ci_compare(const struct dentry *parent,
-		const struct inode *pinode,
-		const struct dentry *dentry, const struct inode *inode,
+static int cifs_ci_compare(const struct dentry *parent, const struct dentry *dentry,
 		unsigned int len, const char *str, const struct qstr *name)
 {
-	struct nls_table *codepage = CIFS_SB(pinode->i_sb)->local_nls;
+	struct nls_table *codepage = CIFS_SB(parent->d_sb)->local_nls;
 
 	if ((name->len == len) &&
 	    (nls_strnicmp(codepage, name->name, str, len) == 0))
diff --git a/fs/dcache.c b/fs/dcache.c
index f09b9085f7d8..2f8b2937383a 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1723,7 +1723,7 @@ EXPORT_SYMBOL(d_add_ci);
  * Do the slow-case of the dentry name compare.
  *
  * Unlike the dentry_cmp() function, we need to atomically
- * load the name, length and inode information, so that the
+ * load the name and length information, so that the
  * filesystem can rely on them, and can use the 'name' and
  * 'len' information without worrying about walking off the
  * end of memory etc.
@@ -1741,22 +1741,18 @@ enum slow_d_compare {
 
 static noinline enum slow_d_compare slow_dentry_cmp(
 		const struct dentry *parent,
-		struct inode *inode,
 		struct dentry *dentry,
 		unsigned int seq,
 		const struct qstr *name)
 {
 	int tlen = dentry->d_name.len;
 	const char *tname = dentry->d_name.name;
-	struct inode *i = dentry->d_inode;
 
 	if (read_seqcount_retry(&dentry->d_seq, seq)) {
 		cpu_relax();
 		return D_COMP_SEQRETRY;
 	}
-	if (parent->d_op->d_compare(parent, inode,
-				dentry, i,
-				tlen, tname, name))
+	if (parent->d_op->d_compare(parent, dentry, tlen, tname, name))
 		return D_COMP_NOMATCH;
 	return D_COMP_OK;
 }
@@ -1766,7 +1762,6 @@ static noinline enum slow_d_compare slow_dentry_cmp(
  * @parent: parent dentry
  * @name: qstr of name we wish to find
  * @seqp: returns d_seq value at the point where the dentry was found
- * @inode: returns dentry->d_inode when the inode was found valid.
  * Returns: dentry, or NULL
  *
  * __d_lookup_rcu is the dcache lookup function for rcu-walk name
@@ -1793,7 +1788,7 @@ static noinline enum slow_d_compare slow_dentry_cmp(
  */
 struct dentry *__d_lookup_rcu(const struct dentry *parent,
 				const struct qstr *name,
-				unsigned *seqp, struct inode *inode)
+				unsigned *seqp)
 {
 	u64 hashlen = name->hash_len;
 	const unsigned char *str = name->name;
@@ -1827,11 +1822,10 @@ struct dentry *__d_lookup_rcu(const struct dentry *parent,
 seqretry:
 		/*
 		 * The dentry sequence count protects us from concurrent
-		 * renames, and thus protects inode, parent and name fields.
+		 * renames, and thus protects parent and name fields.
 		 *
 		 * The caller must perform a seqcount check in order
-		 * to do anything useful with the returned dentry,
-		 * including using the 'd_inode' pointer.
+		 * to do anything useful with the returned dentry.
 		 *
 		 * NOTE! We do a "raw" seqcount_begin here. That means that
 		 * we don't wait for the sequence count to stabilize if it
@@ -1845,12 +1839,12 @@ seqretry:
 			continue;
 		if (d_unhashed(dentry))
 			continue;
-		*seqp = seq;
 
 		if (unlikely(parent->d_flags & DCACHE_OP_COMPARE)) {
 			if (dentry->d_name.hash != hashlen_hash(hashlen))
 				continue;
-			switch (slow_dentry_cmp(parent, inode, dentry, seq, name)) {
+			*seqp = seq;
+			switch (slow_dentry_cmp(parent, dentry, seq, name)) {
 			case D_COMP_OK:
 				return dentry;
 			case D_COMP_NOMATCH:
@@ -1862,6 +1856,7 @@ seqretry:
 
 		if (dentry->d_name.hash_len != hashlen)
 			continue;
+		*seqp = seq;
 		if (!dentry_cmp(dentry, str, hashlen_len(hashlen)))
 			return dentry;
 	}
@@ -1959,9 +1954,7 @@ struct dentry *__d_lookup(const struct dentry *parent, const struct qstr *name)
 		if (parent->d_flags & DCACHE_OP_COMPARE) {
 			int tlen = dentry->d_name.len;
 			const char *tname = dentry->d_name.name;
-			if (parent->d_op->d_compare(parent, parent->d_inode,
-						dentry, dentry->d_inode,
-						tlen, tname, name))
+			if (parent->d_op->d_compare(parent, dentry, tlen, tname, name))
 				goto next;
 		} else {
 			if (dentry->d_name.len != len)
@@ -1998,7 +1991,7 @@ struct dentry *d_hash_and_lookup(struct dentry *dir, struct qstr *name)
 	 */
 	name->hash = full_name_hash(name->name, name->len);
 	if (dir->d_flags & DCACHE_OP_HASH) {
-		int err = dir->d_op->d_hash(dir, dir->d_inode, name);
+		int err = dir->d_op->d_hash(dir, name);
 		if (unlikely(err < 0))
 			return ERR_PTR(err);
 	}
diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
index 141aee31884f..a8766b880c07 100644
--- a/fs/efivarfs/super.c
+++ b/fs/efivarfs/super.c
@@ -45,8 +45,8 @@ static struct super_block *efivarfs_sb;
  * So we need to perform a case-sensitive match on part 1 and a
  * case-insensitive match on part 2.
  */
-static int efivarfs_d_compare(const struct dentry *parent, const struct inode *pinode,
-			      const struct dentry *dentry, const struct inode *inode,
+static int efivarfs_d_compare(const struct dentry *parent,
+			      const struct dentry *dentry,
 			      unsigned int len, const char *str,
 			      const struct qstr *name)
 {
@@ -63,8 +63,7 @@ static int efivarfs_d_compare(const struct dentry *parent, const struct inode *p
 	return strncasecmp(name->name + guid, str + guid, EFI_VARIABLE_GUID_LEN);
 }
 
-static int efivarfs_d_hash(const struct dentry *dentry,
-			   const struct inode *inode, struct qstr *qstr)
+static int efivarfs_d_hash(const struct dentry *dentry, struct qstr *qstr)
 {
 	unsigned long hash = init_name_hash();
 	const unsigned char *s = qstr->name;
@@ -108,7 +107,7 @@ static struct dentry *efivarfs_alloc_dentry(struct dentry *parent, char *name)
 	q.name = name;
 	q.len = strlen(name);
 
-	err = efivarfs_d_hash(NULL, NULL, &q);
+	err = efivarfs_d_hash(NULL, &q);
 	if (err)
 		return ERR_PTR(err);
 
diff --git a/fs/fat/namei_msdos.c b/fs/fat/namei_msdos.c
index 081b759cff83..a783b0e1272a 100644
--- a/fs/fat/namei_msdos.c
+++ b/fs/fat/namei_msdos.c
@@ -148,8 +148,7 @@ static int msdos_find(struct inode *dir, const unsigned char *name, int len,
  * that the existing dentry can be used. The msdos fs routines will
  * return ENOENT or EINVAL as appropriate.
  */
-static int msdos_hash(const struct dentry *dentry, const struct inode *inode,
-	       struct qstr *qstr)
+static int msdos_hash(const struct dentry *dentry, struct qstr *qstr)
 {
 	struct fat_mount_options *options = &MSDOS_SB(dentry->d_sb)->options;
 	unsigned char msdos_name[MSDOS_NAME];
@@ -165,8 +164,7 @@ static int msdos_hash(const struct dentry *dentry, const struct inode *inode,
  * Compare two msdos names. If either of the names are invalid,
  * we fall back to doing the standard name comparison.
  */
-static int msdos_cmp(const struct dentry *parent, const struct inode *pinode,
-		const struct dentry *dentry, const struct inode *inode,
+static int msdos_cmp(const struct dentry *parent, const struct dentry *dentry,
 		unsigned int len, const char *str, const struct qstr *name)
 {
 	struct fat_mount_options *options = &MSDOS_SB(parent->d_sb)->options;
diff --git a/fs/fat/namei_vfat.c b/fs/fat/namei_vfat.c
index 2da952036a3d..6df8d3d885e5 100644
--- a/fs/fat/namei_vfat.c
+++ b/fs/fat/namei_vfat.c
@@ -107,8 +107,7 @@ static unsigned int vfat_striptail_len(const struct qstr *qstr)
  * that the existing dentry can be used. The vfat fs routines will
  * return ENOENT or EINVAL as appropriate.
  */
-static int vfat_hash(const struct dentry *dentry, const struct inode *inode,
-		struct qstr *qstr)
+static int vfat_hash(const struct dentry *dentry, struct qstr *qstr)
 {
 	qstr->hash = full_name_hash(qstr->name, vfat_striptail_len(qstr));
 	return 0;
@@ -120,8 +119,7 @@ static int vfat_hash(const struct dentry *dentry, const struct inode *inode,
  * that the existing dentry can be used. The vfat fs routines will
  * return ENOENT or EINVAL as appropriate.
  */
-static int vfat_hashi(const struct dentry *dentry, const struct inode *inode,
-		struct qstr *qstr)
+static int vfat_hashi(const struct dentry *dentry, struct qstr *qstr)
 {
 	struct nls_table *t = MSDOS_SB(dentry->d_sb)->nls_io;
 	const unsigned char *name;
@@ -142,8 +140,7 @@ static int vfat_hashi(const struct dentry *dentry, const struct inode *inode,
 /*
  * Case insensitive compare of two vfat names.
  */
-static int vfat_cmpi(const struct dentry *parent, const struct inode *pinode,
-		const struct dentry *dentry, const struct inode *inode,
+static int vfat_cmpi(const struct dentry *parent, const struct dentry *dentry,
 		unsigned int len, const char *str, const struct qstr *name)
 {
 	struct nls_table *t = MSDOS_SB(parent->d_sb)->nls_io;
@@ -162,8 +159,7 @@ static int vfat_cmpi(const struct dentry *parent, const struct inode *pinode,
 /*
  * Case sensitive compare of two vfat names.
  */
-static int vfat_cmp(const struct dentry *parent, const struct inode *pinode,
-		const struct dentry *dentry, const struct inode *inode,
+static int vfat_cmp(const struct dentry *parent, const struct dentry *dentry,
 		unsigned int len, const char *str, const struct qstr *name)
 {
 	unsigned int alen, blen;
diff --git a/fs/gfs2/dentry.c b/fs/gfs2/dentry.c
index 4fddb3c22d25..f2448ab2aac5 100644
--- a/fs/gfs2/dentry.c
+++ b/fs/gfs2/dentry.c
@@ -109,8 +109,7 @@ fail:
 	return 0;
 }
 
-static int gfs2_dhash(const struct dentry *dentry, const struct inode *inode,
-		struct qstr *str)
+static int gfs2_dhash(const struct dentry *dentry, struct qstr *str)
 {
 	str->hash = gfs2_disk_hash(str->name, str->len);
 	return 0;
diff --git a/fs/hfs/hfs_fs.h b/fs/hfs/hfs_fs.h
index a73b11839a41..0524cda47a6e 100644
--- a/fs/hfs/hfs_fs.h
+++ b/fs/hfs/hfs_fs.h
@@ -229,13 +229,10 @@ extern int hfs_part_find(struct super_block *, sector_t *, sector_t *);
 /* string.c */
 extern const struct dentry_operations hfs_dentry_operations;
 
-extern int hfs_hash_dentry(const struct dentry *, const struct inode *,
-		struct qstr *);
+extern int hfs_hash_dentry(const struct dentry *, struct qstr *);
 extern int hfs_strcmp(const unsigned char *, unsigned int,
 		      const unsigned char *, unsigned int);
-extern int hfs_compare_dentry(const struct dentry *parent,
-		const struct inode *pinode,
-		const struct dentry *dentry, const struct inode *inode,
+extern int hfs_compare_dentry(const struct dentry *parent, const struct dentry *dentry,
 		unsigned int len, const char *str, const struct qstr *name);
 
 /* trans.c */
diff --git a/fs/hfs/string.c b/fs/hfs/string.c
index 495a976a3cc9..85b610c3909f 100644
--- a/fs/hfs/string.c
+++ b/fs/hfs/string.c
@@ -51,8 +51,7 @@ static unsigned char caseorder[256] = {
 /*
  * Hash a string to an integer in a case-independent way
  */
-int hfs_hash_dentry(const struct dentry *dentry, const struct inode *inode,
-		struct qstr *this)
+int hfs_hash_dentry(const struct dentry *dentry, struct qstr *this)
 {
 	const unsigned char *name = this->name;
 	unsigned int hash, len = this->len;
@@ -93,8 +92,7 @@ int hfs_strcmp(const unsigned char *s1, unsigned int len1,
  * Test for equality of two strings in the HFS filename character ordering.
  * return 1 on failure and 0 on success
  */
-int hfs_compare_dentry(const struct dentry *parent, const struct inode *pinode,
-		const struct dentry *dentry, const struct inode *inode,
+int hfs_compare_dentry(const struct dentry *parent, const struct dentry *dentry,
 		unsigned int len, const char *str, const struct qstr *name)
 {
 	const unsigned char *n1, *n2;
diff --git a/fs/hfsplus/hfsplus_fs.h b/fs/hfsplus/hfsplus_fs.h
index 60b0a3388b26..ede79317cfb8 100644
--- a/fs/hfsplus/hfsplus_fs.h
+++ b/fs/hfsplus/hfsplus_fs.h
@@ -495,11 +495,8 @@ int hfsplus_uni2asc(struct super_block *,
 		const struct hfsplus_unistr *, char *, int *);
 int hfsplus_asc2uni(struct super_block *,
 		struct hfsplus_unistr *, int, const char *, int);
-int hfsplus_hash_dentry(const struct dentry *dentry,
-		const struct inode *inode, struct qstr *str);
-int hfsplus_compare_dentry(const struct dentry *parent,
-		const struct inode *pinode,
-		const struct dentry *dentry, const struct inode *inode,
+int hfsplus_hash_dentry(const struct dentry *dentry, struct qstr *str);
+int hfsplus_compare_dentry(const struct dentry *parent, const struct dentry *dentry,
 		unsigned int len, const char *str, const struct qstr *name);
 
 /* wrapper.c */
diff --git a/fs/hfsplus/unicode.c b/fs/hfsplus/unicode.c
index 2c2e47dcfdd8..e8ef121a4d8b 100644
--- a/fs/hfsplus/unicode.c
+++ b/fs/hfsplus/unicode.c
@@ -334,8 +334,7 @@ int hfsplus_asc2uni(struct super_block *sb,
  * Composed unicode characters are decomposed and case-folding is performed
  * if the appropriate bits are (un)set on the superblock.
  */
-int hfsplus_hash_dentry(const struct dentry *dentry, const struct inode *inode,
-		struct qstr *str)
+int hfsplus_hash_dentry(const struct dentry *dentry, struct qstr *str)
 {
 	struct super_block *sb = dentry->d_sb;
 	const char *astr;
@@ -386,9 +385,7 @@ int hfsplus_hash_dentry(const struct dentry *dentry, const struct inode *inode,
  * Composed unicode characters are decomposed and case-folding is performed
  * if the appropriate bits are (un)set on the superblock.
  */
-int hfsplus_compare_dentry(const struct dentry *parent,
-		const struct inode *pinode,
-		const struct dentry *dentry, const struct inode *inode,
+int hfsplus_compare_dentry(const struct dentry *parent, const struct dentry *dentry,
 		unsigned int len, const char *str, const struct qstr *name)
 {
 	struct super_block *sb = parent->d_sb;
diff --git a/fs/hpfs/dentry.c b/fs/hpfs/dentry.c
index 05d4816e4e77..fa27980f2229 100644
--- a/fs/hpfs/dentry.c
+++ b/fs/hpfs/dentry.c
@@ -12,8 +12,7 @@
  * Note: the dentry argument is the parent dentry.
  */
 
-static int hpfs_hash_dentry(const struct dentry *dentry, const struct inode *inode,
-		struct qstr *qstr)
+static int hpfs_hash_dentry(const struct dentry *dentry, struct qstr *qstr)
 {
 	unsigned long	 hash;
 	int		 i;
@@ -35,9 +34,7 @@ static int hpfs_hash_dentry(const struct dentry *dentry, const struct inode *ino
 	return 0;
 }
 
-static int hpfs_compare_dentry(const struct dentry *parent,
-		const struct inode *pinode,
-		const struct dentry *dentry, const struct inode *inode,
+static int hpfs_compare_dentry(const struct dentry *parent, const struct dentry *dentry,
 		unsigned int len, const char *str, const struct qstr *name)
 {
 	unsigned al = len;
diff --git a/fs/isofs/inode.c b/fs/isofs/inode.c
index d9b8aebdeb22..c348d6d88624 100644
--- a/fs/isofs/inode.c
+++ b/fs/isofs/inode.c
@@ -28,31 +28,23 @@
 
 #define BEQUIET
 
-static int isofs_hashi(const struct dentry *parent, const struct inode *inode,
-		struct qstr *qstr);
-static int isofs_hash(const struct dentry *parent, const struct inode *inode,
-		struct qstr *qstr);
+static int isofs_hashi(const struct dentry *parent, struct qstr *qstr);
+static int isofs_hash(const struct dentry *parent, struct qstr *qstr);
 static int isofs_dentry_cmpi(const struct dentry *parent,
-		const struct inode *pinode,
-		const struct dentry *dentry, const struct inode *inode,
+		const struct dentry *dentry,
 		unsigned int len, const char *str, const struct qstr *name);
 static int isofs_dentry_cmp(const struct dentry *parent,
-		const struct inode *pinode,
-		const struct dentry *dentry, const struct inode *inode,
+		const struct dentry *dentry,
 		unsigned int len, const char *str, const struct qstr *name);
 
 #ifdef CONFIG_JOLIET
-static int isofs_hashi_ms(const struct dentry *parent, const struct inode *inode,
-		struct qstr *qstr);
-static int isofs_hash_ms(const struct dentry *parent, const struct inode *inode,
-		struct qstr *qstr);
+static int isofs_hashi_ms(const struct dentry *parent, struct qstr *qstr);
+static int isofs_hash_ms(const struct dentry *parent, struct qstr *qstr);
 static int isofs_dentry_cmpi_ms(const struct dentry *parent,
-		const struct inode *pinode,
-		const struct dentry *dentry, const struct inode *inode,
+		const struct dentry *dentry,
 		unsigned int len, const char *str, const struct qstr *name);
 static int isofs_dentry_cmp_ms(const struct dentry *parent,
-		const struct inode *pinode,
-		const struct dentry *dentry, const struct inode *inode,
+		const struct dentry *dentry,
 		unsigned int len, const char *str, const struct qstr *name);
 #endif
 
@@ -265,30 +257,26 @@ static int isofs_dentry_cmp_common(
 }
 
 static int
-isofs_hash(const struct dentry *dentry, const struct inode *inode,
-		struct qstr *qstr)
+isofs_hash(const struct dentry *dentry, struct qstr *qstr)
 {
 	return isofs_hash_common(dentry, qstr, 0);
 }
 
 static int
-isofs_hashi(const struct dentry *dentry, const struct inode *inode,
-		struct qstr *qstr)
+isofs_hashi(const struct dentry *dentry, struct qstr *qstr)
 {
 	return isofs_hashi_common(dentry, qstr, 0);
 }
 
 static int
-isofs_dentry_cmp(const struct dentry *parent, const struct inode *pinode,
-		const struct dentry *dentry, const struct inode *inode,
+isofs_dentry_cmp(const struct dentry *parent, const struct dentry *dentry,
 		unsigned int len, const char *str, const struct qstr *name)
 {
 	return isofs_dentry_cmp_common(len, str, name, 0, 0);
 }
 
 static int
-isofs_dentry_cmpi(const struct dentry *parent, const struct inode *pinode,
-		const struct dentry *dentry, const struct inode *inode,
+isofs_dentry_cmpi(const struct dentry *parent, const struct dentry *dentry,
 		unsigned int len, const char *str, const struct qstr *name)
 {
 	return isofs_dentry_cmp_common(len, str, name, 0, 1);
@@ -296,30 +284,26 @@ isofs_dentry_cmpi(const struct dentry *parent, const struct inode *pinode,
 
 #ifdef CONFIG_JOLIET
 static int
-isofs_hash_ms(const struct dentry *dentry, const struct inode *inode,
-		struct qstr *qstr)
+isofs_hash_ms(const struct dentry *dentry, struct qstr *qstr)
 {
 	return isofs_hash_common(dentry, qstr, 1);
 }
 
 static int
-isofs_hashi_ms(const struct dentry *dentry, const struct inode *inode,
-		struct qstr *qstr)
+isofs_hashi_ms(const struct dentry *dentry, struct qstr *qstr)
 {
 	return isofs_hashi_common(dentry, qstr, 1);
 }
 
 static int
-isofs_dentry_cmp_ms(const struct dentry *parent, const struct inode *pinode,
-		const struct dentry *dentry, const struct inode *inode,
+isofs_dentry_cmp_ms(const struct dentry *parent, const struct dentry *dentry,
 		unsigned int len, const char *str, const struct qstr *name)
 {
 	return isofs_dentry_cmp_common(len, str, name, 1, 0);
 }
 
 static int
-isofs_dentry_cmpi_ms(const struct dentry *parent, const struct inode *pinode,
-		const struct dentry *dentry, const struct inode *inode,
+isofs_dentry_cmpi_ms(const struct dentry *parent, const struct dentry *dentry,
 		unsigned int len, const char *str, const struct qstr *name)
 {
 	return isofs_dentry_cmp_common(len, str, name, 1, 1);
diff --git a/fs/isofs/namei.c b/fs/isofs/namei.c
index c167028844ed..95295640d9c8 100644
--- a/fs/isofs/namei.c
+++ b/fs/isofs/namei.c
@@ -37,8 +37,7 @@ isofs_cmp(struct dentry *dentry, const char *compare, int dlen)
 
 	qstr.name = compare;
 	qstr.len = dlen;
-	return dentry->d_op->d_compare(NULL, NULL, NULL, NULL,
-			dentry->d_name.len, dentry->d_name.name, &qstr);
+	return dentry->d_op->d_compare(NULL, NULL, dentry->d_name.len, dentry->d_name.name, &qstr);
 }
 
 /*
diff --git a/fs/jfs/namei.c b/fs/jfs/namei.c
index 3b91a7ad6086..25e171702de6 100644
--- a/fs/jfs/namei.c
+++ b/fs/jfs/namei.c
@@ -1538,8 +1538,7 @@ const struct file_operations jfs_dir_operations = {
 	.llseek		= generic_file_llseek,
 };
 
-static int jfs_ci_hash(const struct dentry *dir, const struct inode *inode,
-		struct qstr *this)
+static int jfs_ci_hash(const struct dentry *dir, struct qstr *this)
 {
 	unsigned long hash;
 	int i;
@@ -1552,9 +1551,7 @@ static int jfs_ci_hash(const struct dentry *dir, const struct inode *inode,
 	return 0;
 }
 
-static int jfs_ci_compare(const struct dentry *parent,
-		const struct inode *pinode,
-		const struct dentry *dentry, const struct inode *inode,
+static int jfs_ci_compare(const struct dentry *parent, const struct dentry *dentry,
 		unsigned int len, const char *str, const struct qstr *name)
 {
 	int i, result = 1;
diff --git a/fs/namei.c b/fs/namei.c
index 85e40d1c0a8f..2f8285b031a7 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1352,7 +1352,7 @@ static int lookup_fast(struct nameidata *nd,
 	 */
 	if (nd->flags & LOOKUP_RCU) {
 		unsigned seq;
-		dentry = __d_lookup_rcu(parent, &nd->last, &seq, nd->inode);
+		dentry = __d_lookup_rcu(parent, &nd->last, &seq);
 		if (!dentry)
 			goto unlazy;
 
@@ -1787,8 +1787,7 @@ static int link_path_walk(const char *name, struct nameidata *nd)
 			struct dentry *parent = nd->path.dentry;
 			nd->flags &= ~LOOKUP_JUMPED;
 			if (unlikely(parent->d_flags & DCACHE_OP_HASH)) {
-				err = parent->d_op->d_hash(parent, nd->inode,
-							   &this);
+				err = parent->d_op->d_hash(parent, &this);
 				if (err < 0)
 					break;
 			}
@@ -2121,7 +2120,7 @@ struct dentry *lookup_one_len(const char *name, struct dentry *base, int len)
 	 * to use its own hash..
 	 */
 	if (base->d_flags & DCACHE_OP_HASH) {
-		int err = base->d_op->d_hash(base, base->d_inode, &this);
+		int err = base->d_op->d_hash(base, &this);
 		if (err < 0)
 			return ERR_PTR(err);
 	}
diff --git a/fs/ncpfs/dir.c b/fs/ncpfs/dir.c
index 816326093656..08a530e963ff 100644
--- a/fs/ncpfs/dir.c
+++ b/fs/ncpfs/dir.c
@@ -73,10 +73,8 @@ const struct inode_operations ncp_dir_inode_operations =
  * Dentry operations routines
  */
 static int ncp_lookup_validate(struct dentry *, unsigned int);
-static int ncp_hash_dentry(const struct dentry *, const struct inode *,
-		struct qstr *);
-static int ncp_compare_dentry(const struct dentry *, const struct inode *,
-		const struct dentry *, const struct inode *,
+static int ncp_hash_dentry(const struct dentry *, struct qstr *);
+static int ncp_compare_dentry(const struct dentry *, const struct dentry *,
 		unsigned int, const char *, const struct qstr *);
 static int ncp_delete_dentry(const struct dentry *);
 
@@ -119,11 +117,19 @@ static inline int ncp_case_sensitive(const struct inode *i)
 /*
  * Note: leave the hash unchanged if the directory
  * is case-sensitive.
+ *
+ * Accessing the parent inode can be racy under RCU pathwalking.
+ * Use ACCESS_ONCE() to make sure we use _one_ particular inode,
+ * the callers will handle races.
  */
 static int 
-ncp_hash_dentry(const struct dentry *dentry, const struct inode *inode,
-		struct qstr *this)
+ncp_hash_dentry(const struct dentry *dentry, struct qstr *this)
 {
+	struct inode *inode = ACCESS_ONCE(dentry->d_inode);
+
+	if (!inode)
+		return 0;
+
 	if (!ncp_case_sensitive(inode)) {
 		struct super_block *sb = dentry->d_sb;
 		struct nls_table *t;
@@ -140,14 +146,24 @@ ncp_hash_dentry(const struct dentry *dentry, const struct inode *inode,
 	return 0;
 }
 
+/*
+ * Accessing the parent inode can be racy under RCU pathwalking.
+ * Use ACCESS_ONCE() to make sure we use _one_ particular inode,
+ * the callers will handle races.
+ */
 static int
-ncp_compare_dentry(const struct dentry *parent, const struct inode *pinode,
-		const struct dentry *dentry, const struct inode *inode,
+ncp_compare_dentry(const struct dentry *parent, const struct dentry *dentry,
 		unsigned int len, const char *str, const struct qstr *name)
 {
+	struct inode *pinode;
+
 	if (len != name->len)
 		return 1;
 
+	pinode = ACCESS_ONCE(parent->d_inode);
+	if (!pinode)
+		return 1;
+
 	if (ncp_case_sensitive(pinode))
 		return strncmp(str, name->name, len);
 
diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index ac05f33a0dde..50225d19bbce 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -813,15 +813,16 @@ static int sysctl_is_seen(struct ctl_table_header *p)
 	return res;
 }
 
-static int proc_sys_compare(const struct dentry *parent,
-		const struct inode *pinode,
-		const struct dentry *dentry, const struct inode *inode,
+static int proc_sys_compare(const struct dentry *parent, const struct dentry *dentry,
 		unsigned int len, const char *str, const struct qstr *name)
 {
 	struct ctl_table_header *head;
+	struct inode *inode;
+
 	/* Although proc doesn't have negative dentries, rcu-walk means
 	 * that inode here can be NULL */
 	/* AV: can it, indeed? */
+	inode = ACCESS_ONCE(dentry->d_inode);
 	if (!inode)
 		return 1;
 	if (name->len != len)
diff --git a/fs/sysv/namei.c b/fs/sysv/namei.c
index 1c0d5f264767..731b2bbcaab3 100644
--- a/fs/sysv/namei.c
+++ b/fs/sysv/namei.c
@@ -27,8 +27,7 @@ static int add_nondir(struct dentry *dentry, struct inode *inode)
 	return err;
 }
 
-static int sysv_hash(const struct dentry *dentry, const struct inode *inode,
-		struct qstr *qstr)
+static int sysv_hash(const struct dentry *dentry, struct qstr *qstr)
 {
 	/* Truncate the name in place, avoids having to define a compare
 	   function. */
diff --git a/include/linux/dcache.h b/include/linux/dcache.h
index 1a6bb81f0fe5..87694c81feb2 100644
--- a/include/linux/dcache.h
+++ b/include/linux/dcache.h
@@ -146,10 +146,8 @@ enum dentry_d_lock_class
 struct dentry_operations {
 	int (*d_revalidate)(struct dentry *, unsigned int);
 	int (*d_weak_revalidate)(struct dentry *, unsigned int);
-	int (*d_hash)(const struct dentry *, const struct inode *,
-			struct qstr *);
-	int (*d_compare)(const struct dentry *, const struct inode *,
-			const struct dentry *, const struct inode *,
+	int (*d_hash)(const struct dentry *, struct qstr *);
+	int (*d_compare)(const struct dentry *, const struct dentry *,
 			unsigned int, const char *, const struct qstr *);
 	int (*d_delete)(const struct dentry *);
 	void (*d_release)(struct dentry *);
@@ -300,8 +298,7 @@ extern struct dentry *d_lookup(const struct dentry *, const struct qstr *);
 extern struct dentry *d_hash_and_lookup(struct dentry *, struct qstr *);
 extern struct dentry *__d_lookup(const struct dentry *, const struct qstr *);
 extern struct dentry *__d_lookup_rcu(const struct dentry *parent,
-				const struct qstr *name,
-				unsigned *seq, struct inode *inode);
+				const struct qstr *name, unsigned *seq);
 
 /**
  * __d_rcu_to_refcount - take a refcount on dentry if sequence check is ok

Re: Stupid VFS name lookup interface..

[Al Viro]

On Tue, May 21, 2013 at 03:22:44PM -0700, Linus Torvalds wrote:
> The *one* insane exception is ncpfs, which actually wants to look at
> the parent (ie directory) inode data in order to decide if it should
> use a case sensitive hash or not. However, even in that case, I'd
> argue that we could just optimistically do a
> ACCESS_ONCE(dentry->d_inode) and do the compare using the information
> we got from that.
> 
> Because we don't care if the dentry->d_inode is unstable: if we got
> some stale inode, we would hit the dentry_rcuwalk_barrier() case for
> that parent when we later check the sequence numbers. So then we'd
> throw away the comparison result anyway.  We check both the dentry and
> the parent sequence count in lookup_fast(), verifying that they've
> been stable over the sequence.
> 
> So as far as I can tell, the only thing we should worry about might be
> a NULL pointer due to a concurrent rmdir(), but the identity of the
> inode itself we really don't care too much about. Take one or the
> other, and don't crash on NULL.
> 
> There's a similat case going on in proc_sys_compare(). Same logic applies.

In principle, yes, but...  I wonder if those two cases are actually
safe (especially ncpfs) right now.  We dereference the parent inode
there and that could get ugly, whether we've got it from caller or
as ->d_inode.  Let me dig around in that code a bit, OK?

Al, enjoying the excuse to take a break from ->readdir() code audit ;-/

Re: Stupid VFS name lookup interface..

[Linus Torvalds]

On Tue, May 21, 2013 at 3:34 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> In principle, yes, but...  I wonder if those two cases are actually
> safe (especially ncpfs) right now.

Now I can agree that that may well be an issue.

I don't think my patch makes anything worse (because if the inode
isn't stable, we could have hit the before/after cases before, and the
new NULL case is trivial to handle).

But I'm certainly not going to claim that ncpfs doesn't already have a
race as-is. Just claiming that I wouldn't have made it worse ;)

> Let me dig around in that code a bit, OK?

Sure, no problem. This would be 3.11 material anyway, I'd expect.

                 Linus

Re: Stupid VFS name lookup interface..

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 1805 bytes --]

On Tue, May 21, 2013 at 3:22 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Untested patch attached. It compiles cleanly, looks sane, and most of
> it is just making the function prototypes look much nicer. I think it
> works.

Ok, here's another patch in the "let's make the VFS go faster series".
This one, sadly, is not a cleanup.

The concept is simple: right now the inode->i_security pointer chasing
kills us on inode security checking with selinux. So let's move two of
the fields from the selinux security fields directly into the inode.
So instead of doing "inode->i_security->{sid,sclass}", we can just do
"inode->{i_sid,i_sclass}" directly.

It's a very mechanical transform, so it should all be good, but the
reason I don't much like it is that I think other security models
might want to do something like this too, and right now it's
selinux-specific. I could imagine making it just an anonymous union of
size 64 bits or something, and just making one of the union entries be
an (anonymous) struct with those two fields. So it's not conceptually
selinux-specific, but right now it's pretty much a selinux hack.

But it's a selinux-specific hack that really does matter. The
inode_has_perm() and selinux_inode_permission() functions show up
pretty high on kernel profiles that do a lot of filename lookup, and
it's pretty much all just that i_security pointer chasing and extra
cache miss.

With this, inode->i_security is not very hot any more, and we could
move the i_security pointer elsewhere in the inode.

Comments? I don't think this is *pretty* (and I do want to repeat that
it's not even tested yet), but I think it's worth it. We've been very
good at avoiding extra pointer dereferences in the path lookup, this
is one of the few remaining ones.

              Linus

[-- Attachment #2: patch.diff --]
[-- Type: application/octet-stream, Size: 22962 bytes --]

 include/linux/fs.h                |   1 +
 security/selinux/hooks.c          | 194 +++++++++++++++++---------------------
 security/selinux/include/objsec.h |   2 -
 security/selinux/selinuxfs.c      |  10 +-
 4 files changed, 94 insertions(+), 113 deletions(-)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index 43db02e9c9fa..11a11525b7a8 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -536,6 +536,7 @@ struct inode {
 
 #ifdef CONFIG_SECURITY
 	void			*i_security;
+	u32			i_sid, i_sclass;
 #endif
 
 	/* Stat data, not accessed from path walking */
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 5c6f2cd2d095..bcc3ea477b35 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -210,10 +210,10 @@ static int inode_alloc_security(struct inode *inode)
 	mutex_init(&isec->lock);
 	INIT_LIST_HEAD(&isec->list);
 	isec->inode = inode;
-	isec->sid = SECINITSID_UNLABELED;
-	isec->sclass = SECCLASS_FILE;
 	isec->task_sid = sid;
 	inode->i_security = isec;
+	inode->i_sid = SECINITSID_UNLABELED;
+	inode->i_sclass = SECCLASS_FILE;
 
 	return 0;
 }
@@ -505,9 +505,8 @@ static int selinux_get_mnt_opts(const struct super_block *sb,
 	}
 	if (sbsec->flags & ROOTCONTEXT_MNT) {
 		struct inode *root = sbsec->sb->s_root->d_inode;
-		struct inode_security_struct *isec = root->i_security;
 
-		rc = security_sid_to_context(isec->sid, &context, &len);
+		rc = security_sid_to_context(root->i_sid, &context, &len);
 		if (rc)
 			goto out_free;
 		opts->mnt_opts[i] = context;
@@ -636,7 +635,7 @@ static int selinux_set_mnt_opts(struct super_block *sb,
 		case ROOTCONTEXT_MNT:
 			rootcontext_sid = sid;
 
-			if (bad_option(sbsec, ROOTCONTEXT_MNT, root_isec->sid,
+			if (bad_option(sbsec, ROOTCONTEXT_MNT, inode->i_sid,
 					rootcontext_sid))
 				goto out_double_mount;
 
@@ -718,7 +717,7 @@ static int selinux_set_mnt_opts(struct super_block *sb,
 		if (rc)
 			goto out;
 
-		root_isec->sid = rootcontext_sid;
+		inode->i_sid = rootcontext_sid;
 		root_isec->initialized = 1;
 	}
 
@@ -768,9 +767,9 @@ static int selinux_cmp_sb_context(const struct super_block *oldsb,
 	if ((oldflags & DEFCONTEXT_MNT) && old->def_sid != new->def_sid)
 		goto mismatch;
 	if (oldflags & ROOTCONTEXT_MNT) {
-		struct inode_security_struct *oldroot = oldsb->s_root->d_inode->i_security;
-		struct inode_security_struct *newroot = newsb->s_root->d_inode->i_security;
-		if (oldroot->sid != newroot->sid)
+		struct inode *oldroot = oldsb->s_root->d_inode;
+		struct inode *newroot = newsb->s_root->d_inode;
+		if (oldroot->i_sid != newroot->i_sid)
 			goto mismatch;
 	}
 	return 0;
@@ -820,18 +819,15 @@ static int selinux_sb_clone_mnt_opts(const struct super_block *oldsb,
 			newsbsec->sid = sid;
 		if (!set_rootcontext) {
 			struct inode *newinode = newsb->s_root->d_inode;
-			struct inode_security_struct *newisec = newinode->i_security;
-			newisec->sid = sid;
+			newinode->i_sid = sid;
 		}
 		newsbsec->mntpoint_sid = sid;
 	}
 	if (set_rootcontext) {
 		const struct inode *oldinode = oldsb->s_root->d_inode;
-		const struct inode_security_struct *oldisec = oldinode->i_security;
 		struct inode *newinode = newsb->s_root->d_inode;
-		struct inode_security_struct *newisec = newinode->i_security;
 
-		newisec->sid = oldisec->sid;
+		newinode->i_sid = oldinode->i_sid;
 	}
 
 	sb_finish_set_opts(newsb);
@@ -1224,7 +1220,7 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent
 	switch (sbsec->behavior) {
 	case SECURITY_FS_USE_XATTR:
 		if (!inode->i_op->getxattr) {
-			isec->sid = sbsec->def_sid;
+			inode->i_sid = sbsec->def_sid;
 			break;
 		}
 
@@ -1319,39 +1315,39 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent
 			}
 		}
 		kfree(context);
-		isec->sid = sid;
+		inode->i_sid = sid;
 		break;
 	case SECURITY_FS_USE_TASK:
-		isec->sid = isec->task_sid;
+		inode->i_sid = isec->task_sid;
 		break;
 	case SECURITY_FS_USE_TRANS:
 		/* Default to the fs SID. */
-		isec->sid = sbsec->sid;
+		inode->i_sid = sbsec->sid;
 
 		/* Try to obtain a transition SID. */
-		isec->sclass = inode_mode_to_security_class(inode->i_mode);
+		inode->i_sclass = inode_mode_to_security_class(inode->i_mode);
 		rc = security_transition_sid(isec->task_sid, sbsec->sid,
-					     isec->sclass, NULL, &sid);
+					     inode->i_sclass, NULL, &sid);
 		if (rc)
 			goto out_unlock;
-		isec->sid = sid;
+		inode->i_sid = sid;
 		break;
 	case SECURITY_FS_USE_MNTPOINT:
-		isec->sid = sbsec->mntpoint_sid;
+		inode->i_sid = sbsec->mntpoint_sid;
 		break;
 	default:
 		/* Default to the fs superblock SID. */
-		isec->sid = sbsec->sid;
+		inode->i_sid = sbsec->sid;
 
 		if ((sbsec->flags & SE_SBPROC) && !S_ISLNK(inode->i_mode)) {
 			if (opt_dentry) {
-				isec->sclass = inode_mode_to_security_class(inode->i_mode);
+				inode->i_sclass = inode_mode_to_security_class(inode->i_mode);
 				rc = selinux_proc_get_sid(opt_dentry,
-							  isec->sclass,
+							  inode->i_sclass,
 							  &sid);
 				if (rc)
 					goto out_unlock;
-				isec->sid = sid;
+				inode->i_sid = sid;
 			}
 		}
 		break;
@@ -1362,8 +1358,8 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent
 out_unlock:
 	mutex_unlock(&isec->lock);
 out:
-	if (isec->sclass == SECCLASS_FILE)
-		isec->sclass = inode_mode_to_security_class(inode->i_mode);
+	if (inode->i_sclass == SECCLASS_FILE)
+		inode->i_sclass = inode_mode_to_security_class(inode->i_mode);
 	return rc;
 }
 
@@ -1503,7 +1499,6 @@ static int inode_has_perm(const struct cred *cred,
 			  struct common_audit_data *adp,
 			  unsigned flags)
 {
-	struct inode_security_struct *isec;
 	u32 sid;
 
 	validate_creds(cred);
@@ -1512,9 +1507,8 @@ static int inode_has_perm(const struct cred *cred,
 		return 0;
 
 	sid = cred_sid(cred);
-	isec = inode->i_security;
 
-	return avc_has_perm_flags(sid, isec->sid, isec->sclass, perms, adp, flags);
+	return avc_has_perm_flags(sid, inode->i_sid, inode->i_sclass, perms, adp, flags);
 }
 
 /* Same as inode_has_perm, but pass explicit audit data containing
@@ -1592,13 +1586,11 @@ static int may_create(struct inode *dir,
 		      u16 tclass)
 {
 	const struct task_security_struct *tsec = current_security();
-	struct inode_security_struct *dsec;
 	struct superblock_security_struct *sbsec;
 	u32 sid, newsid;
 	struct common_audit_data ad;
 	int rc;
 
-	dsec = dir->i_security;
 	sbsec = dir->i_sb->s_security;
 
 	sid = tsec->sid;
@@ -1607,14 +1599,14 @@ static int may_create(struct inode *dir,
 	ad.type = LSM_AUDIT_DATA_DENTRY;
 	ad.u.dentry = dentry;
 
-	rc = avc_has_perm(sid, dsec->sid, SECCLASS_DIR,
+	rc = avc_has_perm(sid, dir->i_sid, SECCLASS_DIR,
 			  DIR__ADD_NAME | DIR__SEARCH,
 			  &ad);
 	if (rc)
 		return rc;
 
 	if (!newsid || !(sbsec->flags & SE_SBLABELSUPP)) {
-		rc = security_transition_sid(sid, dsec->sid, tclass,
+		rc = security_transition_sid(sid, dir->i_sid, tclass,
 					     &dentry->d_name, &newsid);
 		if (rc)
 			return rc;
@@ -1648,21 +1640,20 @@ static int may_link(struct inode *dir,
 		    int kind)
 
 {
-	struct inode_security_struct *dsec, *isec;
+	struct inode *inode;
 	struct common_audit_data ad;
 	u32 sid = current_sid();
 	u32 av;
 	int rc;
 
-	dsec = dir->i_security;
-	isec = dentry->d_inode->i_security;
+	inode = dentry->d_inode;
 
 	ad.type = LSM_AUDIT_DATA_DENTRY;
 	ad.u.dentry = dentry;
 
 	av = DIR__SEARCH;
 	av |= (kind ? DIR__REMOVE_NAME : DIR__ADD_NAME);
-	rc = avc_has_perm(sid, dsec->sid, SECCLASS_DIR, av, &ad);
+	rc = avc_has_perm(sid, dir->i_sid, SECCLASS_DIR, av, &ad);
 	if (rc)
 		return rc;
 
@@ -1682,7 +1673,7 @@ static int may_link(struct inode *dir,
 		return 0;
 	}
 
-	rc = avc_has_perm(sid, isec->sid, isec->sclass, av, &ad);
+	rc = avc_has_perm(sid, inode->i_sid, inode->i_sclass, av, &ad);
 	return rc;
 }
 
@@ -1691,32 +1682,30 @@ static inline int may_rename(struct inode *old_dir,
 			     struct inode *new_dir,
 			     struct dentry *new_dentry)
 {
-	struct inode_security_struct *old_dsec, *new_dsec, *old_isec, *new_isec;
+	struct inode *old_inode, *new_inode;
 	struct common_audit_data ad;
 	u32 sid = current_sid();
 	u32 av;
 	int old_is_dir, new_is_dir;
 	int rc;
 
-	old_dsec = old_dir->i_security;
-	old_isec = old_dentry->d_inode->i_security;
-	old_is_dir = S_ISDIR(old_dentry->d_inode->i_mode);
-	new_dsec = new_dir->i_security;
+	old_inode = old_dentry->d_inode;
+	old_is_dir = S_ISDIR(old_inode->i_mode);
 
 	ad.type = LSM_AUDIT_DATA_DENTRY;
 
 	ad.u.dentry = old_dentry;
-	rc = avc_has_perm(sid, old_dsec->sid, SECCLASS_DIR,
+	rc = avc_has_perm(sid, old_dir->i_sid, SECCLASS_DIR,
 			  DIR__REMOVE_NAME | DIR__SEARCH, &ad);
 	if (rc)
 		return rc;
-	rc = avc_has_perm(sid, old_isec->sid,
-			  old_isec->sclass, FILE__RENAME, &ad);
+	rc = avc_has_perm(sid, old_inode->i_sid,
+			  old_inode->i_sclass, FILE__RENAME, &ad);
 	if (rc)
 		return rc;
 	if (old_is_dir && new_dir != old_dir) {
-		rc = avc_has_perm(sid, old_isec->sid,
-				  old_isec->sclass, DIR__REPARENT, &ad);
+		rc = avc_has_perm(sid, old_inode->i_sid,
+				  old_inode->i_sclass, DIR__REPARENT, &ad);
 		if (rc)
 			return rc;
 	}
@@ -1725,14 +1714,14 @@ static inline int may_rename(struct inode *old_dir,
 	av = DIR__ADD_NAME | DIR__SEARCH;
 	if (new_dentry->d_inode)
 		av |= DIR__REMOVE_NAME;
-	rc = avc_has_perm(sid, new_dsec->sid, SECCLASS_DIR, av, &ad);
+	rc = avc_has_perm(sid, new_dir->i_sid, SECCLASS_DIR, av, &ad);
 	if (rc)
 		return rc;
-	if (new_dentry->d_inode) {
-		new_isec = new_dentry->d_inode->i_security;
-		new_is_dir = S_ISDIR(new_dentry->d_inode->i_mode);
-		rc = avc_has_perm(sid, new_isec->sid,
-				  new_isec->sclass,
+	new_inode = new_dentry->d_inode;
+	if (new_inode) {
+		new_is_dir = S_ISDIR(new_inode->i_mode);
+		rc = avc_has_perm(sid, new_inode->i_sid,
+				  new_inode->i_sclass,
 				  (new_is_dir ? DIR__RMDIR : FILE__UNLINK), &ad);
 		if (rc)
 			return rc;
@@ -2027,7 +2016,7 @@ static int selinux_bprm_set_creds(struct linux_binprm *bprm)
 			return -EPERM;
 	} else {
 		/* Check for a default transition on this program. */
-		rc = security_transition_sid(old_tsec->sid, isec->sid,
+		rc = security_transition_sid(old_tsec->sid, inode->i_sid,
 					     SECCLASS_PROCESS, NULL,
 					     &new_tsec->sid);
 		if (rc)
@@ -2042,7 +2031,7 @@ static int selinux_bprm_set_creds(struct linux_binprm *bprm)
 		new_tsec->sid = old_tsec->sid;
 
 	if (new_tsec->sid == old_tsec->sid) {
-		rc = avc_has_perm(old_tsec->sid, isec->sid,
+		rc = avc_has_perm(old_tsec->sid, inode->i_sid,
 				  SECCLASS_FILE, FILE__EXECUTE_NO_TRANS, &ad);
 		if (rc)
 			return rc;
@@ -2053,7 +2042,7 @@ static int selinux_bprm_set_creds(struct linux_binprm *bprm)
 		if (rc)
 			return rc;
 
-		rc = avc_has_perm(new_tsec->sid, isec->sid,
+		rc = avc_has_perm(new_tsec->sid, inode->i_sid,
 				  SECCLASS_FILE, FILE__ENTRYPOINT, &ad);
 		if (rc)
 			return rc;
@@ -2422,10 +2411,10 @@ static int selinux_sb_remount(struct super_block *sb, void *data)
 				goto out_bad_option;
 			break;
 		case ROOTCONTEXT_MNT: {
-			struct inode_security_struct *root_isec;
-			root_isec = sb->s_root->d_inode->i_security;
+			struct inode *root_inode;
+			root_inode = sb->s_root->d_inode;
 
-			if (bad_option(sbsec, ROOTCONTEXT_MNT, root_isec->sid, sid))
+			if (bad_option(sbsec, ROOTCONTEXT_MNT, root_inode->i_sid, sid))
 				goto out_bad_option;
 			break;
 		}
@@ -2520,13 +2509,11 @@ static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
 				       void **value, size_t *len)
 {
 	const struct task_security_struct *tsec = current_security();
-	struct inode_security_struct *dsec;
 	struct superblock_security_struct *sbsec;
 	u32 sid, newsid, clen;
 	int rc;
 	char *namep = NULL, *context;
 
-	dsec = dir->i_security;
 	sbsec = dir->i_sb->s_security;
 
 	sid = tsec->sid;
@@ -2536,7 +2523,7 @@ static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
 	    (sbsec->behavior == SECURITY_FS_USE_MNTPOINT))
 		newsid = sbsec->mntpoint_sid;
 	else if (!newsid || !(sbsec->flags & SE_SBLABELSUPP)) {
-		rc = security_transition_sid(sid, dsec->sid,
+		rc = security_transition_sid(sid, dir->i_sid,
 					     inode_mode_to_security_class(inode->i_mode),
 					     qstr, &newsid);
 		if (rc) {
@@ -2552,8 +2539,8 @@ static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
 	/* Possibly defer initialization to selinux_complete_init. */
 	if (sbsec->flags & SE_SBINITIALIZED) {
 		struct inode_security_struct *isec = inode->i_security;
-		isec->sclass = inode_mode_to_security_class(inode->i_mode);
-		isec->sid = newsid;
+		inode->i_sclass = inode_mode_to_security_class(inode->i_mode);
+		inode->i_sid = newsid;
 		isec->initialized = 1;
 	}
 
@@ -2640,13 +2627,12 @@ static noinline int audit_inode_permission(struct inode *inode,
 					   unsigned flags)
 {
 	struct common_audit_data ad;
-	struct inode_security_struct *isec = inode->i_security;
 	int rc;
 
 	ad.type = LSM_AUDIT_DATA_INODE;
 	ad.u.inode = inode;
 
-	rc = slow_avc_audit(current_sid(), isec->sid, isec->sclass, perms,
+	rc = slow_avc_audit(current_sid(), inode->i_sid, inode->i_sclass, perms,
 			    audited, denied, &ad, flags);
 	if (rc)
 		return rc;
@@ -2682,7 +2668,7 @@ static int selinux_inode_permission(struct inode *inode, int mask)
 	sid = cred_sid(cred);
 	isec = inode->i_security;
 
-	rc = avc_has_perm_noaudit(sid, isec->sid, isec->sclass, perms, 0, &avd);
+	rc = avc_has_perm_noaudit(sid, inode->i_sid, inode->i_sclass, perms, 0, &avd);
 	audited = avc_audit_required(perms, &avd, rc,
 				     from_access ? FILE__AUDIT_ACCESS : 0,
 				     &denied);
@@ -2755,7 +2741,6 @@ static int selinux_inode_setxattr(struct dentry *dentry, const char *name,
 				  const void *value, size_t size, int flags)
 {
 	struct inode *inode = dentry->d_inode;
-	struct inode_security_struct *isec = inode->i_security;
 	struct superblock_security_struct *sbsec;
 	struct common_audit_data ad;
 	u32 newsid, sid = current_sid();
@@ -2774,7 +2759,7 @@ static int selinux_inode_setxattr(struct dentry *dentry, const char *name,
 	ad.type = LSM_AUDIT_DATA_DENTRY;
 	ad.u.dentry = dentry;
 
-	rc = avc_has_perm(sid, isec->sid, isec->sclass,
+	rc = avc_has_perm(sid, inode->i_sid, inode->i_sclass,
 			  FILE__RELABELFROM, &ad);
 	if (rc)
 		return rc;
@@ -2810,13 +2795,13 @@ static int selinux_inode_setxattr(struct dentry *dentry, const char *name,
 	if (rc)
 		return rc;
 
-	rc = avc_has_perm(sid, newsid, isec->sclass,
+	rc = avc_has_perm(sid, newsid, inode->i_sclass,
 			  FILE__RELABELTO, &ad);
 	if (rc)
 		return rc;
 
-	rc = security_validate_transition(isec->sid, newsid, sid,
-					  isec->sclass);
+	rc = security_validate_transition(inode->i_sid, newsid, sid,
+					  inode->i_sclass);
 	if (rc)
 		return rc;
 
@@ -2832,7 +2817,6 @@ static void selinux_inode_post_setxattr(struct dentry *dentry, const char *name,
 					int flags)
 {
 	struct inode *inode = dentry->d_inode;
-	struct inode_security_struct *isec = inode->i_security;
 	u32 newsid;
 	int rc;
 
@@ -2849,7 +2833,7 @@ static void selinux_inode_post_setxattr(struct dentry *dentry, const char *name,
 		return;
 	}
 
-	isec->sid = newsid;
+	inode->i_sid = newsid;
 	return;
 }
 
@@ -2887,7 +2871,6 @@ static int selinux_inode_getsecurity(const struct inode *inode, const char *name
 	u32 size;
 	int error;
 	char *context = NULL;
-	struct inode_security_struct *isec = inode->i_security;
 
 	if (strcmp(name, XATTR_SELINUX_SUFFIX))
 		return -EOPNOTSUPP;
@@ -2904,10 +2887,10 @@ static int selinux_inode_getsecurity(const struct inode *inode, const char *name
 	error = selinux_capable(current_cred(), &init_user_ns, CAP_MAC_ADMIN,
 				SECURITY_CAP_NOAUDIT);
 	if (!error)
-		error = security_sid_to_context_force(isec->sid, &context,
+		error = security_sid_to_context_force(inode->i_sid, &context,
 						      &size);
 	else
-		error = security_sid_to_context(isec->sid, &context, &size);
+		error = security_sid_to_context(inode->i_sid, &context, &size);
 	if (error)
 		return error;
 	error = size;
@@ -2937,7 +2920,7 @@ static int selinux_inode_setsecurity(struct inode *inode, const char *name,
 	if (rc)
 		return rc;
 
-	isec->sid = newsid;
+	inode->i_sid = newsid;
 	isec->initialized = 1;
 	return 0;
 }
@@ -2952,8 +2935,7 @@ static int selinux_inode_listsecurity(struct inode *inode, char *buffer, size_t
 
 static void selinux_inode_getsecid(const struct inode *inode, u32 *secid)
 {
-	struct inode_security_struct *isec = inode->i_security;
-	*secid = isec->sid;
+	*secid = inode->i_sid;
 }
 
 /* file security operations */
@@ -2975,14 +2957,13 @@ static int selinux_file_permission(struct file *file, int mask)
 {
 	struct inode *inode = file_inode(file);
 	struct file_security_struct *fsec = file->f_security;
-	struct inode_security_struct *isec = inode->i_security;
 	u32 sid = current_sid();
 
 	if (!mask)
 		/* No permission to check.  Existence test. */
 		return 0;
 
-	if (sid == fsec->sid && fsec->isid == isec->sid &&
+	if (sid == fsec->sid && fsec->isid == inode->i_sid &&
 	    fsec->pseqno == avc_policy_seqno())
 		/* No change since file_open check. */
 		return 0;
@@ -3238,10 +3219,10 @@ static int selinux_file_receive(struct file *file)
 static int selinux_file_open(struct file *file, const struct cred *cred)
 {
 	struct file_security_struct *fsec;
-	struct inode_security_struct *isec;
+	struct inode *inode;
 
 	fsec = file->f_security;
-	isec = file_inode(file)->i_security;
+	inode = file_inode(file);
 	/*
 	 * Save inode label and policy sequence number
 	 * at open-time so that selinux_file_permission
@@ -3249,7 +3230,7 @@ static int selinux_file_open(struct file *file, const struct cred *cred)
 	 * Task label is already saved in the file security
 	 * struct as its SID.
 	 */
-	fsec->isid = isec->sid;
+	fsec->isid = inode->i_sid;
 	fsec->pseqno = avc_policy_seqno();
 	/*
 	 * Since the inode label or policy seqno may have changed
@@ -3359,18 +3340,17 @@ static int selinux_kernel_act_as(struct cred *new, u32 secid)
  */
 static int selinux_kernel_create_files_as(struct cred *new, struct inode *inode)
 {
-	struct inode_security_struct *isec = inode->i_security;
 	struct task_security_struct *tsec = new->security;
 	u32 sid = current_sid();
 	int ret;
 
-	ret = avc_has_perm(sid, isec->sid,
+	ret = avc_has_perm(sid, inode->i_sid,
 			   SECCLASS_KERNEL_SERVICE,
 			   KERNEL_SERVICE__CREATE_FILES_AS,
 			   NULL);
 
 	if (ret == 0)
-		tsec->create_sid = isec->sid;
+		tsec->create_sid = inode->i_sid;
 	return ret;
 }
 
@@ -3500,7 +3480,7 @@ static void selinux_task_to_inode(struct task_struct *p,
 	struct inode_security_struct *isec = inode->i_security;
 	u32 sid = task_sid(p);
 
-	isec->sid = sid;
+	inode->i_sid = sid;
 	isec->initialized = 1;
 }
 
@@ -3790,16 +3770,17 @@ static int selinux_socket_post_create(struct socket *sock, int family,
 				      int type, int protocol, int kern)
 {
 	const struct task_security_struct *tsec = current_security();
-	struct inode_security_struct *isec = SOCK_INODE(sock)->i_security;
+	struct inode *inode = SOCK_INODE(sock);
+	struct inode_security_struct *isec = inode->i_security;
 	struct sk_security_struct *sksec;
 	int err = 0;
 
-	isec->sclass = socket_type_to_security_class(family, type, protocol);
+	inode->i_sclass = socket_type_to_security_class(family, type, protocol);
 
 	if (kern)
-		isec->sid = SECINITSID_KERNEL;
+		inode->i_sid = SECINITSID_KERNEL;
 	else {
-		err = socket_sockcreate_sid(tsec, isec->sclass, &(isec->sid));
+		err = socket_sockcreate_sid(tsec, inode->i_sclass, &(inode->i_sid));
 		if (err)
 			return err;
 	}
@@ -3808,8 +3789,8 @@ static int selinux_socket_post_create(struct socket *sock, int family,
 
 	if (sock->sk) {
 		sksec = sock->sk->sk_security;
-		sksec->sid = isec->sid;
-		sksec->sclass = isec->sclass;
+		sksec->sid = inode->i_sid;
+		sksec->sclass = inode->i_sclass;
 		err = selinux_netlbl_socket_post_create(sock->sk, family);
 	}
 
@@ -3983,18 +3964,19 @@ static int selinux_socket_listen(struct socket *sock, int backlog)
 static int selinux_socket_accept(struct socket *sock, struct socket *newsock)
 {
 	int err;
-	struct inode_security_struct *isec;
+	struct inode *inode, *newinode;
 	struct inode_security_struct *newisec;
 
 	err = sock_has_perm(current, sock->sk, SOCKET__ACCEPT);
 	if (err)
 		return err;
 
-	newisec = SOCK_INODE(newsock)->i_security;
+	newinode = SOCK_INODE(newsock);
+	newisec = newinode->i_security;
 
-	isec = SOCK_INODE(sock)->i_security;
-	newisec->sclass = isec->sclass;
-	newisec->sid = isec->sid;
+	inode = SOCK_INODE(sock);
+	newinode->i_sclass = inode->i_sclass;
+	newinode->i_sid = inode->i_sid;
 	newisec->initialized = 1;
 
 	return 0;
@@ -4327,13 +4309,13 @@ static void selinux_sk_getsecid(struct sock *sk, u32 *secid)
 
 static void selinux_sock_graft(struct sock *sk, struct socket *parent)
 {
-	struct inode_security_struct *isec = SOCK_INODE(parent)->i_security;
+	struct inode *inode = SOCK_INODE(parent);
 	struct sk_security_struct *sksec = sk->sk_security;
 
 	if (sk->sk_family == PF_INET || sk->sk_family == PF_INET6 ||
 	    sk->sk_family == PF_UNIX)
-		isec->sid = sksec->sid;
-	sksec->sclass = isec->sclass;
+		inode->i_sid = sksec->sid;
+	sksec->sclass = inode->i_sclass;
 }
 
 static int selinux_inet_conn_request(struct sock *sk, struct sk_buff *skb,
diff --git a/security/selinux/include/objsec.h b/security/selinux/include/objsec.h
index aa47bcabb5f6..f48693889e27 100644
--- a/security/selinux/include/objsec.h
+++ b/security/selinux/include/objsec.h
@@ -40,8 +40,6 @@ struct inode_security_struct {
 	struct inode *inode;	/* back pointer to inode object */
 	struct list_head list;	/* list of inode_security_struct */
 	u32 task_sid;		/* SID of creating task */
-	u32 sid;		/* SID of this object */
-	u16 sclass;		/* security class of this object */
 	unsigned char initialized;	/* initialization flag */
 	struct mutex lock;
 };
diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
index ff427733c290..1591a3c042dc 100644
--- a/security/selinux/selinuxfs.c
+++ b/security/selinux/selinuxfs.c
@@ -1262,12 +1262,12 @@ static int sel_make_bools(void)
 		if (len >= PAGE_SIZE)
 			goto out;
 
-		isec = (struct inode_security_struct *)inode->i_security;
+		isec = inode->i_security;
 		ret = security_genfs_sid("selinuxfs", page, SECCLASS_FILE, &sid);
 		if (ret)
 			goto out;
 
-		isec->sid = sid;
+		inode->i_sid = sid;
 		isec->initialized = 1;
 		inode->i_fop = &sel_bool_ops;
 		inode->i_ino = i|SEL_BOOL_INO_OFFSET;
@@ -1827,9 +1827,9 @@ static int sel_fill_super(struct super_block *sb, void *data, int silent)
 		goto err;
 
 	inode->i_ino = ++sel_last_ino;
-	isec = (struct inode_security_struct *)inode->i_security;
-	isec->sid = SECINITSID_DEVNULL;
-	isec->sclass = SECCLASS_CHR_FILE;
+	isec = inode->i_security;
+	inode->i_sid = SECINITSID_DEVNULL;
+	inode->i_sclass = SECCLASS_CHR_FILE;
 	isec->initialized = 1;
 
 	init_special_inode(inode, S_IFCHR | S_IRUGO | S_IWUGO, MKDEV(MEM_MAJOR, 3));

Re: Stupid VFS name lookup interface..

[Al Viro]

On Fri, May 24, 2013 at 08:21:08PM -0700, Linus Torvalds wrote:
> On Tue, May 21, 2013 at 3:22 PM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > Untested patch attached. It compiles cleanly, looks sane, and most of
> > it is just making the function prototypes look much nicer. I think it
> > works.
> 
> Ok, here's another patch in the "let's make the VFS go faster series".
> This one, sadly, is not a cleanup.
> 
> The concept is simple: right now the inode->i_security pointer chasing
> kills us on inode security checking with selinux. So let's move two of
> the fields from the selinux security fields directly into the inode.
> So instead of doing "inode->i_security->{sid,sclass}", we can just do
> "inode->{i_sid,i_sclass}" directly.
> 
> It's a very mechanical transform, so it should all be good, but the
> reason I don't much like it is that I think other security models
> might want to do something like this too, and right now it's
> selinux-specific. I could imagine making it just an anonymous union of
> size 64 bits or something, and just making one of the union entries be
> an (anonymous) struct with those two fields. So it's not conceptually
> selinux-specific, but right now it's pretty much a selinux hack.
> 
> But it's a selinux-specific hack that really does matter. The
> inode_has_perm() and selinux_inode_permission() functions show up
> pretty high on kernel profiles that do a lot of filename lookup, and
> it's pretty much all just that i_security pointer chasing and extra
> cache miss.
> 
> With this, inode->i_security is not very hot any more, and we could
> move the i_security pointer elsewhere in the inode.
> 
> Comments? I don't think this is *pretty* (and I do want to repeat that
> it's not even tested yet), but I think it's worth it. We've been very
> good at avoiding extra pointer dereferences in the path lookup, this
> is one of the few remaining ones.

Well...   The problem I see here is not even selinux per se - it's that
"LSM stacking" insanity.  How would your anon union deal with that?  Which
LSM gets to play with it when we have more than one of those turds around?

Re: Stupid VFS name lookup interface..

[Linus Torvalds]

On Sat, May 25, 2013 at 9:57 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> Well...   The problem I see here is not even selinux per se - it's that
> "LSM stacking" insanity.  How would your anon union deal with that?  Which
> LSM gets to play with it when we have more than one of those turds around?

We don't support stacking anyway. And if we ever do, that will require
some really *major* changes, since right now i_security is owned by
the security module. It's not a list, it's a direct pointer to opaque
per-LSM data.

Of course, if you don't use i_security (only selinux and smack do
right now according to my quick grep), then you could live together
with somebody who does. And that wouldn't change with the new fields,
the same rules would apply. If you can make your security decisions
purely based on standard inode/dentry information (like the common
capabilities code), you'd be ok, and in that sense stacking has
obviously existed for a long while (ie some of the standard unix
capabilities are checked regardless of any LSM ones).

Side note: avoiding the i_security dereference helps, and testing
shows that it drops inode_has_perm and selinux_inode_permission down
the profile list. The bulk of the cost of pathname intensive loads is
now very much the actual path walking itself (__d_lookup_rcu,
path_lookupat, link_path_walk). The security checks are still quite
visible on my "git parallel stat" test-case, with avc_has_perm_noaudit
being 3.5% and the inode_has_perm() and selinux_inode_permission at
around 2.5% each, but I don't really see anything fixable going on any
more. The inode_has_perm() cost is simply because it's one of the
first things that touched the inode itself, so we get the -
unavoidable - cache miss on the second line of the inode.

So the patch is tested, and improves at least profiles. It's probably
not that noticeable in real life, but I like how it makes me go: "I
really don't see any low-hanging fruit any more" on the path lookup.

             Linus

Re: Stupid VFS name lookup interface..

[Casey Schaufler]

On 5/25/2013 9:57 AM, Al Viro wrote:
> On Fri, May 24, 2013 at 08:21:08PM -0700, Linus Torvalds wrote:
>> On Tue, May 21, 2013 at 3:22 PM, Linus Torvalds
>> <torvalds@linux-foundation.org> wrote:
>>> Untested patch attached. It compiles cleanly, looks sane, and most of
>>> it is just making the function prototypes look much nicer. I think it
>>> works.
>> Ok, here's another patch in the "let's make the VFS go faster series".
>> This one, sadly, is not a cleanup.
>>
>> The concept is simple: right now the inode->i_security pointer chasing
>> kills us on inode security checking with selinux. So let's move two of
>> the fields from the selinux security fields directly into the inode.
>> So instead of doing "inode->i_security->{sid,sclass}", we can just do
>> "inode->{i_sid,i_sclass}" directly.
>>
>> It's a very mechanical transform, so it should all be good, but the
>> reason I don't much like it is that I think other security models
>> might want to do something like this too, and right now it's
>> selinux-specific. I could imagine making it just an anonymous union of
>> size 64 bits or something, and just making one of the union entries be
>> an (anonymous) struct with those two fields. So it's not conceptually
>> selinux-specific, but right now it's pretty much a selinux hack.
>>
>> But it's a selinux-specific hack that really does matter. The
>> inode_has_perm() and selinux_inode_permission() functions show up
>> pretty high on kernel profiles that do a lot of filename lookup, and
>> it's pretty much all just that i_security pointer chasing and extra
>> cache miss.
>>
>> With this, inode->i_security is not very hot any more, and we could
>> move the i_security pointer elsewhere in the inode.
>>
>> Comments? I don't think this is *pretty* (and I do want to repeat that
>> it's not even tested yet), but I think it's worth it. We've been very
>> good at avoiding extra pointer dereferences in the path lookup, this
>> is one of the few remaining ones.
> Well...   The problem I see here is not even selinux per se - it's that
> "LSM stacking" insanity.  How would your anon union deal with that?  Which
> LSM gets to play with it when we have more than one of those turds around?

I don't know that the terms "insanity" and "turds" really do
the situation justice, but Al has a firm grasp on the nut of the issue.

The LSM stacking I've been working on (v14 due "real soon") would
render this change useless, as you'd have to have the multiple
instances of the special fields just as you'd need multiple blob
pointers. That would have to reintroduce the indirection you're
trying to be rid of. I have been working on the assumption that
the single blob pointer was all that could ever go into the inode.
If that's not true stacking could get considerably easier and could
have less performance impact.

Now I'll put on my Smack maintainer hat. Performance improvement is
always welcome, but I would rather see attention to performance of
the LSM architecture than SELinux specific hacks. The LSM blob
pointer scheme is there so that you (Linus) don't have to see the
dreadful things that we security people are doing. Is it time to
get past that level of disassociation? Or, and I really hate asking
this, have you fallen into the SELinux camp?


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

Re: Stupid VFS name lookup interface..

[Linus Torvalds]

On Sat, May 25, 2013 at 11:33 AM, Casey Schaufler
<casey@schaufler-ca.com> wrote:
>
> Now I'll put on my Smack maintainer hat. Performance improvement is
> always welcome, but I would rather see attention to performance of
> the LSM architecture than SELinux specific hacks.

I haven't seen huge issues with performance at that level.

> The LSM blob
> pointer scheme is there so that you (Linus) don't have to see the
> dreadful things that we security people are doing. Is it time to
> get past that level of disassociation? Or, and I really hate asking
> this, have you fallen into the SELinux camp?

I only have selinux performance to look at, since I run Fedora. I used
to actually turn it off entirely, because it impacted VFS performance
so horribly. We fixed it. I (and Al) spent time to make sure that we
don't need to drop RCU lookup just because we call into the security
layers etc.

But I haven't even looked at what non-selinux setups do to
performance. Last time I tried Ubuntu (they still use apparmor, no?),
"make modules_install ; make install" didn't work for the kernel, and
if the Ubuntu people don't want to support kernel engineers, I
certainly am not going to bother with them. Who uses smack?

                Linus

Re: Stupid VFS name lookup interface..

[James Morris]

On Sat, 25 May 2013, Al Viro wrote:

> Well...   The problem I see here is not even selinux per se - it's that
> "LSM stacking" insanity.

FWIW, I don't see a concrete rationale for merging this stacking work. That 
doesn't mean there won't be one in the future, but I wouldn't let the idea 
of a future possible change prevent this patch from being merged.




-- 
James Morris
<jmorris@namei.org>

Re: Stupid VFS name lookup interface..

[James Morris]

On Sat, 25 May 2013, Linus Torvalds wrote:

> But I haven't even looked at what non-selinux setups do to
> performance. Last time I tried Ubuntu (they still use apparmor, no?),
> "make modules_install ; make install" didn't work for the kernel, and
> if the Ubuntu people don't want to support kernel engineers, I
> certainly am not going to bother with them. Who uses smack?

Tizen, perhaps a few others.


-- 
James Morris
<jmorris@namei.org>

Re: Stupid VFS name lookup interface..

[Linus Torvalds]

On Sat, May 25, 2013 at 10:04 PM, James Morris <jmorris@namei.org> wrote:
> On Sat, 25 May 2013, Linus Torvalds wrote:
>
>> But I haven't even looked at what non-selinux setups do to
>> performance. Last time I tried Ubuntu (they still use apparmor, no?),
>> "make modules_install ; make install" didn't work for the kernel, and
>> if the Ubuntu people don't want to support kernel engineers, I
>> certainly am not going to bother with them. Who uses smack?
>
> Tizen, perhaps a few others.

Btw, it really would be good if security people started realizing that
performance matters. It's annoying to see the security lookups cause
50% performance degradations on pathname lookup (and no, I'm not
exaggerating, that's literally what it was before we fixed it - and
no, by "we" I don't mean security people).

There's a really simple benchmark that is actually fairly relevant:
build a reasonable kernel ("make localmodconfig" or similar - not the
normal distro kernel that has everything enabled) without debugging or
other crud enabled, run that kernel, and then re-build the fully built
kernel to make sure it's all in the disk cache. Then, when you don't
need any IO, and don't need to recompile anything, do a "make -j".

Assuming you have a reasonably modern desktop machine, it should take
something like 5-10 seconds, of which almost everything is just "make"
doing lots of stat() calls to see that everything is fully built. If
it takes any longer, you're doing something wrong.

Once you are at that point, just do "perf record -f -e cycles:pp make
-j" and then "perf report" on the thing.

(The "-e cycles:pp" is not necessary for the rough information, but it
helps if you then want to go and annotate the assembler to see where
the costs come from).

If you see security functions at the top, you know that the security
routines take more time than the real work the kernel is doing, and
should realize that that would be a problem.

Right now (zooming into the kernel only - ignoring the fact that make
really spends a fair amount of time in user space) I get

  9.79%      make  [k] __d_lookup_rcu
  5.48%      make  [k] link_path_walk
  2.94%      make  [k] avc_has_perm_noaudit
  2.47%      make  [k] selinux_inode_permission
  2.25%      make  [k] path_lookupat
  1.89%      make  [k] generic_fillattr
  1.50%      make  [k] lookup_fast
  1.27%      make  [k] copy_user_generic_string
  1.17%      make  [k] generic_permission
  1.15%      make  [k] dput
  1.12%      make  [k] inode_has_perm.constprop.58
  1.11%      make  [k] __inode_permission
  1.08%      make  [k] kmem_cache_alloc
  ...

so the permission checking is certainly quite noticeable, but it's by
no means dominant. This is with both of the patches I've posted, but
the numbers weren't all that different before (inode_has_perm and
selinux_inode_permission used to be higher up in the list, now
avc_has_perm_noaudit is the top selinux cost - which actually makes
some amount of sense).

So it's easy to have a fairly real-world performance profile that
shows path lookup costs on a real test.

                  Linus

Re: Stupid VFS name lookup interface..

[Theodore Ts'o]

On Sat, May 25, 2013 at 11:33:46AM -0700, Casey Schaufler wrote:
> Now I'll put on my Smack maintainer hat. Performance improvement is
> always welcome, but I would rather see attention to performance of
> the LSM architecture than SELinux specific hacks. The LSM blob
> pointer scheme is there so that you (Linus) don't have to see the
> dreadful things that we security people are doing. Is it time to
> get past that level of disassociation? Or, and I really hate asking
> this, have you fallen into the SELinux camp?

What part of the LSM architecture are you proposing be optimized?  The
LSM layer is pretty thin, partially because the various different
security approaches don't agree with each other on fairly fundamental
issues.  What sort of optimization opportunities you are suggesting?
Are there changes that can be made that all of the major security LSM
maintainers would actually agree with?

I've been re-reading the thread on LKML which was spawned when SMACK
was proposed for upstream inclusion:

    http://thread.gmane.org/gmane.linux.kernel/585903/focus=586412

Have any of the arguments over the proper security models changed over
or have gotten resolved over the past six years, while I haven't been
looking?

						- Ted

Re: Stupid VFS name lookup interface..

[Casey Schaufler]

On 5/25/2013 10:19 PM, Linus Torvalds wrote:
> On Sat, May 25, 2013 at 10:04 PM, James Morris <jmorris@namei.org> wrote:
>> On Sat, 25 May 2013, Linus Torvalds wrote:
>>
>>> But I haven't even looked at what non-selinux setups do to
>>> performance. Last time I tried Ubuntu (they still use apparmor, no?),
>>> "make modules_install ; make install" didn't work for the kernel, and
>>> if the Ubuntu people don't want to support kernel engineers, I
>>> certainly am not going to bother with them. Who uses smack?
>> Tizen, perhaps a few others.
> Btw, it really would be good if security people started realizing that
> performance matters. It's annoying to see the security lookups cause
> 50% performance degradations on pathname lookup (and no, I'm not
> exaggerating, that's literally what it was before we fixed it - and
> no, by "we" I don't mean security people).

I think that we have a pretty good idea that performance matters.
I have never, not even once, tried to introduce a "security" feature
that was not subject to an objection based on its performance impact.
We are also extremely tuned into the desire that when our security
features are not present the impact of their potential availability
has to be zero.

The whole secid philosophy comes out of the need to keep security out
of other people's way. It has performance impact. Sure, SELinux
hashes lookups, but a blob pointer gets you right where you want to be.
When we are constrained in unnatural ways there are going to be
consequences. Performance is one. Code complexity is another.

One need look no further than the recent discussions regarding Paul
Moore's suggested changes to sk_buff to see just how seriously
performance considerations impact security development. Because the
security data in the sk_buff is limited to a u32 instead of a blob
pointer labeled networking performance is seriously worse than
unlabeled. Paul has gone to heroic lengths to come up with a change
that meets all of the performance criteria, and has still been
rejected. Not because the stated issues haven't been addressed, but
because someone else might have changes that "need those cache lines"
in the future.

Two of the recent Smack changes have been performance improvements.
Smack is currently under serious scrutiny for performance as it is
heading into small machines.

I'm not saying we can't do better, or that we (at least I) don't
appreciate any help we can get. We are, however, bombarded with
concern over the performance impact of what we're up to. All too
often it's not constructive criticism. Sometimes it is downright
hostile.

>
> There's a really simple benchmark that is actually fairly relevant:
> build a reasonable kernel ("make localmodconfig" or similar - not the
> normal distro kernel that has everything enabled) without debugging or
> other crud enabled, run that kernel, and then re-build the fully built
> kernel to make sure it's all in the disk cache. Then, when you don't
> need any IO, and don't need to recompile anything, do a "make -j".
>
> Assuming you have a reasonably modern desktop machine, it should take
> something like 5-10 seconds, of which almost everything is just "make"
> doing lots of stat() calls to see that everything is fully built. If
> it takes any longer, you're doing something wrong.
>
> Once you are at that point, just do "perf record -f -e cycles:pp make
> -j" and then "perf report" on the thing.
>
> (The "-e cycles:pp" is not necessary for the rough information, but it
> helps if you then want to go and annotate the assembler to see where
> the costs come from).
>
> If you see security functions at the top, you know that the security
> routines take more time than the real work the kernel is doing, and
> should realize that that would be a problem.
>
> Right now (zooming into the kernel only - ignoring the fact that make
> really spends a fair amount of time in user space) I get
>
>   9.79%      make  [k] __d_lookup_rcu
>   5.48%      make  [k] link_path_walk
>   2.94%      make  [k] avc_has_perm_noaudit
>   2.47%      make  [k] selinux_inode_permission
>   2.25%      make  [k] path_lookupat
>   1.89%      make  [k] generic_fillattr
>   1.50%      make  [k] lookup_fast
>   1.27%      make  [k] copy_user_generic_string
>   1.17%      make  [k] generic_permission
>   1.15%      make  [k] dput
>   1.12%      make  [k] inode_has_perm.constprop.58
>   1.11%      make  [k] __inode_permission
>   1.08%      make  [k] kmem_cache_alloc
>   ...
>
> so the permission checking is certainly quite noticeable, but it's by
> no means dominant. This is with both of the patches I've posted, but
> the numbers weren't all that different before (inode_has_perm and
> selinux_inode_permission used to be higher up in the list, now
> avc_has_perm_noaudit is the top selinux cost - which actually makes
> some amount of sense).
>
> So it's easy to have a fairly real-world performance profile that
> shows path lookup costs on a real test.
>
>                   Linus
>

Re: Stupid VFS name lookup interface..

[Linus Torvalds]

On Sun, May 26, 2013 at 10:59 AM, Casey Schaufler
<casey@schaufler-ca.com> wrote:
>
> The whole secid philosophy comes out of the need to keep security out
> of other people's way. It has performance impact. Sure, SELinux
> hashes lookups, but a blob pointer gets you right where you want to be.
> When we are constrained in unnatural ways there are going to be
> consequences. Performance is one. Code complexity is another.

Quite frankly, I'd like to possibly introduce a cache of security
decisions at least for the common filesystem operations (and the
per-pathcomponent lookup is *the* most common one, but the per-stat
one is pretty bad too), and put that cache at the VFS level, so that
the security people can *not* screw it up, and so that we don't call
down to the security layer at all 99% of the time.

Once that happens, we don't care any more what security people do.

It has been how we have fixed performance problems for filesystems
every single time. It's simply not possible to have a generic
interface to 50+ different filesystems and expect that kind of generic
interface to be high-performance - but when we've been able to
abstract it out as a cache in front of the filesystem operations, it
is suddenly quite reasonable to spend a lot of effort making that
cache go fast like a bat out of hell.

It started with the dentry cache and the page cache, but we now do the
POSIX ACL's that way too, because it just wasn't reasonable to call
down to the filesystem to look up ACL's and have all the complex "we
do this with RCU locks held" semantics.

Is there something similar we could do for the security layer? We
don't have 50+ different security models, but we do have several. If
the different security modules could agree on some kind of generic
"security ID" model so that we could cache things (see fs/namei.c and
get_cached_acl_rcu() for example), it would be a great thing.

Then selinux could get rid of it's hashed lookups entirely, because
that whole "cache the security ID" would be handled by generic code.

But that *would* require that there would be some abstract notion of
security ID/context that we could use in generic code *WITHOUT* the
need to call down the the security subsystem.

The indirect calls are expensive, but they are expensive not because
an indirect call itself is particularly expensive (although that's
true on some architectures too), but because the whole notion of "I'm
calling down to the lower-level non-generic code" means that we can't
do inlining, we can't optimize locking, we can't do anything clever.

My selinux patch kept the indirect call, but at least made it cheap.
Could we do even better? And keep it generic?

Btw, if we can do something like that, then nested security modules
likely get much easier to do too, because the nesting would all be
behind the cache. Once it's behind the cache, it doesn't matter if
we'd need to traverse lists etc. The hot case would be able to ignore
it all.

                  Linus

Re: Stupid VFS name lookup interface..

[Casey Schaufler]

On 5/26/2013 5:02 AM, Theodore Ts'o wrote:
> On Sat, May 25, 2013 at 11:33:46AM -0700, Casey Schaufler wrote:
>> Now I'll put on my Smack maintainer hat. Performance improvement is
>> always welcome, but I would rather see attention to performance of
>> the LSM architecture than SELinux specific hacks. The LSM blob
>> pointer scheme is there so that you (Linus) don't have to see the
>> dreadful things that we security people are doing. Is it time to
>> get past that level of disassociation? Or, and I really hate asking
>> this, have you fallen into the SELinux camp?
> What part of the LSM architecture are you proposing be optimized?

Secids are an inherent performance issue.

This thread is all about a performance problem with
the security blob pointer scheme. I don't know what
would be better and general, but I'm willing to learn.


> The
> LSM layer is pretty thin, partially because the various different
> security approaches don't agree with each other on fairly fundamental
> issues.  What sort of optimization opportunities you are suggesting?
> Are there changes that can be made that all of the major security LSM
> maintainers would actually agree with?

As you point out, the various existing LSMs use a variety of
mechanisms to perform their access checks. A big part of what
I see as the "problem" is that the LSM hooks grew organically,
at a time when there was exactly one project being funded.
By the time other LSMs came in to the mainstream we had a
collection of hooks, not an architecture. The LSM architecture
has not been seriously revisited since.

Can we come to agreement? I don't know. I expect so.

>
> I've been re-reading the thread on LKML which was spawned when SMACK
> was proposed for upstream inclusion:
>
>     http://thread.gmane.org/gmane.linux.kernel/585903/focus=586412
>
> Have any of the arguments over the proper security models changed over
> or have gotten resolved over the past six years, while I haven't been
> looking?

I believe that Yama points to a serious change in the way
"operating systems" are being developed. The desktop is not
the sweet spot for Linux development, nor is the enterprise
server. Six years ago the Bell & LaPadula subject/object models
still made sense. Today, we're looking at applications, services
and resources. We don't have LSMs that support those* natively.
We are going to have new LSMs, and soon, if Linux is going to
remain relevant.

---
* SEAndriod is trying. We'll see where that goes.


>
> 						- Ted
>

Re: Stupid VFS name lookup interface..

[Casey Schaufler]

On 5/26/2013 11:17 AM, Linus Torvalds wrote:
> On Sun, May 26, 2013 at 10:59 AM, Casey Schaufler
> <casey@schaufler-ca.com> wrote:
>> The whole secid philosophy comes out of the need to keep security out
>> of other people's way. It has performance impact. Sure, SELinux
>> hashes lookups, but a blob pointer gets you right where you want to be.
>> When we are constrained in unnatural ways there are going to be
>> consequences. Performance is one. Code complexity is another.
> Quite frankly, I'd like to possibly introduce a cache of security
> decisions at least for the common filesystem operations (and the
> per-pathcomponent lookup is *the* most common one, but the per-stat
> one is pretty bad too), and put that cache at the VFS level, so that
> the security people can *not* screw it up, and so that we don't call
> down to the security layer at all 99% of the time.
>
> Once that happens, we don't care any more what security people do.
>
> It has been how we have fixed performance problems for filesystems
> every single time. It's simply not possible to have a generic
> interface to 50+ different filesystems and expect that kind of generic
> interface to be high-performance - but when we've been able to
> abstract it out as a cache in front of the filesystem operations, it
> is suddenly quite reasonable to spend a lot of effort making that
> cache go fast like a bat out of hell.
>
> It started with the dentry cache and the page cache, but we now do the
> POSIX ACL's that way too, because it just wasn't reasonable to call
> down to the filesystem to look up ACL's and have all the complex "we
> do this with RCU locks held" semantics.
>
> Is there something similar we could do for the security layer? We
> don't have 50+ different security models, but we do have several. If
> the different security modules could agree on some kind of generic
> "security ID" model so that we could cache things (see fs/namei.c and
> get_cached_acl_rcu() for example), it would be a great thing.
>
> Then selinux could get rid of it's hashed lookups entirely, because
> that whole "cache the security ID" would be handled by generic code.
>
> But that *would* require that there would be some abstract notion of
> security ID/context that we could use in generic code *WITHOUT* the
> need to call down the the security subsystem.
>
> The indirect calls are expensive, but they are expensive not because
> an indirect call itself is particularly expensive (although that's
> true on some architectures too), but because the whole notion of "I'm
> calling down to the lower-level non-generic code" means that we can't
> do inlining, we can't optimize locking, we can't do anything clever.
>
> My selinux patch kept the indirect call, but at least made it cheap.
> Could we do even better? And keep it generic?

Probably, but we'll need help from people who really understand VFS,
caching and RCU.

> Btw, if we can do something like that, then nested security modules
> likely get much easier to do too, because the nesting would all be
> behind the cache. Once it's behind the cache, it doesn't matter if
> we'd need to traverse lists etc. The hot case would be able to ignore
> it all.

Indeed. It may require significant revision to the existing LSMs.

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

Re: Stupid VFS name lookup interface..

[Theodore Ts'o]

On Sun, May 26, 2013 at 11:23:01AM -0700, Casey Schaufler wrote:
> I believe that Yama points to a serious change in the way
> "operating systems" are being developed. The desktop is not
> the sweet spot for Linux development, nor is the enterprise
> server. Six years ago the Bell & LaPadula subject/object models
> still made sense. Today, we're looking at applications, services
> and resources. We don't have LSMs that support those* natively.
> We are going to have new LSMs, and soon, if Linux is going to
> remain relevant.

Oh, I agree, having worked on a TS project involving real-time Linux
that did not even try to use a Bell-LaPadula security model on the
Linux system.

The challenge is that although the world does seem to be moving
towards using air gap firewalls and separate single-level systems
(using trusted message passing routers and/or trusted hypervisors)
instead of a MLS design, I'm sure there will still be some
applications where people will want a Bell-LaPadula security model.
And if we can't rip out that fundamental assumption, it's not obvious
to me it will be possible to simplify the core LSM architecture.

We also have to consider especially how much sunk costs have been
invested in systems such as SELinux which can support (but which are
not limited to) Bell-LaPadula.  If we simplify the LSM architecture,
but it requires a radical rewrite of systems where massive amounts of
effort have been poured into make them somewhat easier to use
(although personally I'm not convinced that a policy file which is
hundreds of kilobytes qualifies as "easy" to debug or to validate), I
suspect the people who don't feel like throwing all of that heavy
investment away will resist such an initiative mightily.  (Not to
mention that if we break backwards compatibility, we will end up
breaking userspace for deployed enterprise distro's.)

And yet, if we don't rip out some of these assumptions, it's not
obvious to me how much even Linus's caching idea is going to buy us.
A generic caching layer still has to include in its hash all of the
possible inputs that a LSM module might want to use as part of its
access decision.  If this includes the pathname, then you'll have to
lock a whole bunch of dentries while you calculate its hash ---
something that wouldn't be necessary for SELinux, for example.  And
I'm sure SELinux uses some things when making its access decisions
which Smack does not need, and so on.  So a generalized caching scheme
might not result in any any performance wins; it might even be worse
than what we have today!

So I'm dubious --- but maybe this is something which a Security
working group could maybe try to hash out at a face-to-face meeting.
Given our past track record of coming to consensus, I suspect
face-to-face has a higher chance of working --- who knows, maybe hell
will freeze over or pigs will end up nesting in trees.  :-)

Cheers,

					- Ted

Re: Stupid VFS name lookup interface..

[Linus Torvalds]

On Sun, May 26, 2013 at 12:11 PM, Theodore Ts'o <tytso@mit.edu> wrote:
>
> And if we can't rip out that fundamental assumption, it's not obvious
> to me it will be possible to simplify the core LSM architecture.

One thing that may be sufficient is to maintain a complex model as a
*possible* case, but make sure that we handle the simple cases well.

For example, a "security cache" *might* be as simple as a single bit
saying "this inode has no special file permissions outside the normal
UNIX ones".

For example, the biggest win in the whole POSIX ACL cache was not
actually caching POSIX ACL's at all - even though it does that - but
caching the fact that a particular file did *not* have any POSIX ACL's
associated with it. So the POSIX AXL cache was very much designed to
have separate states for "I don't have a cache entry" and "I have an
empty cache entry". And 99% of the time, that negative cache is
sufficient.

Similarly, for at least the filesystem security case, if we had a "the
owner of this file can do all the normal operations on it" bit, that
would generally take care of 99% of all home directory operations. If,
in addition to that, there's some way to mark other "normal"
directories as having only normal UNIX security semantics (ie /home
and /usr), that would take care of most of the rest.

Having to call into the security layer when you cross some special
boundary is fine. It's doing it for every single path component, and
every single 'stat' of a regular file - *THAT* is what kills us.

So complexity could be ok. As long as the common case isn't complex,
and as long as there's a simple way to check that. We don't
necessarily need to simplify things in the general case.

             Linus

Re: Stupid VFS name lookup interface..

[Casey Schaufler]

On 5/26/2013 12:32 PM, Linus Torvalds wrote:
> On Sun, May 26, 2013 at 12:11 PM, Theodore Ts'o <tytso@mit.edu> wrote:
>> And if we can't rip out that fundamental assumption, it's not obvious
>> to me it will be possible to simplify the core LSM architecture.
> One thing that may be sufficient is to maintain a complex model as a
> *possible* case, but make sure that we handle the simple cases well.
>
> For example, a "security cache" *might* be as simple as a single bit
> saying "this inode has no special file permissions outside the normal
> UNIX ones".
>
> For example, the biggest win in the whole POSIX ACL cache was not
> actually caching POSIX ACL's at all - even though it does that - but
> caching the fact that a particular file did *not* have any POSIX ACL's
> associated with it. So the POSIX AXL cache was very much designed to
> have separate states for "I don't have a cache entry" and "I have an
> empty cache entry". And 99% of the time, that negative cache is
> sufficient.

POSIX ACLs work well in this scheme for two reasons. First, they are
optional and only show up on files when someone wants to put one there.
Second, no one seems inclined to use them. Because the existence of an
ACL is rare it makes *lots* of sense to shortcut if there is no ACL.

SELinux contexts and Smack labels are different from ACLs in that
the usual case will be that the label exists and a file without the
extended attribute will be quite rare. Because every file is expected
to have a context/label no check that is as simple or convenient as
what you have with ACLs is available.

> Similarly, for at least the filesystem security case, if we had a "the
> owner of this file can do all the normal operations on it" bit, that
> would generally take care of 99% of all home directory operations. If,
> in addition to that, there's some way to mark other "normal"
> directories as having only normal UNIX security semantics (ie /home
> and /usr), that would take care of most of the rest.

This is pretty close to the Smack "Basic Rule", which says that a process
with the same label as an object can do what it will with it. You only
go looking at the Smack rules if this check fails.

SELinux (Eric, James, correct me where I mislead) may need to do some
calculation based on the process context, containing directory, component
name and (perhaps) other factors before it's possible to determine exactly
what contexts are being compared. It may very well be possible to do some
amount of advanced computation on this, but how that might work is beyond
me.

It might be possible to implement labeling on a filesystem basis, and
achieve performance tricks based on that. Both SELinux and Smack are
capable of providing a single context/label for a filesystem, although
Smack currently always uses extended attributes when they are available.
This would be of limited value in most of the configurations I have seen.


> Having to call into the security layer when you cross some special
> boundary is fine. It's doing it for every single path component, and
> every single 'stat' of a regular file - *THAT* is what kills us.

Hmm. If we associated the access checks with the object rather than the
task we might be able to do something here. That's the VFS caching that
you've been referring to, I expect. On a system with a small number of
contexts/labels I'd expect this to help.

> So complexity could be ok. As long as the common case isn't complex,
> and as long as there's a simple way to check that. We don't
> necessarily need to simplify things in the general case.

Keeping the common case common is the problem with mandatory access
control systems. Once the granularity gremlins get control of a
distribution no one access case is ever going to be the common case.

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

Re: Stupid VFS name lookup interface..

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 408 bytes --]

On Sun, 26 May 2013 08:02:51 -0400, "Theodore Ts'o" said:

> Have any of the arguments over the proper security models changed over
> or have gotten resolved over the past six years, while I haven't been
> looking?

Doubtful, because the security models are addressing different threat
models.  If you can't agree on what you're trying to secure against,
there can be no agreement on how to do the securing.

[-- Attachment #2: Type: application/pgp-signature, Size: 865 bytes --]

Re: Stupid VFS name lookup interface..

[Eric Paris]

On Sat, May 25, 2013 at 10:19 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Sat, May 25, 2013 at 10:04 PM, James Morris <jmorris@namei.org> wrote:
>> On Sat, 25 May 2013, Linus Torvalds wrote:
>>
>>> But I haven't even looked at what non-selinux setups do to
>>> performance. Last time I tried Ubuntu (they still use apparmor, no?),
>>> "make modules_install ; make install" didn't work for the kernel, and
>>> if the Ubuntu people don't want to support kernel engineers, I
>>> certainly am not going to bother with them. Who uses smack?
>>
>> Tizen, perhaps a few others.
>
> Btw, it really would be good if security people started realizing that
> performance matters. It's annoying to see the security lookups cause
> 50% performance degradations on pathname lookup (and no, I'm not
> exaggerating, that's literally what it was before we fixed it - and
> no, by "we" I don't mean security people).

I take a bit of exception to this.  I do care.  Stephen Smalley, the
only other person who does SELinux kernel work, cares.  I don't speak
for other LSMs, but at least both of us who have done anything with
SELinux in the last years care.  I did the RCU work for selinux and
you, sds, and I did a bunch of work to stop wasting so much stack
space which was crapping on performance.  And I'm here again   :)

> Right now (zooming into the kernel only - ignoring the fact that make
> really spends a fair amount of time in user space) I get
>
>   9.79%      make  [k] __d_lookup_rcu
>   5.48%      make  [k] link_path_walk
>   2.94%      make  [k] avc_has_perm_noaudit
>   2.47%      make  [k] selinux_inode_permission
>   2.25%      make  [k] path_lookupat
>   1.89%      make  [k] generic_fillattr
>   1.50%      make  [k] lookup_fast
>   1.27%      make  [k] copy_user_generic_string
>   1.17%      make  [k] generic_permission
>   1.15%      make  [k] dput
>   1.12%      make  [k] inode_has_perm.constprop.58
>   1.11%      make  [k] __inode_permission
>   1.08%      make  [k] kmem_cache_alloc
>   ...

I tried something else, doing caching of the last successful security
check inside the isec.  It isn't race free, so it's not ready for
prime time.  But right now my >1% looks like:

  7.97%          make  [k] __d_lookup_rcu
  5.79%          make  [k] link_path_walk
  3.67%          make  [k] selinux_inode_permission
  2.02%          make  [k] lookup_fast
  1.90%          make  [k] system_call
  1.76%          make  [k] path_lookupat
  1.68%          make  [k] inode_has_perm.isra.45.constprop.61
  1.53%          make  [k] copy_user_enhanced_fast_string
  1.39%          make  [k] generic_permission
  1.35%          make  [k] kmem_cache_free
  1.30%          make  [k] __audit_syscall_exit
  1.13%          make  [k] kmem_cache_alloc
  1.00%          make  [k] strncpy_from_user

How do I tell what is taking time inside selinux_inode_permission?

Re: Stupid VFS name lookup interface..

[Linus Torvalds]

On Thu, May 30, 2013 at 10:28 AM, Eric Paris <eparis@parisplace.org> wrote:
>
> How do I tell what is taking time inside selinux_inode_permission?

Go to "annotate" (just press 'a' when the function is highlighted),
which will show you the disassembly and the cost of each instruction.

That's when you really want to use "-e cycles:pp" to get the
instruction-level profile right, though. Otherwise the cost will
usually be assigned to the instructions following the expensive one.

And I can tell you that the cost is almost certainly the cache miss on
the inode->i_security accesses. Which was the reason for that second
patch that moved "sid" to the inode->i_sid field and avoided the extra
dereference.

             Linus