| Message ID | 20210513183555.28230-1-dai.ngo@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/1] NFSv4: nfs4_proc_set_acl needs to restore NFS_CAP_UIDGID_NOMAP on error. | expand |
On Thu, 2021-05-13 at 14:35 -0400, Dai Ngo wrote: > 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; > } >
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; return err; }
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(-)