[patch 00/14] vfs: permission API cleanup

[patch 00/14] vfs: permission API cleanup

[Miklos Szeredi]

Highlights are:

 - remove nameidata from the i_op->permission() method
 - remove nameidata passing and consolidate permission()/
   vfs_permission()/file_permission() API

It's all fairly trivial stuff, but I will do some LTP testing on it.

This series is based on the vfs-cleanups(*) tree.  If no problems are
found, I'll commit the patches to that tree.

Thanks,
Miklos

(*) git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git vfs-cleanups

--

[patch 01/14] vfs: add path_getxattr()

[Miklos Szeredi]

[-- Attachment #1: path_getxattr.patch --]
[-- Type: text/plain, Size: 7520 bytes --]

From: Miklos Szeredi <mszeredi@suse.cz>

Introduce path_getxattr().  Make vfs_getxattr() static.

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
---
 fs/nfsd/nfs4xdr.c         |    2 +-
 fs/nfsd/vfs.c             |   25 ++++++++++++++++---------
 fs/xattr.c                |   15 ++++++++-------
 include/linux/nfsd/nfsd.h |    3 ++-
 include/linux/xattr.h     |    2 +-
 5 files changed, 28 insertions(+), 19 deletions(-)

Index: linux-2.6/fs/nfsd/nfs4xdr.c
===================================================================
--- linux-2.6.orig/fs/nfsd/nfs4xdr.c	2008-05-14 12:49:18.000000000 +0200
+++ linux-2.6/fs/nfsd/nfs4xdr.c	2008-05-21 13:21:28.000000000 +0200
@@ -1487,7 +1487,7 @@ nfsd4_encode_fattr(struct svc_fh *fhp, s
 	}
 	if (bmval0 & (FATTR4_WORD0_ACL | FATTR4_WORD0_ACLSUPPORT
 			| FATTR4_WORD0_SUPPORTED_ATTRS)) {
-		err = nfsd4_get_nfs4_acl(rqstp, dentry, &acl);
+		err = nfsd4_get_nfs4_acl(rqstp, exp, dentry, &acl);
 		aclsupport = (err == 0);
 		if (bmval0 & FATTR4_WORD0_ACL) {
 			if (err == -EOPNOTSUPP)
Index: linux-2.6/fs/nfsd/vfs.c
===================================================================
--- linux-2.6.orig/fs/nfsd/vfs.c	2008-05-21 13:13:29.000000000 +0200
+++ linux-2.6/fs/nfsd/vfs.c	2008-05-21 13:34:27.000000000 +0200
@@ -416,11 +416,11 @@ out_nfserr:
 #if defined(CONFIG_NFSD_V2_ACL) || \
     defined(CONFIG_NFSD_V3_ACL) || \
     defined(CONFIG_NFSD_V4)
-static ssize_t nfsd_getxattr(struct dentry *dentry, char *key, void **buf)
+static ssize_t nfsd_getxattr(struct path *path, char *key, void **buf)
 {
 	ssize_t buflen;
 
-	buflen = vfs_getxattr(dentry, key, NULL, 0);
+	buflen = path_getxattr(path, key, NULL, 0);
 	if (buflen <= 0)
 		return buflen;
 
@@ -428,7 +428,7 @@ static ssize_t nfsd_getxattr(struct dent
 	if (!*buf)
 		return -ENOMEM;
 
-	return vfs_getxattr(dentry, key, *buf, buflen);
+	return path_getxattr(path, key, *buf, buflen);
 }
 #endif
 
@@ -504,13 +504,13 @@ out_nfserr:
 }
 
 static struct posix_acl *
-_get_posix_acl(struct dentry *dentry, char *key)
+_get_posix_acl(struct path *path, char *key)
 {
 	void *buf = NULL;
 	struct posix_acl *pacl = NULL;
 	int buflen;
 
-	buflen = nfsd_getxattr(dentry, key, &buf);
+	buflen = nfsd_getxattr(path, key, &buf);
 	if (!buflen)
 		buflen = -ENODATA;
 	if (buflen <= 0)
@@ -522,14 +522,19 @@ _get_posix_acl(struct dentry *dentry, ch
 }
 
 int
-nfsd4_get_nfs4_acl(struct svc_rqst *rqstp, struct dentry *dentry, struct nfs4_acl **acl)
+nfsd4_get_nfs4_acl(struct svc_rqst *rqstp, struct svc_export *exp,
+		   struct dentry *dentry, struct nfs4_acl **acl)
 {
+	struct path path = {
+		.mnt = exp->ex_path.mnt,
+		.dentry = dentry,
+	};
 	struct inode *inode = dentry->d_inode;
 	int error = 0;
 	struct posix_acl *pacl = NULL, *dpacl = NULL;
 	unsigned int flags = 0;
 
-	pacl = _get_posix_acl(dentry, POSIX_ACL_XATTR_ACCESS);
+	pacl = _get_posix_acl(&path, POSIX_ACL_XATTR_ACCESS);
 	if (IS_ERR(pacl) && PTR_ERR(pacl) == -ENODATA)
 		pacl = posix_acl_from_mode(inode->i_mode, GFP_KERNEL);
 	if (IS_ERR(pacl)) {
@@ -539,7 +544,7 @@ nfsd4_get_nfs4_acl(struct svc_rqst *rqst
 	}
 
 	if (S_ISDIR(inode->i_mode)) {
-		dpacl = _get_posix_acl(dentry, POSIX_ACL_XATTR_DEFAULT);
+		dpacl = _get_posix_acl(&path, POSIX_ACL_XATTR_DEFAULT);
 		if (IS_ERR(dpacl) && PTR_ERR(dpacl) == -ENODATA)
 			dpacl = NULL;
 		else if (IS_ERR(dpacl)) {
@@ -2000,6 +2005,7 @@ nfsd_racache_init(int cache_size)
 struct posix_acl *
 nfsd_get_posix_acl(struct svc_fh *fhp, int type)
 {
+	struct path path;
 	struct inode *inode = fhp->fh_dentry->d_inode;
 	char *name;
 	void *value = NULL;
@@ -2020,7 +2026,8 @@ nfsd_get_posix_acl(struct svc_fh *fhp, i
 		return ERR_PTR(-EOPNOTSUPP);
 	}
 
-	size = nfsd_getxattr(fhp->fh_dentry, name, &value);
+	fh_to_path(fhp, &path);
+	size = nfsd_getxattr(&path, name, &value);
 	if (size < 0)
 		return ERR_PTR(size);
 
Index: linux-2.6/fs/xattr.c
===================================================================
--- linux-2.6.orig/fs/xattr.c	2008-05-21 13:13:29.000000000 +0200
+++ linux-2.6/fs/xattr.c	2008-05-21 13:30:33.000000000 +0200
@@ -144,8 +144,9 @@ out_noalloc:
 EXPORT_SYMBOL_GPL(xattr_getsecurity);
 
 ssize_t
-vfs_getxattr(struct dentry *dentry, const char *name, void *value, size_t size)
+path_getxattr(struct path *path, const char *name, void *value, size_t size)
 {
+	struct dentry *dentry = path->dentry;
 	struct inode *inode = dentry->d_inode;
 	int error;
 
@@ -177,7 +178,7 @@ nolsm:
 
 	return error;
 }
-EXPORT_SYMBOL_GPL(vfs_getxattr);
+EXPORT_SYMBOL_GPL(path_getxattr);
 
 ssize_t
 vfs_listxattr(struct dentry *d, char *list, size_t size)
@@ -326,7 +327,7 @@ sys_fsetxattr(int fd, const char __user 
  * Extended attribute GET operations
  */
 static ssize_t
-getxattr(struct dentry *d, const char __user *name, void __user *value,
+getxattr(struct path *path, const char __user *name, void __user *value,
 	 size_t size)
 {
 	ssize_t error;
@@ -347,7 +348,7 @@ getxattr(struct dentry *d, const char __
 			return -ENOMEM;
 	}
 
-	error = vfs_getxattr(d, kname, kvalue, size);
+	error = path_getxattr(path, kname, kvalue, size);
 	if (error > 0) {
 		if (size && copy_to_user(value, kvalue, error))
 			error = -EFAULT;
@@ -370,7 +371,7 @@ sys_getxattr(const char __user *path, co
 	error = user_path_walk(path, &nd);
 	if (error)
 		return error;
-	error = getxattr(nd.path.dentry, name, value, size);
+	error = getxattr(&nd.path, name, value, size);
 	path_put(&nd.path);
 	return error;
 }
@@ -385,7 +386,7 @@ sys_lgetxattr(const char __user *path, c
 	error = user_path_walk_link(path, &nd);
 	if (error)
 		return error;
-	error = getxattr(nd.path.dentry, name, value, size);
+	error = getxattr(&nd.path, name, value, size);
 	path_put(&nd.path);
 	return error;
 }
@@ -400,7 +401,7 @@ sys_fgetxattr(int fd, const char __user 
 	if (!f)
 		return error;
 	audit_inode(NULL, f->f_path.dentry);
-	error = getxattr(f->f_path.dentry, name, value, size);
+	error = getxattr(&f->f_path, name, value, size);
 	fput(f);
 	return error;
 }
Index: linux-2.6/include/linux/xattr.h
===================================================================
--- linux-2.6.orig/include/linux/xattr.h	2008-05-19 21:48:17.000000000 +0200
+++ linux-2.6/include/linux/xattr.h	2008-05-21 13:15:13.000000000 +0200
@@ -48,7 +48,7 @@ struct xattr_handler {
 };
 
 ssize_t xattr_getsecurity(struct inode *, const char *, void *, size_t);
-ssize_t vfs_getxattr(struct dentry *, const char *, void *, size_t);
+ssize_t path_getxattr(struct path *, const char *, void *, size_t);
 ssize_t vfs_listxattr(struct dentry *d, char *list, size_t size);
 int path_setxattr(struct path *, const char *, const void *, size_t, int);
 int path_removexattr(struct path *, const char *);
Index: linux-2.6/include/linux/nfsd/nfsd.h
===================================================================
--- linux-2.6.orig/include/linux/nfsd/nfsd.h	2008-05-14 12:49:30.000000000 +0200
+++ linux-2.6/include/linux/nfsd/nfsd.h	2008-05-21 13:34:42.000000000 +0200
@@ -85,7 +85,8 @@ __be32		nfsd_setattr(struct svc_rqst *, 
 #ifdef CONFIG_NFSD_V4
 __be32          nfsd4_set_nfs4_acl(struct svc_rqst *, struct svc_fh *,
                     struct nfs4_acl *);
-int             nfsd4_get_nfs4_acl(struct svc_rqst *, struct dentry *, struct nfs4_acl **);
+int             nfsd4_get_nfs4_acl(struct svc_rqst *, struct svc_export *,
+				struct dentry *, struct nfs4_acl **);
 #endif /* CONFIG_NFSD_V4 */
 __be32		nfsd_create(struct svc_rqst *, struct svc_fh *,
 				char *name, int len, struct iattr *attrs,

--

[patch 02/14] vfs: add path_listxattr()

[Miklos Szeredi]

[-- Attachment #1: path_listxattr.patch --]
[-- Type: text/plain, Size: 3047 bytes --]

From: Miklos Szeredi <mszeredi@suse.cz>

Introduce path_listxattr().  Make vfs_listxattr() static.

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
---
 fs/xattr.c            |   15 ++++++++-------
 include/linux/xattr.h |    2 +-
 2 files changed, 9 insertions(+), 8 deletions(-)

Index: linux-2.6/fs/xattr.c
===================================================================
--- linux-2.6.orig/fs/xattr.c	2008-05-21 13:30:33.000000000 +0200
+++ linux-2.6/fs/xattr.c	2008-05-21 13:39:36.000000000 +0200
@@ -181,8 +181,9 @@ nolsm:
 EXPORT_SYMBOL_GPL(path_getxattr);
 
 ssize_t
-vfs_listxattr(struct dentry *d, char *list, size_t size)
+path_listxattr(struct path *path, char *list, size_t size)
 {
+	struct dentry *d = path->dentry;
 	ssize_t error;
 
 	error = security_inode_listxattr(d);
@@ -198,7 +199,7 @@ vfs_listxattr(struct dentry *d, char *li
 	}
 	return error;
 }
-EXPORT_SYMBOL_GPL(vfs_listxattr);
+EXPORT_SYMBOL_GPL(path_listxattr);
 
 static int
 vfs_removexattr(struct dentry *dentry, const char *name)
@@ -410,7 +411,7 @@ sys_fgetxattr(int fd, const char __user 
  * Extended attribute LIST operations
  */
 static ssize_t
-listxattr(struct dentry *d, char __user *list, size_t size)
+listxattr(struct path *path, char __user *list, size_t size)
 {
 	ssize_t error;
 	char *klist = NULL;
@@ -423,7 +424,7 @@ listxattr(struct dentry *d, char __user 
 			return -ENOMEM;
 	}
 
-	error = vfs_listxattr(d, klist, size);
+	error = path_listxattr(path, klist, size);
 	if (error > 0) {
 		if (size && copy_to_user(list, klist, error))
 			error = -EFAULT;
@@ -445,7 +446,7 @@ sys_listxattr(const char __user *path, c
 	error = user_path_walk(path, &nd);
 	if (error)
 		return error;
-	error = listxattr(nd.path.dentry, list, size);
+	error = listxattr(&nd.path, list, size);
 	path_put(&nd.path);
 	return error;
 }
@@ -459,7 +460,7 @@ sys_llistxattr(const char __user *path, 
 	error = user_path_walk_link(path, &nd);
 	if (error)
 		return error;
-	error = listxattr(nd.path.dentry, list, size);
+	error = listxattr(&nd.path, list, size);
 	path_put(&nd.path);
 	return error;
 }
@@ -474,7 +475,7 @@ sys_flistxattr(int fd, char __user *list
 	if (!f)
 		return error;
 	audit_inode(NULL, f->f_path.dentry);
-	error = listxattr(f->f_path.dentry, list, size);
+	error = listxattr(&f->f_path, list, size);
 	fput(f);
 	return error;
 }
Index: linux-2.6/include/linux/xattr.h
===================================================================
--- linux-2.6.orig/include/linux/xattr.h	2008-05-21 13:15:13.000000000 +0200
+++ linux-2.6/include/linux/xattr.h	2008-05-21 13:37:56.000000000 +0200
@@ -49,7 +49,7 @@ struct xattr_handler {
 
 ssize_t xattr_getsecurity(struct inode *, const char *, void *, size_t);
 ssize_t path_getxattr(struct path *, const char *, void *, size_t);
-ssize_t vfs_listxattr(struct dentry *d, char *list, size_t size);
+ssize_t path_listxattr(struct path *, char *, size_t);
 int path_setxattr(struct path *, const char *, const void *, size_t, int);
 int path_removexattr(struct path *, const char *);
 

--

[patch 03/14] hppfs: remove hppfs_permission

[Miklos Szeredi]

[-- Attachment #1: hppfs_permission_cleanup.patch --]
[-- Type: text/plain, Size: 1145 bytes --]

From: Miklos Szeredi <mszeredi@suse.cz>

hppfs_permission() is equivalent to the '.permission == NULL' case.

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
CC: Jeff Dike <jdike@linux.intel.com>
---
 fs/hppfs/hppfs.c |    7 -------
 1 file changed, 7 deletions(-)

Index: linux-2.6/fs/hppfs/hppfs.c
===================================================================
--- linux-2.6.orig/fs/hppfs/hppfs.c	2008-05-21 13:13:29.000000000 +0200
+++ linux-2.6/fs/hppfs/hppfs.c	2008-05-21 13:41:20.000000000 +0200
@@ -655,20 +655,13 @@ static void *hppfs_follow_link(struct de
 	return proc_dentry->d_inode->i_op->follow_link(proc_dentry, nd);
 }
 
-int hppfs_permission(struct inode *inode, int mask, struct nameidata *nd)
-{
-	return generic_permission(inode, mask, NULL);
-}
-
 static const struct inode_operations hppfs_dir_iops = {
 	.lookup		= hppfs_lookup,
-	.permission	= hppfs_permission,
 };
 
 static const struct inode_operations hppfs_link_iops = {
 	.readlink	= hppfs_readlink,
 	.follow_link	= hppfs_follow_link,
-	.permission	= hppfs_permission,
 };
 
 static struct inode *get_inode(struct super_block *sb, struct dentry *dentry)

--

[patch 04/14] gfs2: dont call permission()

[Miklos Szeredi]

[-- Attachment #1: gfs2_permission_fix.patch --]
[-- Type: text/plain, Size: 6186 bytes --]

From: Miklos Szeredi <mszeredi@suse.cz>

GFS2 calls permission() to verify permissions after locks on the files
have been taken.

For this it's sufficient to call gfs2_permission() instead.  This
results in the following changes:

  - IS_RDONLY() check is not performed
  - IS_IMMUTABLE() check is not performed
  - devcgroup_inode_permission() is not called
  - security_inode_permission() is not called

IS_RDONLY() should be unnecessary anyway, as the per-mount read-only
flag should provide protection against read-only remounts during
operations.  do_gfs2_set_flags() has been fixed to perform
mnt_want_write()/mnt_drop_write() to protect against remounting
read-only.

IS_IMMUTABLE has beed added to gfs2_do_permission()

Repeating the security checks seems to be pointless, as they don't
normally change, and if they do, it's independent of the filesystem
state.

I also suspect the conditional locking in gfs2_do_permission() could
be cleaned up, due to the removal of the implicit recursion.

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
CC: Steven Whitehouse <swhiteho@redhat.com>
---
 fs/gfs2/inode.c     |    6 +++---
 fs/gfs2/inode.h     |    1 +
 fs/gfs2/ops_file.c  |   11 +++++++++--
 fs/gfs2/ops_inode.c |   18 +++++++++++++-----
 4 files changed, 26 insertions(+), 10 deletions(-)

Index: linux-2.6/fs/gfs2/inode.c
===================================================================
--- linux-2.6.orig/fs/gfs2/inode.c	2008-05-21 15:45:11.000000000 +0200
+++ linux-2.6/fs/gfs2/inode.c	2008-05-21 15:45:45.000000000 +0200
@@ -504,7 +504,7 @@ struct inode *gfs2_lookupi(struct inode 
 	}
 
 	if (!is_root) {
-		error = permission(dir, MAY_EXEC, NULL);
+		error = gfs2_do_permission(dir, MAY_EXEC);
 		if (error)
 			goto out;
 	}
@@ -667,7 +667,7 @@ static int create_ok(struct gfs2_inode *
 {
 	int error;
 
-	error = permission(&dip->i_inode, MAY_WRITE | MAY_EXEC, NULL);
+	error = gfs2_do_permission(&dip->i_inode, MAY_WRITE | MAY_EXEC);
 	if (error)
 		return error;
 
@@ -1134,7 +1134,7 @@ int gfs2_unlink_ok(struct gfs2_inode *di
 	if (IS_APPEND(&dip->i_inode))
 		return -EPERM;
 
-	error = permission(&dip->i_inode, MAY_WRITE | MAY_EXEC, NULL);
+	error = gfs2_do_permission(&dip->i_inode, MAY_WRITE | MAY_EXEC);
 	if (error)
 		return error;
 
Index: linux-2.6/fs/gfs2/inode.h
===================================================================
--- linux-2.6.orig/fs/gfs2/inode.h	2008-05-21 15:45:11.000000000 +0200
+++ linux-2.6/fs/gfs2/inode.h	2008-05-21 15:45:45.000000000 +0200
@@ -91,6 +91,7 @@ int gfs2_rmdiri(struct gfs2_inode *dip, 
 		struct gfs2_inode *ip);
 int gfs2_unlink_ok(struct gfs2_inode *dip, const struct qstr *name,
 		   const struct gfs2_inode *ip);
+int gfs2_do_permission(struct inode *inode, int mask);
 int gfs2_ok_to_move(struct gfs2_inode *this, struct gfs2_inode *to);
 int gfs2_readlinki(struct gfs2_inode *ip, char **buf, unsigned int *len);
 int gfs2_glock_nq_atime(struct gfs2_holder *gh);
Index: linux-2.6/fs/gfs2/ops_file.c
===================================================================
--- linux-2.6.orig/fs/gfs2/ops_file.c	2008-05-21 15:45:11.000000000 +0200
+++ linux-2.6/fs/gfs2/ops_file.c	2008-05-21 15:45:45.000000000 +0200
@@ -15,6 +15,7 @@
 #include <linux/uio.h>
 #include <linux/blkdev.h>
 #include <linux/mm.h>
+#include <linux/mount.h>
 #include <linux/fs.h>
 #include <linux/gfs2_ondisk.h>
 #include <linux/ext2_fs.h>
@@ -220,10 +221,14 @@ static int do_gfs2_set_flags(struct file
 	int error;
 	u32 new_flags, flags;
 
-	error = gfs2_glock_nq_init(ip->i_gl, LM_ST_EXCLUSIVE, 0, &gh);
+	error = mnt_want_write(filp->f_path.mnt);
 	if (error)
 		return error;
 
+	error = gfs2_glock_nq_init(ip->i_gl, LM_ST_EXCLUSIVE, 0, &gh);
+	if (error)
+		goto out_drop_write;
+
 	flags = ip->i_di.di_flags;
 	new_flags = (flags & ~mask) | (reqflags & mask);
 	if ((new_flags ^ flags) == 0)
@@ -242,7 +247,7 @@ static int do_gfs2_set_flags(struct file
 	    !capable(CAP_LINUX_IMMUTABLE))
 		goto out;
 	if (!IS_IMMUTABLE(inode)) {
-		error = permission(inode, MAY_WRITE, NULL);
+		error = gfs2_do_permission(inode, MAY_WRITE);
 		if (error)
 			goto out;
 	}
@@ -272,6 +277,8 @@ out_trans_end:
 	gfs2_trans_end(sdp);
 out:
 	gfs2_glock_dq_uninit(&gh);
+out_drop_write:
+	mnt_drop_write(filp->f_path.mnt);
 	return error;
 }
 
Index: linux-2.6/fs/gfs2/ops_inode.c
===================================================================
--- linux-2.6.orig/fs/gfs2/ops_inode.c	2008-05-21 15:45:11.000000000 +0200
+++ linux-2.6/fs/gfs2/ops_inode.c	2008-05-21 15:55:15.000000000 +0200
@@ -163,7 +163,7 @@ static int gfs2_link(struct dentry *old_
 	if (error)
 		goto out;
 
-	error = permission(dir, MAY_WRITE | MAY_EXEC, NULL);
+	error = gfs2_do_permission(dir, MAY_WRITE | MAY_EXEC);
 	if (error)
 		goto out_gunlock;
 
@@ -669,7 +669,7 @@ static int gfs2_rename(struct inode *odi
 			}
 		}
 	} else {
-		error = permission(ndir, MAY_WRITE | MAY_EXEC, NULL);
+		error = gfs2_do_permission(ndir, MAY_WRITE | MAY_EXEC);
 		if (error)
 			goto out_gunlock;
 
@@ -704,7 +704,7 @@ static int gfs2_rename(struct inode *odi
 	/* Check out the dir to be renamed */
 
 	if (dir_rename) {
-		error = permission(odentry->d_inode, MAY_WRITE, NULL);
+		error = gfs2_do_permission(odentry->d_inode, MAY_WRITE);
 		if (error)
 			goto out_gunlock;
 	}
@@ -891,7 +891,7 @@ static void *gfs2_follow_link(struct den
  * Returns: errno
  */
 
-static int gfs2_permission(struct inode *inode, int mask, struct nameidata *nd)
+int gfs2_do_permission(struct inode *inode, int mask)
 {
 	struct gfs2_inode *ip = GFS2_I(inode);
 	struct gfs2_holder i_gh;
@@ -905,13 +905,21 @@ static int gfs2_permission(struct inode 
 		unlock = 1;
 	}
 
-	error = generic_permission(inode, mask, gfs2_check_acl);
+	if ((mask & MAY_WRITE) && IS_IMMUTABLE(inode))
+		error = -EACCES;
+	else
+		error = generic_permission(inode, mask, gfs2_check_acl);
 	if (unlock)
 		gfs2_glock_dq_uninit(&i_gh);
 
 	return error;
 }
 
+static int gfs2_permission(struct inode *inode, int mask, struct nameidata *nd)
+{
+	return gfs2_do_permission(inode, mask);
+}
+
 static int setattr_size(struct inode *inode, struct iattr *attr)
 {
 	struct gfs2_inode *ip = GFS2_I(inode);

--

[patch 05/14] hpfs: dont call permission()

[Miklos Szeredi]

[-- Attachment #1: hpfs_permission_fix.patch --]
[-- Type: text/plain, Size: 1330 bytes --]

From: Miklos Szeredi <mszeredi@suse.cz>

hpfs_unlink() calls permission() prior to truncating the file.  HPFS
doesn't define a .permission method, so replace with explicit call to
generic_permission().

This is equivalent, except that devcgroup_inode_permission() and
security_inode_permission() are not called.

The truncation is just an implementation detail of the unlink, so
these security checks are unnecessary.

I suspect that even calling generic_permission() is unnecessary, since
we shouldn't mind if the file isn't writable.  But I leave that to the
maintainer to decide.

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
CC: Mikulas Patocka <mikulas@artax.karlin.mff.cuni.cz>
---
 fs/hpfs/namei.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-2.6/fs/hpfs/namei.c
===================================================================
--- linux-2.6.orig/fs/hpfs/namei.c	2008-05-21 13:13:29.000000000 +0200
+++ linux-2.6/fs/hpfs/namei.c	2008-05-21 13:41:25.000000000 +0200
@@ -415,7 +415,7 @@ again:
 		d_drop(dentry);
 		spin_lock(&dentry->d_lock);
 		if (atomic_read(&dentry->d_count) > 1 ||
-		    permission(inode, MAY_WRITE, NULL) ||
+		    generic_permission(inode, MAY_WRITE, NULL) ||
 		    !S_ISREG(inode->i_mode) ||
 		    get_write_access(inode)) {
 			spin_unlock(&dentry->d_lock);

--

[patch 06/14] hfsplus: remove hfsplus_permission()

[Miklos Szeredi]

[-- Attachment #1: hfsplus_permission_cleanup.patch --]
[-- Type: text/plain, Size: 1913 bytes --]

From: Miklos Szeredi <mszeredi@suse.cz>

I'm not sure what this function is trying to achieve, but it's not
succeeding: the condition for which it is returning zero is exactly
the same as checked by permission(), which results in -EACCES.

So in the end this is equivalent to the default action.

A similar check is in hfs_permission() but that does actually do
something, though I'm doubtful if it's the right thing.  But we should
probably leave that alone, lest we break something unexpectedly.

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
CC: Roman Zippel <zippel@linux-m68k.org>
---
 fs/hfsplus/inode.c |   13 -------------
 1 file changed, 13 deletions(-)

Index: linux-2.6/fs/hfsplus/inode.c
===================================================================
--- linux-2.6.orig/fs/hfsplus/inode.c	2008-05-21 13:13:29.000000000 +0200
+++ linux-2.6/fs/hfsplus/inode.c	2008-05-21 13:41:26.000000000 +0200
@@ -238,18 +238,6 @@ static void hfsplus_set_perms(struct ino
 	perms->dev = cpu_to_be32(HFSPLUS_I(inode).dev);
 }
 
-static int hfsplus_permission(struct inode *inode, int mask, struct nameidata *nd)
-{
-	/* MAY_EXEC is also used for lookup, if no x bit is set allow lookup,
-	 * open_exec has the same test, so it's still not executable, if a x bit
-	 * is set fall back to standard permission check.
-	 */
-	if (S_ISREG(inode->i_mode) && mask & MAY_EXEC && !(inode->i_mode & 0111))
-		return 0;
-	return generic_permission(inode, mask, NULL);
-}
-
-
 static int hfsplus_file_open(struct inode *inode, struct file *file)
 {
 	if (HFSPLUS_IS_RSRC(inode))
@@ -283,7 +271,6 @@ static int hfsplus_file_release(struct i
 static const struct inode_operations hfsplus_file_inode_operations = {
 	.lookup		= hfsplus_file_lookup,
 	.truncate	= hfsplus_file_truncate,
-	.permission	= hfsplus_permission,
 	.setxattr	= hfsplus_setxattr,
 	.getxattr	= hfsplus_getxattr,
 	.listxattr	= hfsplus_listxattr,

--

[patch 07/14] vfs: pass dentry to permission()

[Miklos Szeredi]

[-- Attachment #1: dentry_permission.patch --]
[-- Type: text/plain, Size: 17226 bytes --]

From: Miklos Szeredi <mszeredi@suse.cz>

The following patches clean up the i_op->permission() method and the
related VFS API.

Here's an overview of the changes:

 - ->permission() is passed a dentry instead of an inode
 - ->permission() is passed a integer flags parameter instead of a
   nameidata pointer
 - all nameidata pointer passing is removed from the permission API
 - the check for execute bit on regular files is moved into ->permission()
 - the permission() and vfs_permission() functions are consolidated to
   a common path_permission() function
 - file_permission() becomes just a helper to call path_permission()

This patch:

Rename permission() to dentry_permission() and pass a dentry to it
instead of an inode.

For ecryptfs this is done with a temparary hack using d_find_alias(),
which will be removed by the next patch.

Otherwise this can be trivially done by passing the dentry down to
functions calling permission().

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
---
 fs/ecryptfs/inode.c |   14 +++++-
 fs/namei.c          |  105 ++++++++++++++++++++++++++++------------------------
 fs/nfsd/nfsfh.c     |    2 
 fs/nfsd/vfs.c       |    5 +-
 fs/xattr.c          |   12 +++--
 include/linux/fs.h  |    2 
 ipc/mqueue.c        |    2 
 7 files changed, 81 insertions(+), 61 deletions(-)

Index: linux-2.6/fs/ecryptfs/inode.c
===================================================================
--- linux-2.6.orig/fs/ecryptfs/inode.c	2008-05-21 15:45:10.000000000 +0200
+++ linux-2.6/fs/ecryptfs/inode.c	2008-05-21 16:36:42.000000000 +0200
@@ -818,11 +818,19 @@ ecryptfs_permission(struct inode *inode,
 
 		nd->path.mnt = ecryptfs_dentry_to_lower_mnt(nd->path.dentry);
 		nd->path.dentry = ecryptfs_dentry_to_lower(nd->path.dentry);
-		rc = permission(ecryptfs_inode_to_lower(inode), mask, nd);
+		rc = dentry_permission(nd->path.dentry, mask, nd);
 		nd->path.mnt = vfsmnt_save;
 		nd->path.dentry = dentry_save;
-        } else
-		rc = permission(ecryptfs_inode_to_lower(inode), mask, NULL);
+	} else {
+		struct dentry *dentry = d_find_alias(inode);
+		struct dentry *lower_dentry;
+
+		rc = -ENOENT;
+		if (dentry) {
+			lower_dentry = ecryptfs_dentry_to_lower(dentry);
+			rc = dentry_permission(lower_dentry, mask, NULL);
+		}
+	}
         return rc;
 }
 
Index: linux-2.6/fs/namei.c
===================================================================
--- linux-2.6.orig/fs/namei.c	2008-05-21 15:45:10.000000000 +0200
+++ linux-2.6/fs/namei.c	2008-05-21 16:36:07.000000000 +0200
@@ -226,8 +226,9 @@ int generic_permission(struct inode *ino
 	return -EACCES;
 }
 
-int permission(struct inode *inode, int mask, struct nameidata *nd)
+int dentry_permission(struct dentry *dentry, int mask, struct nameidata *nd)
 {
+	struct inode *inode = dentry->d_inode;
 	int retval, submask;
 	struct vfsmount *mnt = NULL;
 
@@ -301,7 +302,7 @@ int permission(struct inode *inode, int 
  */
 int vfs_permission(struct nameidata *nd, int mask)
 {
-	return permission(nd->path.dentry->d_inode, mask, nd);
+	return dentry_permission(nd->path.dentry, mask, nd);
 }
 
 /**
@@ -318,7 +319,7 @@ int vfs_permission(struct nameidata *nd,
  */
 int file_permission(struct file *file, int mask)
 {
-	return permission(file->f_path.dentry->d_inode, mask, NULL);
+	return dentry_permission(file->f_path.dentry, mask, NULL);
 }
 
 /*
@@ -1341,7 +1342,7 @@ static struct dentry *lookup_hash(struct
 {
 	int err;
 
-	err = permission(nd->path.dentry->d_inode, MAY_EXEC, nd);
+	err = dentry_permission(nd->path.dentry, MAY_EXEC, nd);
 	if (err)
 		return ERR_PTR(err);
 	return __lookup_hash(&nd->last, nd->path.dentry, nd);
@@ -1389,7 +1390,7 @@ struct dentry *lookup_one_len(const char
 	if (err)
 		return ERR_PTR(err);
 
-	err = permission(base->d_inode, MAY_EXEC, NULL);
+	err = dentry_permission(base, MAY_EXEC, NULL);
 	if (err)
 		return ERR_PTR(err);
 	return __lookup_hash(&this, base, NULL);
@@ -1469,8 +1470,10 @@ static inline int check_sticky(struct in
  * 10. We don't allow removal of NFS sillyrenamed files; it's handled by
  *     nfs_async_unlink().
  */
-static int may_delete(struct inode *dir,struct dentry *victim,int isdir)
+static int may_delete(struct dentry *dir_dentry, struct dentry *victim,
+		      int isdir)
 {
+	struct inode *dir = dir_dentry->d_inode;
 	int error;
 
 	if (!victim->d_inode)
@@ -1479,7 +1482,7 @@ static int may_delete(struct inode *dir,
 	BUG_ON(victim->d_parent->d_inode != dir);
 	audit_inode_child(victim->d_name.name, victim, dir);
 
-	error = permission(dir,MAY_WRITE | MAY_EXEC, NULL);
+	error = dentry_permission(dir_dentry, MAY_WRITE | MAY_EXEC, NULL);
 	if (error)
 		return error;
 	if (IS_APPEND(dir))
@@ -1509,14 +1512,14 @@ static int may_delete(struct inode *dir,
  *  3. We should have write and exec permissions on dir
  *  4. We can't do it if dir is immutable (done in permission())
  */
-static inline int may_create(struct inode *dir, struct dentry *child,
+static inline int may_create(struct dentry *dir_dentry, struct dentry *child,
 			     struct nameidata *nd)
 {
 	if (child->d_inode)
 		return -EEXIST;
-	if (IS_DEADDIR(dir))
+	if (IS_DEADDIR(dir_dentry->d_inode))
 		return -ENOENT;
-	return permission(dir,MAY_WRITE | MAY_EXEC, nd);
+	return dentry_permission(dir_dentry, MAY_WRITE | MAY_EXEC, nd);
 }
 
 /* 
@@ -1579,10 +1582,11 @@ void unlock_rename(struct dentry *p1, st
 	}
 }
 
-static int vfs_create(struct inode *dir, struct dentry *dentry, int mode,
-		struct nameidata *nd)
+static int vfs_create(struct dentry *dir_dentry, struct dentry *dentry,
+		      int mode,	struct nameidata *nd)
 {
-	int error = may_create(dir, dentry, nd);
+	struct inode *dir = dir_dentry->d_inode;
+	int error = may_create(dir_dentry, dentry, nd);
 
 	if (error)
 		return error;
@@ -1607,7 +1611,7 @@ int path_create(struct path *dir_path, s
 	int error = mnt_want_write(dir_path->mnt);
 
 	if (!error) {
-		error = vfs_create(dir_path->dentry->d_inode, dentry, mode, nd);
+		error = vfs_create(dir_path->dentry, dentry, mode, nd);
 		mnt_drop_write(dir_path->mnt);
 	}
 
@@ -1708,7 +1712,7 @@ static int __open_namei_create(struct na
 
 	if (!IS_POSIXACL(dir->d_inode))
 		mode &= ~current->fs->umask;
-	error = vfs_create(dir->d_inode, path->dentry, mode, nd);
+	error = vfs_create(dir, path->dentry, mode, nd);
 	mutex_unlock(&dir->d_inode->i_mutex);
 	dput(nd->path.dentry);
 	nd->path.dentry = path->dentry;
@@ -2034,9 +2038,11 @@ fail:
 }
 EXPORT_SYMBOL_GPL(lookup_create);
 
-static int vfs_mknod(struct inode *dir, struct dentry *dentry, int mode, dev_t dev)
+static int vfs_mknod(struct dentry *dir_dentry, struct dentry *dentry,
+		     int mode, dev_t dev)
 {
-	int error = may_create(dir, dentry, NULL);
+	struct inode *dir = dir_dentry->d_inode;
+	int error = may_create(dir_dentry, dentry, NULL);
 
 	if (error)
 		return error;
@@ -2068,7 +2074,7 @@ int path_mknod(struct path *dir_path, st
 	int error = mnt_want_write(dir_path->mnt);
 
 	if (!error) {
-		error = vfs_mknod(dir_path->dentry->d_inode, dentry, mode, dev);
+		error = vfs_mknod(dir_path->dentry, dentry, mode, dev);
 		mnt_drop_write(dir_path->mnt);
 	}
 
@@ -2131,9 +2137,10 @@ asmlinkage long sys_mknod(const char __u
 	return sys_mknodat(AT_FDCWD, filename, mode, dev);
 }
 
-static int vfs_mkdir(struct inode *dir, struct dentry *dentry, int mode)
+static int vfs_mkdir(struct dentry *dir_dentry, struct dentry *dentry, int mode)
 {
-	int error = may_create(dir, dentry, NULL);
+	struct inode *dir = dir_dentry->d_inode;
+	int error = may_create(dir_dentry, dentry, NULL);
 
 	if (error)
 		return error;
@@ -2158,7 +2165,7 @@ int path_mkdir(struct path *dir_path, st
 	int error = mnt_want_write(dir_path->mnt);
 
 	if (!error) {
-		error = vfs_mkdir(dir_path->dentry->d_inode, dentry, mode);
+		error = vfs_mkdir(dir_path->dentry, dentry, mode);
 		mnt_drop_write(dir_path->mnt);
 	}
 
@@ -2231,9 +2238,10 @@ void dentry_unhash(struct dentry *dentry
 	spin_unlock(&dcache_lock);
 }
 
-static int vfs_rmdir(struct inode *dir, struct dentry *dentry)
+static int vfs_rmdir(struct dentry *dir_dentry, struct dentry *dentry)
 {
-	int error = may_delete(dir, dentry, 1);
+	struct inode *dir = dir_dentry->d_inode;
+	int error = may_delete(dir_dentry, dentry, 1);
 
 	if (error)
 		return error;
@@ -2269,7 +2277,7 @@ int path_rmdir(struct path *dir_path, st
 	int error = mnt_want_write(dir_path->mnt);
 
 	if (!error) {
-		error = vfs_rmdir(dir_path->dentry->d_inode, dentry);
+		error = vfs_rmdir(dir_path->dentry, dentry);
 		mnt_drop_write(dir_path->mnt);
 	}
 
@@ -2324,9 +2332,10 @@ asmlinkage long sys_rmdir(const char __u
 	return do_rmdir(AT_FDCWD, pathname);
 }
 
-static int vfs_unlink(struct inode *dir, struct dentry *dentry)
+static int vfs_unlink(struct dentry *dir_dentry, struct dentry *dentry)
 {
-	int error = may_delete(dir, dentry, 0);
+	struct inode *dir = dir_dentry->d_inode;
+	int error = may_delete(dir_dentry, dentry, 0);
 
 	if (error)
 		return error;
@@ -2360,7 +2369,7 @@ int path_unlink(struct path *dir_path, s
 	int error = mnt_want_write(dir_path->mnt);
 
 	if (!error) {
-		error = vfs_unlink(dir_path->dentry->d_inode, dentry);
+		error = vfs_unlink(dir_path->dentry, dentry);
 		mnt_drop_write(dir_path->mnt);
 	}
 
@@ -2437,9 +2446,11 @@ asmlinkage long sys_unlink(const char __
 	return do_unlinkat(AT_FDCWD, pathname);
 }
 
-static int vfs_symlink(struct inode *dir, struct dentry *dentry, const char *oldname)
+static int vfs_symlink(struct dentry *dir_dentry, struct dentry *dentry,
+		       const char *oldname)
 {
-	int error = may_create(dir, dentry, NULL);
+	struct inode *dir = dir_dentry->d_inode;
+	int error = may_create(dir_dentry, dentry, NULL);
 
 	if (error)
 		return error;
@@ -2464,9 +2475,7 @@ int path_symlink(struct path *dir_path, 
 	int error = mnt_want_write(dir_path->mnt);
 
 	if (!error) {
-		struct inode *dir = dir_path->dentry->d_inode;
-
-		error = vfs_symlink(dir, dentry, oldname);
+		error = vfs_symlink(dir_path->dentry, dentry, oldname);
 		mnt_drop_write(dir_path->mnt);
 	}
 
@@ -2516,15 +2525,17 @@ asmlinkage long sys_symlink(const char _
 	return sys_symlinkat(oldname, AT_FDCWD, newname);
 }
 
-static int vfs_link(struct dentry *old_dentry, struct inode *dir, struct dentry *new_dentry)
+static int vfs_link(struct dentry *old_dentry, struct dentry *new_dir_dentry,
+		    struct dentry *new_dentry)
 {
+	struct inode *dir = new_dir_dentry->d_inode;
 	struct inode *inode = old_dentry->d_inode;
 	int error;
 
 	if (!inode)
 		return -ENOENT;
 
-	error = may_create(dir, new_dentry, NULL);
+	error = may_create(new_dir_dentry, new_dentry, NULL);
 	if (error)
 		return error;
 
@@ -2560,9 +2571,7 @@ int path_link(struct dentry *old_dentry,
 	int error = mnt_want_write(dir_path->mnt);
 
 	if (!error) {
-		struct inode *dir = dir_path->dentry->d_inode;
-
-		error = vfs_link(old_dentry, dir, new_dentry);
+		error = vfs_link(old_dentry, dir_path->dentry, new_dentry);
 		mnt_drop_write(dir_path->mnt);
 	}
 
@@ -2672,7 +2681,7 @@ static int vfs_rename_dir(struct inode *
 	 * we'll need to flip '..'.
 	 */
 	if (new_dir != old_dir) {
-		error = permission(old_dentry->d_inode, MAY_WRITE, NULL);
+		error = dentry_permission(old_dentry, MAY_WRITE, NULL);
 		if (error)
 			return error;
 	}
@@ -2732,9 +2741,11 @@ static int vfs_rename_other(struct inode
 	return error;
 }
 
-static int vfs_rename(struct inode *old_dir, struct dentry *old_dentry,
-		      struct inode *new_dir, struct dentry *new_dentry)
+static int vfs_rename(struct dentry *old_dir_dentry, struct dentry *old_dentry,
+		      struct dentry *new_dir_dentry, struct dentry *new_dentry)
 {
+	struct inode *old_dir = old_dir_dentry->d_inode;
+	struct inode *new_dir = new_dir_dentry->d_inode;
 	int error;
 	int is_dir = S_ISDIR(old_dentry->d_inode->i_mode);
 	const char *old_name;
@@ -2742,14 +2753,14 @@ static int vfs_rename(struct inode *old_
 	if (old_dentry->d_inode == new_dentry->d_inode)
  		return 0;
  
-	error = may_delete(old_dir, old_dentry, is_dir);
+	error = may_delete(old_dir_dentry, old_dentry, is_dir);
 	if (error)
 		return error;
 
 	if (!new_dentry->d_inode)
-		error = may_create(new_dir, new_dentry, NULL);
+		error = may_create(new_dir_dentry, new_dentry, NULL);
 	else
-		error = may_delete(new_dir, new_dentry, is_dir);
+		error = may_delete(new_dir_dentry, new_dentry, is_dir);
 	if (error)
 		return error;
 
@@ -2785,10 +2796,8 @@ int path_rename(struct path *old_dir_pat
 
 	error = mnt_want_write(mnt);
 	if (!error) {
-		struct inode *old_dir = old_dir_path->dentry->d_inode;
-		struct inode *new_dir = new_dir_path->dentry->d_inode;
-
-		error = vfs_rename(old_dir, old_dentry, new_dir, new_dentry);
+		error = vfs_rename(old_dir_path->dentry, old_dentry,
+				   new_dir_path->dentry, new_dentry);
 		mnt_drop_write(mnt);
 	}
 
@@ -3041,7 +3050,7 @@ EXPORT_SYMBOL(page_symlink);
 EXPORT_SYMBOL(page_symlink_inode_operations);
 EXPORT_SYMBOL(path_lookup);
 EXPORT_SYMBOL(vfs_path_lookup);
-EXPORT_SYMBOL(permission);
+EXPORT_SYMBOL(dentry_permission);
 EXPORT_SYMBOL(vfs_permission);
 EXPORT_SYMBOL(file_permission);
 EXPORT_SYMBOL(unlock_rename);
Index: linux-2.6/fs/nfsd/nfsfh.c
===================================================================
--- linux-2.6.orig/fs/nfsd/nfsfh.c	2008-05-21 15:45:10.000000000 +0200
+++ linux-2.6/fs/nfsd/nfsfh.c	2008-05-21 16:36:07.000000000 +0200
@@ -51,7 +51,7 @@ static int nfsd_acceptable(void *expv, s
 		/* make sure parents give x permission to user */
 		int err;
 		parent = dget_parent(tdentry);
-		err = permission(parent->d_inode, MAY_EXEC, NULL);
+		err = dentry_permission(parent, MAY_EXEC, NULL);
 		if (err < 0) {
 			dput(parent);
 			break;
Index: linux-2.6/fs/nfsd/vfs.c
===================================================================
--- linux-2.6.orig/fs/nfsd/vfs.c	2008-05-21 15:45:45.000000000 +0200
+++ linux-2.6/fs/nfsd/vfs.c	2008-05-21 16:36:07.000000000 +0200
@@ -1942,12 +1942,13 @@ nfsd_permission(struct svc_rqst *rqstp, 
 	    inode->i_uid == current->fsuid)
 		return 0;
 
-	err = permission(inode, acc & (MAY_READ|MAY_WRITE|MAY_EXEC), NULL);
+	err = dentry_permission(dentry, acc & (MAY_READ|MAY_WRITE|MAY_EXEC),
+				NULL);
 
 	/* Allow read access to binaries even when mode 111 */
 	if (err == -EACCES && S_ISREG(inode->i_mode) &&
 	    acc == (MAY_READ | MAY_OWNER_OVERRIDE))
-		err = permission(inode, MAY_EXEC, NULL);
+		err = dentry_permission(dentry, MAY_EXEC, NULL);
 
 	return err? nfserrno(err) : 0;
 }
Index: linux-2.6/include/linux/fs.h
===================================================================
--- linux-2.6.orig/include/linux/fs.h	2008-05-21 15:45:10.000000000 +0200
+++ linux-2.6/include/linux/fs.h	2008-05-21 16:36:07.000000000 +0200
@@ -1758,7 +1758,7 @@ extern sector_t bmap(struct inode *, sec
 #endif
 extern int notify_change(struct dentry *, struct iattr *);
 extern int path_setattr(struct path *, struct iattr *);
-extern int permission(struct inode *, int, struct nameidata *);
+extern int dentry_permission(struct dentry *, int, struct nameidata *);
 extern int generic_permission(struct inode *, int,
 		int (*check_acl)(struct inode *, int));
 
Index: linux-2.6/fs/xattr.c
===================================================================
--- linux-2.6.orig/fs/xattr.c	2008-05-21 15:45:45.000000000 +0200
+++ linux-2.6/fs/xattr.c	2008-05-21 16:36:07.000000000 +0200
@@ -26,8 +26,10 @@
  * because different namespaces have very different rules.
  */
 static int
-xattr_permission(struct inode *inode, const char *name, int mask)
+xattr_permission(struct dentry *dentry, const char *name, int mask)
 {
+	struct inode *inode = dentry->d_inode;
+
 	/*
 	 * We can never set or remove an extended attribute on a read-only
 	 * filesystem  or on an immutable / append-only inode.
@@ -63,7 +65,7 @@ xattr_permission(struct inode *inode, co
 			return -EPERM;
 	}
 
-	return permission(inode, mask, NULL);
+	return dentry_permission(dentry, mask, NULL);
 }
 
 static int
@@ -73,7 +75,7 @@ vfs_setxattr(struct dentry *dentry, cons
 	struct inode *inode = dentry->d_inode;
 	int error;
 
-	error = xattr_permission(inode, name, MAY_WRITE);
+	error = xattr_permission(dentry, name, MAY_WRITE);
 	if (error)
 		return error;
 
@@ -150,7 +152,7 @@ path_getxattr(struct path *path, const c
 	struct inode *inode = dentry->d_inode;
 	int error;
 
-	error = xattr_permission(inode, name, MAY_READ);
+	error = xattr_permission(dentry, name, MAY_READ);
 	if (error)
 		return error;
 
@@ -210,7 +212,7 @@ vfs_removexattr(struct dentry *dentry, c
 	if (!inode->i_op->removexattr)
 		return -EOPNOTSUPP;
 
-	error = xattr_permission(inode, name, MAY_WRITE);
+	error = xattr_permission(dentry, name, MAY_WRITE);
 	if (error)
 		return error;
 
Index: linux-2.6/ipc/mqueue.c
===================================================================
--- linux-2.6.orig/ipc/mqueue.c	2008-05-21 15:45:10.000000000 +0200
+++ linux-2.6/ipc/mqueue.c	2008-05-21 16:36:07.000000000 +0200
@@ -653,7 +653,7 @@ static int oflag2acc[O_ACCMODE] = { MAY_
 		return ERR_PTR(-EINVAL);
 	}
 
-	if (permission(dentry->d_inode, oflag2acc[oflag & O_ACCMODE], NULL)) {
+	if (dentry_permission(dentry, oflag2acc[oflag & O_ACCMODE], NULL)) {
 		dput(dentry);
 		mntput(mqueue_mnt);
 		return ERR_PTR(-EACCES);

--

[patch 08/14] vfs: cleanup i_op->permission()

[Miklos Szeredi]

[-- Attachment #1: iop_permission_pass_dentry.patch --]
[-- Type: text/plain, Size: 29915 bytes --]

From: Miklos Szeredi <mszeredi@suse.cz>

Signature of i_op->permission() is changed from
	int (*permission) (struct inode *inode, int mask, struct nameidata *nd);
to
	int (*permission) (struct dentry *dentry, int mask, int flags);

Lots of filesystems need to be updated, but the only ones actually
making use of the dentry (instead of the inode) or flags are:

 - ecryptfs: uses both dentry and flags
 - fuse: checks LOOKUP_ACCESS and LOOKUP_CHDIR in flags
 - nfs: checks LOOKUP_ACCESS and LOOKUP_OPEN in flags
 - proc_sys_permission: uses the dentry

The new code should be equivalent to the old.

The only exception is that proc_sys_permission() will always have the
dentry available, based on which it can look up the sysctl table
entry, and use that to check permissions.  This should in theory make
the permission checking more consistent, although hopefully it won't
have any visible effect.

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
---
 Documentation/filesystems/Locking |    2 +-
 Documentation/filesystems/vfs.txt |    2 +-
 fs/afs/internal.h                 |    4 +---
 fs/afs/security.c                 |    3 ++-
 fs/bad_inode.c                    |    3 +--
 fs/cifs/cifsfs.c                  |    3 ++-
 fs/coda/dir.c                     |    5 +++--
 fs/coda/pioctl.c                  |    6 ++----
 fs/ecryptfs/inode.c               |   26 ++++----------------------
 fs/ext2/acl.c                     |    4 ++--
 fs/ext2/acl.h                     |    2 +-
 fs/ext3/acl.c                     |    4 ++--
 fs/ext3/acl.h                     |    2 +-
 fs/ext4/acl.c                     |    4 ++--
 fs/ext4/acl.h                     |    2 +-
 fs/fuse/dir.c                     |    5 +++--
 fs/gfs2/ops_inode.c               |    4 ++--
 fs/hfs/inode.c                    |    5 +++--
 fs/hostfs/hostfs_kern.c           |    3 ++-
 fs/jffs2/acl.c                    |    4 ++--
 fs/jffs2/acl.h                    |    2 +-
 fs/jfs/acl.c                      |    4 ++--
 fs/jfs/jfs_acl.h                  |    2 +-
 fs/namei.c                        |    3 ++-
 fs/nfs/dir.c                      |    8 ++++----
 fs/ocfs2/file.c                   |    3 ++-
 fs/ocfs2/file.h                   |    3 +--
 fs/proc/base.c                    |    4 ++--
 fs/proc/proc_sysctl.c             |    7 +++----
 fs/reiserfs/xattr.c               |    4 +++-
 fs/smbfs/file.c                   |    4 ++--
 fs/xfs/linux-2.6/xfs_iops.c       |    6 +++---
 include/linux/coda_linux.h        |    2 +-
 include/linux/fs.h                |    2 +-
 include/linux/nfs_fs.h            |    2 +-
 include/linux/reiserfs_xattr.h    |    2 +-
 include/linux/shmem_fs.h          |    2 +-
 mm/shmem_acl.c                    |    4 ++--
 38 files changed, 71 insertions(+), 86 deletions(-)

Index: linux-2.6/fs/afs/security.c
===================================================================
--- linux-2.6.orig/fs/afs/security.c	2008-05-21 15:45:10.000000000 +0200
+++ linux-2.6/fs/afs/security.c	2008-05-21 16:38:28.000000000 +0200
@@ -284,8 +284,9 @@ static int afs_check_permit(struct afs_v
  * - AFS ACLs are attached to directories only, and a file is controlled by its
  *   parent directory's ACL
  */
-int afs_permission(struct inode *inode, int mask, struct nameidata *nd)
+int afs_permission(struct dentry *dentry, int mask, int flags)
 {
+	struct inode *inode = dentry->d_inode;
 	struct afs_vnode *vnode = AFS_FS_I(inode);
 	afs_access_t uninitialized_var(access);
 	struct key *key;
Index: linux-2.6/fs/bad_inode.c
===================================================================
--- linux-2.6.orig/fs/bad_inode.c	2008-05-21 15:45:10.000000000 +0200
+++ linux-2.6/fs/bad_inode.c	2008-05-21 16:38:28.000000000 +0200
@@ -243,8 +243,7 @@ static int bad_inode_readlink(struct den
 	return -EIO;
 }
 
-static int bad_inode_permission(struct inode *inode, int mask,
-			struct nameidata *nd)
+static int bad_inode_permission(struct dentry *dentry, int mask, int flags)
 {
 	return -EIO;
 }
Index: linux-2.6/include/linux/fs.h
===================================================================
--- linux-2.6.orig/include/linux/fs.h	2008-05-21 16:36:07.000000000 +0200
+++ linux-2.6/include/linux/fs.h	2008-05-21 16:38:28.000000000 +0200
@@ -1264,7 +1264,7 @@ struct inode_operations {
 	void * (*follow_link) (struct dentry *, struct nameidata *);
 	void (*put_link) (struct dentry *, struct nameidata *, void *);
 	void (*truncate) (struct inode *);
-	int (*permission) (struct inode *, int, struct nameidata *);
+	int (*permission) (struct dentry *, int, int);
 	int (*setattr) (struct dentry *, struct iattr *);
 	int (*getattr) (struct vfsmount *mnt, struct dentry *, struct kstat *);
 	int (*setxattr) (struct dentry *, const char *,const void *,size_t,int);
Index: linux-2.6/fs/cifs/cifsfs.c
===================================================================
--- linux-2.6.orig/fs/cifs/cifsfs.c	2008-05-21 15:45:10.000000000 +0200
+++ linux-2.6/fs/cifs/cifsfs.c	2008-05-21 16:38:28.000000000 +0200
@@ -268,8 +268,9 @@ cifs_statfs(struct dentry *dentry, struc
 	return 0;
 }
 
-static int cifs_permission(struct inode *inode, int mask, struct nameidata *nd)
+static int cifs_permission(struct dentry *dentry, int mask, int flags)
 {
+	struct inode *inode = dentry->d_inode;
 	struct cifs_sb_info *cifs_sb;
 
 	cifs_sb = CIFS_SB(inode->i_sb);
Index: linux-2.6/fs/coda/dir.c
===================================================================
--- linux-2.6.orig/fs/coda/dir.c	2008-05-21 15:45:10.000000000 +0200
+++ linux-2.6/fs/coda/dir.c	2008-05-21 16:38:28.000000000 +0200
@@ -137,9 +137,10 @@ exit:
 }
 
 
-int coda_permission(struct inode *inode, int mask, struct nameidata *nd)
+int coda_permission(struct dentry *dentry, int mask, int flags)
 {
-        int error = 0;
+	struct inode *inode = dentry->d_inode;
+	int error = 0;
  
 	if (!mask)
 		return 0; 
Index: linux-2.6/fs/coda/pioctl.c
===================================================================
--- linux-2.6.orig/fs/coda/pioctl.c	2008-05-21 15:45:10.000000000 +0200
+++ linux-2.6/fs/coda/pioctl.c	2008-05-21 16:38:28.000000000 +0200
@@ -24,8 +24,7 @@
 #include <linux/coda_psdev.h>
 
 /* pioctl ops */
-static int coda_ioctl_permission(struct inode *inode, int mask,
-				 struct nameidata *nd);
+static int coda_ioctl_permission(struct dentry *dentry, int mask, int flags);
 static int coda_pioctl(struct inode * inode, struct file * filp, 
                        unsigned int cmd, unsigned long user_data);
 
@@ -42,8 +41,7 @@ const struct file_operations coda_ioctl_
 };
 
 /* the coda pioctl inode ops */
-static int coda_ioctl_permission(struct inode *inode, int mask,
-				 struct nameidata *nd)
+static int coda_ioctl_permission(struct dentry *dentry, int mask, int flags)
 {
         return 0;
 }
Index: linux-2.6/fs/ecryptfs/inode.c
===================================================================
--- linux-2.6.orig/fs/ecryptfs/inode.c	2008-05-21 16:36:42.000000000 +0200
+++ linux-2.6/fs/ecryptfs/inode.c	2008-05-21 16:38:28.000000000 +0200
@@ -808,30 +808,12 @@ out:
 }
 
 static int
-ecryptfs_permission(struct inode *inode, int mask, struct nameidata *nd)
+ecryptfs_permission(struct dentry *dentry, int mask, int flags)
 {
-	int rc;
+	struct dentry *lower_dentry = ecryptfs_dentry_to_lower(dentry);
+	struct nameidata nd = { .flags = flags };
 
-        if (nd) {
-		struct vfsmount *vfsmnt_save = nd->path.mnt;
-		struct dentry *dentry_save = nd->path.dentry;
-
-		nd->path.mnt = ecryptfs_dentry_to_lower_mnt(nd->path.dentry);
-		nd->path.dentry = ecryptfs_dentry_to_lower(nd->path.dentry);
-		rc = dentry_permission(nd->path.dentry, mask, nd);
-		nd->path.mnt = vfsmnt_save;
-		nd->path.dentry = dentry_save;
-	} else {
-		struct dentry *dentry = d_find_alias(inode);
-		struct dentry *lower_dentry;
-
-		rc = -ENOENT;
-		if (dentry) {
-			lower_dentry = ecryptfs_dentry_to_lower(dentry);
-			rc = dentry_permission(lower_dentry, mask, NULL);
-		}
-	}
-        return rc;
+	return dentry_permission(lower_dentry, mask, &nd);
 }
 
 /**
Index: linux-2.6/fs/ext2/acl.c
===================================================================
--- linux-2.6.orig/fs/ext2/acl.c	2008-05-21 15:45:10.000000000 +0200
+++ linux-2.6/fs/ext2/acl.c	2008-05-21 16:38:28.000000000 +0200
@@ -294,9 +294,9 @@ ext2_check_acl(struct inode *inode, int 
 }
 
 int
-ext2_permission(struct inode *inode, int mask, struct nameidata *nd)
+ext2_permission(struct dentry *dentry, int mask, int flags)
 {
-	return generic_permission(inode, mask, ext2_check_acl);
+	return generic_permission(dentry->d_inode, mask, ext2_check_acl);
 }
 
 /*
Index: linux-2.6/fs/ext3/acl.c
===================================================================
--- linux-2.6.orig/fs/ext3/acl.c	2008-05-21 15:45:10.000000000 +0200
+++ linux-2.6/fs/ext3/acl.c	2008-05-21 16:38:28.000000000 +0200
@@ -299,9 +299,9 @@ ext3_check_acl(struct inode *inode, int 
 }
 
 int
-ext3_permission(struct inode *inode, int mask, struct nameidata *nd)
+ext3_permission(struct dentry *dentry, int mask, int flags)
 {
-	return generic_permission(inode, mask, ext3_check_acl);
+	return generic_permission(dentry->d_inode, mask, ext3_check_acl);
 }
 
 /*
Index: linux-2.6/fs/ext4/acl.c
===================================================================
--- linux-2.6.orig/fs/ext4/acl.c	2008-05-21 15:45:10.000000000 +0200
+++ linux-2.6/fs/ext4/acl.c	2008-05-21 16:38:28.000000000 +0200
@@ -299,9 +299,9 @@ ext4_check_acl(struct inode *inode, int 
 }
 
 int
-ext4_permission(struct inode *inode, int mask, struct nameidata *nd)
+ext4_permission(struct dentry *dentry, int mask, int flags)
 {
-	return generic_permission(inode, mask, ext4_check_acl);
+	return generic_permission(dentry->d_inode, mask, ext4_check_acl);
 }
 
 /*
Index: linux-2.6/fs/fuse/dir.c
===================================================================
--- linux-2.6.orig/fs/fuse/dir.c	2008-05-21 15:45:10.000000000 +0200
+++ linux-2.6/fs/fuse/dir.c	2008-05-21 16:38:28.000000000 +0200
@@ -886,8 +886,9 @@ static int fuse_access(struct inode *ino
  * access request is sent.  Execute permission is still checked
  * locally based on file mode.
  */
-static int fuse_permission(struct inode *inode, int mask, struct nameidata *nd)
+static int fuse_permission(struct dentry *dentry, int mask, int flags)
 {
+	struct inode *inode = dentry->d_inode;
 	struct fuse_conn *fc = get_fuse_conn(inode);
 	bool refreshed = false;
 	int err = 0;
@@ -921,7 +922,7 @@ static int fuse_permission(struct inode 
 		   exist.  So if permissions are revoked this won't be
 		   noticed immediately, only after the attribute
 		   timeout has expired */
-	} else if (nd && (nd->flags & (LOOKUP_ACCESS | LOOKUP_CHDIR))) {
+	} else if (flags & (LOOKUP_ACCESS | LOOKUP_CHDIR)) {
 		err = fuse_access(inode, mask);
 	} else if ((mask & MAY_EXEC) && S_ISREG(inode->i_mode)) {
 		if (!(inode->i_mode & S_IXUGO)) {
Index: linux-2.6/fs/gfs2/ops_inode.c
===================================================================
--- linux-2.6.orig/fs/gfs2/ops_inode.c	2008-05-21 16:01:35.000000000 +0200
+++ linux-2.6/fs/gfs2/ops_inode.c	2008-05-21 16:38:28.000000000 +0200
@@ -915,9 +915,9 @@ int gfs2_do_permission(struct inode *ino
 	return error;
 }
 
-static int gfs2_permission(struct inode *inode, int mask, struct nameidata *nd)
+static int gfs2_permission(struct dentry *dentry, int mask, int flags)
 {
-	return gfs2_do_permission(inode, mask);
+	return gfs2_do_permission(dentry->d_inode, mask);
 }
 
 static int setattr_size(struct inode *inode, struct iattr *attr)
Index: linux-2.6/fs/hfs/inode.c
===================================================================
--- linux-2.6.orig/fs/hfs/inode.c	2008-05-21 15:45:10.000000000 +0200
+++ linux-2.6/fs/hfs/inode.c	2008-05-21 16:38:28.000000000 +0200
@@ -511,9 +511,10 @@ void hfs_clear_inode(struct inode *inode
 	}
 }
 
-static int hfs_permission(struct inode *inode, int mask,
-			  struct nameidata *nd)
+static int hfs_permission(struct dentry *dentry, int mask, int flags)
 {
+	struct inode *inode = dentry->d_inode;
+
 	if (S_ISREG(inode->i_mode) && mask & MAY_EXEC)
 		return 0;
 	return generic_permission(inode, mask, NULL);
Index: linux-2.6/fs/hostfs/hostfs_kern.c
===================================================================
--- linux-2.6.orig/fs/hostfs/hostfs_kern.c	2008-05-21 15:45:10.000000000 +0200
+++ linux-2.6/fs/hostfs/hostfs_kern.c	2008-05-21 16:38:28.000000000 +0200
@@ -822,8 +822,9 @@ int hostfs_rename(struct inode *from_ino
 	return err;
 }
 
-int hostfs_permission(struct inode *ino, int desired, struct nameidata *nd)
+int hostfs_permission(struct dentry *dentry, int desired, int flags)
 {
+	struct inode *ino = dentry->d_inode;
 	char *name;
 	int r = 0, w = 0, x = 0, err;
 
Index: linux-2.6/fs/jffs2/acl.c
===================================================================
--- linux-2.6.orig/fs/jffs2/acl.c	2008-05-21 15:45:10.000000000 +0200
+++ linux-2.6/fs/jffs2/acl.c	2008-05-21 16:38:28.000000000 +0200
@@ -314,9 +314,9 @@ static int jffs2_check_acl(struct inode 
 	return -EAGAIN;
 }
 
-int jffs2_permission(struct inode *inode, int mask, struct nameidata *nd)
+int jffs2_permission(struct dentry *dentry, int mask, int flags)
 {
-	return generic_permission(inode, mask, jffs2_check_acl);
+	return generic_permission(dentry->d_inode, mask, jffs2_check_acl);
 }
 
 int jffs2_init_acl_pre(struct inode *dir_i, struct inode *inode, int *i_mode)
Index: linux-2.6/fs/jfs/acl.c
===================================================================
--- linux-2.6.orig/fs/jfs/acl.c	2008-05-21 15:45:10.000000000 +0200
+++ linux-2.6/fs/jfs/acl.c	2008-05-21 16:38:28.000000000 +0200
@@ -140,9 +140,9 @@ static int jfs_check_acl(struct inode *i
 	return -EAGAIN;
 }
 
-int jfs_permission(struct inode *inode, int mask, struct nameidata *nd)
+int jfs_permission(struct dentry *dentry, int mask, int flags)
 {
-	return generic_permission(inode, mask, jfs_check_acl);
+	return generic_permission(dentry->d_inode, mask, jfs_check_acl);
 }
 
 int jfs_init_acl(tid_t tid, struct inode *inode, struct inode *dir)
Index: linux-2.6/fs/nfs/dir.c
===================================================================
--- linux-2.6.orig/fs/nfs/dir.c	2008-05-21 15:45:10.000000000 +0200
+++ linux-2.6/fs/nfs/dir.c	2008-05-21 16:38:28.000000000 +0200
@@ -1929,8 +1929,9 @@ int nfs_may_open(struct inode *inode, st
 	return nfs_do_access(inode, cred, nfs_open_permission_mask(openflags));
 }
 
-int nfs_permission(struct inode *inode, int mask, struct nameidata *nd)
+int nfs_permission(struct dentry *dentry, int mask, int flags)
 {
+	struct inode *inode = dentry->d_inode;
 	struct rpc_cred *cred;
 	int res = 0;
 
@@ -1939,7 +1940,7 @@ int nfs_permission(struct inode *inode, 
 	if (mask == 0)
 		goto out;
 	/* Is this sys_access() ? */
-	if (nd != NULL && (nd->flags & LOOKUP_ACCESS))
+	if (flags & LOOKUP_ACCESS)
 		goto force_lookup;
 
 	switch (inode->i_mode & S_IFMT) {
@@ -1948,8 +1949,7 @@ int nfs_permission(struct inode *inode, 
 		case S_IFREG:
 			/* NFSv4 has atomic_open... */
 			if (nfs_server_capable(inode, NFS_CAP_ATOMIC_OPEN)
-					&& nd != NULL
-					&& (nd->flags & LOOKUP_OPEN))
+					&& (flags & LOOKUP_OPEN))
 				goto out;
 			break;
 		case S_IFDIR:
Index: linux-2.6/include/linux/nfs_fs.h
===================================================================
--- linux-2.6.orig/include/linux/nfs_fs.h	2008-05-21 15:45:10.000000000 +0200
+++ linux-2.6/include/linux/nfs_fs.h	2008-05-21 16:38:28.000000000 +0200
@@ -322,7 +322,7 @@ extern int nfs_refresh_inode(struct inod
 extern int nfs_post_op_update_inode(struct inode *inode, struct nfs_fattr *fattr);
 extern int nfs_post_op_update_inode_force_wcc(struct inode *inode, struct nfs_fattr *fattr);
 extern int nfs_getattr(struct vfsmount *, struct dentry *, struct kstat *);
-extern int nfs_permission(struct inode *, int, struct nameidata *);
+extern int nfs_permission(struct dentry *, int, int);
 extern int nfs_open(struct inode *, struct file *);
 extern int nfs_release(struct inode *, struct file *);
 extern int nfs_attribute_timeout(struct inode *inode);
Index: linux-2.6/fs/ocfs2/file.c
===================================================================
--- linux-2.6.orig/fs/ocfs2/file.c	2008-05-21 15:45:10.000000000 +0200
+++ linux-2.6/fs/ocfs2/file.c	2008-05-21 16:38:28.000000000 +0200
@@ -1176,8 +1176,9 @@ bail:
 	return err;
 }
 
-int ocfs2_permission(struct inode *inode, int mask, struct nameidata *nd)
+int ocfs2_permission(struct dentry *dentry, int mask, int flags)
 {
+	struct inode *inode = dentry->d_inode;
 	int ret;
 
 	mlog_entry_void();
Index: linux-2.6/fs/ocfs2/file.h
===================================================================
--- linux-2.6.orig/fs/ocfs2/file.h	2008-05-21 15:45:10.000000000 +0200
+++ linux-2.6/fs/ocfs2/file.h	2008-05-21 16:38:28.000000000 +0200
@@ -62,8 +62,7 @@ int ocfs2_lock_allocators(struct inode *
 int ocfs2_setattr(struct dentry *dentry, struct iattr *attr);
 int ocfs2_getattr(struct vfsmount *mnt, struct dentry *dentry,
 		  struct kstat *stat);
-int ocfs2_permission(struct inode *inode, int mask,
-		     struct nameidata *nd);
+int ocfs2_permission(struct dentry *dentry, int mask, int flags);
 
 int ocfs2_should_update_atime(struct inode *inode,
 			      struct vfsmount *vfsmnt);
Index: linux-2.6/fs/proc/base.c
===================================================================
--- linux-2.6.orig/fs/proc/base.c	2008-05-21 15:45:10.000000000 +0200
+++ linux-2.6/fs/proc/base.c	2008-05-21 16:38:28.000000000 +0200
@@ -1814,9 +1814,9 @@ static const struct file_operations proc
  * /proc/pid/fd needs a special permission handler so that a process can still
  * access /proc/self/fd after it has executed a setuid().
  */
-static int proc_fd_permission(struct inode *inode, int mask,
-				struct nameidata *nd)
+static int proc_fd_permission(struct dentry *dentry, int mask, int flags)
 {
+	struct inode *inode = dentry->d_inode;
 	int rv;
 
 	rv = generic_permission(inode, mask, NULL);
Index: linux-2.6/fs/proc/proc_sysctl.c
===================================================================
--- linux-2.6.orig/fs/proc/proc_sysctl.c	2008-05-21 15:45:10.000000000 +0200
+++ linux-2.6/fs/proc/proc_sysctl.c	2008-05-21 16:38:28.000000000 +0200
@@ -343,7 +343,7 @@ out:
 	return ret;
 }
 
-static int proc_sys_permission(struct inode *inode, int mask, struct nameidata *nd)
+static int proc_sys_permission(struct dentry *dentry, int mask, int flags)
 {
 	/*
 	 * sysctl entries that are not writeable,
@@ -351,7 +351,7 @@ static int proc_sys_permission(struct in
 	 */
 	struct ctl_table_header *head;
 	struct ctl_table *table;
-	struct dentry *dentry;
+	struct inode *inode = dentry->d_inode;
 	int mode;
 	int depth;
 	int error;
@@ -376,10 +376,9 @@ static int proc_sys_permission(struct in
 	/* If we can't get a sysctl table entry the permission
 	 * checks on the cached mode will have to be enough.
 	 */
-	if (!nd || !depth)
+	if (!depth)
 		goto out;
 
-	dentry = nd->path.dentry;
 	table = do_proc_sys_lookup(dentry->d_parent, &dentry->d_name, &head);
 
 	/* If the entry does not exist deny permission */
Index: linux-2.6/fs/reiserfs/xattr.c
===================================================================
--- linux-2.6.orig/fs/reiserfs/xattr.c	2008-05-21 15:45:10.000000000 +0200
+++ linux-2.6/fs/reiserfs/xattr.c	2008-05-21 16:38:28.000000000 +0200
@@ -1270,8 +1270,10 @@ static int reiserfs_check_acl(struct ino
 	return error;
 }
 
-int reiserfs_permission(struct inode *inode, int mask, struct nameidata *nd)
+int reiserfs_permission(struct dentry *dentry, int mask, int flags)
 {
+	struct inode *inode = dentry->d_inode;
+
 	/*
 	 * We don't do permission checks on the internal objects.
 	 * Permissions are determined by the "owning" object.
Index: linux-2.6/include/linux/reiserfs_xattr.h
===================================================================
--- linux-2.6.orig/include/linux/reiserfs_xattr.h	2008-05-21 15:45:10.000000000 +0200
+++ linux-2.6/include/linux/reiserfs_xattr.h	2008-05-21 16:38:28.000000000 +0200
@@ -55,7 +55,7 @@ int reiserfs_removexattr(struct dentry *
 int reiserfs_delete_xattrs(struct inode *inode);
 int reiserfs_chown_xattrs(struct inode *inode, struct iattr *attrs);
 int reiserfs_xattr_init(struct super_block *sb, int mount_flags);
-int reiserfs_permission(struct inode *inode, int mask, struct nameidata *nd);
+int reiserfs_permission(struct dentry *dentry, int mask, int flags);
 
 int reiserfs_xattr_del(struct inode *, const char *);
 int reiserfs_xattr_get(const struct inode *, const char *, void *, size_t);
Index: linux-2.6/fs/smbfs/file.c
===================================================================
--- linux-2.6.orig/fs/smbfs/file.c	2008-05-21 15:45:10.000000000 +0200
+++ linux-2.6/fs/smbfs/file.c	2008-05-21 16:38:28.000000000 +0200
@@ -408,9 +408,9 @@ smb_file_release(struct inode *inode, st
  * privileges, so we need our own check for this.
  */
 static int
-smb_file_permission(struct inode *inode, int mask, struct nameidata *nd)
+smb_file_permission(struct dentry *dentry, int mask, int flags)
 {
-	int mode = inode->i_mode;
+	int mode = dentry->d_inode->i_mode;
 	int error = 0;
 
 	VERBOSE("mode=%x, mask=%x\n", mode, mask);
Index: linux-2.6/fs/xfs/linux-2.6/xfs_iops.c
===================================================================
--- linux-2.6.orig/fs/xfs/linux-2.6/xfs_iops.c	2008-05-21 15:45:10.000000000 +0200
+++ linux-2.6/fs/xfs/linux-2.6/xfs_iops.c	2008-05-21 16:38:28.000000000 +0200
@@ -588,11 +588,11 @@ xfs_check_acl(
 
 STATIC int
 xfs_vn_permission(
-	struct inode		*inode,
+	struct dentry		*dentry,
 	int			mask,
-	struct nameidata	*nd)
+	int			flags)
 {
-	return generic_permission(inode, mask, xfs_check_acl);
+	return generic_permission(dentry->d_inode, mask, xfs_check_acl);
 }
 #else
 #define xfs_vn_permission NULL
Index: linux-2.6/include/linux/shmem_fs.h
===================================================================
--- linux-2.6.orig/include/linux/shmem_fs.h	2008-05-21 15:45:10.000000000 +0200
+++ linux-2.6/include/linux/shmem_fs.h	2008-05-21 16:38:28.000000000 +0200
@@ -43,7 +43,7 @@ static inline struct shmem_inode_info *S
 }
 
 #ifdef CONFIG_TMPFS_POSIX_ACL
-int shmem_permission(struct inode *, int, struct nameidata *);
+int shmem_permission(struct dentry *, int, int);
 int shmem_acl_init(struct inode *, struct inode *);
 void shmem_acl_destroy_inode(struct inode *);
 
Index: linux-2.6/mm/shmem_acl.c
===================================================================
--- linux-2.6.orig/mm/shmem_acl.c	2008-05-21 15:45:10.000000000 +0200
+++ linux-2.6/mm/shmem_acl.c	2008-05-21 16:38:28.000000000 +0200
@@ -191,7 +191,7 @@ shmem_check_acl(struct inode *inode, int
  * shmem_permission  -  permission() inode operation
  */
 int
-shmem_permission(struct inode *inode, int mask, struct nameidata *nd)
+shmem_permission(struct dentry *dentry, int mask, int flags)
 {
-	return generic_permission(inode, mask, shmem_check_acl);
+	return generic_permission(dentry->d_inode, mask, shmem_check_acl);
 }
Index: linux-2.6/fs/afs/internal.h
===================================================================
--- linux-2.6.orig/fs/afs/internal.h	2008-05-21 15:45:10.000000000 +0200
+++ linux-2.6/fs/afs/internal.h	2008-05-21 16:38:28.000000000 +0200
@@ -469,8 +469,6 @@ extern bool afs_cm_incoming_call(struct 
 extern const struct inode_operations afs_dir_inode_operations;
 extern const struct file_operations afs_dir_file_operations;
 
-extern int afs_permission(struct inode *, int, struct nameidata *);
-
 /*
  * file.c
  */
@@ -605,7 +603,7 @@ extern void afs_clear_permits(struct afs
 extern void afs_cache_permit(struct afs_vnode *, struct key *, long);
 extern void afs_zap_permits(struct rcu_head *);
 extern struct key *afs_request_key(struct afs_cell *);
-extern int afs_permission(struct inode *, int, struct nameidata *);
+extern int afs_permission(struct dentry *, int, int);
 
 /*
  * server.c
Index: linux-2.6/include/linux/coda_linux.h
===================================================================
--- linux-2.6.orig/include/linux/coda_linux.h	2008-05-21 15:45:10.000000000 +0200
+++ linux-2.6/include/linux/coda_linux.h	2008-05-21 16:38:28.000000000 +0200
@@ -37,7 +37,7 @@ extern const struct file_operations coda
 /* operations shared over more than one file */
 int coda_open(struct inode *i, struct file *f);
 int coda_release(struct inode *i, struct file *f);
-int coda_permission(struct inode *inode, int mask, struct nameidata *nd);
+int coda_permission(struct dentry *dentry, int mask, int flags);
 int coda_revalidate_inode(struct dentry *);
 int coda_getattr(struct vfsmount *, struct dentry *, struct kstat *);
 int coda_setattr(struct dentry *, struct iattr *);
Index: linux-2.6/fs/ext2/acl.h
===================================================================
--- linux-2.6.orig/fs/ext2/acl.h	2008-05-21 15:45:10.000000000 +0200
+++ linux-2.6/fs/ext2/acl.h	2008-05-21 16:38:28.000000000 +0200
@@ -58,7 +58,7 @@ static inline int ext2_acl_count(size_t 
 #define EXT2_ACL_NOT_CACHED ((void *)-1)
 
 /* acl.c */
-extern int ext2_permission (struct inode *, int, struct nameidata *);
+extern int ext2_permission(struct dentry *, int, int);
 extern int ext2_acl_chmod (struct inode *);
 extern int ext2_init_acl (struct inode *, struct inode *);
 
Index: linux-2.6/fs/ext3/acl.h
===================================================================
--- linux-2.6.orig/fs/ext3/acl.h	2008-05-21 15:45:10.000000000 +0200
+++ linux-2.6/fs/ext3/acl.h	2008-05-21 16:38:28.000000000 +0200
@@ -58,7 +58,7 @@ static inline int ext3_acl_count(size_t 
 #define EXT3_ACL_NOT_CACHED ((void *)-1)
 
 /* acl.c */
-extern int ext3_permission (struct inode *, int, struct nameidata *);
+extern int ext3_permission(struct dentry *, int, int);
 extern int ext3_acl_chmod (struct inode *);
 extern int ext3_init_acl (handle_t *, struct inode *, struct inode *);
 
Index: linux-2.6/fs/ext4/acl.h
===================================================================
--- linux-2.6.orig/fs/ext4/acl.h	2008-05-21 15:45:10.000000000 +0200
+++ linux-2.6/fs/ext4/acl.h	2008-05-21 16:38:28.000000000 +0200
@@ -58,7 +58,7 @@ static inline int ext4_acl_count(size_t 
 #define EXT4_ACL_NOT_CACHED ((void *)-1)
 
 /* acl.c */
-extern int ext4_permission (struct inode *, int, struct nameidata *);
+extern int ext4_permission(struct dentry *, int, int);
 extern int ext4_acl_chmod (struct inode *);
 extern int ext4_init_acl (handle_t *, struct inode *, struct inode *);
 
Index: linux-2.6/fs/jffs2/acl.h
===================================================================
--- linux-2.6.orig/fs/jffs2/acl.h	2008-05-21 15:45:10.000000000 +0200
+++ linux-2.6/fs/jffs2/acl.h	2008-05-21 16:38:28.000000000 +0200
@@ -28,7 +28,7 @@ struct jffs2_acl_header {
 
 #define JFFS2_ACL_NOT_CACHED ((void *)-1)
 
-extern int jffs2_permission(struct inode *, int, struct nameidata *);
+extern int jffs2_permission(struct dentry *, int, int);
 extern int jffs2_acl_chmod(struct inode *);
 extern int jffs2_init_acl_pre(struct inode *, struct inode *, int *);
 extern int jffs2_init_acl_post(struct inode *);
Index: linux-2.6/fs/jfs/jfs_acl.h
===================================================================
--- linux-2.6.orig/fs/jfs/jfs_acl.h	2008-05-21 15:45:10.000000000 +0200
+++ linux-2.6/fs/jfs/jfs_acl.h	2008-05-21 16:38:28.000000000 +0200
@@ -20,7 +20,7 @@
 
 #ifdef CONFIG_JFS_POSIX_ACL
 
-int jfs_permission(struct inode *, int, struct nameidata *);
+int jfs_permission(struct dentry *, int, int);
 int jfs_init_acl(tid_t, struct inode *, struct inode *);
 int jfs_setattr(struct dentry *, struct iattr *);
 
Index: linux-2.6/fs/namei.c
===================================================================
--- linux-2.6.orig/fs/namei.c	2008-05-21 16:36:07.000000000 +0200
+++ linux-2.6/fs/namei.c	2008-05-21 16:38:28.000000000 +0200
@@ -264,7 +264,8 @@ int dentry_permission(struct dentry *den
 	/* Ordinary permission routines do not understand MAY_APPEND. */
 	submask = mask & ~MAY_APPEND;
 	if (inode->i_op && inode->i_op->permission) {
-		retval = inode->i_op->permission(inode, submask, nd);
+		retval = inode->i_op->permission(dentry, submask,
+						 nd ? nd->flags : 0);
 		if (!retval) {
 			/*
 			 * Exec permission on a regular file is denied if none
Index: linux-2.6/Documentation/filesystems/Locking
===================================================================
--- linux-2.6.orig/Documentation/filesystems/Locking	2008-05-21 15:45:10.000000000 +0200
+++ linux-2.6/Documentation/filesystems/Locking	2008-05-21 16:38:28.000000000 +0200
@@ -44,7 +44,7 @@ ata *);
 	int (*readlink) (struct dentry *, char __user *,int);
 	int (*follow_link) (struct dentry *, struct nameidata *);
 	void (*truncate) (struct inode *);
-	int (*permission) (struct inode *, int, struct nameidata *);
+	int (*permission) (struct dentry *, int, int);
 	int (*setattr) (struct dentry *, struct iattr *);
 	int (*getattr) (struct vfsmount *, struct dentry *, struct kstat *);
 	int (*setxattr) (struct dentry *, const char *,const void *,size_t,int);
Index: linux-2.6/Documentation/filesystems/vfs.txt
===================================================================
--- linux-2.6.orig/Documentation/filesystems/vfs.txt	2008-05-21 15:45:10.000000000 +0200
+++ linux-2.6/Documentation/filesystems/vfs.txt	2008-05-21 16:38:28.000000000 +0200
@@ -326,7 +326,7 @@ struct inode_operations {
         void * (*follow_link) (struct dentry *, struct nameidata *);
         void (*put_link) (struct dentry *, struct nameidata *, void *);
 	void (*truncate) (struct inode *);
-	int (*permission) (struct inode *, int, struct nameidata *);
+	int (*permission) (struct dentry *, int, int);
 	int (*setattr) (struct dentry *, struct iattr *);
 	int (*getattr) (struct vfsmount *mnt, struct dentry *, struct kstat *);
 	int (*setxattr) (struct dentry *, const char *,const void *,size_t,int);

--

[patch 09/14] security: dont pass nameidata to security_inode_permission()

[Miklos Szeredi]

[-- Attachment #1: security_permission_flags.patch --]
[-- Type: text/plain, Size: 6527 bytes --]

From: Miklos Szeredi <mszeredi@suse.cz>

Only pass nameidata->flags instead of the nameidata to
security_inode_permission(), synchronizing it with i_op->permission().

Currently no security module uses the nameidata parameter.

The other change in ->permission() is that a dentry is passed instead
of an inode.  Leave this till AppArmor integration, since that will
need a struct path instead of an inode.

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
CC: James Morris <jmorris@namei.org>
CC: Stephen Smalley <sds@tycho.nsa.gov>
CC: Eric Paris <eparis@redhat.com>
CC: Casey Schaufler <casey@schaufler-ca.com>
---
 fs/namei.c                 |    4 ++--
 include/linux/security.h   |    8 ++++----
 security/dummy.c           |    2 +-
 security/security.c        |    4 ++--
 security/selinux/hooks.c   |    5 ++---
 security/smack/smack_lsm.c |    5 ++---
 6 files changed, 13 insertions(+), 15 deletions(-)

Index: linux-2.6/fs/namei.c
===================================================================
--- linux-2.6.orig/fs/namei.c	2008-05-21 13:41:30.000000000 +0200
+++ linux-2.6/fs/namei.c	2008-05-21 13:41:33.000000000 +0200
@@ -288,7 +288,7 @@ int dentry_permission(struct dentry *den
 	if (retval)
 		return retval;
 
-	return security_inode_permission(inode, mask, nd);
+	return security_inode_permission(inode, mask, nd ? nd->flags : 0);
 }
 
 /**
@@ -488,7 +488,7 @@ static int exec_permission_lite(struct i
 
 	return -EACCES;
 ok:
-	return security_inode_permission(inode, MAY_EXEC, nd);
+	return security_inode_permission(inode, MAY_EXEC, nd->flags);
 }
 
 /*
Index: linux-2.6/include/linux/security.h
===================================================================
--- linux-2.6.orig/include/linux/security.h	2008-05-21 13:13:29.000000000 +0200
+++ linux-2.6/include/linux/security.h	2008-05-21 13:41:33.000000000 +0200
@@ -407,7 +407,7 @@ static inline void security_free_mnt_opt
  *	called when the actual read/write operations are performed.
  *	@inode contains the inode structure to check.
  *	@mask contains the permission mask.
- *	@nd contains the nameidata (may be NULL).
+ *	@flags contains the lookup flags
  *	Return 0 if permission is granted.
  * @inode_setattr:
  *	Check permission before setting file attributes.  Note that the kernel
@@ -1370,7 +1370,7 @@ struct security_operations {
 			     struct inode *new_dir, struct dentry *new_dentry);
 	int (*inode_readlink) (struct dentry *dentry);
 	int (*inode_follow_link) (struct dentry *dentry, struct nameidata *nd);
-	int (*inode_permission) (struct inode *inode, int mask, struct nameidata *nd);
+	int (*inode_permission) (struct inode *inode, int mask, int flags);
 	int (*inode_setattr)	(struct dentry *dentry, struct iattr *attr);
 	int (*inode_getattr) (struct vfsmount *mnt, struct dentry *dentry);
 	void (*inode_delete) (struct inode *inode);
@@ -1641,7 +1641,7 @@ int security_inode_rename(struct inode *
 			  struct inode *new_dir, struct dentry *new_dentry);
 int security_inode_readlink(struct dentry *dentry);
 int security_inode_follow_link(struct dentry *dentry, struct nameidata *nd);
-int security_inode_permission(struct inode *inode, int mask, struct nameidata *nd);
+int security_inode_permission(struct inode *inode, int mask, int flags);
 int security_inode_setattr(struct dentry *dentry, struct iattr *attr);
 int security_inode_getattr(struct vfsmount *mnt, struct dentry *dentry);
 void security_inode_delete(struct inode *inode);
@@ -2033,7 +2033,7 @@ static inline int security_inode_follow_
 }
 
 static inline int security_inode_permission(struct inode *inode, int mask,
-					     struct nameidata *nd)
+					     int flags)
 {
 	return 0;
 }
Index: linux-2.6/security/dummy.c
===================================================================
--- linux-2.6.orig/security/dummy.c	2008-05-21 13:13:29.000000000 +0200
+++ linux-2.6/security/dummy.c	2008-05-21 13:41:33.000000000 +0200
@@ -345,7 +345,7 @@ static int dummy_inode_follow_link (stru
 	return 0;
 }
 
-static int dummy_inode_permission (struct inode *inode, int mask, struct nameidata *nd)
+static int dummy_inode_permission (struct inode *inode, int mask, int flags)
 {
 	return 0;
 }
Index: linux-2.6/security/security.c
===================================================================
--- linux-2.6.orig/security/security.c	2008-05-21 13:13:29.000000000 +0200
+++ linux-2.6/security/security.c	2008-05-21 13:41:33.000000000 +0200
@@ -463,11 +463,11 @@ int security_inode_follow_link(struct de
 	return security_ops->inode_follow_link(dentry, nd);
 }
 
-int security_inode_permission(struct inode *inode, int mask, struct nameidata *nd)
+int security_inode_permission(struct inode *inode, int mask, int flags)
 {
 	if (unlikely(IS_PRIVATE(inode)))
 		return 0;
-	return security_ops->inode_permission(inode, mask, nd);
+	return security_ops->inode_permission(inode, mask, flags);
 }
 
 int security_inode_setattr(struct dentry *dentry, struct iattr *attr)
Index: linux-2.6/security/selinux/hooks.c
===================================================================
--- linux-2.6.orig/security/selinux/hooks.c	2008-05-21 13:13:29.000000000 +0200
+++ linux-2.6/security/selinux/hooks.c	2008-05-21 13:41:33.000000000 +0200
@@ -2579,12 +2579,11 @@ static int selinux_inode_follow_link(str
 	return dentry_has_perm(current, NULL, dentry, FILE__READ);
 }
 
-static int selinux_inode_permission(struct inode *inode, int mask,
-				    struct nameidata *nd)
+static int selinux_inode_permission(struct inode *inode, int mask, int flags)
 {
 	int rc;
 
-	rc = secondary_ops->inode_permission(inode, mask, nd);
+	rc = secondary_ops->inode_permission(inode, mask, flags);
 	if (rc)
 		return rc;
 
Index: linux-2.6/security/smack/smack_lsm.c
===================================================================
--- linux-2.6.orig/security/smack/smack_lsm.c	2008-05-21 13:13:29.000000000 +0200
+++ linux-2.6/security/smack/smack_lsm.c	2008-05-21 13:41:33.000000000 +0200
@@ -515,14 +515,13 @@ static int smack_inode_rename(struct ino
  * smack_inode_permission - Smack version of permission()
  * @inode: the inode in question
  * @mask: the access requested
- * @nd: unused
+ * @flags: unused
  *
  * This is the important Smack hook.
  *
  * Returns 0 if access is permitted, -EACCES otherwise
  */
-static int smack_inode_permission(struct inode *inode, int mask,
-				  struct nameidata *nd)
+static int smack_inode_permission(struct inode *inode, int mask, int flags)
 {
 	/*
 	 * No permission to check. Existence test. Yup, it's there.

--

[patch 10/14] vfs: pass flags to dentry_permission()

[Miklos Szeredi]

[-- Attachment #1: dentry_permission_flags.patch --]
[-- Type: text/plain, Size: 8665 bytes --]

From: Miklos Szeredi <mszeredi@suse.cz>

Switch last argument of dentry_permission() from nameidata to flags.

This requires the MNT_NOEXEC checking to be moved from
dentry_permission() to vfs_permission().

The following dentry_permission() callers passed a non-NULL nameidata:

vfs_permission()

  This remained almost equivalent, except that ordering of the
  MNT_NOEXEC check and the IS_RDONLY/IS_IMMUTABLE checks has been
  changed.  However the IS_RDONLY check should never trigger, because
  of the per-mount read-only checking.  The IS_IMMUTABLE check returns
  the same error value (-EACCES) as the MNT_NOEXEC check, so this
  change is not visible.

lookup_hash()

  Make it call vfs_permission().

may_create()

  This does not always have the nameidata available, but it operates
  on a directory, so the MNT_NOEXEC check would never have triggered
  anyway.

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
---
 fs/ecryptfs/inode.c |    3 +--
 fs/namei.c          |   49 +++++++++++++++++++++++++------------------------
 fs/nfsd/nfsfh.c     |    2 +-
 fs/nfsd/vfs.c       |    5 ++---
 fs/xattr.c          |    2 +-
 include/linux/fs.h  |    2 +-
 ipc/mqueue.c        |    2 +-
 7 files changed, 32 insertions(+), 33 deletions(-)

Index: linux-2.6/fs/ecryptfs/inode.c
===================================================================
--- linux-2.6.orig/fs/ecryptfs/inode.c	2008-05-21 16:38:28.000000000 +0200
+++ linux-2.6/fs/ecryptfs/inode.c	2008-05-21 17:44:30.000000000 +0200
@@ -811,9 +811,8 @@ static int
 ecryptfs_permission(struct dentry *dentry, int mask, int flags)
 {
 	struct dentry *lower_dentry = ecryptfs_dentry_to_lower(dentry);
-	struct nameidata nd = { .flags = flags };
 
-	return dentry_permission(lower_dentry, mask, &nd);
+	return dentry_permission(lower_dentry, mask, flags);
 }
 
 /**
Index: linux-2.6/fs/namei.c
===================================================================
--- linux-2.6.orig/fs/namei.c	2008-05-21 17:33:07.000000000 +0200
+++ linux-2.6/fs/namei.c	2008-05-21 17:55:13.000000000 +0200
@@ -226,14 +226,10 @@ int generic_permission(struct inode *ino
 	return -EACCES;
 }
 
-int dentry_permission(struct dentry *dentry, int mask, struct nameidata *nd)
+int dentry_permission(struct dentry *dentry, int mask, int flags)
 {
 	struct inode *inode = dentry->d_inode;
 	int retval, submask;
-	struct vfsmount *mnt = NULL;
-
-	if (nd)
-		mnt = nd->path.mnt;
 
 	if (mask & MAY_WRITE) {
 		umode_t mode = inode->i_mode;
@@ -252,20 +248,10 @@ int dentry_permission(struct dentry *den
 			return -EACCES;
 	}
 
-	if ((mask & MAY_EXEC) && S_ISREG(inode->i_mode)) {
-		/*
-		 * MAY_EXEC on regular files is denied if the fs is mounted
-		 * with the "noexec" flag.
-		 */
-		if (mnt && (mnt->mnt_flags & MNT_NOEXEC))
-			return -EACCES;
-	}
-
 	/* Ordinary permission routines do not understand MAY_APPEND. */
 	submask = mask & ~MAY_APPEND;
 	if (inode->i_op && inode->i_op->permission) {
-		retval = inode->i_op->permission(dentry, submask,
-						 nd ? nd->flags : 0);
+		retval = inode->i_op->permission(dentry, submask, flags);
 		if (!retval) {
 			/*
 			 * Exec permission on a regular file is denied if none
@@ -288,7 +274,7 @@ int dentry_permission(struct dentry *den
 	if (retval)
 		return retval;
 
-	return security_inode_permission(inode, mask, nd ? nd->flags : 0);
+	return security_inode_permission(inode, mask, flags);
 }
 
 /**
@@ -303,7 +289,21 @@ int dentry_permission(struct dentry *den
  */
 int vfs_permission(struct nameidata *nd, int mask)
 {
-	return dentry_permission(nd->path.dentry, mask, nd);
+	struct dentry *dentry = nd->path.dentry;
+	struct inode *inode = dentry->d_inode;
+
+	if ((mask & MAY_EXEC) && S_ISREG(inode->i_mode)) {
+		struct vfsmount *mnt = nd->path.mnt;
+
+		/*
+		 * MAY_EXEC on regular files is denied if the fs is mounted
+		 * with the "noexec" flag.
+		 */
+		if (mnt->mnt_flags & MNT_NOEXEC)
+			return -EACCES;
+	}
+
+	return dentry_permission(dentry, mask, nd->flags);
 }
 
 /**
@@ -320,7 +320,7 @@ int vfs_permission(struct nameidata *nd,
  */
 int file_permission(struct file *file, int mask)
 {
-	return dentry_permission(file->f_path.dentry, mask, NULL);
+	return dentry_permission(file->f_path.dentry, mask, 0);
 }
 
 /*
@@ -1343,7 +1343,7 @@ static struct dentry *lookup_hash(struct
 {
 	int err;
 
-	err = dentry_permission(nd->path.dentry, MAY_EXEC, nd);
+	err = vfs_permission(nd, MAY_EXEC);
 	if (err)
 		return ERR_PTR(err);
 	return __lookup_hash(&nd->last, nd->path.dentry, nd);
@@ -1391,7 +1391,7 @@ struct dentry *lookup_one_len(const char
 	if (err)
 		return ERR_PTR(err);
 
-	err = dentry_permission(base, MAY_EXEC, NULL);
+	err = dentry_permission(base, MAY_EXEC, 0);
 	if (err)
 		return ERR_PTR(err);
 	return __lookup_hash(&this, base, NULL);
@@ -1483,7 +1483,7 @@ static int may_delete(struct dentry *dir
 	BUG_ON(victim->d_parent->d_inode != dir);
 	audit_inode_child(victim->d_name.name, victim, dir);
 
-	error = dentry_permission(dir_dentry, MAY_WRITE | MAY_EXEC, NULL);
+	error = dentry_permission(dir_dentry, MAY_WRITE | MAY_EXEC, 0);
 	if (error)
 		return error;
 	if (IS_APPEND(dir))
@@ -1520,7 +1520,8 @@ static inline int may_create(struct dent
 		return -EEXIST;
 	if (IS_DEADDIR(dir_dentry->d_inode))
 		return -ENOENT;
-	return dentry_permission(dir_dentry, MAY_WRITE | MAY_EXEC, nd);
+	return dentry_permission(dir_dentry, MAY_WRITE | MAY_EXEC,
+				 nd ? nd->flags : 0);
 }
 
 /* 
@@ -2682,7 +2683,7 @@ static int vfs_rename_dir(struct inode *
 	 * we'll need to flip '..'.
 	 */
 	if (new_dir != old_dir) {
-		error = dentry_permission(old_dentry, MAY_WRITE, NULL);
+		error = dentry_permission(old_dentry, MAY_WRITE, 0);
 		if (error)
 			return error;
 	}
Index: linux-2.6/fs/nfsd/nfsfh.c
===================================================================
--- linux-2.6.orig/fs/nfsd/nfsfh.c	2008-05-21 16:36:07.000000000 +0200
+++ linux-2.6/fs/nfsd/nfsfh.c	2008-05-21 17:44:30.000000000 +0200
@@ -51,7 +51,7 @@ static int nfsd_acceptable(void *expv, s
 		/* make sure parents give x permission to user */
 		int err;
 		parent = dget_parent(tdentry);
-		err = dentry_permission(parent, MAY_EXEC, NULL);
+		err = dentry_permission(parent, MAY_EXEC, 0);
 		if (err < 0) {
 			dput(parent);
 			break;
Index: linux-2.6/fs/nfsd/vfs.c
===================================================================
--- linux-2.6.orig/fs/nfsd/vfs.c	2008-05-21 16:36:07.000000000 +0200
+++ linux-2.6/fs/nfsd/vfs.c	2008-05-21 17:44:30.000000000 +0200
@@ -1942,13 +1942,12 @@ nfsd_permission(struct svc_rqst *rqstp, 
 	    inode->i_uid == current->fsuid)
 		return 0;
 
-	err = dentry_permission(dentry, acc & (MAY_READ|MAY_WRITE|MAY_EXEC),
-				NULL);
+	err = dentry_permission(dentry, acc & (MAY_READ|MAY_WRITE|MAY_EXEC), 0);
 
 	/* Allow read access to binaries even when mode 111 */
 	if (err == -EACCES && S_ISREG(inode->i_mode) &&
 	    acc == (MAY_READ | MAY_OWNER_OVERRIDE))
-		err = dentry_permission(dentry, MAY_EXEC, NULL);
+		err = dentry_permission(dentry, MAY_EXEC, 0);
 
 	return err? nfserrno(err) : 0;
 }
Index: linux-2.6/fs/xattr.c
===================================================================
--- linux-2.6.orig/fs/xattr.c	2008-05-21 16:36:07.000000000 +0200
+++ linux-2.6/fs/xattr.c	2008-05-21 17:44:30.000000000 +0200
@@ -65,7 +65,7 @@ xattr_permission(struct dentry *dentry, 
 			return -EPERM;
 	}
 
-	return dentry_permission(dentry, mask, NULL);
+	return dentry_permission(dentry, mask, 0);
 }
 
 static int
Index: linux-2.6/include/linux/fs.h
===================================================================
--- linux-2.6.orig/include/linux/fs.h	2008-05-21 16:38:28.000000000 +0200
+++ linux-2.6/include/linux/fs.h	2008-05-21 17:44:30.000000000 +0200
@@ -1758,7 +1758,7 @@ extern sector_t bmap(struct inode *, sec
 #endif
 extern int notify_change(struct dentry *, struct iattr *);
 extern int path_setattr(struct path *, struct iattr *);
-extern int dentry_permission(struct dentry *, int, struct nameidata *);
+extern int dentry_permission(struct dentry *, int, int);
 extern int generic_permission(struct inode *, int,
 		int (*check_acl)(struct inode *, int));
 
Index: linux-2.6/ipc/mqueue.c
===================================================================
--- linux-2.6.orig/ipc/mqueue.c	2008-05-21 16:36:07.000000000 +0200
+++ linux-2.6/ipc/mqueue.c	2008-05-21 17:44:30.000000000 +0200
@@ -653,7 +653,7 @@ static int oflag2acc[O_ACCMODE] = { MAY_
 		return ERR_PTR(-EINVAL);
 	}
 
-	if (dentry_permission(dentry, oflag2acc[oflag & O_ACCMODE], NULL)) {
+	if (dentry_permission(dentry, oflag2acc[oflag & O_ACCMODE], 0)) {
 		dput(dentry);
 		mntput(mqueue_mnt);
 		return ERR_PTR(-EACCES);

--

[patch 11/14] vfs: move executable checking into ->permission()

[Miklos Szeredi]

[-- Attachment #1: permission_exec.patch --]
[-- Type: text/plain, Size: 7389 bytes --]

From: Miklos Szeredi <mszeredi@suse.cz>

For execute permission on a regular files we need to check if file has
any execute bits at all, regardless of capabilites.

This check is normally performed by generic_permission() but was also
added to the case when the filesystem defines its own ->permission()
method.  In the latter case the filesystem should be responsible for
performing this check.

So create a helper function exec_permission() that checks returns
-EACCESS if MAY_EXEC is present, the inode is a regular file and no
execute bits are present in inode->i_mode.

Call this function from filesystems which don't call
generic_permission() from their ->permission() methods and which
aren't already performing this check.  Also remove the check from
dentry_permission().

The new code should be equivalent to the old.

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
---
 fs/cifs/cifsfs.c      |    2 +-
 fs/coda/dir.c         |    4 ++++
 fs/coda/pioctl.c      |    2 +-
 fs/hfs/inode.c        |    2 +-
 fs/namei.c            |   38 +++++++++++++++++++++++---------------
 fs/nfs/dir.c          |    3 +++
 fs/proc/proc_sysctl.c |    3 +++
 fs/smbfs/file.c       |    6 +++---
 include/linux/fs.h    |    1 +
 9 files changed, 40 insertions(+), 21 deletions(-)

Index: linux-2.6/fs/cifs/cifsfs.c
===================================================================
--- linux-2.6.orig/fs/cifs/cifsfs.c	2008-05-21 13:41:30.000000000 +0200
+++ linux-2.6/fs/cifs/cifsfs.c	2008-05-21 13:41:36.000000000 +0200
@@ -276,7 +276,7 @@ static int cifs_permission(struct dentry
 	cifs_sb = CIFS_SB(inode->i_sb);
 
 	if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_NO_PERM)
-		return 0;
+		return exec_permission(inode, mask);
 	else /* file mode might have been restricted at mount time
 		on the client (above and beyond ACL on servers) for
 		servers which do not support setting and viewing mode bits,
Index: linux-2.6/fs/coda/dir.c
===================================================================
--- linux-2.6.orig/fs/coda/dir.c	2008-05-21 13:41:30.000000000 +0200
+++ linux-2.6/fs/coda/dir.c	2008-05-21 13:41:36.000000000 +0200
@@ -145,6 +145,10 @@ int coda_permission(struct dentry *dentr
 	if (!mask)
 		return 0; 
 
+	error = exec_permission(inode, mask);
+	if (error)
+		return error;
+
 	lock_kernel();
 
 	if (coda_cache_check(inode, mask))
Index: linux-2.6/fs/coda/pioctl.c
===================================================================
--- linux-2.6.orig/fs/coda/pioctl.c	2008-05-21 13:41:30.000000000 +0200
+++ linux-2.6/fs/coda/pioctl.c	2008-05-21 13:41:36.000000000 +0200
@@ -43,7 +43,7 @@ const struct file_operations coda_ioctl_
 /* the coda pioctl inode ops */
 static int coda_ioctl_permission(struct dentry *dentry, int mask, int flags)
 {
-        return 0;
+	return exec_permission(dentry->d_inode, mask);
 }
 
 static int coda_pioctl(struct inode * inode, struct file * filp, 
Index: linux-2.6/fs/hfs/inode.c
===================================================================
--- linux-2.6.orig/fs/hfs/inode.c	2008-05-21 13:41:30.000000000 +0200
+++ linux-2.6/fs/hfs/inode.c	2008-05-21 13:41:36.000000000 +0200
@@ -516,7 +516,7 @@ static int hfs_permission(struct dentry 
 	struct inode *inode = dentry->d_inode;
 
 	if (S_ISREG(inode->i_mode) && mask & MAY_EXEC)
-		return 0;
+		return exec_permission(inode, mask);
 	return generic_permission(inode, mask, NULL);
 }
 
Index: linux-2.6/fs/nfs/dir.c
===================================================================
--- linux-2.6.orig/fs/nfs/dir.c	2008-05-21 13:41:30.000000000 +0200
+++ linux-2.6/fs/nfs/dir.c	2008-05-21 13:41:36.000000000 +0200
@@ -1975,6 +1975,9 @@ force_lookup:
 		res = PTR_ERR(cred);
 	unlock_kernel();
 out:
+	if (res == 0)
+		res = exec_permission(inode, mask);
+
 	dfprintk(VFS, "NFS: permission(%s/%ld), mask=0x%x, res=%d\n",
 		inode->i_sb->s_id, inode->i_ino, mask, res);
 	return res;
Index: linux-2.6/fs/proc/proc_sysctl.c
===================================================================
--- linux-2.6.orig/fs/proc/proc_sysctl.c	2008-05-21 13:41:30.000000000 +0200
+++ linux-2.6/fs/proc/proc_sysctl.c	2008-05-21 13:41:36.000000000 +0200
@@ -390,6 +390,9 @@ static int proc_sys_permission(struct de
 	error = sysctl_perm(head->root, table, mask);
 out:
 	sysctl_head_finish(head);
+	if (!error)
+		error = exec_permission(inode, mask);
+
 	return error;
 }
 
Index: linux-2.6/fs/smbfs/file.c
===================================================================
--- linux-2.6.orig/fs/smbfs/file.c	2008-05-21 13:41:30.000000000 +0200
+++ linux-2.6/fs/smbfs/file.c	2008-05-21 13:41:36.000000000 +0200
@@ -411,15 +411,15 @@ static int
 smb_file_permission(struct dentry *dentry, int mask, int flags)
 {
 	int mode = dentry->d_inode->i_mode;
-	int error = 0;
 
 	VERBOSE("mode=%x, mask=%x\n", mode, mask);
 
 	/* Look at user permissions */
 	mode >>= 6;
 	if ((mode & 7 & mask) != mask)
-		error = -EACCES;
-	return error;
+		return -EACCES;
+
+	return exec_permission(dentry->d_inode, mask);
 }
 
 const struct file_operations smb_file_operations =
Index: linux-2.6/fs/namei.c
===================================================================
--- linux-2.6.orig/fs/namei.c	2008-05-21 13:41:34.000000000 +0200
+++ linux-2.6/fs/namei.c	2008-05-21 13:41:36.000000000 +0200
@@ -226,6 +226,26 @@ int generic_permission(struct inode *ino
 	return -EACCES;
 }
 
+/**
+ * exec_permission - check for general execute permission on file
+ * @inode:	inode to check access rights for
+ * @mask:	right to check for
+ *
+ * Exec permission on a regular file is denied if none of the execute
+ * bits are set.
+ *
+ * This needs to be called by filesystems which define a
+ * ->permission() method, and don't call generic_permission().
+ */
+int exec_permission(struct inode *inode, int mask)
+{
+	if ((mask & MAY_EXEC) && S_ISREG(inode->i_mode) &&
+	    !(inode->i_mode & S_IXUGO))
+		return -EACCES;
+
+	return 0;
+}
+
 int dentry_permission(struct dentry *dentry, int mask, int flags)
 {
 	struct inode *inode = dentry->d_inode;
@@ -250,23 +270,11 @@ int dentry_permission(struct dentry *den
 
 	/* Ordinary permission routines do not understand MAY_APPEND. */
 	submask = mask & ~MAY_APPEND;
-	if (inode->i_op && inode->i_op->permission) {
+	if (inode->i_op && inode->i_op->permission)
 		retval = inode->i_op->permission(dentry, submask, flags);
-		if (!retval) {
-			/*
-			 * Exec permission on a regular file is denied if none
-			 * of the execute bits are set.
-			 *
-			 * This check should be done by the ->permission()
-			 * method.
-			 */
-			if ((mask & MAY_EXEC) && S_ISREG(inode->i_mode) &&
-			    !(inode->i_mode & S_IXUGO))
-				return -EACCES;
-		}
-	} else {
+	else
 		retval = generic_permission(inode, submask, NULL);
-	}
+
 	if (retval)
 		return retval;
 
Index: linux-2.6/include/linux/fs.h
===================================================================
--- linux-2.6.orig/include/linux/fs.h	2008-05-21 13:41:34.000000000 +0200
+++ linux-2.6/include/linux/fs.h	2008-05-21 13:41:36.000000000 +0200
@@ -1761,6 +1761,7 @@ extern int path_setattr(struct path *, s
 extern int dentry_permission(struct dentry *, int, int);
 extern int generic_permission(struct inode *, int,
 		int (*check_acl)(struct inode *, int));
+extern int exec_permission(struct inode *, int);
 
 extern int get_write_access(struct inode *);
 extern int deny_write_access(struct file *);

--

[patch 12/14] vfs: create path_permission()

[Miklos Szeredi]

[-- Attachment #1: path_permission.patch --]
[-- Type: text/plain, Size: 7835 bytes --]

From: Miklos Szeredi <mszeredi@suse.cz>

Push nameidata further up the call chain, completely removing it from
the permission API.

Switch calls of vfs_permission() to path_permission().  Instead of
nameidata, pass the path and nameidata->flags to this function.

This is a trivially equivalent transformation.

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
---
 fs/exec.c          |    4 ++--
 fs/inotify_user.c  |    2 +-
 fs/namei.c         |   23 ++++++++++++-----------
 fs/open.c          |    8 ++++----
 fs/utimes.c        |    2 +-
 include/linux/fs.h |    2 +-
 net/unix/af_unix.c |    2 +-
 7 files changed, 22 insertions(+), 21 deletions(-)

Index: linux-2.6/fs/exec.c
===================================================================
--- linux-2.6.orig/fs/exec.c	2008-05-21 18:14:45.000000000 +0200
+++ linux-2.6/fs/exec.c	2008-05-21 18:15:02.000000000 +0200
@@ -116,7 +116,7 @@ asmlinkage long sys_uselib(const char __
 	if (!S_ISREG(nd.path.dentry->d_inode->i_mode))
 		goto exit;
 
-	error = vfs_permission(&nd, MAY_READ | MAY_EXEC);
+	error = path_permission(&nd.path, MAY_READ | MAY_EXEC, nd.flags);
 	if (error)
 		goto exit;
 
@@ -664,7 +664,7 @@ struct file *open_exec(const char *name)
 		struct inode *inode = nd.path.dentry->d_inode;
 		file = ERR_PTR(-EACCES);
 		if (S_ISREG(inode->i_mode)) {
-			int err = vfs_permission(&nd, MAY_EXEC);
+			int err = path_permission(&nd.path, MAY_EXEC, nd.flags);
 			file = ERR_PTR(err);
 			if (!err) {
 				file = nameidata_to_filp(&nd,
Index: linux-2.6/fs/inotify_user.c
===================================================================
--- linux-2.6.orig/fs/inotify_user.c	2008-05-21 18:14:45.000000000 +0200
+++ linux-2.6/fs/inotify_user.c	2008-05-21 18:15:02.000000000 +0200
@@ -365,7 +365,7 @@ static int find_inode(const char __user 
 	if (error)
 		return error;
 	/* you can only watch an inode if you have read permissions on it */
-	error = vfs_permission(nd, MAY_READ);
+	error = path_permission(&nd->path, MAY_READ, nd->flags);
 	if (error)
 		path_put(&nd->path);
 	return error;
Index: linux-2.6/fs/namei.c
===================================================================
--- linux-2.6.orig/fs/namei.c	2008-05-21 18:14:45.000000000 +0200
+++ linux-2.6/fs/namei.c	2008-05-21 18:15:02.000000000 +0200
@@ -286,22 +286,23 @@ int dentry_permission(struct dentry *den
 }
 
 /**
- * vfs_permission  -  check for access rights to a given path
- * @nd:		lookup result that describes the path
+ * path_permission  -  check for access rights to a given path
+ * @path:	lookup result that describes the path
  * @mask:	right to check for (%MAY_READ, %MAY_WRITE, %MAY_EXEC)
+ * @flags:	lookup flags
  *
  * Used to check for read/write/execute permissions on a path.
  * We use "fsuid" for this, letting us set arbitrary permissions
  * for filesystem access without changing the "normal" uids which
  * are used for other things.
  */
-int vfs_permission(struct nameidata *nd, int mask)
+int path_permission(struct path *path, int mask, int flags)
 {
-	struct dentry *dentry = nd->path.dentry;
+	struct dentry *dentry = path->dentry;
 	struct inode *inode = dentry->d_inode;
 
 	if ((mask & MAY_EXEC) && S_ISREG(inode->i_mode)) {
-		struct vfsmount *mnt = nd->path.mnt;
+		struct vfsmount *mnt = path->mnt;
 
 		/*
 		 * MAY_EXEC on regular files is denied if the fs is mounted
@@ -311,7 +312,7 @@ int vfs_permission(struct nameidata *nd,
 			return -EACCES;
 	}
 
-	return dentry_permission(dentry, mask, nd->flags);
+	return dentry_permission(dentry, mask, flags);
 }
 
 /**
@@ -324,7 +325,7 @@ int vfs_permission(struct nameidata *nd,
  *
  * Note:
  *	Do not use this function in new code.  All access checks should
- *	be done using vfs_permission().
+ *	be done using path_permission().
  */
 int file_permission(struct file *file, int mask)
 {
@@ -903,7 +904,7 @@ static int __link_path_walk(const char *
 		nd->flags |= LOOKUP_CONTINUE;
 		err = exec_permission_lite(inode, nd);
 		if (err == -EAGAIN)
-			err = vfs_permission(nd, MAY_EXEC);
+			err = path_permission(&nd->path, MAY_EXEC, nd->flags);
  		if (err)
 			break;
 
@@ -1351,7 +1352,7 @@ static struct dentry *lookup_hash(struct
 {
 	int err;
 
-	err = vfs_permission(nd, MAY_EXEC);
+	err = path_permission(&nd->path, MAY_EXEC, nd->flags);
 	if (err)
 		return ERR_PTR(err);
 	return __lookup_hash(&nd->last, nd->path.dentry, nd);
@@ -1658,7 +1659,7 @@ int may_open(struct nameidata *nd, int a
 		flag &= ~O_TRUNC;
 	}
 
-	error = vfs_permission(nd, acc_mode);
+	error = path_permission(&nd->path, acc_mode, nd->flags);
 	if (error)
 		return error;
 	/*
@@ -3061,7 +3062,7 @@ EXPORT_SYMBOL(page_symlink_inode_operati
 EXPORT_SYMBOL(path_lookup);
 EXPORT_SYMBOL(vfs_path_lookup);
 EXPORT_SYMBOL(dentry_permission);
-EXPORT_SYMBOL(vfs_permission);
+EXPORT_SYMBOL(path_permission);
 EXPORT_SYMBOL(file_permission);
 EXPORT_SYMBOL(unlock_rename);
 EXPORT_SYMBOL(vfs_follow_link);
Index: linux-2.6/fs/open.c
===================================================================
--- linux-2.6.orig/fs/open.c	2008-05-21 18:14:45.000000000 +0200
+++ linux-2.6/fs/open.c	2008-05-21 18:15:02.000000000 +0200
@@ -267,7 +267,7 @@ static long do_sys_truncate(const char _
 	if (error)
 		goto dput_and_out;
 
-	error = vfs_permission(&nd, MAY_WRITE);
+	error = path_permission(&nd.path, MAY_WRITE, nd.flags);
 	if (error)
 		goto mnt_drop_write_and_out;
 
@@ -471,7 +471,7 @@ asmlinkage long sys_faccessat(int dfd, c
 	if (res)
 		goto out;
 
-	res = vfs_permission(&nd, mode);
+	res = path_permission(&nd.path, mode, nd.flags);
 	/* SuS v2 requires we report a read only fs too */
 	if(res || !(mode & S_IWOTH) ||
 	   special_file(nd.path.dentry->d_inode->i_mode))
@@ -514,7 +514,7 @@ asmlinkage long sys_chdir(const char __u
 	if (error)
 		goto out;
 
-	error = vfs_permission(&nd, MAY_EXEC);
+	error = path_permission(&nd.path, MAY_EXEC, nd.flags);
 	if (error)
 		goto dput_and_out;
 
@@ -561,7 +561,7 @@ asmlinkage long sys_chroot(const char __
 	if (error)
 		goto out;
 
-	error = vfs_permission(&nd, MAY_EXEC);
+	error = path_permission(&nd.path, MAY_EXEC, nd.flags);
 	if (error)
 		goto dput_and_out;
 
Index: linux-2.6/fs/utimes.c
===================================================================
--- linux-2.6.orig/fs/utimes.c	2008-05-21 18:14:45.000000000 +0200
+++ linux-2.6/fs/utimes.c	2008-05-21 18:15:02.000000000 +0200
@@ -141,7 +141,7 @@ static int do_utimes_name(int dfd, char 
 			goto out_path_put;
 
 		if (!is_owner_or_cap(inode)) {
-			error = vfs_permission(&nd, MAY_WRITE);
+			error = path_permission(&nd.path, MAY_WRITE, nd.flags);
 			if (error)
 				goto out_path_put;
 		}
Index: linux-2.6/include/linux/fs.h
===================================================================
--- linux-2.6.orig/include/linux/fs.h	2008-05-21 18:14:45.000000000 +0200
+++ linux-2.6/include/linux/fs.h	2008-05-21 18:15:02.000000000 +0200
@@ -1123,7 +1123,7 @@ extern void unlock_super(struct super_bl
 /*
  * VFS helper functions..
  */
-extern int vfs_permission(struct nameidata *, int);
+extern int path_permission(struct path *, int, int);
 extern int path_create(struct path *, struct dentry *, int, struct nameidata *);
 extern int path_mkdir(struct path *, struct dentry *, int);
 extern int path_mknod(struct path *, struct dentry *, int, dev_t);
Index: linux-2.6/net/unix/af_unix.c
===================================================================
--- linux-2.6.orig/net/unix/af_unix.c	2008-05-21 18:14:45.000000000 +0200
+++ linux-2.6/net/unix/af_unix.c	2008-05-21 18:15:02.000000000 +0200
@@ -713,7 +713,7 @@ static struct sock *unix_find_other(stru
 		err = path_lookup(sunname->sun_path, LOOKUP_FOLLOW, &nd);
 		if (err)
 			goto fail;
-		err = vfs_permission(&nd, MAY_WRITE);
+		err = path_permission(&nd.path, MAY_WRITE, nd.flags);
 		if (err)
 			goto put_fail;
 

--

[patch 13/14] vfs: dont use dentry_permission()

[Miklos Szeredi]

[-- Attachment #1: path_permission_dentry.patch --]
[-- Type: text/plain, Size: 9272 bytes --]

From: Miklos Szeredi <mszeredi@suse.cz>

Move most calls of dentry_permission() to path_permission().  The
remaining few uses are all in namei.c, so dentry_permission() can be
made static, further simplifying the API.

This will have the side effect, that MNT_NOEXEC checking on the mount
will be performed for these callers as well.  But this should not
cause any problems.

At this point file_permission() is just a helper for
path_permission(), so remove comment about not being preferred.

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
---
 fs/ecryptfs/inode.c |    7 +++++--
 fs/namei.c          |   12 +++---------
 fs/nfsd/nfsfh.c     |   11 ++++++-----
 fs/nfsd/vfs.c       |    8 ++++++--
 fs/xattr.c          |   22 ++++++++++++----------
 include/linux/fs.h  |    1 -
 ipc/mqueue.c        |   15 +++++++++------
 7 files changed, 41 insertions(+), 35 deletions(-)

Index: linux-2.6/fs/ecryptfs/inode.c
===================================================================
--- linux-2.6.orig/fs/ecryptfs/inode.c	2008-05-21 18:16:31.000000000 +0200
+++ linux-2.6/fs/ecryptfs/inode.c	2008-05-21 18:17:08.000000000 +0200
@@ -810,9 +810,12 @@ out:
 static int
 ecryptfs_permission(struct dentry *dentry, int mask, int flags)
 {
-	struct dentry *lower_dentry = ecryptfs_dentry_to_lower(dentry);
+	struct path lower_path;
+
+	lower_path.mnt = ecryptfs_dentry_to_lower_mnt(dentry);
+	lower_path.dentry = ecryptfs_dentry_to_lower(dentry);
 
-	return dentry_permission(lower_dentry, mask, flags);
+	return path_permission(&lower_path, mask, flags);
 }
 
 /**
Index: linux-2.6/fs/namei.c
===================================================================
--- linux-2.6.orig/fs/namei.c	2008-05-21 18:16:31.000000000 +0200
+++ linux-2.6/fs/namei.c	2008-05-21 18:17:08.000000000 +0200
@@ -246,7 +246,7 @@ int exec_permission(struct inode *inode,
 	return 0;
 }
 
-int dentry_permission(struct dentry *dentry, int mask, int flags)
+static int dentry_permission(struct dentry *dentry, int mask, int flags)
 {
 	struct inode *inode = dentry->d_inode;
 	int retval, submask;
@@ -320,16 +320,11 @@ int path_permission(struct path *path, i
  * @file:	file to check access rights for
  * @mask:	right to check for (%MAY_READ, %MAY_WRITE, %MAY_EXEC)
  *
- * Used to check for read/write/execute permissions on an already opened
- * file.
- *
- * Note:
- *	Do not use this function in new code.  All access checks should
- *	be done using path_permission().
+ * This is a helper for path_permission().
  */
 int file_permission(struct file *file, int mask)
 {
-	return dentry_permission(file->f_path.dentry, mask, 0);
+	return path_permission(&file->f_path, mask, 0);
 }
 
 /*
@@ -3061,7 +3056,6 @@ EXPORT_SYMBOL(page_symlink);
 EXPORT_SYMBOL(page_symlink_inode_operations);
 EXPORT_SYMBOL(path_lookup);
 EXPORT_SYMBOL(vfs_path_lookup);
-EXPORT_SYMBOL(dentry_permission);
 EXPORT_SYMBOL(path_permission);
 EXPORT_SYMBOL(file_permission);
 EXPORT_SYMBOL(unlock_rename);
Index: linux-2.6/fs/nfsd/nfsfh.c
===================================================================
--- linux-2.6.orig/fs/nfsd/nfsfh.c	2008-05-21 18:16:31.000000000 +0200
+++ linux-2.6/fs/nfsd/nfsfh.c	2008-05-21 18:17:08.000000000 +0200
@@ -41,7 +41,7 @@ static int nfsd_acceptable(void *expv, s
 	struct svc_export *exp = expv;
 	int rv;
 	struct dentry *tdentry;
-	struct dentry *parent;
+	struct path parent = { .mnt = exp->ex_path.mnt };
 
 	if (exp->ex_flags & NFSEXP_NOSUBTREECHECK)
 		return 1;
@@ -50,14 +50,15 @@ static int nfsd_acceptable(void *expv, s
 	while (tdentry != exp->ex_path.dentry && !IS_ROOT(tdentry)) {
 		/* make sure parents give x permission to user */
 		int err;
-		parent = dget_parent(tdentry);
-		err = dentry_permission(parent, MAY_EXEC, 0);
+
+		parent.dentry = dget_parent(tdentry);
+		err = path_permission(&parent, MAY_EXEC, 0);
 		if (err < 0) {
-			dput(parent);
+			dput(parent.dentry);
 			break;
 		}
 		dput(tdentry);
-		tdentry = parent;
+		tdentry = parent.dentry;
 	}
 	if (tdentry != exp->ex_path.dentry)
 		dprintk("nfsd_acceptable failed at %p %s\n", tdentry, tdentry->d_name.name);
Index: linux-2.6/fs/nfsd/vfs.c
===================================================================
--- linux-2.6.orig/fs/nfsd/vfs.c	2008-05-21 18:16:31.000000000 +0200
+++ linux-2.6/fs/nfsd/vfs.c	2008-05-21 18:17:08.000000000 +0200
@@ -1878,6 +1878,10 @@ nfsd_permission(struct svc_rqst *rqstp, 
 {
 	struct inode	*inode = dentry->d_inode;
 	int		err;
+	struct path	path = {
+		.mnt = exp->ex_path.mnt,
+		.dentry = dentry,
+	};
 
 	if (acc == MAY_NOP)
 		return 0;
@@ -1942,12 +1946,12 @@ nfsd_permission(struct svc_rqst *rqstp, 
 	    inode->i_uid == current->fsuid)
 		return 0;
 
-	err = dentry_permission(dentry, acc & (MAY_READ|MAY_WRITE|MAY_EXEC), 0);
+	err = path_permission(&path, acc & (MAY_READ|MAY_WRITE|MAY_EXEC), 0);
 
 	/* Allow read access to binaries even when mode 111 */
 	if (err == -EACCES && S_ISREG(inode->i_mode) &&
 	    acc == (MAY_READ | MAY_OWNER_OVERRIDE))
-		err = dentry_permission(dentry, MAY_EXEC, 0);
+		err = path_permission(&path, MAY_EXEC, 0);
 
 	return err? nfserrno(err) : 0;
 }
Index: linux-2.6/ipc/mqueue.c
===================================================================
--- linux-2.6.orig/ipc/mqueue.c	2008-05-21 18:16:31.000000000 +0200
+++ linux-2.6/ipc/mqueue.c	2008-05-21 18:17:08.000000000 +0200
@@ -647,19 +647,22 @@ static struct file *do_open(struct dentr
 static int oflag2acc[O_ACCMODE] = { MAY_READ, MAY_WRITE,
 					MAY_READ | MAY_WRITE };
 
+	struct path path = {
+		.mnt = mqueue_mnt,
+		.dentry = dentry,
+	};
+
 	if ((oflag & O_ACCMODE) == (O_RDWR | O_WRONLY)) {
-		dput(dentry);
-		mntput(mqueue_mnt);
+		path_put(&path);
 		return ERR_PTR(-EINVAL);
 	}
 
-	if (dentry_permission(dentry, oflag2acc[oflag & O_ACCMODE], 0)) {
-		dput(dentry);
-		mntput(mqueue_mnt);
+	if (path_permission(&path, oflag2acc[oflag & O_ACCMODE], 0)) {
+		path_put(&path);
 		return ERR_PTR(-EACCES);
 	}
 
-	return dentry_open(dentry, mqueue_mnt, oflag);
+	return dentry_open(path.dentry, path.mnt, oflag);
 }
 
 asmlinkage long sys_mq_open(const char __user *u_name, int oflag, mode_t mode,
Index: linux-2.6/fs/xattr.c
===================================================================
--- linux-2.6.orig/fs/xattr.c	2008-05-21 18:16:31.000000000 +0200
+++ linux-2.6/fs/xattr.c	2008-05-21 18:17:08.000000000 +0200
@@ -26,9 +26,9 @@
  * because different namespaces have very different rules.
  */
 static int
-xattr_permission(struct dentry *dentry, const char *name, int mask)
+xattr_permission(struct path *path, const char *name, int mask)
 {
-	struct inode *inode = dentry->d_inode;
+	struct inode *inode = path->dentry->d_inode;
 
 	/*
 	 * We can never set or remove an extended attribute on a read-only
@@ -65,17 +65,18 @@ xattr_permission(struct dentry *dentry, 
 			return -EPERM;
 	}
 
-	return dentry_permission(dentry, mask, 0);
+	return path_permission(path, mask, 0);
 }
 
 static int
-vfs_setxattr(struct dentry *dentry, const char *name, const void *value,
+vfs_setxattr(struct path *path, const char *name, const void *value,
 		size_t size, int flags)
 {
+	struct dentry *dentry = path->dentry;
 	struct inode *inode = dentry->d_inode;
 	int error;
 
-	error = xattr_permission(dentry, name, MAY_WRITE);
+	error = xattr_permission(path, name, MAY_WRITE);
 	if (error)
 		return error;
 
@@ -110,7 +111,7 @@ int path_setxattr(struct path *path, con
 	int error = mnt_want_write(path->mnt);
 
 	if (!error) {
-		error = vfs_setxattr(path->dentry, name, value, size, flags);
+		error = vfs_setxattr(path, name, value, size, flags);
 		mnt_drop_write(path->mnt);
 	}
 
@@ -152,7 +153,7 @@ path_getxattr(struct path *path, const c
 	struct inode *inode = dentry->d_inode;
 	int error;
 
-	error = xattr_permission(dentry, name, MAY_READ);
+	error = xattr_permission(path, name, MAY_READ);
 	if (error)
 		return error;
 
@@ -204,15 +205,16 @@ path_listxattr(struct path *path, char *
 EXPORT_SYMBOL_GPL(path_listxattr);
 
 static int
-vfs_removexattr(struct dentry *dentry, const char *name)
+vfs_removexattr(struct path *path, const char *name)
 {
+	struct dentry *dentry = path->dentry;
 	struct inode *inode = dentry->d_inode;
 	int error;
 
 	if (!inode->i_op->removexattr)
 		return -EOPNOTSUPP;
 
-	error = xattr_permission(dentry, name, MAY_WRITE);
+	error = xattr_permission(path, name, MAY_WRITE);
 	if (error)
 		return error;
 
@@ -234,7 +236,7 @@ int path_removexattr(struct path *path, 
 	int error = mnt_want_write(path->mnt);
 
 	if (!error) {
-		error = vfs_removexattr(path->dentry, name);
+		error = vfs_removexattr(path, name);
 		mnt_drop_write(path->mnt);
 	}
 
Index: linux-2.6/include/linux/fs.h
===================================================================
--- linux-2.6.orig/include/linux/fs.h	2008-05-21 18:15:02.000000000 +0200
+++ linux-2.6/include/linux/fs.h	2008-05-21 18:17:08.000000000 +0200
@@ -1758,7 +1758,6 @@ extern sector_t bmap(struct inode *, sec
 #endif
 extern int notify_change(struct dentry *, struct iattr *);
 extern int path_setattr(struct path *, struct iattr *);
-extern int dentry_permission(struct dentry *, int, int);
 extern int generic_permission(struct inode *, int,
 		int (*check_acl)(struct inode *, int));
 extern int exec_permission(struct inode *, int);

--

[patch 14/14] vfs: path_permission() clean up flags

[Miklos Szeredi]

[-- Attachment #1: permission_flags_fixes.patch --]
[-- Type: text/plain, Size: 9055 bytes --]

From: Miklos Szeredi <mszeredi@suse.cz>

Currently callers of path_permission() either pass zero or
nameidata->flags as the flags argument.  Passing lookup flags to
filesystems is completely unecessary, only the "intent" flags are
interesting.

More specifically nfs uses LOOKUP_ACCESS and LOOKUP_OPEN flags, while
fuse uses LOOKUP_ACCESS and LOOKUP_CHDIR flags.

So clean up path_permission() calls to just pass these flags.

In case of LOOKUP_CHDIR and LOOKUP_ACCESS the lookup routines need not
be passed these flags at all, they are only needed for the permission
checks.

Also remove the nameidata argument of may_create().  NFS doesn't need
LOOKUP_OPEN in this case, since it handles the creation checks on the
parent directory specially anyway.

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
---
 fs/exec.c          |    4 ++--
 fs/inotify_user.c  |    2 +-
 fs/namei.c         |   25 +++++++++++--------------
 fs/open.c          |   15 +++++++--------
 fs/utimes.c        |    2 +-
 net/unix/af_unix.c |    2 +-
 6 files changed, 23 insertions(+), 27 deletions(-)

Index: linux-2.6/fs/exec.c
===================================================================
--- linux-2.6.orig/fs/exec.c	2008-05-21 18:15:02.000000000 +0200
+++ linux-2.6/fs/exec.c	2008-05-21 18:30:34.000000000 +0200
@@ -116,7 +116,7 @@ asmlinkage long sys_uselib(const char __
 	if (!S_ISREG(nd.path.dentry->d_inode->i_mode))
 		goto exit;
 
-	error = path_permission(&nd.path, MAY_READ | MAY_EXEC, nd.flags);
+	error = path_permission(&nd.path, MAY_READ | MAY_EXEC, 0);
 	if (error)
 		goto exit;
 
@@ -664,7 +664,7 @@ struct file *open_exec(const char *name)
 		struct inode *inode = nd.path.dentry->d_inode;
 		file = ERR_PTR(-EACCES);
 		if (S_ISREG(inode->i_mode)) {
-			int err = path_permission(&nd.path, MAY_EXEC, nd.flags);
+			int err = path_permission(&nd.path, MAY_EXEC, 0);
 			file = ERR_PTR(err);
 			if (!err) {
 				file = nameidata_to_filp(&nd,
Index: linux-2.6/fs/inotify_user.c
===================================================================
--- linux-2.6.orig/fs/inotify_user.c	2008-05-21 18:15:02.000000000 +0200
+++ linux-2.6/fs/inotify_user.c	2008-05-21 18:30:34.000000000 +0200
@@ -365,7 +365,7 @@ static int find_inode(const char __user 
 	if (error)
 		return error;
 	/* you can only watch an inode if you have read permissions on it */
-	error = path_permission(&nd->path, MAY_READ, nd->flags);
+	error = path_permission(&nd->path, MAY_READ, 0);
 	if (error)
 		path_put(&nd->path);
 	return error;
Index: linux-2.6/fs/namei.c
===================================================================
--- linux-2.6.orig/fs/namei.c	2008-05-21 18:17:08.000000000 +0200
+++ linux-2.6/fs/namei.c	2008-05-21 18:36:27.000000000 +0200
@@ -899,7 +899,7 @@ static int __link_path_walk(const char *
 		nd->flags |= LOOKUP_CONTINUE;
 		err = exec_permission_lite(inode, nd);
 		if (err == -EAGAIN)
-			err = path_permission(&nd->path, MAY_EXEC, nd->flags);
+			err = path_permission(&nd->path, MAY_EXEC, 0);
  		if (err)
 			break;
 
@@ -1180,7 +1180,6 @@ static int do_path_lookup(int dfd, const
 
 		nd->path = file->f_path;
 		path_get(&file->f_path);
-
 		fput_light(file, fput_needed);
 	}
 
@@ -1347,7 +1346,7 @@ static struct dentry *lookup_hash(struct
 {
 	int err;
 
-	err = path_permission(&nd->path, MAY_EXEC, nd->flags);
+	err = path_permission(&nd->path, MAY_EXEC, 0);
 	if (err)
 		return ERR_PTR(err);
 	return __lookup_hash(&nd->last, nd->path.dentry, nd);
@@ -1517,15 +1516,13 @@ static int may_delete(struct dentry *dir
  *  3. We should have write and exec permissions on dir
  *  4. We can't do it if dir is immutable (done in permission())
  */
-static inline int may_create(struct dentry *dir_dentry, struct dentry *child,
-			     struct nameidata *nd)
+static inline int may_create(struct dentry *dir_dentry, struct dentry *child)
 {
 	if (child->d_inode)
 		return -EEXIST;
 	if (IS_DEADDIR(dir_dentry->d_inode))
 		return -ENOENT;
-	return dentry_permission(dir_dentry, MAY_WRITE | MAY_EXEC,
-				 nd ? nd->flags : 0);
+	return dentry_permission(dir_dentry, MAY_WRITE | MAY_EXEC, 0);
 }
 
 /* 
@@ -1592,7 +1589,7 @@ static int vfs_create(struct dentry *dir
 		      int mode,	struct nameidata *nd)
 {
 	struct inode *dir = dir_dentry->d_inode;
-	int error = may_create(dir_dentry, dentry, nd);
+	int error = may_create(dir_dentry, dentry);
 
 	if (error)
 		return error;
@@ -1654,7 +1651,7 @@ int may_open(struct nameidata *nd, int a
 		flag &= ~O_TRUNC;
 	}
 
-	error = path_permission(&nd->path, acc_mode, nd->flags);
+	error = path_permission(&nd->path, acc_mode, nd->flags & LOOKUP_OPEN);
 	if (error)
 		return error;
 	/*
@@ -2048,7 +2045,7 @@ static int vfs_mknod(struct dentry *dir_
 		     int mode, dev_t dev)
 {
 	struct inode *dir = dir_dentry->d_inode;
-	int error = may_create(dir_dentry, dentry, NULL);
+	int error = may_create(dir_dentry, dentry);
 
 	if (error)
 		return error;
@@ -2146,7 +2143,7 @@ asmlinkage long sys_mknod(const char __u
 static int vfs_mkdir(struct dentry *dir_dentry, struct dentry *dentry, int mode)
 {
 	struct inode *dir = dir_dentry->d_inode;
-	int error = may_create(dir_dentry, dentry, NULL);
+	int error = may_create(dir_dentry, dentry);
 
 	if (error)
 		return error;
@@ -2456,7 +2453,7 @@ static int vfs_symlink(struct dentry *di
 		       const char *oldname)
 {
 	struct inode *dir = dir_dentry->d_inode;
-	int error = may_create(dir_dentry, dentry, NULL);
+	int error = may_create(dir_dentry, dentry);
 
 	if (error)
 		return error;
@@ -2541,7 +2538,7 @@ static int vfs_link(struct dentry *old_d
 	if (!inode)
 		return -ENOENT;
 
-	error = may_create(new_dir_dentry, new_dentry, NULL);
+	error = may_create(new_dir_dentry, new_dentry);
 	if (error)
 		return error;
 
@@ -2764,7 +2761,7 @@ static int vfs_rename(struct dentry *old
 		return error;
 
 	if (!new_dentry->d_inode)
-		error = may_create(new_dir_dentry, new_dentry, NULL);
+		error = may_create(new_dir_dentry, new_dentry);
 	else
 		error = may_delete(new_dir_dentry, new_dentry, is_dir);
 	if (error)
Index: linux-2.6/fs/open.c
===================================================================
--- linux-2.6.orig/fs/open.c	2008-05-21 18:15:02.000000000 +0200
+++ linux-2.6/fs/open.c	2008-05-21 18:30:34.000000000 +0200
@@ -267,7 +267,7 @@ static long do_sys_truncate(const char _
 	if (error)
 		goto dput_and_out;
 
-	error = path_permission(&nd.path, MAY_WRITE, nd.flags);
+	error = path_permission(&nd.path, MAY_WRITE, 0);
 	if (error)
 		goto mnt_drop_write_and_out;
 
@@ -467,11 +467,11 @@ asmlinkage long sys_faccessat(int dfd, c
 	else
 		current->cap_effective = current->cap_permitted;
 
-	res = __user_walk_fd(dfd, filename, LOOKUP_FOLLOW|LOOKUP_ACCESS, &nd);
+	res = __user_walk_fd(dfd, filename, LOOKUP_FOLLOW, &nd);
 	if (res)
 		goto out;
 
-	res = path_permission(&nd.path, mode, nd.flags);
+	res = path_permission(&nd.path, mode, LOOKUP_ACCESS);
 	/* SuS v2 requires we report a read only fs too */
 	if(res || !(mode & S_IWOTH) ||
 	   special_file(nd.path.dentry->d_inode->i_mode))
@@ -509,12 +509,11 @@ asmlinkage long sys_chdir(const char __u
 	struct nameidata nd;
 	int error;
 
-	error = __user_walk(filename,
-			    LOOKUP_FOLLOW|LOOKUP_DIRECTORY|LOOKUP_CHDIR, &nd);
+	error = __user_walk(filename, LOOKUP_FOLLOW|LOOKUP_DIRECTORY, &nd);
 	if (error)
 		goto out;
 
-	error = path_permission(&nd.path, MAY_EXEC, nd.flags);
+	error = path_permission(&nd.path, MAY_EXEC, LOOKUP_CHDIR);
 	if (error)
 		goto dput_and_out;
 
@@ -543,7 +542,7 @@ asmlinkage long sys_fchdir(unsigned int 
 	if (!S_ISDIR(inode->i_mode))
 		goto out_putf;
 
-	error = file_permission(file, MAY_EXEC);
+	error = path_permission(&file->f_path, MAY_EXEC, LOOKUP_CHDIR);
 	if (!error)
 		set_fs_pwd(current->fs, &file->f_path);
 out_putf:
@@ -561,7 +560,7 @@ asmlinkage long sys_chroot(const char __
 	if (error)
 		goto out;
 
-	error = path_permission(&nd.path, MAY_EXEC, nd.flags);
+	error = path_permission(&nd.path, MAY_EXEC, 0);
 	if (error)
 		goto dput_and_out;
 
Index: linux-2.6/net/unix/af_unix.c
===================================================================
--- linux-2.6.orig/net/unix/af_unix.c	2008-05-21 18:15:02.000000000 +0200
+++ linux-2.6/net/unix/af_unix.c	2008-05-21 18:30:34.000000000 +0200
@@ -713,7 +713,7 @@ static struct sock *unix_find_other(stru
 		err = path_lookup(sunname->sun_path, LOOKUP_FOLLOW, &nd);
 		if (err)
 			goto fail;
-		err = path_permission(&nd.path, MAY_WRITE, nd.flags);
+		err = path_permission(&nd.path, MAY_WRITE, 0);
 		if (err)
 			goto put_fail;
 
Index: linux-2.6/fs/utimes.c
===================================================================
--- linux-2.6.orig/fs/utimes.c	2008-05-21 18:15:02.000000000 +0200
+++ linux-2.6/fs/utimes.c	2008-05-21 18:30:34.000000000 +0200
@@ -141,7 +141,7 @@ static int do_utimes_name(int dfd, char 
 			goto out_path_put;
 
 		if (!is_owner_or_cap(inode)) {
-			error = path_permission(&nd.path, MAY_WRITE, nd.flags);
+			error = path_permission(&nd.path, MAY_WRITE, 0);
 			if (error)
 				goto out_path_put;
 		}

--

Re: [patch 11/14] vfs: move executable checking into ->permission()

[Trond Myklebust]

On Wed, 2008-05-21 at 19:15 +0200, Miklos Szeredi wrote:

> +/**
> + * exec_permission - check for general execute permission on file
> + * @inode:	inode to check access rights for
> + * @mask:	right to check for
> + *
> + * Exec permission on a regular file is denied if none of the execute
> + * bits are set.
> + *
> + * This needs to be called by filesystems which define a
> + * ->permission() method, and don't call generic_permission().
> + */
> +int exec_permission(struct inode *inode, int mask)
> +{
> +	if ((mask & MAY_EXEC) && S_ISREG(inode->i_mode) &&
> +	    !(inode->i_mode & S_IXUGO))
> +		return -EACCES;
> +
> +	return 0;
> +}

Hmm... What if !(mask & MAY_EXEC)? AFAICS, the above will return 0.
A better approach would be to use something like

	if (!(mask & MAY_EXEC))
		return -EACCES;
	if (S_ISREG(inode->i_mode) && !(inode->i_mode & S_IXUGO))
		return -EACCES;
	return 0;

Re: [patch 11/14] vfs: move executable checking into ->permission()

[Miklos Szeredi]

> > +/**
> > + * exec_permission - check for general execute permission on file
> > + * @inode:	inode to check access rights for
> > + * @mask:	right to check for
> > + *
> > + * Exec permission on a regular file is denied if none of the execute
> > + * bits are set.
> > + *
> > + * This needs to be called by filesystems which define a
> > + * ->permission() method, and don't call generic_permission().
> > + */
> > +int exec_permission(struct inode *inode, int mask)
> > +{
> > +	if ((mask & MAY_EXEC) && S_ISREG(inode->i_mode) &&
> > +	    !(inode->i_mode & S_IXUGO))
> > +		return -EACCES;
> > +
> > +	return 0;
> > +}
> 
> Hmm... What if !(mask & MAY_EXEC)? AFAICS, the above will return 0.
> A better approach would be to use something like
> 
> 	if (!(mask & MAY_EXEC))
> 		return -EACCES;
> 	if (S_ISREG(inode->i_mode) && !(inode->i_mode & S_IXUGO))
> 		return -EACCES;
> 	return 0;

No, we don't want to deny read or write (that's up to the filesystem
how it handles it), just want to deny execute if no x bits are set in
the mode.

I didn't really pay attention to individual filesystems, including
NFS, just mechanically moved the check from permission() to
->permission().  So it's possible that the code could be further
optimized in some cases.

Miklos

Re: [patch 11/14] vfs: move executable checking into ->permission()

[Trond Myklebust]

On Wed, 2008-05-21 at 21:09 +0200, Miklos Szeredi wrote:

> No, we don't want to deny read or write (that's up to the filesystem
> how it handles it), just want to deny execute if no x bits are set in
> the mode.

OK, but when I see something with the name 'exec_permission()', I assume
that it is going to check for whether or not I have execute permission.

If that is not the case, then can we please either change the function,
or change the name?

Cheers
  Trond

Re: [patch 11/14] vfs: move executable checking into ->permission()

[Miklos Szeredi]

> > No, we don't want to deny read or write (that's up to the filesystem
> > how it handles it), just want to deny execute if no x bits are set in
> > the mode.
> 
> OK, but when I see something with the name 'exec_permission()', I assume
> that it is going to check for whether or not I have execute permission.
> 
> If that is not the case, then can we please either change the function,
> or change the name?

Yeah, the name's not very good.  Especially that there's an
exec_permission_lite() in the same file, which has nothing to do with
this.

Will try to think of something better.

Miklos

Re: [patch 07/14] vfs: pass dentry to permission()

[Al Viro]

On Wed, May 21, 2008 at 07:15:05PM +0200, Miklos Szeredi wrote:
> From: Miklos Szeredi <mszeredi@suse.cz>
> 
> The following patches clean up the i_op->permission() method and the
> related VFS API.
> 
> Here's an overview of the changes:
> 
>  - ->permission() is passed a dentry instead of an inode
>  - ->permission() is passed a integer flags parameter instead of a
>    nameidata pointer

No.  Take a good look at the instances.

	a) only one aberrant case cares about dentry, and for extremely
wrong reasons.  /proc/sys/ stuff.  ecryptfs, of course, will be happy
with any variant.
	b) few flags that are looked at are trivially mapped to new MAY_...

I have a patch series that does it, but it involves tons of fixing the
sysctl handling to be finished ;-/  And yes, we need sysctl to quit
doing the "I want to get ctl_table entry, so I'll do very painful search
by dentry every damn time" in any case - look at that code, it's far
too ugly to live.

IOW, consider this ->permission() API change NAKed.

Re: [patch 06/14] hfsplus: remove hfsplus_permission()

[Roman Zippel]

Hi,

On Wed, 21 May 2008, Miklos Szeredi wrote:

> From: Miklos Szeredi <mszeredi@suse.cz>
> 
> I'm not sure what this function is trying to achieve, but it's not
> succeeding: the condition for which it is returning zero is exactly
> the same as checked by permission(), which results in -EACCES.
> 
> So in the end this is equivalent to the default action.

No, it's not, it allows for HFS+ specific special case to allow the lookup 
of the resource fork.

bye, Roman

Re: [patch 09/14] security: dont pass nameidata to security_inode_permission()

[James Morris]

On Wed, 21 May 2008, Miklos Szeredi wrote:

> From: Miklos Szeredi <mszeredi@suse.cz>
> 
> Only pass nameidata->flags instead of the nameidata to
> security_inode_permission(), synchronizing it with i_op->permission().
> 
> Currently no security module uses the nameidata parameter.
> 
> The other change in ->permission() is that a dentry is passed instead
> of an inode.  Leave this till AppArmor integration, since that will
> need a struct path instead of an inode.
> 
> Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
> CC: James Morris <jmorris@namei.org>
> CC: Stephen Smalley <sds@tycho.nsa.gov>
> CC: Eric Paris <eparis@redhat.com>
> CC: Casey Schaufler <casey@schaufler-ca.com>

Acked-by: James Morris <jmorris@namei.org>



-- 
James Morris
<jmorris@namei.org>

Re: [patch 07/14] vfs: pass dentry to permission()

[Miklos Szeredi]

> > From: Miklos Szeredi <mszeredi@suse.cz>
> > 
> > The following patches clean up the i_op->permission() method and the
> > related VFS API.
> > 
> > Here's an overview of the changes:
> > 
> >  - ->permission() is passed a dentry instead of an inode
> >  - ->permission() is passed a integer flags parameter instead of a
> >    nameidata pointer
> 
> No.  Take a good look at the instances.
> 
> 	a) only one aberrant case cares about dentry, and for extremely
> wrong reasons.  /proc/sys/ stuff.  ecryptfs, of course, will be happy
> with any variant.

Yeah.  The real reason I changed to dentry is not /proc/sys or
ecryptfs, but because it's the Right Thing.  And if we have to touch
all instances anyway, then it basically comes for free.

Why dentry?  Because of filesystems which operate on names and not
inodes.  Currently all these manage to get by, because their
->permission() just uses the cached attributes.  But that should not
necessarily be the case.

Look at the inode_operations, each and every one passes a dentry (or
dentries) to the filesystem, except permission().

OTOH there isn't any immediate need for this, it's just an interface
rationalization thing, so...

> 	b) few flags that are looked at are trivially mapped to new MAY_...

OK.

> I have a patch series that does it, but it involves tons of fixing the
> sysctl handling to be finished ;-/  And yes, we need sysctl to quit
> doing the "I want to get ctl_table entry, so I'll do very painful search
> by dentry every damn time" in any case - look at that code, it's far
> too ugly to live.

I shut my eyes and just squinted while touching that code ;)

> 
> IOW, consider this ->permission() API change NAKed.

I'll fix b) and repost.  There's a bit more to this series, than just
the ->permission() API change.

Miklos

Re: [patch 07/14] vfs: pass dentry to permission()

[Miklos Szeredi]

> I have a patch series that does it, but it involves tons of fixing the
> sysctl handling to be finished ;-/

Btw, could you please make this unfinished series available somewhere?
A tarball on hera or whatever.

Thanks,
Miklos

Re: [patch 06/14] hfsplus: remove hfsplus_permission()

[Miklos Szeredi]

> > From: Miklos Szeredi <mszeredi@suse.cz>
> > 
> > I'm not sure what this function is trying to achieve, but it's not
> > succeeding: the condition for which it is returning zero is exactly
> > the same as checked by permission(), which results in -EACCES.
> > 
> > So in the end this is equivalent to the default action.
> 
> No, it's not, it allows for HFS+ specific special case to allow the lookup 
> of the resource fork.

Sorry I just don't see how that code would allow anything.  The only
place hfsplus_permission() is called is from permission() in namei.c,
and in that case it _is_ equivalent.  Look:

hfsplus_permission():

	if (S_ISREG(inode->i_mode) && mask & MAY_EXEC && !(inode->i_mode & 0111))
		return 0;

permission():

		retval = inode->i_op->permission(inode, submask, nd);
		if (!retval) {
			/*
			 * Exec permission on a regular file is denied if none
			 * of the execute bits are set.
			 *
			 * This check should be done by the ->permission()
			 * method.
			 */
			if ((mask & MAY_EXEC) && S_ISREG(inode->i_mode) &&
			    !(inode->i_mode & S_IXUGO))
				return -EACCES;

Miklos

Re: [patch 06/14] hfsplus: remove hfsplus_permission()

[Roman Zippel]

Hi,

On Thu, 22 May 2008, Miklos Szeredi wrote:

> > No, it's not, it allows for HFS+ specific special case to allow the lookup 
> > of the resource fork.
> 
> Sorry I just don't see how that code would allow anything.  The only
> place hfsplus_permission() is called is from permission() in namei.c,
> and in that case it _is_ equivalent.  Look:
> 
> hfsplus_permission():
> 
> 	if (S_ISREG(inode->i_mode) && mask & MAY_EXEC && !(inode->i_mode & 0111))
> 		return 0;
> 
> permission():
> 
> 		retval = inode->i_op->permission(inode, submask, nd);
> 		if (!retval) {
> 			/*
> 			 * Exec permission on a regular file is denied if none
> 			 * of the execute bits are set.
> 			 *
> 			 * This check should be done by the ->permission()
> 			 * method.
> 			 */
> 			if ((mask & MAY_EXEC) && S_ISREG(inode->i_mode) &&
> 			    !(inode->i_mode & S_IXUGO))
> 				return -EACCES;

That check didn't used to be there and that the HFS+ check is older than 
that might have given you the idea that it at least used to work.
So now the only way for a fs to differentiate between lookup and exec is 
gone... :-(

bye, Roman

Re: [patch 06/14] hfsplus: remove hfsplus_permission()

[Miklos Szeredi]

> That check didn't used to be there and that the HFS+ check is older than 
> that might have given you the idea that it at least used to work.
> So now the only way for a fs to differentiate between lookup and exec is 
> gone... :-(

That check was added quite some time ago:

   commit a343bb7750e6a098909c34f5c5dfddbc4fa40053
   Author: Trond Myklebust <Trond.Myklebust@netapp.com>
   Date:   Tue Aug 22 20:06:03 2006 -0400
   
       VFS: Fix access("file", X_OK) in the presence of ACLs

Also it sounds just plain wrong to allow execution without an x bit.
It could cause nasty surprises at least.  What was the intended
purpose of that code, and why did nobody notice when it stopped
working?

Miklos

Re: [patch 06/14] hfsplus: remove hfsplus_permission()

[Roman Zippel]

Hi,

On Thu, 22 May 2008, Miklos Szeredi wrote:

> > That check didn't used to be there and that the HFS+ check is older than 
> > that might have given you the idea that it at least used to work.
> > So now the only way for a fs to differentiate between lookup and exec is 
> > gone... :-(
> 
> That check was added quite some time ago:
> 
>    commit a343bb7750e6a098909c34f5c5dfddbc4fa40053
>    Author: Trond Myklebust <Trond.Myklebust@netapp.com>
>    Date:   Tue Aug 22 20:06:03 2006 -0400
>    
>        VFS: Fix access("file", X_OK) in the presence of ACLs
> 
> Also it sounds just plain wrong to allow execution without an x bit.
> It could cause nasty surprises at least.  What was the intended
> purpose of that code, and why did nobody notice when it stopped
> working?

As I said to allow lookup on files.

bye, Roman

Re: [patch 06/14] hfsplus: remove hfsplus_permission()

[Miklos Szeredi]

> > > That check didn't used to be there and that the HFS+ check is older than 
> > > that might have given you the idea that it at least used to work.
> > > So now the only way for a fs to differentiate between lookup and exec is 
> > > gone... :-(
> > 
> > That check was added quite some time ago:
> > 
> >    commit a343bb7750e6a098909c34f5c5dfddbc4fa40053
> >    Author: Trond Myklebust <Trond.Myklebust@netapp.com>
> >    Date:   Tue Aug 22 20:06:03 2006 -0400
> >    
> >        VFS: Fix access("file", X_OK) in the presence of ACLs
> > 
> > Also it sounds just plain wrong to allow execution without an x bit.
> > It could cause nasty surprises at least.  What was the intended
> > purpose of that code, and why did nobody notice when it stopped
> > working?
> 
> As I said to allow lookup on files.

That requires a quite bit more support from the VFS than just allowing
lookup to work on regular files without x bits.  You'll have big
trouble with hard links for example: the VFS doesn't like non-leaf
dentries to be aliased.

Besides hfsplus_permission() did not differentiate between execve on
the file and lookup on that file, allowing both.  Which is obviously
wrong.

Miklos

Re: [patch 04/14] gfs2: dont call permission()

[Steven Whitehouse]

Hi,

On Wed, 2008-05-21 at 19:15 +0200, Miklos Szeredi wrote:
> plain text document attachment (gfs2_permission_fix.patch)
> From: Miklos Szeredi <mszeredi@suse.cz>
> 
> GFS2 calls permission() to verify permissions after locks on the files
> have been taken.
> 
> For this it's sufficient to call gfs2_permission() instead.  This
> results in the following changes:
> 
>   - IS_RDONLY() check is not performed
>   - IS_IMMUTABLE() check is not performed
>   - devcgroup_inode_permission() is not called
>   - security_inode_permission() is not called
> 
> IS_RDONLY() should be unnecessary anyway, as the per-mount read-only
> flag should provide protection against read-only remounts during
> operations.  do_gfs2_set_flags() has been fixed to perform
> mnt_want_write()/mnt_drop_write() to protect against remounting
> read-only.
> 
> IS_IMMUTABLE has beed added to gfs2_do_permission()
> 
That looks ok, but I wonder do we really need gfs2_do_permission() and
gfs2_permission when the only difference seems to be one argument?

> Repeating the security checks seems to be pointless, as they don't
> normally change, and if they do, it's independent of the filesystem
> state.
> 
I hope eventually we can fix this by allowing GFS2 to do its own
lookups, via a suitable VFS library function. I understand that is the
preferred option to replace "open intents" (which we don't currently use
anyway) in the longer term.

> I also suspect the conditional locking in gfs2_do_permission() could
> be cleaned up, due to the removal of the implicit recursion.
> 
In order to be sure we'd have to check that there are no NFS code paths
left which can reach this code. That has usually been the reason for
conditional locking.

In general the patch looks ok to me, and since it doesn't appear to
depend on anything else, I can drop it in my GFS2 git tree if that would
be helpful at this stage,

Steve.

Re: [patch 03/14] hppfs: remove hppfs_permission

[Christoph Hellwig]

On Wed, May 21, 2008 at 07:15:01PM +0200, Miklos Szeredi wrote:
> From: Miklos Szeredi <mszeredi@suse.cz>
> 
> hppfs_permission() is equivalent to the '.permission == NULL' case.


Looks good.

Re: [patch 04/14] gfs2: dont call permission()

[Christoph Hellwig]

Bad idea.  You're duplicating bits out of permission for no good
reason.  I spent quite a bit of effort to make sure we don't have
this duplicated logic around.

Re: [patch 05/14] hpfs: dont call permission()

[Christoph Hellwig]

On Wed, May 21, 2008 at 07:15:03PM +0200, Miklos Szeredi wrote:
> From: Miklos Szeredi <mszeredi@suse.cz>
> 
> hpfs_unlink() calls permission() prior to truncating the file.  HPFS
> doesn't define a .permission method, so replace with explicit call to
> generic_permission().
> 
> This is equivalent, except that devcgroup_inode_permission() and
> security_inode_permission() are not called.
> 
> The truncation is just an implementation detail of the unlink, so
> these security checks are unnecessary.
> 
> I suspect that even calling generic_permission() is unnecessary, since
> we shouldn't mind if the file isn't writable.  But I leave that to the
> maintainer to decide.

Okay to me but rather pointless.  And I agree to the last stance that
the whole logic in hpfs_unlink really needs sorting out by someone who
understands that filesystem.

Re: [patch 06/14] hfsplus: remove hfsplus_permission()

[Christoph Hellwig]

On Thu, May 22, 2008 at 02:37:21PM +0200, Miklos Szeredi wrote:
> That requires a quite bit more support from the VFS than just allowing
> lookup to work on regular files without x bits.  You'll have big
> trouble with hard links for example: the VFS doesn't like non-leaf
> dentries to be aliased.
> 
> Besides hfsplus_permission() did not differentiate between execve on
> the file and lookup on that file, allowing both.  Which is obviously
> wrong.

Yes.  Roman, if you care about details about the files as directories
look in the reiser4 threads as it did something quite similar.

Re: [patch 11/14] vfs: move executable checking into ->permission()

[Christoph Hellwig]

On Wed, May 21, 2008 at 07:15:09PM +0200, Miklos Szeredi wrote:
> From: Miklos Szeredi <mszeredi@suse.cz>
> 
> For execute permission on a regular files we need to check if file has
> any execute bits at all, regardless of capabilites.
> 
> This check is normally performed by generic_permission() but was also
> added to the case when the filesystem defines its own ->permission()
> method.  In the latter case the filesystem should be responsible for
> performing this check.
> 
> So create a helper function exec_permission() that checks returns
> -EACCESS if MAY_EXEC is present, the inode is a regular file and no
> execute bits are present in inode->i_mode.
> 
> Call this function from filesystems which don't call
> generic_permission() from their ->permission() methods and which
> aren't already performing this check.  Also remove the check from
> dentry_permission().
> 
> The new code should be equivalent to the old.

There's no point moving this into the filesystem.

Re: [patch 04/14] gfs2: dont call permission()

[Miklos Szeredi]

> On Wed, 2008-05-21 at 19:15 +0200, Miklos Szeredi wrote:
> > plain text document attachment (gfs2_permission_fix.patch)
> > From: Miklos Szeredi <mszeredi@suse.cz>
> > 
> > GFS2 calls permission() to verify permissions after locks on the files
> > have been taken.
> > 
> > For this it's sufficient to call gfs2_permission() instead.  This
> > results in the following changes:
> > 
> >   - IS_RDONLY() check is not performed
> >   - IS_IMMUTABLE() check is not performed
> >   - devcgroup_inode_permission() is not called
> >   - security_inode_permission() is not called
> > 
> > IS_RDONLY() should be unnecessary anyway, as the per-mount read-only
> > flag should provide protection against read-only remounts during
> > operations.  do_gfs2_set_flags() has been fixed to perform
> > mnt_want_write()/mnt_drop_write() to protect against remounting
> > read-only.
> > 
> > IS_IMMUTABLE has beed added to gfs2_do_permission()
> > 
> That looks ok, but I wonder do we really need gfs2_do_permission() and
> gfs2_permission when the only difference seems to be one argument?

Later in this series ->permission() is changed to take a dentry as the
first argument, so a separate function would've had to be reintroduced
anyway.

> > Repeating the security checks seems to be pointless, as they don't
> > normally change, and if they do, it's independent of the filesystem
> > state.
> > 
> I hope eventually we can fix this by allowing GFS2 to do its own
> lookups, via a suitable VFS library function. I understand that is the
> preferred option to replace "open intents" (which we don't currently use
> anyway) in the longer term.
> 
> > I also suspect the conditional locking in gfs2_do_permission() could
> > be cleaned up, due to the removal of the implicit recursion.
> > 
> In order to be sure we'd have to check that there are no NFS code paths
> left which can reach this code. That has usually been the reason for
> conditional locking.

OK, my impression was that in this case the conditional locking was
because of things like:

  gfs2_create()
    gfs2_createi()
      create_ok()
        permission()
	   gfs2_permission()

So moving the locing out of gfs2_do_permission() into gfs_permission()
these cases should be fixed.  I don't know about NFS.

> In general the patch looks ok to me, and since it doesn't appear to
> depend on anything else, I can drop it in my GFS2 git tree if that would
> be helpful at this stage,

Yes, that would help.

Thanks,
Miklos

Re: [patch 04/14] gfs2: dont call permission()

[Christoph Hellwig]

On Fri, May 23, 2008 at 10:12:06AM +0100, Steven Whitehouse wrote:
> > I also suspect the conditional locking in gfs2_do_permission() could
> > be cleaned up, due to the removal of the implicit recursion.
> > 
> In order to be sure we'd have to check that there are no NFS code paths
> left which can reach this code. That has usually been the reason for
> conditional locking.
> 
> In general the patch looks ok to me, and since it doesn't appear to
> depend on anything else, I can drop it in my GFS2 git tree if that would
> be helpful at this stage,

The NFS ->lookup inside filldir recursion is still there, but I plan to
fix that for .27.  You'll be Cc'ed on that patch.

Re: [patch 04/14] gfs2: dont call permission()

[Miklos Szeredi]

> Bad idea.  You're duplicating bits out of permission for no good
> reason.  I spent quite a bit of effort to make sure we don't have
> this duplicated logic around.

In this case you are wrong.  Look at the ugly conditional locking
gfs2_permission() does, which is probably due to the fact that it's
doing a recursion via calling permission() from inside already locked
parts in the filesystem.

That's _much_ worse than a duplicated IS_IMMUTABLE() call.  Which btw,
is a filesystem implementation detail: it needs to re-check the
immutability of the file after it has been locked.  I'm not even sure
it's strictly needed.  Steven?

Generally this sort of recursion through the VFS is ugly and
unnecessary, it's much better to provide helper for what the VFS is
doing if there's a lot of duplication.  But in this case there's
really no point in that at all.

Miklos

Re: [patch 11/14] vfs: move executable checking into ->permission()

[Miklos Szeredi]

> > From: Miklos Szeredi <mszeredi@suse.cz>
> > 
> > For execute permission on a regular files we need to check if file has
> > any execute bits at all, regardless of capabilites.
> > 
> > This check is normally performed by generic_permission() but was also
> > added to the case when the filesystem defines its own ->permission()
> > method.  In the latter case the filesystem should be responsible for
> > performing this check.
> > 
> > So create a helper function exec_permission() that checks returns
> > -EACCESS if MAY_EXEC is present, the inode is a regular file and no
> > execute bits are present in inode->i_mode.
> > 
> > Call this function from filesystems which don't call
> > generic_permission() from their ->permission() methods and which
> > aren't already performing this check.  Also remove the check from
> > dentry_permission().
> > 
> > The new code should be equivalent to the old.
> 
> There's no point moving this into the filesystem.

Yes, there is.  Fuse already has special code, because it need to
refresh the attributes first before doing the "are there any x bits in
mode?" check.  And because there's some duplication, I've introduced a
helper.  That's exactly how we handle this sort of thing.

Miklos

Re: [patch 04/14] gfs2: dont call permission()

[Miklos Szeredi]

> Generally this sort of recursion through the VFS is ugly and
> unnecessary, it's much better to provide helper for what the VFS is
> doing if there's a lot of duplication.  But in this case there's
> really no point in that at all.

And that's true of lookup_one_len() as well, the last bit of horror
left in this permission() saga.  It's called from lots of places,
often for doing things it's not at all meant to do (e.g. where caller
_knows_ it will return negative, etc...).  One day maybe I'll have
strength to clean that up as well..., but not now.

Miklos

Re: [patch 04/14] gfs2: dont call permission()

[Steven Whitehouse]

Hi,

On Fri, 2008-05-23 at 11:48 +0200, Miklos Szeredi wrote:
> > Bad idea.  You're duplicating bits out of permission for no good
> > reason.  I spent quite a bit of effort to make sure we don't have
> > this duplicated logic around.
> 
> In this case you are wrong.  Look at the ugly conditional locking
> gfs2_permission() does, which is probably due to the fact that it's
> doing a recursion via calling permission() from inside already locked
> parts in the filesystem.
> 
> That's _much_ worse than a duplicated IS_IMMUTABLE() call.  Which btw,
> is a filesystem implementation detail: it needs to re-check the
> immutability of the file after it has been locked.  I'm not even sure
> it's strictly needed.  Steven?
> 
Given the fact that (a) its only a very minor change and (b) as soon as
we have a solution to what we really want to do:

 - inode/file operation:
   - Do lookup via VFS
   - Get GFS2 glock
     - Do perm check via VFS
     - Do actual operation
   - Drop GFS2 glock

as opposed to the current situation of:

 - Do lookup via VFS:
   - Get GFS2 glock
   - Do perm check
   - Drop GFS2 glock
 - inode/file operation:
   - Get GFS2 glock
   - Recheck perms
   - Do the actual operation
   - Drop GFS2 glock

then the rechecking of perms will no longer be required anyway. I'm
inclined to agree with Miklos and say that its ok, but strictly on the
basis that its days are already numbered.

> Generally this sort of recursion through the VFS is ugly and
> unnecessary, it's much better to provide helper for what the VFS is
> doing if there's a lot of duplication.  But in this case there's
> really no point in that at all.
> 
> Miklos

Indeed I've spent a lot of time tracking down these cases and at least
now its possible to see them all by greping for
gfs2_glock_is_locked_by_me() in gfs2, whereas previously these cases
were hidden and less obvious,

Steve.

Re: [patch 04/14] gfs2: dont call permission()

[Miklos Szeredi]

> Given the fact that (a) its only a very minor change and (b) as soon as
> we have a solution to what we really want to do:
> 
>  - inode/file operation:
>    - Do lookup via VFS
>    - Get GFS2 glock
>      - Do perm check via VFS
>      - Do actual operation
>    - Drop GFS2 glock

Well, fuse/nfs already do something similar, except they have their
actual permission checking in the server, as opposed to the vfs.  They
basically do:

 ->permission() does nothing

 ->foo_operation() does everything, including permission checking

The reality is a bit more complicated, and both nfs and fuse do
sometimes check permissions in ->permission() but most of the cases,
when they know that the permission will be checked later anyway they
just omit it.

Of course this would mean, that for example the LSM security checks
are not done within the gfs locked region.  Does that matter?

Miklos

Re: [patch 06/14] hfsplus: remove hfsplus_permission()

[Roman Zippel]

Hi,

On Thu, 22 May 2008, Miklos Szeredi wrote:

> > As I said to allow lookup on files.
> 
> That requires a quite bit more support from the VFS than just allowing
> lookup to work on regular files without x bits.  You'll have big
> trouble with hard links for example: the VFS doesn't like non-leaf
> dentries to be aliased.
> 
> Besides hfsplus_permission() did not differentiate between execve on
> the file and lookup on that file, allowing both.  Which is obviously
> wrong.

I'm not saying it's perfect. I wished there was a MAY_LOOKUP to clearly 
differentiate both cases.
For now I'd rather keep it as it is and I'll look into it when I find the 
time.

bye, Roman

Re: [patch 06/14] hfsplus: remove hfsplus_permission()

[Roman Zippel]

Hi,

On Fri, 23 May 2008, Christoph Hellwig wrote:

> Yes.  Roman, if you care about details about the files as directories
> look in the reiser4 threads as it did something quite similar.

I was afraid this would come up. :)
I only need something much simpler, I don't need to create/move/unlink 
anything, I only need a lookup+open.
The reiser4 stuff goes a little too far, so very much would like to avoid 
wading through this mess...

bye, Roman

Re: [patch 06/14] hfsplus: remove hfsplus_permission()

[Miklos Szeredi]

> > Yes.  Roman, if you care about details about the files as directories
> > look in the reiser4 threads as it did something quite similar.
> 
> I was afraid this would come up. :)
> I only need something much simpler, I don't need to create/move/unlink 
> anything, I only need a lookup+open.

Doesn't matter.  As long as filesystem has hard links and allows
lookup on these, userspace can screw up the kernel.  Badly.

I did some work on this with auto-mounting/umounting directories on
regular files.  Hope to dig it up sometimes, but there are lots of
nasty little details to worry about...

Miklos

Re: [patch 06/14] hfsplus: remove hfsplus_permission()

[Miklos Szeredi]

> > > Yes.  Roman, if you care about details about the files as directories
> > > look in the reiser4 threads as it did something quite similar.
> > 
> > I was afraid this would come up. :)
> > I only need something much simpler, I don't need to create/move/unlink 
> > anything, I only need a lookup+open.
> 
> Doesn't matter.  As long as filesystem has hard links and allows
> lookup on these, userspace can screw up the kernel.  Badly.

And so I strongly suggest this patch as well.  Apparently this feature
is not even used by anybody, otherwise somebody would have noticed
when Trond's fix broke execute permission checking on regular files...

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
---
 fs/hfsplus/inode.c |   44 --------------------------------------------
 1 file changed, 44 deletions(-)

Index: linux-2.6/fs/hfsplus/inode.c
===================================================================
--- linux-2.6.orig/fs/hfsplus/inode.c	2008-05-22 18:27:02.000000000 +0200
+++ linux-2.6/fs/hfsplus/inode.c	2008-05-23 17:42:49.000000000 +0200
@@ -142,49 +142,6 @@ struct dentry_operations hfsplus_dentry_
 	.d_compare    = hfsplus_compare_dentry,
 };
 
-static struct dentry *hfsplus_file_lookup(struct inode *dir, struct dentry *dentry,
-					  struct nameidata *nd)
-{
-	struct hfs_find_data fd;
-	struct super_block *sb = dir->i_sb;
-	struct inode *inode = NULL;
-	int err;
-
-	if (HFSPLUS_IS_RSRC(dir) || strcmp(dentry->d_name.name, "rsrc"))
-		goto out;
-
-	inode = HFSPLUS_I(dir).rsrc_inode;
-	if (inode)
-		goto out;
-
-	inode = new_inode(sb);
-	if (!inode)
-		return ERR_PTR(-ENOMEM);
-
-	inode->i_ino = dir->i_ino;
-	INIT_LIST_HEAD(&HFSPLUS_I(inode).open_dir_list);
-	init_MUTEX(&HFSPLUS_I(inode).extents_lock);
-	HFSPLUS_I(inode).flags = HFSPLUS_FLG_RSRC;
-
-	hfs_find_init(HFSPLUS_SB(sb).cat_tree, &fd);
-	err = hfsplus_find_cat(sb, dir->i_ino, &fd);
-	if (!err)
-		err = hfsplus_cat_read_inode(inode, &fd);
-	hfs_find_exit(&fd);
-	if (err) {
-		iput(inode);
-		return ERR_PTR(err);
-	}
-	HFSPLUS_I(inode).rsrc_inode = dir;
-	HFSPLUS_I(dir).rsrc_inode = inode;
-	igrab(dir);
-	hlist_add_head(&inode->i_hash, &HFSPLUS_SB(sb).rsrc_inodes);
-	mark_inode_dirty(inode);
-out:
-	d_add(dentry, inode);
-	return NULL;
-}
-
 static void hfsplus_get_perms(struct inode *inode, struct hfsplus_perm *perms, int dir)
 {
 	struct super_block *sb = inode->i_sb;
@@ -269,7 +226,6 @@ static int hfsplus_file_release(struct i
 }
 
 static const struct inode_operations hfsplus_file_inode_operations = {
-	.lookup		= hfsplus_file_lookup,
 	.truncate	= hfsplus_file_truncate,
 	.setxattr	= hfsplus_setxattr,
 	.getxattr	= hfsplus_getxattr,

Re: [patch 06/14] hfsplus: remove hfsplus_permission()

[Roman Zippel]

Hi,

On Fri, 23 May 2008, Miklos Szeredi wrote:

> And so I strongly suggest this patch as well.  Apparently this feature
> is not even used by anybody, otherwise somebody would have noticed
> when Trond's fix broke execute permission checking on regular files...

And I'm very strongly against this.
If you wanted to rip out the resource inode logic, it would be more than 
this.
At worst this code does nothing right now, so why this sudden rush to 
remove this without even an attempt of fixing it?

bye, Roman

Re: [patch 06/14] hfsplus: remove hfsplus_permission()

[Miklos Szeredi]

> > And so I strongly suggest this patch as well.  Apparently this feature
> > is not even used by anybody, otherwise somebody would have noticed
> > when Trond's fix broke execute permission checking on regular files...
> 
> And I'm very strongly against this.
> If you wanted to rip out the resource inode logic, it would be more than 
> this.
> At worst this code does nothing right now, so why this sudden rush to 
> remove this without even an attempt of fixing it?

Because

 a) it's nontrivial to fix (even understanding the problem is
 nontrivial, see Documentation/filesystems/directory-locking)

 b) the feature is obviously unused, so the easiest fix is simply to
 remove it for the time being.

We generally don't leave code around which is known to be deadlockable
by unprivileged users.  Of course if you have a better fix, then
that's fine.

Miklos

Re: [patch 06/14] hfsplus: remove hfsplus_permission()

[Roman Zippel]

Hi,

On Fri, 23 May 2008, Miklos Szeredi wrote:

>  a) it's nontrivial to fix (even understanding the problem is
>  nontrivial, see Documentation/filesystems/directory-locking)

Please provide a concrete example, as long as I only get handwaving I can 
only wave back.

>  b) the feature is obviously unused, so the easiest fix is simply to
>  remove it for the time being.

You have a weird definition of "obviously unused", not every user may even 
have noticed it's gone, e.g. there are some rsync versions, which support 
this and these won't complain if they can't open the resource fork.

bye, Roman

Re: [patch 06/14] hfsplus: remove hfsplus_permission()

[Miklos Szeredi]

> >  a) it's nontrivial to fix (even understanding the problem is
> >  nontrivial, see Documentation/filesystems/directory-locking)
> 
> Please provide a concrete example, as long as I only get handwaving I can 
> only wave back.

Semi-concrete: link(2) locks the target's parent and the source.
Cross-directory rename(2) locks both parents.  If link's target is a
file which has children, this can result in an ABBA deadlock.  That's
_before_ the filesystem's ->link() or ->rename() function is called.

> >  b) the feature is obviously unused, so the easiest fix is simply to
> >  remove it for the time being.
> 
> You have a weird definition of "obviously unused", not every user may even 
> have noticed it's gone, e.g. there are some rsync versions, which support 
> this and these won't complain if they can't open the resource fork.

Dunno.  What's the difference between "it wasn't used" and "it didn't
work and nobody noticed"?  I think not much :)

Miklos

Re: [patch 06/14] hfsplus: remove hfsplus_permission()

[Roman Zippel]

Hi,

On Fri, 23 May 2008, Miklos Szeredi wrote:

> Semi-concrete: link(2) locks the target's parent and the source.
> Cross-directory rename(2) locks both parents.  If link's target is a
> file which has children, this can result in an ABBA deadlock.  That's
> _before_ the filesystem's ->link() or ->rename() function is called.

I'm afraid you have to be more concrete than this. 
hfsplus_file_inode_operations doesn't have a link or rename operation, so 
if one your source parents above involves a file, it shouldn't get this 
far or you have to describe your scenario in more detail.

bye, Roman

Re: [patch 06/14] hfsplus: remove hfsplus_permission()

[Miklos Szeredi]

> > Semi-concrete: link(2) locks the target's parent and the source.
> > Cross-directory rename(2) locks both parents.  If link's target is a
> > file which has children, this can result in an ABBA deadlock.  That's
> > _before_ the filesystem's ->link() or ->rename() function is called.
> 
> I'm afraid you have to be more concrete than this. 
> hfsplus_file_inode_operations doesn't have a link or rename operation, so 
> if one your source parents above involves a file, it shouldn't get this 
> far or you have to describe your scenario in more detail.

Heh, you just want me to do all the hard work :)

OK, I'll go along:

  /mnt/a/x is a regular file

  ln /mnt/a/x /mnt/b/x
  rename /mnt/a/x /mnt/b/x/p & ln /mnt/b/x /mnt/a/q

Neither ->rename nor ->link is required to be defined on x.  The
rename will first lock x, then a.  The link will first lock a then x,
AFAICS.

This is probably just one of several ways to make it deadlock.

Hmm?

Miklos

Re: [patch 04/14] gfs2: dont call permission()

[Christoph Hellwig]

On Fri, May 23, 2008 at 11:48:28AM +0200, Miklos Szeredi wrote:
> > Bad idea.  You're duplicating bits out of permission for no good
> > reason.  I spent quite a bit of effort to make sure we don't have
> > this duplicated logic around.
> 
> In this case you are wrong.  Look at the ugly conditional locking
> gfs2_permission() does, which is probably due to the fact that it's
> doing a recursion via calling permission() from inside already locked
> parts in the filesystem.

The conditional locking will have to stay anyway until the NFS issues
are sorted out.  And as Steve mention I'd rather eventually sort out
the need why gfs2 needs these permission/foo_permission calls at all.

Currently out VFS support for clustered filesystem sucks quite badly,
and we need some way to allow callouts into cluster locks around the
whole namespace operation so we can rely on a single permission check.

That beeing said once the nfs issues are sorted out you patch might
not be too bad it it kills all remaining recursive locking in gfs.

But that's left up to Steve to decide.

Re: [patch 04/14] gfs2: dont call permission()

[Christoph Hellwig]

On Fri, May 23, 2008 at 12:00:26PM +0200, Miklos Szeredi wrote:
> And that's true of lookup_one_len() as well, the last bit of horror
> left in this permission() saga.  It's called from lots of places,
> often for doing things it's not at all meant to do (e.g. where caller
> _knows_ it will return negative, etc...).  One day maybe I'll have
> strength to clean that up as well..., but not now.

lookup_one_len is not just a horror for permission, it's a nighmare
in general.  I'm not convinced any use of it except for silly renames
is actually valid.