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