[RFC] vfs: cleanup of permission()

[RFC] vfs: cleanup of permission()

[Herbert Poetzl]

Hi Andrew! Christoph! Al!

after thinking some time about the oracle words
(sent in reply to previous BME submissions) we 
(Sam and I) came to the conclusion that it would 
be a good idea to remove the nameidata introduced
in September 2003 from the inode permission()
checks, so that vfs_permission() can take care
of them ...

this is in two parts, the first one does the 
removal and the second one fixes up nfs and fuse
by passing the relevant nd_flags via the mask

Note: this is just a suggestion, so please let
      us know what you think 

[RFC 1/2] vfs: remove nameidata from *_permission()

[Herbert Poetzl]

remove struct nameidata from *_permission() and 
related security functions.

Signed-off-by: Herbert Pötzl <herbert@13thfloor.at>
Signed-off-by: Sam Vilain <sam.vilain@catalyst.net.nz>
---

diff -NurpP --minimal linux-2.6.16-rc5/fs/cifs/cifsfs.c linux-2.6.16-rc5-vfs0.02.1/fs/cifs/cifsfs.c
--- linux-2.6.16-rc5/fs/cifs/cifsfs.c	2006-02-28 03:41:04 +0100
+++ linux-2.6.16-rc5-vfs0.02.1/fs/cifs/cifsfs.c	2006-02-28 05:08:53 +0100
@@ -220,7 +220,7 @@ cifs_statfs(struct super_block *sb, stru
 				   longer available? */
 }
 
-static int cifs_permission(struct inode * inode, int mask, struct nameidata *nd)
+static int cifs_permission(struct inode * inode, int mask)
 {
 	struct cifs_sb_info *cifs_sb;
 
diff -NurpP --minimal linux-2.6.16-rc5/fs/coda/dir.c linux-2.6.16-rc5-vfs0.02.1/fs/coda/dir.c
--- linux-2.6.16-rc5/fs/coda/dir.c	2006-02-28 03:41:04 +0100
+++ linux-2.6.16-rc5-vfs0.02.1/fs/coda/dir.c	2006-02-28 05:08:18 +0100
@@ -151,7 +151,7 @@ exit:
 }
 
 
-int coda_permission(struct inode *inode, int mask, struct nameidata *nd)
+int coda_permission(struct inode *inode, int mask)
 {
         int error = 0;
  
diff -NurpP --minimal linux-2.6.16-rc5/fs/coda/pioctl.c linux-2.6.16-rc5-vfs0.02.1/fs/coda/pioctl.c
--- linux-2.6.16-rc5/fs/coda/pioctl.c	2004-08-14 12:55:47 +0200
+++ linux-2.6.16-rc5-vfs0.02.1/fs/coda/pioctl.c	2006-02-28 06:04:48 +0100
@@ -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 inode *inode, int mask);
 static int coda_pioctl(struct inode * inode, struct file * filp, 
                        unsigned int cmd, unsigned long user_data);
 
@@ -42,8 +41,7 @@ struct file_operations coda_ioctl_operat
 };
 
 /* the coda pioctl inode ops */
-static int coda_ioctl_permission(struct inode *inode, int mask,
-				 struct nameidata *nd)
+static int coda_ioctl_permission(struct inode *inode, int mask)
 {
         return 0;
 }
diff -NurpP --minimal linux-2.6.16-rc5/fs/ext2/acl.c linux-2.6.16-rc5-vfs0.02.1/fs/ext2/acl.c
--- linux-2.6.16-rc5/fs/ext2/acl.c	2006-02-28 03:41:04 +0100
+++ linux-2.6.16-rc5-vfs0.02.1/fs/ext2/acl.c	2006-02-28 05:09:10 +0100
@@ -294,7 +294,7 @@ ext2_check_acl(struct inode *inode, int 
 }
 
 int
-ext2_permission(struct inode *inode, int mask, struct nameidata *nd)
+ext2_permission(struct inode *inode, int mask)
 {
 	return generic_permission(inode, mask, ext2_check_acl);
 }
diff -NurpP --minimal linux-2.6.16-rc5/fs/ext3/acl.c linux-2.6.16-rc5-vfs0.02.1/fs/ext3/acl.c
--- linux-2.6.16-rc5/fs/ext3/acl.c	2006-02-28 03:41:04 +0100
+++ linux-2.6.16-rc5-vfs0.02.1/fs/ext3/acl.c	2006-02-28 05:09:22 +0100
@@ -299,7 +299,7 @@ ext3_check_acl(struct inode *inode, int 
 }
 
 int
-ext3_permission(struct inode *inode, int mask, struct nameidata *nd)
+ext3_permission(struct inode *inode, int mask)
 {
 	return generic_permission(inode, mask, ext3_check_acl);
 }
diff -NurpP --minimal linux-2.6.16-rc5/fs/fuse/dir.c linux-2.6.16-rc5-vfs0.02.1/fs/fuse/dir.c
--- linux-2.6.16-rc5/fs/fuse/dir.c	2006-02-28 03:41:05 +0100
+++ linux-2.6.16-rc5-vfs0.02.1/fs/fuse/dir.c	2006-02-28 05:56:38 +0100
@@ -702,7 +702,7 @@ 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 inode *inode, int mask)
 {
 	struct fuse_conn *fc = get_fuse_conn(inode);
 
diff -NurpP --minimal linux-2.6.16-rc5/fs/hfs/inode.c linux-2.6.16-rc5-vfs0.02.1/fs/hfs/inode.c
--- linux-2.6.16-rc5/fs/hfs/inode.c	2006-02-28 03:41:05 +0100
+++ linux-2.6.16-rc5-vfs0.02.1/fs/hfs/inode.c	2006-02-28 05:11:18 +0100
@@ -519,8 +519,7 @@ void hfs_clear_inode(struct inode *inode
 	}
 }
 
-static int hfs_permission(struct inode *inode, int mask,
-			  struct nameidata *nd)
+static int hfs_permission(struct inode *inode, int mask)
 {
 	if (S_ISREG(inode->i_mode) && mask & MAY_EXEC)
 		return 0;
diff -NurpP --minimal linux-2.6.16-rc5/fs/hfsplus/inode.c linux-2.6.16-rc5-vfs0.02.1/fs/hfsplus/inode.c
--- linux-2.6.16-rc5/fs/hfsplus/inode.c	2006-02-28 03:41:05 +0100
+++ linux-2.6.16-rc5-vfs0.02.1/fs/hfsplus/inode.c	2006-02-28 05:11:30 +0100
@@ -237,7 +237,7 @@ 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)
+static int hfsplus_permission(struct inode *inode, int mask)
 {
 	/* 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
diff -NurpP --minimal linux-2.6.16-rc5/fs/hostfs/hostfs_kern.c linux-2.6.16-rc5-vfs0.02.1/fs/hostfs/hostfs_kern.c
--- linux-2.6.16-rc5/fs/hostfs/hostfs_kern.c	2006-01-03 17:29:56 +0100
+++ linux-2.6.16-rc5-vfs0.02.1/fs/hostfs/hostfs_kern.c	2006-02-28 05:11:51 +0100
@@ -796,7 +796,7 @@ int hostfs_rename(struct inode *from_ino
 	return(err);
 }
 
-int hostfs_permission(struct inode *ino, int desired, struct nameidata *nd)
+int hostfs_permission(struct inode *ino, int desired)
 {
 	char *name;
 	int r = 0, w = 0, x = 0, err;
diff -NurpP --minimal linux-2.6.16-rc5/fs/hpfs/namei.c linux-2.6.16-rc5-vfs0.02.1/fs/hpfs/namei.c
--- linux-2.6.16-rc5/fs/hpfs/namei.c	2004-08-14 12:55:19 +0200
+++ linux-2.6.16-rc5-vfs0.02.1/fs/hpfs/namei.c	2006-02-28 04:54:18 +0100
@@ -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) ||
+		    permission(inode, MAY_WRITE) ||
 		    !S_ISREG(inode->i_mode) ||
 		    get_write_access(inode)) {
 			spin_unlock(&dentry->d_lock);
diff -NurpP --minimal linux-2.6.16-rc5/fs/jfs/acl.c linux-2.6.16-rc5-vfs0.02.1/fs/jfs/acl.c
--- linux-2.6.16-rc5/fs/jfs/acl.c	2005-10-28 20:49:44 +0200
+++ linux-2.6.16-rc5-vfs0.02.1/fs/jfs/acl.c	2006-02-28 05:12:01 +0100
@@ -140,7 +140,7 @@ 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 inode *inode, int mask)
 {
 	return generic_permission(inode, mask, jfs_check_acl);
 }
diff -NurpP --minimal linux-2.6.16-rc5/fs/jfs/jfs_acl.h linux-2.6.16-rc5-vfs0.02.1/fs/jfs/jfs_acl.h
--- linux-2.6.16-rc5/fs/jfs/jfs_acl.h	2005-10-28 20:49:44 +0200
+++ linux-2.6.16-rc5-vfs0.02.1/fs/jfs/jfs_acl.h	2006-02-28 06:07:00 +0100
@@ -20,7 +20,7 @@
 
 #ifdef CONFIG_JFS_POSIX_ACL
 
-int jfs_permission(struct inode *, int, struct nameidata *);
+int jfs_permission(struct inode *, int);
 int jfs_init_acl(tid_t, struct inode *, struct inode *);
 int jfs_setattr(struct dentry *, struct iattr *);
 
diff -NurpP --minimal linux-2.6.16-rc5/fs/namei.c linux-2.6.16-rc5-vfs0.02.1/fs/namei.c
--- linux-2.6.16-rc5/fs/namei.c	2006-02-28 03:41:07 +0100
+++ linux-2.6.16-rc5-vfs0.02.1/fs/namei.c	2006-02-28 05:34:28 +0100
@@ -225,7 +225,7 @@ int generic_permission(struct inode *ino
 	return -EACCES;
 }
 
-int permission(struct inode *inode, int mask, struct nameidata *nd)
+int permission(struct inode *inode, int mask)
 {
 	int retval, submask;
 
@@ -250,13 +250,13 @@ int permission(struct inode *inode, int 
 	/* 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(inode, submask);
 	else
 		retval = generic_permission(inode, submask, NULL);
 	if (retval)
 		return retval;
 
-	return security_inode_permission(inode, mask, nd);
+	return security_inode_permission(inode, mask);
 }
 
 /**
@@ -271,7 +271,8 @@ int permission(struct inode *inode, int 
  */
 int vfs_permission(struct nameidata *nd, int mask)
 {
-	return permission(nd->dentry->d_inode, mask, nd);
+	/* do nd based stuff here */
+	return permission(nd->dentry->d_inode, mask);
 }
 
 /**
@@ -288,7 +289,8 @@ int vfs_permission(struct nameidata *nd,
  */
 int file_permission(struct file *file, int mask)
 {
-	return permission(file->f_dentry->d_inode, mask, NULL);
+	/* do file based stuff here */
+	return permission(file->f_dentry->d_inode, mask);
 }
 
 /*
@@ -425,7 +427,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);
 }
 
 /*
@@ -1219,7 +1221,8 @@ static struct dentry * __lookup_hash(str
 	int err;
 
 	inode = base->d_inode;
-	err = permission(inode, MAY_EXEC, nd);
+	/* should become vfs_permission() */
+	err = permission(inode, MAY_EXEC);
 	dentry = ERR_PTR(err);
 	if (err)
 		goto out;
@@ -1354,7 +1357,7 @@ static int may_delete(struct inode *dir,
 
 	BUG_ON(victim->d_parent->d_inode != dir);
 
-	error = permission(dir,MAY_WRITE | MAY_EXEC, NULL);
+	error = permission(dir,MAY_WRITE | MAY_EXEC);
 	if (error)
 		return error;
 	if (IS_APPEND(dir))
@@ -1391,7 +1394,8 @@ static inline int may_create(struct inod
 		return -EEXIST;
 	if (IS_DEADDIR(dir))
 		return -ENOENT;
-	return permission(dir,MAY_WRITE | MAY_EXEC, nd);
+	/* should become vfs_permission() */
+	return permission(dir,MAY_WRITE | MAY_EXEC);
 }
 
 /* 
@@ -2313,7 +2317,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 = permission(old_dentry->d_inode, MAY_WRITE);
 		if (error)
 			return error;
 	}
diff -NurpP --minimal linux-2.6.16-rc5/fs/nfs/dir.c linux-2.6.16-rc5-vfs0.02.1/fs/nfs/dir.c
--- linux-2.6.16-rc5/fs/nfs/dir.c	2006-02-28 03:41:07 +0100
+++ linux-2.6.16-rc5-vfs0.02.1/fs/nfs/dir.c	2006-02-28 05:56:38 +0100
@@ -1635,7 +1635,7 @@ out:
 	return -EACCES;
 }
 
-int nfs_permission(struct inode *inode, int mask, struct nameidata *nd)
+int nfs_permission(struct inode *inode, int mask)
 {
 	struct rpc_cred *cred;
 	int res = 0;
diff -NurpP --minimal linux-2.6.16-rc5/fs/nfsd/nfsfh.c linux-2.6.16-rc5-vfs0.02.1/fs/nfsd/nfsfh.c
--- linux-2.6.16-rc5/fs/nfsd/nfsfh.c	2005-06-22 02:38:37 +0200
+++ linux-2.6.16-rc5-vfs0.02.1/fs/nfsd/nfsfh.c	2006-02-28 05:35:53 +0100
@@ -56,7 +56,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 = permission(parent->d_inode, MAY_EXEC);
 		if (err < 0) {
 			dput(parent);
 			break;
diff -NurpP --minimal linux-2.6.16-rc5/fs/nfsd/vfs.c linux-2.6.16-rc5-vfs0.02.1/fs/nfsd/vfs.c
--- linux-2.6.16-rc5/fs/nfsd/vfs.c	2006-02-28 03:41:07 +0100
+++ linux-2.6.16-rc5-vfs0.02.1/fs/nfsd/vfs.c	2006-02-28 05:36:11 +0100
@@ -1817,12 +1817,12 @@ nfsd_permission(struct svc_export *exp, 
 	    inode->i_uid == current->fsuid)
 		return 0;
 
-	err = permission(inode, acc & (MAY_READ|MAY_WRITE|MAY_EXEC), NULL);
+	err = permission(inode, acc & (MAY_READ|MAY_WRITE|MAY_EXEC));
 
 	/* 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 = permission(inode, MAY_EXEC);
 
 	return err? nfserrno(err) : 0;
 }
diff -NurpP --minimal linux-2.6.16-rc5/fs/proc/base.c linux-2.6.16-rc5-vfs0.02.1/fs/proc/base.c
--- linux-2.6.16-rc5/fs/proc/base.c	2006-02-28 03:41:08 +0100
+++ linux-2.6.16-rc5-vfs0.02.1/fs/proc/base.c	2006-02-28 05:14:35 +0100
@@ -579,14 +579,14 @@ static int proc_check_root(struct inode 
 	return proc_check_chroot(root, vfsmnt);
 }
 
-static int proc_permission(struct inode *inode, int mask, struct nameidata *nd)
+static int proc_permission(struct inode *inode, int mask)
 {
 	if (generic_permission(inode, mask, NULL) != 0)
 		return -EACCES;
 	return proc_check_root(inode);
 }
 
-static int proc_task_permission(struct inode *inode, int mask, struct nameidata *nd)
+static int proc_task_permission(struct inode *inode, int mask)
 {
 	struct dentry *root;
 	struct vfsmount *vfsmnt;
diff -NurpP --minimal linux-2.6.16-rc5/fs/reiserfs/xattr.c linux-2.6.16-rc5-vfs0.02.1/fs/reiserfs/xattr.c
--- linux-2.6.16-rc5/fs/reiserfs/xattr.c	2006-02-28 03:41:08 +0100
+++ linux-2.6.16-rc5-vfs0.02.1/fs/reiserfs/xattr.c	2006-02-28 05:14:55 +0100
@@ -1343,7 +1343,7 @@ static int reiserfs_check_acl(struct ino
 	return error;
 }
 
-int reiserfs_permission(struct inode *inode, int mask, struct nameidata *nd)
+int reiserfs_permission(struct inode *inode, int mask)
 {
 	/*
 	 * We don't do permission checks on the internal objects.
diff -NurpP --minimal linux-2.6.16-rc5/fs/smbfs/file.c linux-2.6.16-rc5-vfs0.02.1/fs/smbfs/file.c
--- linux-2.6.16-rc5/fs/smbfs/file.c	2006-02-28 03:41:08 +0100
+++ linux-2.6.16-rc5-vfs0.02.1/fs/smbfs/file.c	2006-02-28 05:15:06 +0100
@@ -387,7 +387,7 @@ 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 inode *inode, int mask)
 {
 	int mode = inode->i_mode;
 	int error = 0;
diff -NurpP --minimal linux-2.6.16-rc5/fs/xattr.c linux-2.6.16-rc5-vfs0.02.1/fs/xattr.c
--- linux-2.6.16-rc5/fs/xattr.c	2006-02-28 03:41:08 +0100
+++ linux-2.6.16-rc5-vfs0.02.1/fs/xattr.c	2006-02-28 05:35:18 +0100
@@ -58,7 +58,7 @@ xattr_permission(struct inode *inode, co
 			return -EPERM;
 	}
 
-	return permission(inode, mask, NULL);
+	return permission(inode, mask);
 }
 
 int
diff -NurpP --minimal linux-2.6.16-rc5/fs/xfs/linux-2.6/xfs_iops.c linux-2.6.16-rc5-vfs0.02.1/fs/xfs/linux-2.6/xfs_iops.c
--- linux-2.6.16-rc5/fs/xfs/linux-2.6/xfs_iops.c	2006-02-28 03:41:09 +0100
+++ linux-2.6.16-rc5-vfs0.02.1/fs/xfs/linux-2.6/xfs_iops.c	2006-02-28 05:15:25 +0100
@@ -614,8 +614,7 @@ linvfs_put_link(
 STATIC int
 linvfs_permission(
 	struct inode	*inode,
-	int		mode,
-	struct nameidata *nd)
+	int		mode)
 {
 	vnode_t		*vp = LINVFS_GET_VP(inode);
 	int		error;
diff -NurpP --minimal linux-2.6.16-rc5/include/linux/fs.h linux-2.6.16-rc5-vfs0.02.1/include/linux/fs.h
--- linux-2.6.16-rc5/include/linux/fs.h	2006-02-28 03:41:18 +0100
+++ linux-2.6.16-rc5-vfs0.02.1/include/linux/fs.h	2006-02-28 05:29:44 +0100
@@ -1040,7 +1040,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 inode *, 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);
@@ -1465,7 +1465,8 @@ extern int do_remount_sb(struct super_bl
 			 void *data, int force);
 extern sector_t bmap(struct inode *, sector_t);
 extern int notify_change(struct dentry *, struct iattr *);
-extern int permission(struct inode *, int, struct nameidata *);
+extern int permission(struct inode *, int);
+// extern int vfs_permission(struct nameidata *, int);
 extern int generic_permission(struct inode *, int,
 		int (*check_acl)(struct inode *, int));
 
diff -NurpP --minimal linux-2.6.16-rc5/include/linux/nfs_fs.h linux-2.6.16-rc5-vfs0.02.1/include/linux/nfs_fs.h
--- linux-2.6.16-rc5/include/linux/nfs_fs.h	2006-02-28 03:41:19 +0100
+++ linux-2.6.16-rc5-vfs0.02.1/include/linux/nfs_fs.h	2006-02-28 05:58:49 +0100
@@ -296,7 +296,7 @@ extern struct inode *nfs_fhget(struct su
 extern int nfs_refresh_inode(struct inode *, struct nfs_fattr *);
 extern int nfs_post_op_update_inode(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 inode *, int);
 extern int nfs_access_get_cached(struct inode *, struct rpc_cred *, struct nfs_access_entry *);
 extern void nfs_access_add_cache(struct inode *, struct nfs_access_entry *);
 extern int nfs_open(struct inode *, struct file *);
diff -NurpP --minimal linux-2.6.16-rc5/include/linux/security.h linux-2.6.16-rc5-vfs0.02.1/include/linux/security.h
--- linux-2.6.16-rc5/include/linux/security.h	2006-02-28 03:41:19 +0100
+++ linux-2.6.16-rc5-vfs0.02.1/include/linux/security.h	2006-02-28 06:09:30 +0100
@@ -346,7 +346,6 @@ struct swap_info_struct;
  *	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).
  *	Return 0 if permission is granted.
  * @inode_setattr:
  *	Check permission before setting file attributes.  Note that the kernel
@@ -1157,7 +1156,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 (*inode_setattr)	(struct dentry *dentry, struct iattr *attr);
 	int (*inode_getattr) (struct vfsmount *mnt, struct dentry *dentry);
         void (*inode_delete) (struct inode *inode);
@@ -1606,12 +1605,11 @@ static inline int security_inode_follow_
 	return security_ops->inode_follow_link (dentry, nd);
 }
 
-static inline int security_inode_permission (struct inode *inode, int mask,
-					     struct nameidata *nd)
+static inline int security_inode_permission (struct inode *inode, int mask)
 {
 	if (unlikely (IS_PRIVATE (inode)))
 		return 0;
-	return security_ops->inode_permission (inode, mask, nd);
+	return security_ops->inode_permission (inode, mask);
 }
 
 static inline int security_inode_setattr (struct dentry *dentry,
@@ -2270,8 +2268,7 @@ static inline int security_inode_follow_
 	return 0;
 }
 
-static inline int security_inode_permission (struct inode *inode, int mask,
-					     struct nameidata *nd)
+static inline int security_inode_permission (struct inode *inode, int mask)
 {
 	return 0;
 }
diff -NurpP --minimal linux-2.6.16-rc5/ipc/mqueue.c linux-2.6.16-rc5-vfs0.02.1/ipc/mqueue.c
--- linux-2.6.16-rc5/ipc/mqueue.c	2006-02-28 03:41:21 +0100
+++ linux-2.6.16-rc5-vfs0.02.1/ipc/mqueue.c	2006-02-28 05:28:20 +0100
@@ -639,7 +639,7 @@ static int oflag2acc[O_ACCMODE] = { MAY_
 		return ERR_PTR(-EINVAL);
 	}
 
-	if (permission(dentry->d_inode, oflag2acc[oflag & O_ACCMODE], NULL)) {
+	if (permission(dentry->d_inode, oflag2acc[oflag & O_ACCMODE])) {
 		dput(dentry);
 		mntput(mqueue_mnt);
 		return ERR_PTR(-EACCES);
diff -NurpP --minimal linux-2.6.16-rc5/security/dummy.c linux-2.6.16-rc5-vfs0.02.1/security/dummy.c
--- linux-2.6.16-rc5/security/dummy.c	2006-02-28 03:41:33 +0100
+++ linux-2.6.16-rc5-vfs0.02.1/security/dummy.c	2006-02-28 05:16:36 +0100
@@ -324,7 +324,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)
 {
 	return 0;
 }
diff -NurpP --minimal linux-2.6.16-rc5/security/seclvl.c linux-2.6.16-rc5-vfs0.02.1/security/seclvl.c
--- linux-2.6.16-rc5/security/seclvl.c	2006-02-28 03:41:33 +0100
+++ linux-2.6.16-rc5-vfs0.02.1/security/seclvl.c	2006-02-28 05:19:56 +0100
@@ -422,7 +422,7 @@ static void seclvl_bd_release(struct ino
  * seclvl 1, we only deny writes to *mounted* block devices.
  */
 static int
-seclvl_inode_permission(struct inode *inode, int mask, struct nameidata *nd)
+seclvl_inode_permission(struct inode *inode, int mask)
 {
 	if (current->pid != 1 && S_ISBLK(inode->i_mode) && (mask & MAY_WRITE)) {
 		switch (seclvl) {
diff -NurpP --minimal linux-2.6.16-rc5/security/selinux/hooks.c linux-2.6.16-rc5-vfs0.02.1/security/selinux/hooks.c
--- linux-2.6.16-rc5/security/selinux/hooks.c	2006-02-28 03:41:34 +0100
+++ linux-2.6.16-rc5-vfs0.02.1/security/selinux/hooks.c	2006-02-28 05:20:22 +0100
@@ -2052,12 +2052,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 rc;
 
-	rc = secondary_ops->inode_permission(inode, mask, nd);
+	rc = secondary_ops->inode_permission(inode, mask);
 	if (rc)
 		return rc;
 

[RFC 2/2] vfs: fixup nfs and fuse by passing nd_flags via mask

[Herbert Poetzl]

fixup nfs and fuse nameidata related checks by 
passing the relevant flags via the mask.

Signed-off-by: Herbert Pötzl <herbert@13thfloor.at>
Acked-by: Sam Vilain <sam.vilain@catalyst.net.nz>
---

diff -NurpP --minimal linux-2.6.16-rc5-vfs0.02.1/fs/fuse/dir.c linux-2.6.16-rc5-vfs0.02.2/fs/fuse/dir.c
--- linux-2.6.16-rc5-vfs0.02.1/fs/fuse/dir.c	2006-02-28 05:56:38 +0100
+++ linux-2.6.16-rc5-vfs0.02.2/fs/fuse/dir.c	2006-02-28 05:49:57 +0100
@@ -731,8 +731,9 @@ static int fuse_permission(struct inode 
 		if ((mask & MAY_EXEC) && !S_ISDIR(mode) && !(mode & S_IXUGO))
 			return -EACCES;
 
-		if (nd && (nd->flags & LOOKUP_ACCESS))
+		if (mask & LOOKUP_ACCESS)
 			return fuse_access(inode, mask);
+
 		return 0;
 	}
 }
diff -NurpP --minimal linux-2.6.16-rc5-vfs0.02.1/fs/namei.c linux-2.6.16-rc5-vfs0.02.2/fs/namei.c
--- linux-2.6.16-rc5-vfs0.02.1/fs/namei.c	2006-02-28 05:34:28 +0100
+++ linux-2.6.16-rc5-vfs0.02.2/fs/namei.c	2006-02-28 05:47:29 +0100
@@ -271,8 +271,11 @@ int permission(struct inode *inode, int 
  */
 int vfs_permission(struct nameidata *nd, int mask)
 {
+	int nd_flags = nd->flags &
+		(LOOKUP_ACCESS | LOOKUP_CREATE | LOOKUP_OPEN);
+
 	/* do nd based stuff here */
-	return permission(nd->dentry->d_inode, mask);
+	return permission(nd->dentry->d_inode, mask | nd_flags);
 }
 
 /**
diff -NurpP --minimal linux-2.6.16-rc5-vfs0.02.1/fs/nfs/dir.c linux-2.6.16-rc5-vfs0.02.2/fs/nfs/dir.c
--- linux-2.6.16-rc5-vfs0.02.1/fs/nfs/dir.c	2006-02-28 05:56:38 +0100
+++ linux-2.6.16-rc5-vfs0.02.2/fs/nfs/dir.c	2006-02-28 05:51:36 +0100
@@ -1643,7 +1643,7 @@ int nfs_permission(struct inode *inode, 
 	if (mask == 0)
 		goto out;
 	/* Is this sys_access() ? */
-	if (nd != NULL && (nd->flags & LOOKUP_ACCESS))
+	if (mask & LOOKUP_ACCESS)
 		goto force_lookup;
 
 	switch (inode->i_mode & S_IFMT) {
@@ -1652,8 +1652,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))
+					&& (mask & LOOKUP_OPEN))
 				goto out;
 			break;
 		case S_IFDIR:

Re: [RFC] vfs: cleanup of permission()

[Trond Myklebust]

On Tue, 2006-02-28 at 06:26 +0100, Herbert Poetzl wrote:
> Hi Andrew! Christoph! Al!
> 
> after thinking some time about the oracle words
> (sent in reply to previous BME submissions) we 
> (Sam and I) came to the conclusion that it would 
> be a good idea to remove the nameidata introduced
> in September 2003 from the inode permission()
> checks, so that vfs_permission() can take care
> of them ...

Why? There may be perfectly legitimate reasons for the filesystem to
request information about the path. I can think of server failover
situations in NFSv4 where the client may need to look up the filehandle
for the file on the new server before it can service the ACCESS call.

> this is in two parts, the first one does the 
> removal and the second one fixes up nfs and fuse
> by passing the relevant nd_flags via the mask
> 
> Note: this is just a suggestion, so please let
>       us know what you think 

Firstly, the fact that the lookup intent flags happen not to collide
with MAY_* is a complete fluke, not a design. The numerical values of
either set of flags could change tomorrow for all you know.

Secondly, an intent is _not_ a permissions mask by any stretch of the
imagination.

IOW: at the very least make that intent flag a separate parameter.

Cheers,
  Trond

Re: [RFC] vfs: cleanup of permission()

[tvrtko.ursulin]

> On Tue, 2006-02-28 at 06:26 +0100, Herbert Poetzl wrote:
> Hi Andrew! Christoph! Al!
> 
> after thinking some time about the oracle words
> (sent in reply to previous BME submissions) we 
> (Sam and I) came to the conclusion that it would 
> be a good idea to remove the nameidata introduced
> in September 2003 from the inode permission()
> checks, so that vfs_permission() can take care
> of them ...

Could you please provide a link to that 'previous BME submissions'? 
Thanks.

Also, since you are modifying LSM interfaces, why not discuss it on the 
LSM mailing list?

And finally, please don't remove nameidata. Modules out there depend on it 
and we at Sophos are about to release a new product which needs it as 
well. The plan was to announce the whole thing parallel with the release, 
but after spotting your post I was prompted to react ahead of the 
schedule. However, I am very busy at the moment so the actual announcment 
with full details will have to wait for a week or two.

Re: [RFC] vfs: cleanup of permission()

[Arjan van de Ven]

> 
> And finally, please don't remove nameidata. Modules out there depend on it 

are those modules about to merged into the kernel? The current intent
infrastructure isn't fulfilling what it should do well, and from what
I've seen on the discussions it sounds that the best way forward is to
undo the current implementation and then roll out one which caters to
the needs of the existing users better.

As external module, you have little say so far simply because your usage
isn't visible. I'd urge you to quickly submit your code so that the
things you need from this are better visible to the people who are
thinking and working on the redesign.

> and we at Sophos are about to release a new product which needs it as 
> well. 

I assume we're talking about an open source product, or at least kernel
component, here?

Re: [RFC] vfs: cleanup of permission()

[tvrtko.ursulin]

Arjan van de Ven <arjan@infradead.org> wrote on 01/03/2006 12:37:51:

> > And finally, please don't remove nameidata. Modules out there depend 
on it 
> 
> are those modules about to merged into the kernel? The current intent

See third paragraph of my reply.

> infrastructure isn't fulfilling what it should do well, and from what
> I've seen on the discussions it sounds that the best way forward is to
> undo the current implementation and then roll out one which caters to
> the needs of the existing users better.

That I don't know so I can't comment at the moment. I haven't seen 
anything on linux-security-modules recently?
 
> As external module, you have little say so far simply because your usage
> isn't visible. I'd urge you to quickly submit your code so that the
> things you need from this are better visible to the people who are
> thinking and working on the redesign.

I know all that, but it is a complicated matter to discuss. That's why I 
was planning to make a comprehensive announcement which would discuss most 
of the hot topics. Ideally yes, I would like to merge, but it won't happen 
now. The first thing I would like to do is establish common ground with 
other security vendors so that we could approach the problem together. 
Personaly, I am not sure whether insisting that everything should be a 
part of kernel is a right thing to do even though I think I understand all 
the up- and down-sides of both policies.

Having said all this above, I am afraid that there will be no other choice 
but to start working on the announcement asap. :)
 
> > and we at Sophos are about to release a new product which needs it as 
> > well. 
> 
> I assume we're talking about an open source product, or at least kernel
> component, here?

Of course. Kernel component might be known to some as Talpa and it is 
released under GPL.

Re: [RFC] vfs: cleanup of permission()

[Sam Vilain]

tvrtko.ursulin@sophos.com wrote:
>>As external module, you have little say so far simply because your usage
>>isn't visible. I'd urge you to quickly submit your code so that the
>>things you need from this are better visible to the people who are
>>thinking and working on the redesign.
> I know all that, but it is a complicated matter to discuss. That's why I 
> was planning to make a comprehensive announcement which would discuss most 
> of the hot topics. Ideally yes, I would like to merge, but it won't happen 
> now. The first thing I would like to do is establish common ground with 
> other security vendors so that we could approach the problem together. 
> Personaly, I am not sure whether insisting that everything should be a 
> part of kernel is a right thing to do even though I think I understand all 
> the up- and down-sides of both policies.
> 
> Having said all this above, I am afraid that there will be no other choice 
> but to start working on the announcement asap. :)

Perhaps you can put up the work in progress on a git repository somewhere?

Sam.

Re: [RFC] vfs: cleanup of permission()

[Herbert Poetzl]

On Wed, Mar 01, 2006 at 12:28:25PM +0000, tvrtko.ursulin@sophos.com wrote:
> > On Tue, 2006-02-28 at 06:26 +0100, Herbert Poetzl wrote:
> > Hi Andrew! Christoph! Al!
> > 
> > after thinking some time about the oracle words
> > (sent in reply to previous BME submissions) we 
> > (Sam and I) came to the conclusion that it would 
> > be a good idea to remove the nameidata introduced
> > in September 2003 from the inode permission()
> > checks, so that vfs_permission() can take care
> > of them ...
> 
> Could you please provide a link to that 'previous BME submissions'?

here you go: http://lkml.org/lkml/2006/1/21/19

> Thanks.

you're welcome!

> Also, since you are modifying LSM interfaces, why not discuss it on
> the LSM mailing list?

no problem with that, will cc the lsm folks next time
(feel free to bounce the messages)

for now, here is a link to this thread:
  http://lkml.org/lkml/2006/2/28/4

> And finally, please don't remove nameidata. Modules out there depend
> on it and we at Sophos are about to release a new product which needs
> it as well. The plan was to announce the whole thing parallel with the
> release, but after spotting your post I was prompted to react ahead
> of the schedule. However, I am very busy at the moment so the actual
> announcment with full details will have to wait for a week or two.

thing is, permission() does inode based checks
and the nameidata is not even provided in most
cases, so you cannot rely on that information
anyway

it would probably be better to have some kind
of vfs_permission, which uses dentry/vfsmnt for
decisions on the vfs layer, this would also allow
to cover most of the cases where nameidata is not
available (for example the filep based stuff)

best,
Herbert

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

Re: [RFC] vfs: cleanup of permission()

[Sam Vilain]

tvrtko.ursulin@sophos.com wrote:
> Also, since you are modifying LSM interfaces, why not discuss it on the 
> LSM mailing list?
> 
> And finally, please don't remove nameidata. Modules out there depend on it 
> and we at Sophos are about to release a new product which needs it as 
> well. The plan was to announce the whole thing parallel with the release, 
> but after spotting your post I was prompted to react ahead of the 
> schedule. However, I am very busy at the moment so the actual announcment 
> with full details will have to wait for a week or two.

You are treating the per-FS security hooks as if they were VFS security 
hooks.  This was an easy mistake to make, as the appearance of a (struct 
nameidata*) sure makes it look like a VFS call.

However, most functions in the kernel don't pass anything in that 
nameidata slot.  Some (eg, syscalls that work on open FDs) can't, 
either.  So the fact that it does not guarantee VFS context information 
in all situations means permission() is not a VFS function.

ie, we don't disagree with what you're trying to do, but if you want 
path information then you should be working at the VFS layer, not the FS 
layer.

Perhaps you could first come up with a patch to the LSM base that adds 
VFS hooks rather than FS hooks and make your new system use those hooks? 
  I think that it might be more obvious where such hooks should go, 
after applying this patch.

What we're aiming for, on the permission() front, is that all system 
calls, ioctls, etc, call either vfs_permission() or file_permission(). 
Only those two functions should end up calling permission() directly.

Sam.

Re: [RFC] vfs: cleanup of permission()

[Herbert Poetzl]

On Wed, Mar 01, 2006 at 12:45:44AM -0800, Trond Myklebust wrote:
> On Tue, 2006-02-28 at 06:26 +0100, Herbert Poetzl wrote:
> > Hi Andrew! Christoph! Al!
> > 
> > after thinking some time about the oracle words
> > (sent in reply to previous BME submissions) we 
> > (Sam and I) came to the conclusion that it would 
> > be a good idea to remove the nameidata introduced
> > in September 2003 from the inode permission()
> > checks, so that vfs_permission() can take care
> > of them ...
> 
> Why? There may be perfectly legitimate reasons for the filesystem to
> request information about the path. I can think of server failover
> situations in NFSv4 where the client may need to look up the
> filehandle for the file on the new server before it can service the
> ACCESS call.

the second part is actually a hack to help nfs and fuse
to get the 'required' information until there is a proper
interface (at the vfs not inode level) to pass relevant
information (probably dentry/vfsmount/flags)

> > this is in two parts, the first one does the 
> > removal and the second one fixes up nfs and fuse
> > by passing the relevant nd_flags via the mask
> > 
> > Note: this is just a suggestion, so please let
> >       us know what you think 
> 
> Firstly, the fact that the lookup intent flags happen not to collide
> with MAY_* is a complete fluke, not a design. The numerical values of
> either set of flags could change tomorrow for all you know.
>
> Secondly, an intent is _not_ a permissions mask by any stretch of the
> imagination.

see above

> IOW: at the very least make that intent flag a separate parameter.

IMHO it would be good to remove them completely form the
current permission() checks.

best,
Herbert

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

Re: [RFC] vfs: cleanup of permission()

[Trond Myklebust]

On Wed, 2006-03-01 at 14:11 +0100, Herbert Poetzl wrote:

> the second part is actually a hack to help nfs and fuse
> to get the 'required' information until there is a proper
> interface (at the vfs not inode level) to pass relevant
> information (probably dentry/vfsmount/flags)

The nameidata _IS_ the vfs structure for storing path context
information. You seem to be suggesting we need yet another one. Why?

> > > this is in two parts, the first one does the 
> > > removal and the second one fixes up nfs and fuse
> > > by passing the relevant nd_flags via the mask
> > > 
> > > Note: this is just a suggestion, so please let
> > >       us know what you think 
> > 
> > Firstly, the fact that the lookup intent flags happen not to collide
> > with MAY_* is a complete fluke, not a design. The numerical values of
> > either set of flags could change tomorrow for all you know.
> >
> > Secondly, an intent is _not_ a permissions mask by any stretch of the
> > imagination.
> 
> see above
> 
> > IOW: at the very least make that intent flag a separate parameter.
> 
> IMHO it would be good to remove them completely form the
> current permission() checks.

Vetoed!

Redundant RPC calls have performance costs to the client, the server and
the network. That intent information is there in order to allow the
filesystem to figure out whether or not it needs to do the permissions
check, or if that check is already being done by other operations.

Removing the intents are therefore not an option.

Cheers,
  Trond

Re: [RFC] vfs: cleanup of permission()

[Sam Vilain]

Trond Myklebust wrote:
>>the second part is actually a hack to help nfs and fuse
>>to get the 'required' information until there is a proper
>>interface (at the vfs not inode level) to pass relevant
>>information (probably dentry/vfsmount/flags)
> The nameidata _IS_ the vfs structure for storing path context
> information. You seem to be suggesting we need yet another one. Why?

Because you can't make a nameidata without a lookup, and file based 
operations don't do a lookup.  However you still have the vfsmnt and 
inode hanging off the file struct.

Either that or we make a dummy nameidata structure for this situation, 
possibly a filehandle relative lookup as used by openat() et al.

>>>Secondly, an intent is _not_ a permissions mask by any stretch of the
>>>imagination.
>>see above
>>>IOW: at the very least make that intent flag a separate parameter.
>>IMHO it would be good to remove them completely form the
>>current permission() checks.
> Vetoed!
> Redundant RPC calls have performance costs to the client, the server and
> the network. That intent information is there in order to allow the
> filesystem to figure out whether or not it needs to do the permissions
> check, or if that check is already being done by other operations.
> Removing the intents are therefore not an option.

OK, so we either make it an extra parameter or 'properly' stack them 
into a single word.  Do you have any preferences either way there?

Sam.

Re: [RFC] vfs: cleanup of permission()

[Trond Myklebust]

On Thu, 2006-03-02 at 14:35 +1300, Sam Vilain wrote:
> Trond Myklebust wrote:
> >>the second part is actually a hack to help nfs and fuse
> >>to get the 'required' information until there is a proper
> >>interface (at the vfs not inode level) to pass relevant
> >>information (probably dentry/vfsmount/flags)
> > The nameidata _IS_ the vfs structure for storing path context
> > information. You seem to be suggesting we need yet another one. Why?
> 
> Because you can't make a nameidata without a lookup, and file based 
> operations don't do a lookup.  However you still have the vfsmnt and 
> inode hanging off the file struct.

Which is fine by NFS, at least. We don't care about performing
permissions checks in those cases anyway, so a NULL nameidata is OK. The
only cases where we currently need to take action in ->permission() are

 1) path walks (checking MAY_EXEC on the directory)
 2) open() (on NFSv2/v3 only)
 3) access()

In the future we can/should probably eliminate (2) by making an
atomic_open() for NFSv2/v3, so that we can correctly set the credential
that was used for the open() permissions check in the struct file.

> Either that or we make a dummy nameidata structure for this situation, 
> possibly a filehandle relative lookup as used by openat() et al.
> 
> >>>Secondly, an intent is _not_ a permissions mask by any stretch of the
> >>>imagination.
> >>see above
> >>>IOW: at the very least make that intent flag a separate parameter.
> >>IMHO it would be good to remove them completely form the
> >>current permission() checks.
> > Vetoed!
> > Redundant RPC calls have performance costs to the client, the server and
> > the network. That intent information is there in order to allow the
> > filesystem to figure out whether or not it needs to do the permissions
> > check, or if that check is already being done by other operations.
> > Removing the intents are therefore not an option.
> 
> OK, so we either make it an extra parameter or 'properly' stack them 
> into a single word.  Do you have any preferences either way there?

An extra parameter should suffice for (1) and (2).

Cheers,
  Trond

Re: [RFC] vfs: cleanup of permission()

[Sam Vilain]

Trond Myklebust wrote:
>>after thinking some time about the oracle words
>>(sent in reply to previous BME submissions) we 
>>(Sam and I) came to the conclusion that it would 
>>be a good idea to remove the nameidata introduced
>>in September 2003 from the inode permission()
>>checks, so that vfs_permission() can take care
>>of them ...
> Why? There may be perfectly legitimate reasons for the filesystem to
> request information about the path.

Part of what we're trying to achieve is clearly seperating VFS from FS 
operations.  A lot of the problems with supporting the per-vfsmnt 
options stemmed from this confusion.

In fact this was largely developed from your feedback on the prior 
thread¹ :) we audited the uses, and you were right - some places really 
couldn't set that field properly.  After some playing and discussion, we 
came to guess that this kind of cleanup was a good way to satisfy your 
concerns as well as Christoph's².

¹ http://xrl.us/j9et
² http://xrl.us/j9eo

The only place that the nameidata structure is currently used is in 
those two places, and given that it *does* appear to represent FS vs VFS 
brain damage to some extent, it's quite important we get this right 
before umpteen security modules spring up using this bad hook ... the 
API is broken (doesn't cover (struct file*)-based operations, only 
lookup (struct nameidata*), and parameter is optional).

> I can think of server failover
> situations in NFSv4 where the client may need to look up the filehandle
> for the file on the new server before it can service the ACCESS call.

The current code doesn't seem to use that right now, at least not via 
permission().  Either way, the information should be available - it just 
needs to hook in somewhere else.

> Firstly, the fact that the lookup intent flags happen not to collide
> with MAY_* is a complete fluke, not a design. The numerical values of
> either set of flags could change tomorrow for all you know.
 > Secondly, an intent is _not_ a permissions mask by any stretch of the
 > imagination.

Yeah, I had this in there at one point as a safety check;

#if (_LOOKUP_MASK ^ _ACCESS_MASK) != (_LOOKUP_MASK | _ACCESS_MASK)
#error splode
#endif

You are right, it should either be converted to two parameters or the 
combination made more formal.

> IOW: at the very least make that intent flag a separate parameter.

Yes, or rename it mask_and_intent perhaps if they were to be combined in 
the same word (which makes sense, with a grand total of 7 bits being 
used, and gcc not being in a position to spot that and optimise for us).

> 
> Cheers,
>   Trond

Thanks for your feedback!

Sam.