[PATCH 0/3] Fix regression in NFSv3 ACL setting

[PATCH 0/3] Fix regression in NFSv3 ACL setting

[NeilBrown]

Commit 013cdf1088d7 ("nfs: use generic posix ACL infrastructure for v3
Posix ACLs") introduce a regression for NFSv3 ACL setting.
An owner should be able to set an ACL, but the new code tests for
ownership in a way that is not reliable for NFSv3.  For NFSv3 the only
reliable test is to send the request to the server and see if it works.

The first patch introduces MAY_ACT_AS_OWNER and relies on the
filesystem to do the appropriate ownership test.  This touches
several filesystems, hence the long 'Cc' list.
Following two patches are small code cleanups relating to this.

Thanks,
NeilBrown


---

NeilBrown (3):
      VFS: introduce MAY_ACT_AS_OWNER
      VFS: allow MAY_ flags to be easily extended.
      NFSD - Use MAY_ACT_AS_OWNER


 fs/afs/security.c  |   10 ++++++++++
 fs/attr.c          |   12 +++++-------
 fs/coda/dir.c      |   10 ++++++++++
 fs/fcntl.c         |    2 +-
 fs/fuse/dir.c      |   10 ++++++++++
 fs/namei.c         |    9 +++++++++
 fs/nfs/dir.c       |    8 ++++++++
 fs/nfsd/vfs.c      |   11 ++++++-----
 fs/nfsd/vfs.h      |   33 ++++++++++++++++++---------------
 fs/posix_acl.c     |    2 +-
 fs/xattr.c         |    2 +-
 include/linux/fs.h |   10 ++++++++++
 12 files changed, 89 insertions(+), 30 deletions(-)

--
Signature

[PATCH 1/3] VFS: introduce MAY_ACT_AS_OWNER

[NeilBrown]

A few places in VFS, particularly set_posix_acl(), use
inode_owner_or_capable() to check if the caller has "owner"
access to the inode.
This assumes that it is valid to test inode->i_uid, which is not
always the case.  Particularly in the case of NFS it is not valid to
us i_uid (or i_mode) for permission tests - the server needs to make
the decision.

As a result if the server is remapping uids
(e.g. all-squash,anon_uid=1000),
then all users should have ownership access, but most users will not
be able to set acls.

This patch moves the ownership test to inode_permission and
i_op->permission.
A new flag for these functions, MAY_ACT_AS_OWNER is introduced.
generic_permission() now handles this correctly and many
i_op->permission functions call this function() and don't need any
changes.  A few are changed to handle MAY_ACT_AS_OWNER exactly
as generic_permission() does, using inode_owner_or_capable().
For these filesystems, no behavioural change should be noticed.

For NFS, nfs_permission is changed to always return 0 (success) if
MAY_ACT_AS_OWNER.  For NFS, any operations which use this flag should
be sent to the server, and the server will succeed or fail as
appropriate.

Fixes: 013cdf1088d7 ("nfs: use generic posix ACL infrastructure for v3 Posix ACLs")
Signed-off-by: NeilBrown <neilb@suse.com>
---
 fs/afs/security.c  |   10 ++++++++++
 fs/attr.c          |   12 +++++-------
 fs/coda/dir.c      |   10 ++++++++++
 fs/fcntl.c         |    2 +-
 fs/fuse/dir.c      |   10 ++++++++++
 fs/namei.c         |    9 +++++++++
 fs/nfs/dir.c       |    8 ++++++++
 fs/posix_acl.c     |    2 +-
 fs/xattr.c         |    2 +-
 include/linux/fs.h |    8 ++++++++
 10 files changed, 63 insertions(+), 10 deletions(-)

diff --git a/fs/afs/security.c b/fs/afs/security.c
index 81dfedb7879f..ac2e39de8bff 100644
--- a/fs/afs/security.c
+++ b/fs/afs/security.c
@@ -349,6 +349,16 @@ int afs_permission(struct inode *inode, int mask)
 	if (mask & MAY_NOT_BLOCK)
 		return -ECHILD;
 
+	/* Short-circuit for owner */
+	if (mask & MAY_ACT_AS_OWNER) {
+		if (inode_owner_or_capable(inode))
+			return 0;
+		mask &= ~MAY_ACT_AS_OWNER;
+		if (!mask)
+			/* No other permission will suffice */
+			return -EACCES;
+	}
+
 	_enter("{{%x:%u},%lx},%x,",
 	       vnode->fid.vid, vnode->fid.vnode, vnode->flags, mask);
 
diff --git a/fs/attr.c b/fs/attr.c
index d22e8187477f..c1160bd9416b 100644
--- a/fs/attr.c
+++ b/fs/attr.c
@@ -87,7 +87,7 @@ int setattr_prepare(struct dentry *dentry, struct iattr *attr)
 
 	/* Make sure a caller can chmod. */
 	if (ia_valid & ATTR_MODE) {
-		if (!inode_owner_or_capable(inode))
+		if (inode_permission(inode, MAY_ACT_AS_OWNER) < 0)
 			return -EPERM;
 		/* Also check the setgid bit! */
 		if (!in_group_p((ia_valid & ATTR_GID) ? attr->ia_gid :
@@ -98,7 +98,7 @@ int setattr_prepare(struct dentry *dentry, struct iattr *attr)
 
 	/* Check for setting the inode time. */
 	if (ia_valid & (ATTR_MTIME_SET | ATTR_ATIME_SET | ATTR_TIMES_SET)) {
-		if (!inode_owner_or_capable(inode))
+		if (inode_permission(inode, MAY_ACT_AS_OWNER) < 0)
 			return -EPERM;
 	}
 
@@ -246,11 +246,9 @@ int notify_change(struct dentry * dentry, struct iattr * attr, struct inode **de
 		if (IS_IMMUTABLE(inode))
 			return -EPERM;
 
-		if (!inode_owner_or_capable(inode)) {
-			error = inode_permission(inode, MAY_WRITE);
-			if (error)
-				return error;
-		}
+		error = inode_permission(inode, MAY_ACT_AS_OWNER | MAY_WRITE);
+		if (error)
+			return error;
 	}
 
 	if ((ia_valid & ATTR_MODE)) {
diff --git a/fs/coda/dir.c b/fs/coda/dir.c
index 00876ddadb43..7e31f68d4973 100644
--- a/fs/coda/dir.c
+++ b/fs/coda/dir.c
@@ -80,6 +80,16 @@ int coda_permission(struct inode *inode, int mask)
 	if (mask & MAY_NOT_BLOCK)
 		return -ECHILD;
 
+	/* Short-circuit for owner */
+	if (mask & MAY_ACT_AS_OWNER) {
+		if (inode_owner_or_capable(inode))
+			return 0;
+		mask &= ~MAY_ACT_AS_OWNER;
+		if (!mask)
+			/* No other permission will suffice */
+			return -EACCES;
+	}
+
 	mask &= MAY_READ | MAY_WRITE | MAY_EXEC;
  
 	if (!mask)
diff --git a/fs/fcntl.c b/fs/fcntl.c
index 4137d96534a6..cc1d51150584 100644
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -46,7 +46,7 @@ static int setfl(int fd, struct file * filp, unsigned long arg)
 
 	/* O_NOATIME can only be set by the owner or superuser */
 	if ((arg & O_NOATIME) && !(filp->f_flags & O_NOATIME))
-		if (!inode_owner_or_capable(inode))
+		if (inode_permission(inode, MAY_ACT_AS_OWNER) < 0)
 			return -EPERM;
 
 	/* required for strict SunOS emulation */
diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index 0979609d6eba..3ff5a8f3a086 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -1118,6 +1118,16 @@ static int fuse_permission(struct inode *inode, int mask)
 	if (!fuse_allow_current_process(fc))
 		return -EACCES;
 
+	/* Short-circuit for owner */
+	if (mask & MAY_ACT_AS_OWNER) {
+		if (inode_owner_or_capable(inode))
+			return 0;
+		mask &= ~MAY_ACT_AS_OWNER;
+		if (!mask)
+			/* No other permission will suffice */
+			return -EACCES;
+	}
+
 	/*
 	 * If attributes are needed, refresh them before proceeding
 	 */
diff --git a/fs/namei.c b/fs/namei.c
index 0cab6494978c..a033a0f5c284 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -335,6 +335,15 @@ int generic_permission(struct inode *inode, int mask)
 {
 	int ret;
 
+	/* Short-circuit for owner */
+	if (mask & MAY_ACT_AS_OWNER) {
+		if (inode_owner_or_capable(inode))
+			return 0;
+		mask &= ~MAY_ACT_AS_OWNER;
+		if (!mask)
+			/* No other permission will suffice */
+			return -EACCES;
+	}
 	/*
 	 * Do the basic permission checks.
 	 */
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 8bfaa658b2c1..1fe54374b7c9 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -2524,6 +2524,14 @@ int nfs_permission(struct inode *inode, int mask)
 
 	nfs_inc_stats(inode, NFSIOS_VFSACCESS);
 
+	/* Short-circuit for owner */
+	if (mask & MAY_ACT_AS_OWNER)
+		/*
+		 * Ownership will be tested by server when we
+		 * actually try operation.
+		 */
+		return 0;
+
 	if ((mask & (MAY_READ | MAY_WRITE | MAY_EXEC)) == 0)
 		goto out;
 	/* Is this sys_access() ? */
diff --git a/fs/posix_acl.c b/fs/posix_acl.c
index 2fd0fde16fe1..a90c7dca892a 100644
--- a/fs/posix_acl.c
+++ b/fs/posix_acl.c
@@ -863,7 +863,7 @@ set_posix_acl(struct inode *inode, int type, struct posix_acl *acl)
 
 	if (type == ACL_TYPE_DEFAULT && !S_ISDIR(inode->i_mode))
 		return acl ? -EACCES : 0;
-	if (!inode_owner_or_capable(inode))
+	if (inode_permission(inode, MAY_ACT_AS_OWNER) < 0)
 		return -EPERM;
 
 	if (acl) {
diff --git a/fs/xattr.c b/fs/xattr.c
index daa732550088..78faa09a577b 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -126,7 +126,7 @@ xattr_permission(struct inode *inode, const char *name, int mask)
 		if (!S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode))
 			return (mask & MAY_WRITE) ? -EPERM : -ENODATA;
 		if (S_ISDIR(inode->i_mode) && (inode->i_mode & S_ISVTX) &&
-		    (mask & MAY_WRITE) && !inode_owner_or_capable(inode))
+		    (mask & MAY_WRITE) && inode_permission(inode, MAY_ACT_AS_OWNER) < 0)
 			return -EPERM;
 	}
 
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 551cbc5574d7..5a8878a88cbb 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -94,6 +94,14 @@ typedef int (dio_iodone_t)(struct kiocb *iocb, loff_t offset,
 #define MAY_CHDIR		0x00000040
 /* called from RCU mode, don't block */
 #define MAY_NOT_BLOCK		0x00000080
+/*
+ * File Owner is always allowed to perform pending
+ * operation.  If current user is an owner, or if
+ * filesystem performs permission check at time-of-operation,
+ * then succeed, else require some other permission
+ * if listed.
+ */
+#define MAY_ACT_AS_OWNER	0x00000100
 
 /*
  * flags in file.f_mode.  Note that FMODE_READ and FMODE_WRITE must correspond

[PATCH 2/3] VFS: allow MAY_ flags to be easily extended.

[NeilBrown]

NFSD uses permission flags similar to the MAY_* flags,
with some overlap, and depends on the overlap matching.
This is currently a little fragile and hard to extend.

So add __MAY_UNUSED to identify the first unused flag,
and have NFSD use that flag and later flags for its
non-standard permissions.

Signed-off-by: NeilBrown <neilb@suse.com>
---
 fs/nfsd/vfs.h      |   33 ++++++++++++++++++---------------
 include/linux/fs.h |    2 ++
 2 files changed, 20 insertions(+), 15 deletions(-)

diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
index a7e107309f76..2b1c70d3757a 100644
--- a/fs/nfsd/vfs.h
+++ b/fs/nfsd/vfs.h
@@ -13,23 +13,26 @@
  * Flags for nfsd_permission
  */
 #define NFSD_MAY_NOP			0
-#define NFSD_MAY_EXEC			0x001 /* == MAY_EXEC */
-#define NFSD_MAY_WRITE			0x002 /* == MAY_WRITE */
-#define NFSD_MAY_READ			0x004 /* == MAY_READ */
-#define NFSD_MAY_SATTR			0x008
-#define NFSD_MAY_TRUNC			0x010
-#define NFSD_MAY_LOCK			0x020
-#define NFSD_MAY_MASK			0x03f
+#define NFSD_MAY_EXEC			MAY_EXEC
+#define NFSD_MAY_WRITE			MAY_WRITE
+#define NFSD_MAY_READ			MAY_READ
+#define NFSD_MAY_SATTR			(__MAY_UNUSED << 0)
+#define NFSD_MAY_TRUNC			(__MAY_UNUSED << 1)
+#define NFSD_MAY_LOCK			(__MAY_UNUSED << 2)
+#define __NFSD_MAY_FIRST_HINT		(__MAY_UNUSED << 3)
+#define NFSD_MAY_MASK			(__NFSD_MAY_FIRST_HINT - 1)
 
 /* extra hints to permission and open routines: */
-#define NFSD_MAY_OWNER_OVERRIDE		0x040
-#define NFSD_MAY_LOCAL_ACCESS		0x080 /* for device special files */
-#define NFSD_MAY_BYPASS_GSS_ON_ROOT	0x100
-#define NFSD_MAY_NOT_BREAK_LEASE	0x200
-#define NFSD_MAY_BYPASS_GSS		0x400
-#define NFSD_MAY_READ_IF_EXEC		0x800
-
-#define NFSD_MAY_64BIT_COOKIE		0x1000 /* 64 bit readdir cookies for >= NFSv3 */
+#define NFSD_MAY_OWNER_OVERRIDE		(__NFSD_MAY_FIRST_HINT << 0)
+/* for device special files */
+#define NFSD_MAY_LOCAL_ACCESS		(__NFSD_MAY_FIRST_HINT << 1)
+#define NFSD_MAY_BYPASS_GSS_ON_ROOT	(__NFSD_MAY_FIRST_HINT << 2)
+#define NFSD_MAY_NOT_BREAK_LEASE	(__NFSD_MAY_FIRST_HINT << 3)
+#define NFSD_MAY_BYPASS_GSS		(__NFSD_MAY_FIRST_HINT << 4)
+#define NFSD_MAY_READ_IF_EXEC		(__NFSD_MAY_FIRST_HINT << 5)
+
+/* 64 bit readdir cookies for >= NFSv3 */
+#define NFSD_MAY_64BIT_COOKIE		(__NFSD_MAY_FIRST_HINT << 6)
 
 #define NFSD_MAY_CREATE		(NFSD_MAY_EXEC|NFSD_MAY_WRITE)
 #define NFSD_MAY_REMOVE		(NFSD_MAY_EXEC|NFSD_MAY_WRITE|NFSD_MAY_TRUNC)
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 5a8878a88cbb..b0076cb0f738 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -103,6 +103,8 @@ typedef int (dio_iodone_t)(struct kiocb *iocb, loff_t offset,
  */
 #define MAY_ACT_AS_OWNER	0x00000100
 
+#define	__MAY_UNUSED		0x00000100
+
 /*
  * flags in file.f_mode.  Note that FMODE_READ and FMODE_WRITE must correspond
  * to O_WRONLY and O_RDWR via the strange trick in do_dentry_open()

[PATCH 3/3] NFSD - Use MAY_ACT_AS_OWNER

[NeilBrown]

The NFSD_MAY_OWNER_OVERRIDE has exactly the same meaning
as the new MAY_ACT_AS_OWNER flag, so simplify the
code by making use of the identity.

The move NFSD_MAY_OWNER_OVERRIDE into NFSD_MAY_MASK, but that
is not a problem is it is always set together with a flag
that is already in the MASK.

Signed-off-by: NeilBrown <neilb@suse.com>
---
 fs/nfsd/vfs.c |   11 ++++++-----
 fs/nfsd/vfs.h |   14 +++++++-------
 2 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 55a099e47ba2..d89d23e6e2fe 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -2038,12 +2038,13 @@ nfsd_permission(struct svc_rqst *rqstp, struct svc_export *exp,
 	 * We must trust the client to do permission checking - using "ACCESS"
 	 * with NFSv3.
 	 */
-	if ((acc & NFSD_MAY_OWNER_OVERRIDE) &&
-	    uid_eq(inode->i_uid, current_fsuid()))
-		return 0;
 
-	/* This assumes  NFSD_MAY_{READ,WRITE,EXEC} == MAY_{READ,WRITE,EXEC} */
-	err = inode_permission(inode, acc & (MAY_READ|MAY_WRITE|MAY_EXEC));
+	/*
+	 * This works as NFSD_MAY_{READ,WRITE,EXEC} == MAY_{READ,WRITE,EXEC}
+	 * and NFSD_MAY_OWNER_OVERRIDE == MAY_ACT_AS_OWNER
+	 */
+	err = inode_permission(inode, (acc & (MAY_READ|MAY_WRITE|
+					      MAY_EXEC|MAY_ACT_AS_OWNER)));
 
 	/* Allow read access to binaries even when mode 111 */
 	if (err == -EACCES && S_ISREG(inode->i_mode) &&
diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
index 2b1c70d3757a..f6e96dba76a5 100644
--- a/fs/nfsd/vfs.h
+++ b/fs/nfsd/vfs.h
@@ -16,6 +16,7 @@
 #define NFSD_MAY_EXEC			MAY_EXEC
 #define NFSD_MAY_WRITE			MAY_WRITE
 #define NFSD_MAY_READ			MAY_READ
+#define NFSD_MAY_OWNER_OVERRIDE		MAY_ACT_AS_OWNER
 #define NFSD_MAY_SATTR			(__MAY_UNUSED << 0)
 #define NFSD_MAY_TRUNC			(__MAY_UNUSED << 1)
 #define NFSD_MAY_LOCK			(__MAY_UNUSED << 2)
@@ -23,16 +24,15 @@
 #define NFSD_MAY_MASK			(__NFSD_MAY_FIRST_HINT - 1)
 
 /* extra hints to permission and open routines: */
-#define NFSD_MAY_OWNER_OVERRIDE		(__NFSD_MAY_FIRST_HINT << 0)
 /* for device special files */
-#define NFSD_MAY_LOCAL_ACCESS		(__NFSD_MAY_FIRST_HINT << 1)
-#define NFSD_MAY_BYPASS_GSS_ON_ROOT	(__NFSD_MAY_FIRST_HINT << 2)
-#define NFSD_MAY_NOT_BREAK_LEASE	(__NFSD_MAY_FIRST_HINT << 3)
-#define NFSD_MAY_BYPASS_GSS		(__NFSD_MAY_FIRST_HINT << 4)
-#define NFSD_MAY_READ_IF_EXEC		(__NFSD_MAY_FIRST_HINT << 5)
+#define NFSD_MAY_LOCAL_ACCESS		(__NFSD_MAY_FIRST_HINT << 0)
+#define NFSD_MAY_BYPASS_GSS_ON_ROOT	(__NFSD_MAY_FIRST_HINT << 1)
+#define NFSD_MAY_NOT_BREAK_LEASE	(__NFSD_MAY_FIRST_HINT << 2)
+#define NFSD_MAY_BYPASS_GSS		(__NFSD_MAY_FIRST_HINT << 3)
+#define NFSD_MAY_READ_IF_EXEC		(__NFSD_MAY_FIRST_HINT << 4)
 
 /* 64 bit readdir cookies for >= NFSv3 */
-#define NFSD_MAY_64BIT_COOKIE		(__NFSD_MAY_FIRST_HINT << 6)
+#define NFSD_MAY_64BIT_COOKIE		(__NFSD_MAY_FIRST_HINT << 5)
 
 #define NFSD_MAY_CREATE		(NFSD_MAY_EXEC|NFSD_MAY_WRITE)
 #define NFSD_MAY_REMOVE		(NFSD_MAY_EXEC|NFSD_MAY_WRITE|NFSD_MAY_TRUNC)

[PATCH 2/3 v2] VFS: allow MAY_ flags to be easily extended.

[NeilBrown]

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


NFSD uses permission flags similar to the MAY_* flags,
with some overlap, and depends on the overlap matching.
This is currently a little fragile and hard to extend.

So add __MAY_UNUSED to identify the first unused flag,
and have NFSD use that flag and later flags for its
non-standard permissions.

Signed-off-by: NeilBrown <neilb@suse.com>
---

v1 of this patch had an obvious bug which, of course, I couldn't see
until after posting.
__MAY_UNUSED had the same value as MAY_ACT_AS_OWNER - so it wasn't
unused!

NeilBrown


 fs/nfsd/vfs.h      | 33 ++++++++++++++++++---------------
 include/linux/fs.h |  2 ++
 2 files changed, 20 insertions(+), 15 deletions(-)

diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
index a7e107309f76..2b1c70d3757a 100644
--- a/fs/nfsd/vfs.h
+++ b/fs/nfsd/vfs.h
@@ -13,23 +13,26 @@
  * Flags for nfsd_permission
  */
 #define NFSD_MAY_NOP			0
-#define NFSD_MAY_EXEC			0x001 /* == MAY_EXEC */
-#define NFSD_MAY_WRITE			0x002 /* == MAY_WRITE */
-#define NFSD_MAY_READ			0x004 /* == MAY_READ */
-#define NFSD_MAY_SATTR			0x008
-#define NFSD_MAY_TRUNC			0x010
-#define NFSD_MAY_LOCK			0x020
-#define NFSD_MAY_MASK			0x03f
+#define NFSD_MAY_EXEC			MAY_EXEC
+#define NFSD_MAY_WRITE			MAY_WRITE
+#define NFSD_MAY_READ			MAY_READ
+#define NFSD_MAY_SATTR			(__MAY_UNUSED << 0)
+#define NFSD_MAY_TRUNC			(__MAY_UNUSED << 1)
+#define NFSD_MAY_LOCK			(__MAY_UNUSED << 2)
+#define __NFSD_MAY_FIRST_HINT		(__MAY_UNUSED << 3)
+#define NFSD_MAY_MASK			(__NFSD_MAY_FIRST_HINT - 1)
 
 /* extra hints to permission and open routines: */
-#define NFSD_MAY_OWNER_OVERRIDE		0x040
-#define NFSD_MAY_LOCAL_ACCESS		0x080 /* for device special files */
-#define NFSD_MAY_BYPASS_GSS_ON_ROOT	0x100
-#define NFSD_MAY_NOT_BREAK_LEASE	0x200
-#define NFSD_MAY_BYPASS_GSS		0x400
-#define NFSD_MAY_READ_IF_EXEC		0x800
-
-#define NFSD_MAY_64BIT_COOKIE		0x1000 /* 64 bit readdir cookies for >= NFSv3 */
+#define NFSD_MAY_OWNER_OVERRIDE		(__NFSD_MAY_FIRST_HINT << 0)
+/* for device special files */
+#define NFSD_MAY_LOCAL_ACCESS		(__NFSD_MAY_FIRST_HINT << 1)
+#define NFSD_MAY_BYPASS_GSS_ON_ROOT	(__NFSD_MAY_FIRST_HINT << 2)
+#define NFSD_MAY_NOT_BREAK_LEASE	(__NFSD_MAY_FIRST_HINT << 3)
+#define NFSD_MAY_BYPASS_GSS		(__NFSD_MAY_FIRST_HINT << 4)
+#define NFSD_MAY_READ_IF_EXEC		(__NFSD_MAY_FIRST_HINT << 5)
+
+/* 64 bit readdir cookies for >= NFSv3 */
+#define NFSD_MAY_64BIT_COOKIE		(__NFSD_MAY_FIRST_HINT << 6)
 
 #define NFSD_MAY_CREATE		(NFSD_MAY_EXEC|NFSD_MAY_WRITE)
 #define NFSD_MAY_REMOVE		(NFSD_MAY_EXEC|NFSD_MAY_WRITE|NFSD_MAY_TRUNC)
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 5a8878a88cbb..9fc914c259f9 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -103,6 +103,8 @@ typedef int (dio_iodone_t)(struct kiocb *iocb, loff_t offset,
  */
 #define MAY_ACT_AS_OWNER	0x00000100
 
+#define	__MAY_UNUSED		0x00000200
+
 /*
  * flags in file.f_mode.  Note that FMODE_READ and FMODE_WRITE must correspond
  * to O_WRONLY and O_RDWR via the strange trick in do_dentry_open()
-- 
2.14.0.rc0.dirty


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 1/3] VFS: introduce MAY_ACT_AS_OWNER

[David Howells]

NeilBrown <neilb@suse.com> wrote:

> diff --git a/fs/afs/security.c b/fs/afs/security.c
> index 81dfedb7879f..ac2e39de8bff 100644
> --- a/fs/afs/security.c
> +++ b/fs/afs/security.c
> @@ -349,6 +349,16 @@ int afs_permission(struct inode *inode, int mask)
>  	if (mask & MAY_NOT_BLOCK)
>  		return -ECHILD;
>  
> +	/* Short-circuit for owner */
> +	if (mask & MAY_ACT_AS_OWNER) {
> +		if (inode_owner_or_capable(inode))

You don't know that inode->i_uid in meaningful.  You may have noticed that
afs_permission() ignores i_uid and i_gid entirely.  It queries the server (if
this information is not otherwise cached) to ask what permits the user is
granted - where the user identity is defined by the key returned from
afs_request_key()[*].

So, NAK for the afs piece.

David

[*] If there's no appropriate key, anonymous permits will be used.

Re: [PATCH 1/3] VFS: introduce MAY_ACT_AS_OWNER

[Jan Harkes]

Same for Coda.

uid/gid/mode don't mean anything, access is based on the directory ACL and the authentication token that is held by the userspace cache manager and ultimately decided by the servers.

Unless someone broke this recently and made permission checks uid based I would expect no change. If this is broken by a recent commit I expect something similar to what NFS is trying to do by allowing the actual check to be passed down.

Jan

On October 4, 2018 10:10:13 AM EDT, David Howells <dhowells@redhat.com> wrote:
>NeilBrown <neilb@suse.com> wrote:
>
>> diff --git a/fs/afs/security.c b/fs/afs/security.c
>> index 81dfedb7879f..ac2e39de8bff 100644
>> --- a/fs/afs/security.c
>> +++ b/fs/afs/security.c
>> @@ -349,6 +349,16 @@ int afs_permission(struct inode *inode, int
>mask)
>>  	if (mask & MAY_NOT_BLOCK)
>>  		return -ECHILD;
>>  
>> +	/* Short-circuit for owner */
>> +	if (mask & MAY_ACT_AS_OWNER) {
>> +		if (inode_owner_or_capable(inode))
>
>You don't know that inode->i_uid in meaningful.  You may have noticed
>that
>afs_permission() ignores i_uid and i_gid entirely.  It queries the
>server (if
>this information is not otherwise cached) to ask what permits the user
>is
>granted - where the user identity is defined by the key returned from
>afs_request_key()[*].
>
>So, NAK for the afs piece.
>
>David
>
>[*] If there's no appropriate key, anonymous permits will be used.

Re: [PATCH 1/3] VFS: introduce MAY_ACT_AS_OWNER

[NeilBrown]

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

On Thu, Oct 04 2018, David Howells wrote:

> NeilBrown <neilb@suse.com> wrote:
>
>> diff --git a/fs/afs/security.c b/fs/afs/security.c
>> index 81dfedb7879f..ac2e39de8bff 100644
>> --- a/fs/afs/security.c
>> +++ b/fs/afs/security.c
>> @@ -349,6 +349,16 @@ int afs_permission(struct inode *inode, int mask)
>>  	if (mask & MAY_NOT_BLOCK)
>>  		return -ECHILD;
>>  
>> +	/* Short-circuit for owner */
>> +	if (mask & MAY_ACT_AS_OWNER) {
>> +		if (inode_owner_or_capable(inode))
>
> You don't know that inode->i_uid in meaningful.  You may have noticed that
> afs_permission() ignores i_uid and i_gid entirely.  It queries the server (if
> this information is not otherwise cached) to ask what permits the user is
> granted - where the user identity is defined by the key returned from
> afs_request_key()[*].
>
> So, NAK for the afs piece.

Thanks for the review.
As afs doesn't use the generic xattr code and doesn't call
setattr_prepare(), this is all largely irrelevant for afs.

afs_permission() will probably only get MAY_ACT_AS_OWNER passed when
someone uses fcntl(F_SETFL) to set the O_NOATIME flag.
Currently a permission test based on UID is performed which, as you say,
is wrong.  My patch simply preserved this current (wrong) behaviour.
Shall I change it to always allow access, like with NFS?
Probably O_NOATIME is ignored, in which case f_op->check_flags should
probably report -EINVAL (???) ... or might that cause a regression?

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 1/3] VFS: introduce MAY_ACT_AS_OWNER

[NeilBrown]

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

On Thu, Oct 04 2018, Jan Harkes wrote:

> Same for Coda.
>
> uid/gid/mode don't mean anything, access is based on the directory ACL and the authentication token that is held by the userspace cache manager and ultimately decided by the servers.
>
> Unless someone broke this recently and made permission checks uid based I would expect no change. If this is broken by a recent commit I expect something similar to what NFS is trying to do by allowing the actual check to be passed down.

As with afs, the only permission check I can find that is uid based and
which actually affects coda is the check for use fcntl(F_SETFL) to set
O_NOATIME. I suspect that is irrelevant for coda.
I'll resubmit with the same code for both NFS and code - and probably
AFS.

Thanks,
NeilBrown


>
> Jan
>
> On October 4, 2018 10:10:13 AM EDT, David Howells <dhowells@redhat.com> wrote:
>>NeilBrown <neilb@suse.com> wrote:
>>
>>> diff --git a/fs/afs/security.c b/fs/afs/security.c
>>> index 81dfedb7879f..ac2e39de8bff 100644
>>> --- a/fs/afs/security.c
>>> +++ b/fs/afs/security.c
>>> @@ -349,6 +349,16 @@ int afs_permission(struct inode *inode, int
>>mask)
>>>  	if (mask & MAY_NOT_BLOCK)
>>>  		return -ECHILD;
>>>  
>>> +	/* Short-circuit for owner */
>>> +	if (mask & MAY_ACT_AS_OWNER) {
>>> +		if (inode_owner_or_capable(inode))
>>
>>You don't know that inode->i_uid in meaningful.  You may have noticed
>>that
>>afs_permission() ignores i_uid and i_gid entirely.  It queries the
>>server (if
>>this information is not otherwise cached) to ask what permits the user
>>is
>>granted - where the user identity is defined by the key returned from
>>afs_request_key()[*].
>>
>>So, NAK for the afs piece.
>>
>>David
>>
>>[*] If there's no appropriate key, anonymous permits will be used.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 1/3] VFS: introduce MAY_ACT_AS_OWNER

[David Howells]

NeilBrown <neilb@suse.com> wrote:

> Thanks for the review.
> As afs doesn't use the generic xattr code and doesn't call
> setattr_prepare(), this is all largely irrelevant for afs.

Yeah - there's no xattr support yet.

> afs_permission() will probably only get MAY_ACT_AS_OWNER passed when
> someone uses fcntl(F_SETFL) to set the O_NOATIME flag.

There's no atime in AFS.

> Currently a permission test based on UID is performed which, as you say,
> is wrong.  My patch simply preserved this current (wrong) behaviour.
> Shall I change it to always allow access, like with NFS?

You have to have an appropriate key to be able to do anything not granted to
anonymous with the server.  If the server says your key (or lack thereof) is
allowed to do something, you can do it; if it does not, you can't.

> Probably O_NOATIME is ignored, in which case f_op->check_flags should
> probably report -EINVAL (???) ... or might that cause a regression?

No atime.  I just ignore things like O_NOATIME.  You will be able to query the
filesystem with fsinfo() hopefully soon to find out if there is an atime and
if you can disable it.

Davod