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::[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
#
v2: detect NFS4ERR_BADOWNER and NFS4ERR_BADNAME and skip retry
in nfs4_proc_set_acl.
Signed-off-by: Dai Ngo <[email protected]>
---
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);
--
2.9.5
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::[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
> #
>
> v2: detect NFS4ERR_BADOWNER and NFS4ERR_BADNAME and skip retry
> in nfs4_proc_set_acl.
> Signed-off-by: Dai Ngo <[email protected]>
> ---
> 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, [email protected] 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::[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
> > #
> >
> > v2: detect NFS4ERR_BADOWNER and NFS4ERR_BADNAME and skip retry
> > in nfs4_proc_set_acl.
> > Signed-off-by: Dai Ngo <[email protected]>
> > ---
> > 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
[email protected]
On 5/28/21 10:41 AM, Trond Myklebust wrote:
> On Fri, 2021-05-28 at 10:30 -0700, [email protected] 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::[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
>>> #
>>>
>>> v2: detect NFS4ERR_BADOWNER and NFS4ERR_BADNAME and skip retry
>>> in nfs4_proc_set_acl.
>>> Signed-off-by: Dai Ngo <[email protected]>
>>> ---
>>> 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.
>