2021-05-13 23:06:40

by Dai Ngo

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

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::[email protected]: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 <[email protected]>
---
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;
return err;
}

--
2.9.5

2021-05-19 20:21:35

by Trond Myklebust

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

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::[email protected]: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 <[email protected]>
> ---
>  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
[email protected]