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
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 <[email protected]>
---
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
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 <[email protected]>
---
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()
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 <[email protected]>
---
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)
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 <[email protected]>
---
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
NeilBrown <[email protected]> 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.
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 <[email protected]> wrote:
>NeilBrown <[email protected]> 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.
On Thu, Oct 04 2018, David Howells wrote:
> NeilBrown <[email protected]> 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
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 <[email protected]> wrote:
>>NeilBrown <[email protected]> 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.
NeilBrown <[email protected]> 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