Thread (2 messages) 2 messages, 2 authors, 2021-05-19

Re: [PATCH 1/1] NFSv4: nfs4_proc_set_acl needs to restore NFS_CAP_UIDGID_NOMAP on error.

From: Trond Myklebust <hidden>
Date: 2021-05-19 19:22:08

On Thu, 2021-05-13 at 14:35 -0400, Dai Ngo wrote:
quoted hunk ↗ jump to hunk
Currently if __nfs4_proc_set_acl fails with NFS4ERR_BADOWNER it
re-enables the idmapper by clearing NFS_CAP_UIDGID_NOMAP before
retrying again. The NFS_CAP_UIDGID_NOMAP remains cleared even if
the retry fails. This causes problem for subsequent setattr
requests for v4 server that does not have idmapping configured.

Steps to reproduce the problem:

 # mount -o vers=4.1,sec=sys server:/export/test /tmp/mnt
 # touch /tmp/mnt/file1
 # chown 99 /tmp/mnt/file1
 # nfs4_setfacl -a A::unknown.user@xyz.com:wrtncy /tmp/mnt/file1
 Failed setxattr operation: Invalid argument
 # chown 99 /tmp/mnt/file1
 chown: changing ownership of ‘/tmp/mnt/file1’: Invalid argument
 # umount /tmp/mnt
 # mount -o vers=4.1,sec=sys server:/export/test /tmp/mnt
 # chown 99 /tmp/mnt/file1
 #

Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
---
 fs/nfs/nfs4proc.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index c65c4b41e2c1..e12630e3bb7c 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -5926,13 +5926,20 @@ static int __nfs4_proc_set_acl(struct inode
*inode, const void *buf, size_t bufl
 static int nfs4_proc_set_acl(struct inode *inode, const void *buf,
size_t buflen)
 {
        struct nfs4_exception exception = { };
-       int err;
+       struct nfs_server *server = NFS_SERVER(inode);
+       int err, nomap;
+
+       nomap = server->caps & NFS_CAP_UIDGID_NOMAP;
        do {
                err = __nfs4_proc_set_acl(inode, buf, buflen);
                trace_nfs4_set_acl(inode, err);
                err = nfs4_handle_exception(NFS_SERVER(inode), err,
                                &exception);
        } while (exception.retry);
+
+       /* if retry still fails then restore NFS_CAP_UIDGID_NOMAP
setting */
+       if (err && nomap)
+               server->caps |= NFS_CAP_UIDGID_NOMAP;
If the server returns NFS4ERR_BADOWNER or NFS4ERR_BADNAME, why even
call nfs4_handle_exception()? There is nothing the kernel can do about
it, and changing the value of NFS_CAP_UIDGID_NOMAP isn't going to help
because the kernel isn't involved in encoding the ACEs.

IOW: Why not just exit with an appropriate error (-EINVAL perhaps?)
immediately.
        return err;
 }
 
-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help