From: Miklos Szeredi <mszeredi@suse.cz>

Like most other inode operations ->permission() should take a dentry
instead of an inode.  This is necessary for filesystems which operate
on names not on inodes.

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
---
 fs/afs/internal.h                  |    2 +-
 fs/afs/security.c                  |    3 ++-
 fs/bad_inode.c                     |    2 +-
 fs/btrfs/inode.c                   |    4 +++-
 fs/btrfs/ioctl.c                   |    8 ++++----
 fs/ceph/inode.c                    |    3 ++-
 fs/ceph/super.h                    |    2 +-
 fs/cifs/cifsfs.c                   |    3 ++-
 fs/coda/dir.c                      |    3 ++-
 fs/coda/pioctl.c                   |    4 ++--
 fs/ecryptfs/inode.c                |    4 ++--
 fs/fuse/dir.c                      |    3 ++-
 fs/gfs2/ops_inode.c                |   11 ++++++++---
 fs/hostfs/hostfs_kern.c            |    3 ++-
 fs/logfs/dir.c                     |    6 ------
 fs/namei.c                         |   37 ++++++++++++++++++++-----------------
 fs/namespace.c                     |    2 +-
 fs/nfs/dir.c                       |    3 ++-
 fs/nfsd/nfsfh.c                    |    2 +-
 fs/nfsd/vfs.c                      |    4 ++--
 fs/nilfs2/nilfs.h                  |    2 +-
 fs/notify/fanotify/fanotify_user.c |    2 +-
 fs/notify/inotify/inotify_user.c   |    2 +-
 fs/ocfs2/file.c                    |    3 ++-
 fs/ocfs2/file.h                    |    2 +-
 fs/ocfs2/refcounttree.c            |    4 ++--
 fs/open.c                          |   10 +++++-----
 fs/proc/base.c                     |    3 ++-
 fs/proc/proc_sysctl.c              |    3 ++-
 fs/reiserfs/xattr.c                |    4 +++-
 fs/smbfs/file.c                    |    4 ++--
 fs/sysfs/inode.c                   |    3 ++-
 fs/sysfs/sysfs.h                   |    2 +-
 fs/utimes.c                        |    2 +-
 fs/xattr.c                         |   12 +++++++-----
 include/linux/coda_linux.h         ...
[ message continues ]

I may be missing something but I looked at your patch series and I see
no good reason for this patch at all. You just churned a lot of code for
something that you don't even have a need for in the patch set. Your
only two new callers of this function could just as easily have used the
inode since it isn't doing anything special with the dentry. It actually
pulls the inode out of it and uses it in generic_permission and
security_inode_permission. If you are going to change this you should
also change generic_permission as well. Honestly I'd rather see the
dentry requirement removed from inode operations instead but
unfortunately this isn't possible as I found out with my attempts to
remove the dentry requirement for get/setxattr


--

[ message continues ]

On Thu, 26 Aug 2010 16:24:02 -0400


union_permission needs the dentry to get access to d_fsdata, which caches the
upperpath and lowerpath which were found at lookup time.

Is that what you missed?

NeilBrown
--

[ message continues ]

You're correct I missed the line where that was being pulled out of the
dentry. The better question for me would be why do it this way as
opposed to what the union file systems are doing. If neither UnionFS or
AUFS are having to make this change so I'd like a much better
explination for this change. I'm not seeing enough information in the
form of why he designed the prototype this way to justify a change that
the other implementations don't seem to need.

Dave

--

[ message continues ]

For reference, union mounts needs something like this.  The current
patch set creates a inode_permission() variant for use in the VFS in
cases where we may copy up.  The superblock of the target is read-only
and we have to ignore that - if the topmost parent directory is
unioned.

Here's the patch, which I suspect needs to be rewritten:

commit 72a84672ccf2a923bac120e3ac75ac106d36b4ae
Author: Valerie Aurora <vaurora@redhat.com>
Date:   Sat Mar 20 17:34:53 2010 -0700

    VFS: Split inode_permission() and create path_permission()
    
    Split inode_permission() into inode and file-system-dependent parts.
    Create path_permission() to check permission based on the path to the
    inode.  This is for union mounts, in which an inode can be located on
    a read-only lower layer file system but is still writable, since we
    will copy it up to the writable top layer file system.  So in that
    case, we want to ignore MS_RDONLY on the lower layer.  To make this
    decision, we must know the path (vfsmount, dentry) of both the target
    and its parent.
    
    XXX - so ugly!

diff --git a/fs/namei.c b/fs/namei.c
index 1358a89..2b2bf41 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -241,29 +241,20 @@ int generic_permission(struct inode *inode, int mask,
 }
 
 /**
- * inode_permission  -  check for access rights to a given inode
+ * __inode_permission  -  check for access rights to a given inode
  * @inode:	inode to check permission on
  * @mask:	right to check for (%MAY_READ, %MAY_WRITE, %MAY_EXEC)
  *
  * Used to check for read/write/execute permissions on an inode.
- * We use "fsuid" for this, letting us set arbitrary permissions
- * for filesystem access without changing the "normal" uids which
- * are used for other things.
+ *
+ * This does not check for a read-only file system.  You probably want
+ * inode_permission().
  */
-int inode_permission(struct inode *inode, int mask)
+static int __inode_permission(struct inode *inode, int mask)
 {
 	int ...
[ message continues ]

It seems that your reply to [0/5] has the description that I was asking
for in my last response.


--

[ message continues ]