| Message ID | 20210519211510.60253-1-dai.ngo@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/1] NFSv4: nfs4_proc_set_acl needs to restore NFS_CAP_UIDGID_NOMAP on error. | expand |
Hi Trond, Just a reminder that this patch is ready for your review. 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);
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.
On 5/28/21 10:41 AM, Trond Myklebust wrote: > 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. it's ok. It can wait for the next pull. Thanks, -Dai > >> 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. >
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);
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(+)