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

[Trond Myklebust <trondmy@hammerspace.com>]

On Fri, 2021-05-28 at 10:30 -0700, dai.ngo@oracle.com wrote:
> Hi Trond,
> 
> Just a reminder that this patch is ready for your review.

Sorry. I missed that update for some reason.

> 
> Thanks,
> -Dai
> 
> On 5/19/21 2:15 PM, 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.
> > 
> > This patch modifies nfs4_proc_set_acl to detect NFS4ERR_BADOWNER
> > and NFS4ERR_BADNAME and skips the retry, since the kernel isn't
> > involved in encoding the ACEs, and return -EINVAL.
> > 
> > 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
> >   #
> > 
> > v2: detect NFS4ERR_BADOWNER and NFS4ERR_BADNAME and skip retry
> >         in nfs4_proc_set_acl.
> > Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
> > ---
> >   fs/nfs/nfs4proc.c | 8 ++++++++
> >   1 file changed, 8 insertions(+)
> > 
> > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> > index 87d04f2c9385..4e052c7f0614 100644
> > --- a/fs/nfs/nfs4proc.c
> > +++ b/fs/nfs/nfs4proc.c
> > @@ -5968,6 +5968,14 @@ static int nfs4_proc_set_acl(struct inode
> > *inode, const void *buf, size_t buflen
> >         do {
> >                 err = __nfs4_proc_set_acl(inode, buf, buflen);
> >                 trace_nfs4_set_acl(inode, err);
> > +               if (err == -NFS4ERR_BADOWNER || err == -
> > NFS4ERR_BADNAME) {
> > +                       /*
> > +                        * no need to retry since the kernel
> > +                        * isn't involved in encoding the ACEs.
> > +                        */
> > +                       err = -EINVAL;
> > +                       break;
> > +               }
> >                 err = nfs4_handle_exception(NFS_SERVER(inode), err,
> >                                 &exception);
> >         } while (exception.retry);

Yes, this looks OK.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com