Re: [PATCH] Replace exec_permission_lite() with inlined vfs_permission()

From: Paul Menage (pmenage@ensim.com)
Date: Fri May 03 2002 - 04:03:47 EST


How about something like the patch below?

- i_op->permission() now takes an extra argument, "dcache_locked", and
should return -EAGAIN if it's called with the dcache_lock held and it
can't handle it.

- callers should use permission() or permission_locked() as appropriate

- some filesystems (sysctl, smbfs, coda ioctl) can ignore dcache_locked,
as their permission methods never doing anything involving blocking or
locks

- procfs root checks now rely on the dcache_lock to synchronise access
to current->fs->* and target_proc->fs->* and avoid taking reference
counts on dentries and mounts.

- nfs and coda give -EAGAIN if they would need to make an rpc call when
dcache_lock is held

- intermezzo gives up immediately if dcache_lock is held. Also fixed
horribly ugly abuse of i_op->permission.

If this is OK, let me know and I'll split it up into a few smaller
chunks and send it to Linus (and the relevant FS maintainers).

i_op->permission() also looks like an easy candidate for BKL removal.

Paul

diff -aur -X /mnt/elbrus/home/pmenage/dontdiff linux-2.5.13/Documentation/filesystems/Locking linux-2.5.13-permission/Documentation/filesystems/Locking
--- linux-2.5.13/Documentation/filesystems/Locking Thu May 2 17:22:48 2002
+++ linux-2.5.13-permission/Documentation/filesystems/Locking Fri May 3 00:54:17 2002
@@ -41,7 +41,7 @@
         int (*readlink) (struct dentry *, char *,int);
         int (*follow_link) (struct dentry *, struct nameidata *);
         void (*truncate) (struct inode *);
- int (*permission) (struct inode *, int);
+ int (*permission) (struct inode *, int, int dcache_locked);
         int (*revalidate) (struct dentry *);
         int (*setattr) (struct dentry *, struct iattr *);
         int (*getattr) (struct dentry *, struct iattr *);
@@ -66,7 +66,7 @@
 follow_link: no no
 truncate: no yes (see below)
 setattr: no yes
-permission: yes no
+permission: no (see below)
 getattr: (see below)
 revalidate: no (see below)
 setxattr: no yes
@@ -76,6 +76,7 @@
         Additionally, ->rmdir(), ->unlink() and ->rename() have ->i_sem on
 victim.
         cross-directory ->rename() has (per-superblock) ->s_vfs_rename_sem.
+ ->permission() has BKL if dcache_locked is 0, otherwise has dcache_lock
         ->revalidate(), it may be called both with and without the i_sem
 on dentry->d_inode.
         ->truncate() is never called directly - it's a callback, not a
diff -aur -X /mnt/elbrus/home/pmenage/dontdiff linux-2.5.13/Documentation/filesystems/vfs.txt linux-2.5.13-permission/Documentation/filesystems/vfs.txt
--- linux-2.5.13/Documentation/filesystems/vfs.txt Thu May 2 17:22:42 2002
+++ linux-2.5.13-permission/Documentation/filesystems/vfs.txt Fri May 3 00:53:22 2002
@@ -253,7 +253,7 @@
         int (*writepage) (struct file *, struct page *);
         int (*bmap) (struct inode *,int);
         void (*truncate) (struct inode *);
- int (*permission) (struct inode *, int);
+ int (*permission) (struct inode *, int, int);
         int (*smap) (struct inode *,int);
         int (*updatepage) (struct file *, struct page *, const char *,
                                 unsigned long, unsigned int, int);
diff -aur -X /mnt/elbrus/home/pmenage/dontdiff linux-2.5.13/fs/coda/dir.c linux-2.5.13-permission/fs/coda/dir.c
--- linux-2.5.13/fs/coda/dir.c Thu May 2 17:22:42 2002
+++ linux-2.5.13-permission/fs/coda/dir.c Fri May 3 01:40:27 2002
@@ -146,9 +146,8 @@
 }
 
 
-int coda_permission(struct inode *inode, int mask)
+int coda_permission(struct inode *inode, int mask, int dcache_locked)
 {
- umode_t mode = inode->i_mode;
         int error;
  
         if (!mask)
@@ -159,6 +158,9 @@
         if (coda_cache_check(inode, mask))
                 return 0;
 
+ if(dcache_locked)
+ return -EAGAIN;
+
         error = venus_access(inode->i_sb, coda_i2f(inode), mask);
     
         if (!error)
diff -aur -X /mnt/elbrus/home/pmenage/dontdiff linux-2.5.13/fs/coda/pioctl.c linux-2.5.13-permission/fs/coda/pioctl.c
--- linux-2.5.13/fs/coda/pioctl.c Thu May 2 17:22:55 2002
+++ linux-2.5.13-permission/fs/coda/pioctl.c Fri May 3 01:39:18 2002
@@ -24,25 +24,8 @@
 #include <linux/coda_fs_i.h>
 #include <linux/coda_psdev.h>
 
-/* pioctl ops */
-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);
-
-/* exported from this file */
-struct inode_operations coda_ioctl_inode_operations =
-{
- permission: coda_ioctl_permission,
- setattr: coda_notify_change,
-};
-
-struct file_operations coda_ioctl_operations = {
- owner: THIS_MODULE,
- ioctl: coda_pioctl,
-};
-
 /* the coda pioctl inode ops */
-static int coda_ioctl_permission(struct inode *inode, int mask)
+static int coda_ioctl_permission(struct inode *inode, int mask, int dcache_locked)
 {
         return 0;
 }
@@ -92,3 +75,15 @@
         return error;
 }
 
+/* exported from this file */
+struct inode_operations coda_ioctl_inode_operations =
+{
+ permission: coda_ioctl_permission,
+ setattr: coda_notify_change,
+};
+
+struct file_operations coda_ioctl_operations = {
+ owner: THIS_MODULE,
+ ioctl: coda_pioctl,
+};
+
diff -aur -X /mnt/elbrus/home/pmenage/dontdiff linux-2.5.13/fs/intermezzo/dir.c linux-2.5.13-permission/fs/intermezzo/dir.c
--- linux-2.5.13/fs/intermezzo/dir.c Thu May 2 17:22:45 2002
+++ linux-2.5.13-permission/fs/intermezzo/dir.c Fri May 3 01:39:18 2002
@@ -70,7 +70,7 @@
 /*
  * these are initialized in super.c
  */
-extern int presto_permission(struct inode *inode, int mask);
+extern int presto_permission(struct inode *inode, int mask, int dcache_locked);
 int presto_ilookup_uid = 0;
 
 extern int presto_prep(struct dentry *, struct presto_cache **,
@@ -782,12 +782,15 @@
  * appropriate permission function. Thus we do not worry here about ACLs
  * or EAs. -SHP
  */
-int presto_permission(struct inode *inode, int mask)
+int presto_permission(struct inode *inode, int mask, int dcache_locked)
 {
         unsigned short mode = inode->i_mode;
         struct presto_cache *cache;
         int rc;
 
+ if(dcache_locked)
+ return -EAGAIN;
+
         ENTRY;
         if ( presto_can_ilookup() && !(mask & S_IWOTH)) {
                 CDEBUG(D_CACHE, "ilookup on %ld OK\n", inode->i_ino);
@@ -804,23 +807,15 @@
 
                 if ( S_ISREG(mode) && fiops && fiops->permission ) {
                         EXIT;
- return fiops->permission(inode, mask);
+ return fiops->permission(inode, mask, dcache_locked);
                 }
                 if ( S_ISDIR(mode) && diops && diops->permission ) {
                         EXIT;
- return diops->permission(inode, mask);
+ return diops->permission(inode, mask, dcache_locked);
                 }
         }
 
- /* The cache filesystem doesn't have its own permission function,
- * but we don't want to duplicate the VFS code here. In order
- * to avoid looping from permission calling this function again,
- * we temporarily override the permission operation while we call
- * the VFS permission function.
- */
- inode->i_op->permission = NULL;
- rc = permission(inode, mask);
- inode->i_op->permission = &presto_permission;
+ rc = vfs_permission(inode, mask);
 
         EXIT;
         return rc;
diff -aur -X /mnt/elbrus/home/pmenage/dontdiff linux-2.5.13/fs/intermezzo/file.c linux-2.5.13-permission/fs/intermezzo/file.c
--- linux-2.5.13/fs/intermezzo/file.c Thu May 2 17:22:41 2002
+++ linux-2.5.13-permission/fs/intermezzo/file.c Fri May 3 01:39:18 2002
@@ -46,7 +46,7 @@
 /*
  * these are initialized in super.c
  */
-extern int presto_permission(struct inode *inode, int mask);
+extern int presto_permission(struct inode *inode, int mask, int dcache_locked);
 extern int presto_opendir_upcall(int minor, struct dentry *de, int async);
 
 extern int presto_prep(struct dentry *, struct presto_cache **,
diff -aur -X /mnt/elbrus/home/pmenage/dontdiff linux-2.5.13/fs/namei.c linux-2.5.13-permission/fs/namei.c
--- linux-2.5.13/fs/namei.c Thu May 2 18:57:38 2002
+++ linux-2.5.13-permission/fs/namei.c Fri May 3 01:39:18 2002
@@ -153,7 +153,7 @@
  * for filesystem access without changing the "normal" uids which
  * are used for other things..
  */
-int vfs_permission(struct inode * inode, int mask)
+inline int vfs_permission(struct inode * inode, int mask)
 {
         umode_t mode = inode->i_mode;
 
@@ -201,12 +201,19 @@
         return -EACCES;
 }
 
+static inline int permission_locked(struct inode *inode, int mask) {
+
+ if (inode->i_op && inode->i_op->permission)
+ return inode->i_op->permission(inode, mask, 1);
+ return vfs_permission(inode, mask);
+}
+
 int permission(struct inode * inode,int mask)
 {
         if (inode->i_op && inode->i_op->permission) {
                 int retval;
                 lock_kernel();
- retval = inode->i_op->permission(inode, mask);
+ retval = inode->i_op->permission(inode, mask, 0);
                 unlock_kernel();
                 return retval;
         }
@@ -300,40 +307,6 @@
 }
 
 /*
- * Short-cut version of permission(), for calling by
- * path_walk(), when dcache lock is held. Combines parts
- * of permission() and vfs_permission(), and tests ONLY for
- * MAY_EXEC permission.
- *
- * If appropriate, check DAC only. If not appropriate, or
- * short-cut DAC fails, then call permission() to do more
- * complete permission check.
- */
-static inline int exec_permission_lite(struct inode *inode)
-{
- umode_t mode = inode->i_mode;
-
- if ((inode->i_op && inode->i_op->permission))
- return -EAGAIN;
-
- if (current->fsuid == inode->i_uid)
- mode >>= 6;
- else if (in_group_p(inode->i_gid))
- mode >>= 3;
-
- if (mode & MAY_EXEC)
- return 0;
-
- if ((inode->i_mode & S_IXUGO) && capable(CAP_DAC_OVERRIDE))
- return 0;
-
- if (S_ISDIR(inode->i_mode) && capable(CAP_DAC_READ_SEARCH))
- return 0;
-
- return -EACCES;
-}
-
-/*
  * This is called when everything else fails, and we actually have
  * to go to the low-level filesystem to find out what we should do..
  *
@@ -578,7 +551,7 @@
                 struct qstr this;
                 unsigned int c;
 
- err = exec_permission_lite(inode);
+ err = permission_locked(inode, MAY_EXEC);
                 if (err == -EAGAIN) {
                         unlock_nd(nd);
                         err = permission(inode, MAY_EXEC);
diff -aur -X /mnt/elbrus/home/pmenage/dontdiff linux-2.5.13/fs/nfs/dir.c linux-2.5.13-permission/fs/nfs/dir.c
--- linux-2.5.13/fs/nfs/dir.c Thu May 2 17:22:53 2002
+++ linux-2.5.13-permission/fs/nfs/dir.c Fri May 3 01:39:18 2002
@@ -1100,7 +1100,7 @@
 }
 
 int
-nfs_permission(struct inode *inode, int mask)
+nfs_permission(struct inode *inode, int mask, int dcache_locked)
 {
         int error = vfs_permission(inode, mask);
 
@@ -1120,6 +1120,10 @@
             && (current->fsuid != 0) && (current->fsgid != 0)
             && error != -EACCES)
                 goto out;
+
+ error = -EAGAIN;
+ if (dcache_locked)
+ goto out;
 
         error = NFS_PROTO(inode)->access(inode, mask, 0);
 
diff -aur -X /mnt/elbrus/home/pmenage/dontdiff linux-2.5.13/fs/proc/base.c linux-2.5.13-permission/fs/proc/base.c
--- linux-2.5.13/fs/proc/base.c Thu May 2 17:22:40 2002
+++ linux-2.5.13-permission/fs/proc/base.c Fri May 3 01:45:57 2002
@@ -180,16 +180,23 @@
         return result;
 }
 
-static int proc_root_link(struct inode *inode, struct dentry **dentry, struct vfsmount **mnt)
-{
- struct fs_struct *fs;
- int result = -ENOENT;
+static struct fs_struct *get_task_fs(struct inode *inode) {
+
+ struct fs_struct *fs = NULL;
         task_lock(proc_task(inode));
         fs = proc_task(inode)->fs;
         if(fs)
                 atomic_inc(&fs->count);
         task_unlock(proc_task(inode));
- if (fs) {
+ return fs;
+}
+
+static int proc_root_link(struct inode *inode, struct dentry **dentry, struct vfsmount **mnt)
+{
+ int result = -ENOENT;
+ struct fs_struct *fs = get_task_fs(inode);
+
+ if(fs) {
                 read_lock(&fs->lock);
                 *mnt = mntget(fs->rootmnt);
                 *dentry = dget(fs->root);
@@ -262,20 +269,24 @@
 
 /* permission checks */
 
-static int proc_check_root(struct inode *inode)
+static int proc_check_root(struct inode *inode, int locked)
 {
         struct dentry *de, *base, *root;
         struct vfsmount *our_vfsmnt, *vfsmnt, *mnt;
- int res = 0;
+ struct fs_struct *fs = get_task_fs(inode);
+ int res = -EACCES;
 
- if (proc_root_link(inode, &root, &vfsmnt)) /* Ewww... */
+ if (!fs)
                 return -ENOENT;
- read_lock(&current->fs->lock);
- our_vfsmnt = mntget(current->fs->rootmnt);
- base = dget(current->fs->root);
- read_unlock(&current->fs->lock);
+
+ if(!locked)
+ spin_lock(&dcache_lock);
+
+ root = fs->root;
+ vfsmnt = fs->rootmnt;
+ our_vfsmnt = current->fs->rootmnt;
+ base = current->fs->root;
 
- spin_lock(&dcache_lock);
         de = root;
         mnt = vfsmnt;
 
@@ -288,25 +299,24 @@
 
         if (!is_subdir(de, base))
                 goto out;
- spin_unlock(&dcache_lock);
 
-exit:
- dput(base);
- mntput(our_vfsmnt);
- dput(root);
- mntput(mnt);
+ res = 0;
+
+ out:
+ if(!locked)
+ spin_unlock(&dcache_lock);
+
+ put_fs_struct(fs);
+
         return res;
-out:
- spin_unlock(&dcache_lock);
- res = -EACCES;
- goto exit;
 }
 
-static int proc_permission(struct inode *inode, int mask)
+static int proc_permission(struct inode *inode, int mask, int dcache_locked)
 {
- if (vfs_permission(inode, mask) != 0)
- return -EACCES;
- return proc_check_root(inode);
+ int ret;
+ if ((ret = vfs_permission(inode, mask)) != 0)
+ return ret;
+ return proc_check_root(inode, dcache_locked);
 }
 
 static ssize_t pid_maps_read(struct file * file, char * buf,
@@ -534,7 +544,7 @@
 
         if (current->fsuid != inode->i_uid && !capable(CAP_DAC_OVERRIDE))
                 goto out;
- error = proc_check_root(inode);
+ error = proc_check_root(inode, 0);
         if (error)
                 goto out;
 
@@ -576,7 +586,7 @@
 
         if (current->fsuid != inode->i_uid && !capable(CAP_DAC_OVERRIDE))
                 goto out;
- error = proc_check_root(inode);
+ error = proc_check_root(inode, 0);
         if (error)
                 goto out;
 
diff -aur -X /mnt/elbrus/home/pmenage/dontdiff linux-2.5.13/fs/smbfs/file.c linux-2.5.13-permission/fs/smbfs/file.c
--- linux-2.5.13/fs/smbfs/file.c Thu May 2 17:22:56 2002
+++ linux-2.5.13-permission/fs/smbfs/file.c Fri May 3 01:39:18 2002
@@ -369,7 +369,7 @@
  * privileges, so we need our own check for this.
  */
 static int
-smb_file_permission(struct inode *inode, int mask)
+smb_file_permission(struct inode *inode, int mask, int dcache_locked)
 {
         int mode = inode->i_mode;
         int error = 0;
diff -aur -X /mnt/elbrus/home/pmenage/dontdiff linux-2.5.13/include/linux/coda_linux.h linux-2.5.13-permission/include/linux/coda_linux.h
--- linux-2.5.13/include/linux/coda_linux.h Thu May 2 17:22:43 2002
+++ linux-2.5.13-permission/include/linux/coda_linux.h Fri May 3 01:20:32 2002
@@ -38,7 +38,7 @@
 int coda_open(struct inode *i, struct file *f);
 int coda_flush(struct file *f);
 int coda_release(struct inode *i, struct file *f);
-int coda_permission(struct inode *inode, int mask);
+int coda_permission(struct inode *inode, int mask, int dcache_locked);
 int coda_revalidate_inode(struct dentry *);
 int coda_notify_change(struct dentry *, struct iattr *);
 int coda_isnullfid(ViceFid *fid);
diff -aur -X /mnt/elbrus/home/pmenage/dontdiff linux-2.5.13/include/linux/dcache.h linux-2.5.13-permission/include/linux/dcache.h
--- linux-2.5.13/include/linux/dcache.h Thu May 2 18:30:16 2002
+++ linux-2.5.13-permission/include/linux/dcache.h Thu May 2 21:53:55 2002
@@ -86,6 +86,7 @@
 
 struct dentry_operations {
         int (*d_revalidate)(struct dentry *, int);
+ int (*d_revalidate_locked)(struct dentry *, int);
         int (*d_hash) (struct dentry *, struct qstr *);
         int (*d_compare) (struct dentry *, struct qstr *, struct qstr *);
         int (*d_delete)(struct dentry *);
diff -aur -X /mnt/elbrus/home/pmenage/dontdiff linux-2.5.13/include/linux/fs.h linux-2.5.13-permission/include/linux/fs.h
--- linux-2.5.13/include/linux/fs.h Thu May 2 18:30:16 2002
+++ linux-2.5.13-permission/include/linux/fs.h Fri May 3 01:07:25 2002
@@ -756,7 +756,7 @@
         int (*readlink) (struct dentry *, char *,int);
         int (*follow_link) (struct dentry *, struct nameidata *);
         void (*truncate) (struct inode *);
- int (*permission) (struct inode *, int);
+ int (*permission) (struct inode *, int, int);
         int (*revalidate) (struct dentry *);
         int (*setattr) (struct dentry *, struct iattr *);
         int (*getattr) (struct dentry *, struct iattr *);
diff -aur -X /mnt/elbrus/home/pmenage/dontdiff linux-2.5.13/include/linux/nfs_fs.h linux-2.5.13-permission/include/linux/nfs_fs.h
--- linux-2.5.13/include/linux/nfs_fs.h Thu May 2 21:55:39 2002
+++ linux-2.5.13-permission/include/linux/nfs_fs.h Fri May 3 01:19:52 2002
@@ -237,7 +237,7 @@
                                 struct nfs_fattr *);
 extern int __nfs_refresh_inode(struct inode *, struct nfs_fattr *);
 extern int nfs_revalidate(struct dentry *);
-extern int nfs_permission(struct inode *, int);
+extern int nfs_permission(struct inode *, int, int);
 extern int nfs_open(struct inode *, struct file *);
 extern int nfs_release(struct inode *, struct file *);
 extern int __nfs_revalidate_inode(struct nfs_server *, struct inode *);
diff -aur -X /mnt/elbrus/home/pmenage/dontdiff linux-2.5.13/kernel/sysctl.c linux-2.5.13-permission/kernel/sysctl.c
--- linux-2.5.13/kernel/sysctl.c Thu May 2 17:22:41 2002
+++ linux-2.5.13-permission/kernel/sysctl.c Fri May 3 01:21:04 2002
@@ -122,7 +122,7 @@
 
 static ssize_t proc_readsys(struct file *, char *, size_t, loff_t *);
 static ssize_t proc_writesys(struct file *, const char *, size_t, loff_t *);
-static int proc_sys_permission(struct inode *, int);
+static int proc_sys_permission(struct inode *, int, int);
 
 struct file_operations proc_sys_file_operations = {
         read: proc_readsys,
@@ -701,7 +701,7 @@
         return do_rw_proc(1, file, (char *) buf, count, ppos);
 }
 
-static int proc_sys_permission(struct inode *inode, int op)
+static int proc_sys_permission(struct inode *inode, int op, int dcache_locked)
 {
         return test_perm(inode->i_mode, op);
 }

-
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/



This archive was generated by hypermail 2b29 : Tue May 07 2002 - 22:00:19 EST