2023-06-08 15:12:40

by Mkrtchyan, Tigran

[permalink] [raw]
Subject: [PATCH] nfs4: don't map EACCESS and EPERM to EIO

the nfs4_map_errors function converts NFS specific errors to userland
errors. However, it ignores NFS4ERR_PERM and EPERM, which then get
mapped to EIO.

Signed-off-by: Tigran Mkrtchyan <[email protected]>
---
fs/nfs/nfs4proc.c | 2 ++
1 file changed, 2 insertions(+)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index d3665390c4cb..795205fe4f30 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -171,12 +171,14 @@ static int nfs4_map_errors(int err)
case -NFS4ERR_LAYOUTTRYLATER:
case -NFS4ERR_RECALLCONFLICT:
return -EREMOTEIO;
+ case -NFS4ERR_PERM:
case -NFS4ERR_WRONGSEC:
case -NFS4ERR_WRONG_CRED:
return -EPERM;
case -NFS4ERR_BADOWNER:
case -NFS4ERR_BADNAME:
return -EINVAL;
+ case -NFS4ERR_ACCESS:
case -NFS4ERR_SHARE_DENIED:
return -EACCES;
case -NFS4ERR_MINOR_VERS_MISMATCH:
--
2.40.1



2023-06-08 15:43:56

by Trond Myklebust

[permalink] [raw]
Subject: Re: [PATCH] nfs4: don't map EACCESS and EPERM to EIO

Hi Tigran,

On Thu, 2023-06-08 at 16:49 +0200, Tigran Mkrtchyan wrote:
> the nfs4_map_errors function converts NFS specific errors to userland
> errors. However, it ignores NFS4ERR_PERM and EPERM, which then get
> mapped to EIO.
>
> Signed-off-by: Tigran Mkrtchyan <[email protected]>
> ---
>  fs/nfs/nfs4proc.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index d3665390c4cb..795205fe4f30 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -171,12 +171,14 @@ static int nfs4_map_errors(int err)
>         case -NFS4ERR_LAYOUTTRYLATER:
>         case -NFS4ERR_RECALLCONFLICT:
>                 return -EREMOTEIO;
> +       case -NFS4ERR_PERM:
>         case -NFS4ERR_WRONGSEC:
>         case -NFS4ERR_WRONG_CRED:
>                 return -EPERM;
>         case -NFS4ERR_BADOWNER:
>         case -NFS4ERR_BADNAME:
>                 return -EINVAL;
> +       case -NFS4ERR_ACCESS:
>         case -NFS4ERR_SHARE_DENIED:
>                 return -EACCES;
>         case -NFS4ERR_MINOR_VERS_MISMATCH:

Hmm... Aren't both these cases covered by the exception at the top of
the function?

static int nfs4_map_errors(int err)
{
if (err >= -1000)
return err;

As I read it, that should mean that err = -NFS4ERR_ACCESS (= -13) and
err = -NFS4ERR_PERM (= -1) will get returned verbatim.

Are you seeing these NFS4ERR_ACCESS and NFS4ERR_PERM cases hitting the
default: dprintk() when you turn it on?

--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
[email protected]


2023-06-08 18:02:56

by Trond Myklebust

[permalink] [raw]
Subject: Re: [PATCH] nfs4: don't map EACCESS and EPERM to EIO

On Thu, 2023-06-08 at 19:42 +0200, Tigran Mkrtchyan wrote:
> Hi Trond,
>
> I will check and let you know. What we see is EACCESS on layoutget
> reported as EIO to the applications
>

If this is for a write, then that might just be
nfs_mapping_set_error(). In newer kernels, it tries to avoid sending
errors that are unexpected for strictly POSIX applications.

Cheers
Trond

> Best regards,
> Tigran
>
>
> On June 8, 2023 5:33:16 PM GMT+02:00, Trond Myklebust
> <[email protected]> wrote:
> > Hi Tigran,
> >
> > On Thu, 2023-06-08 at 16:49 +0200, Tigran Mkrtchyan wrote:
> > > the nfs4_map_errors function converts NFS specific errors to
> > > userland
> > > errors. However, it ignores NFS4ERR_PERM and EPERM, which then
> > > get
> > > mapped to EIO.
> > >
> > > Signed-off-by: Tigran Mkrtchyan
> > > <[email protected]> fs/nfs/nfs4proc.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> > > index d3665390c4cb..795205fe4f30 100644
> > > --- a/fs/nfs/nfs4proc.c
> > > +++ b/fs/nfs/nfs4proc.c
> > > @@ -171,12 +171,14 @@ static int nfs4_map_errors(int err)
> > >         case -NFS4ERR_LAYOUTTRYLATER:
> > >         case -NFS4ERR_RECALLCONFLICT:
> > >                 return -EREMOTEIO;
> > > +       case -NFS4ERR_PERM:
> > >         case -NFS4ERR_WRONGSEC:
> > >         case -NFS4ERR_WRONG_CRED:
> > >                 return -EPERM;
> > >         case -NFS4ERR_BADOWNER:
> > >         case -NFS4ERR_BADNAME:
> > >                 return -EINVAL;
> > > +       case -NFS4ERR_ACCESS:
> > >         case -NFS4ERR_SHARE_DENIED:
> > >                 return -EACCES;
> > >         case -NFS4ERR_MINOR_VERS_MISMATCH:
> > >
> >
> > Hmm... Aren't both these cases covered by the exception at the top
> > of
> > the function?
> >
> > static int nfs4_map_errors(int err)
> > {
> >         if (err >= -1000)
> >                 return err;
> >
> > As I read it, that should mean that err = -NFS4ERR_ACCESS (= -13)
> > and
> > err = -NFS4ERR_PERM (= -1) will get returned verbatim.
> >
> > Are you seeing these NFS4ERR_ACCESS and NFS4ERR_PERM cases hitting
> > the
> > default: dprintk() when you turn it on?
> >

--
Trond Myklebust
CTO, Hammerspace Inc
1900 S Norfolk St, Suite 350 - #45
San Mateo, CA 94403

http://www.hammerspace.com

2023-06-09 13:33:58

by Mkrtchyan, Tigran

[permalink] [raw]
Subject: Re: [PATCH] nfs4: don't map EACCESS and EPERM to EIO


Hi Trond,

Obviously, the patch is incorrect. The behavior of the upstream kernel and
RHEL kernels are different.

Sorry for the noise,
Tigran.


----- Original Message -----
> From: "Trond Myklebust" <[email protected]>
> To: [email protected], "Tigran Mkrtchyan" <[email protected]>
> Cc: "linux-nfs" <[email protected]>
> Sent: Thursday, 8 June, 2023 19:53:07
> Subject: Re: [PATCH] nfs4: don't map EACCESS and EPERM to EIO

> On Thu, 2023-06-08 at 19:42 +0200, Tigran Mkrtchyan wrote:
>> Hi Trond,
>>
>> I will check and let you know. What we see is EACCESS on layoutget
>> reported as EIO to the applications
>>
>
> If this is for a write, then that might just be
> nfs_mapping_set_error(). In newer kernels, it tries to avoid sending
> errors that are unexpected for strictly POSIX applications.
>
> Cheers
> Trond
>
>> Best regards,
>> Tigran
>>
>>
>> On June 8, 2023 5:33:16 PM GMT+02:00, Trond Myklebust
>> <[email protected]> wrote:
>> > Hi Tigran,
>> >
>> > On Thu, 2023-06-08 at 16:49 +0200, Tigran Mkrtchyan wrote:
>> > > the nfs4_map_errors function converts NFS specific errors to
>> > > userland
>> > > errors. However, it ignores NFS4ERR_PERM and EPERM, which then
>> > > get
>> > > mapped to EIO.
>> > >
>> > > Signed-off-by: Tigran Mkrtchyan
>> > > <[email protected]> fs/nfs/nfs4proc.c | 2 ++
>> > >  1 file changed, 2 insertions(+)
>> > >
>> > > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>> > > index d3665390c4cb..795205fe4f30 100644
>> > > --- a/fs/nfs/nfs4proc.c
>> > > +++ b/fs/nfs/nfs4proc.c
>> > > @@ -171,12 +171,14 @@ static int nfs4_map_errors(int err)
>> > >         case -NFS4ERR_LAYOUTTRYLATER:
>> > >         case -NFS4ERR_RECALLCONFLICT:
>> > >                 return -EREMOTEIO;
>> > > +       case -NFS4ERR_PERM:
>> > >         case -NFS4ERR_WRONGSEC:
>> > >         case -NFS4ERR_WRONG_CRED:
>> > >                 return -EPERM;
>> > >         case -NFS4ERR_BADOWNER:
>> > >         case -NFS4ERR_BADNAME:
>> > >                 return -EINVAL;
>> > > +       case -NFS4ERR_ACCESS:
>> > >         case -NFS4ERR_SHARE_DENIED:
>> > >                 return -EACCES;
>> > >         case -NFS4ERR_MINOR_VERS_MISMATCH:
>> > >
>> >
>> > Hmm... Aren't both these cases covered by the exception at the top
>> > of
>> > the function?
>> >
>> > static int nfs4_map_errors(int err)
>> > {
>> >         if (err >= -1000)
>> >                 return err;
>> >
>> > As I read it, that should mean that err = -NFS4ERR_ACCESS (= -13)
>> > and
>> > err = -NFS4ERR_PERM (= -1) will get returned verbatim.
>> >
>> > Are you seeing these NFS4ERR_ACCESS and NFS4ERR_PERM cases hitting
>> > the
>> > default: dprintk() when you turn it on?
>> >
>
> --
> Trond Myklebust
> CTO, Hammerspace Inc
> 1900 S Norfolk St, Suite 350 - #45
> San Mateo, CA 94403
>
> http://www.hammerspace.com


Attachments:
smime.p7s (2.16 kB)
S/MIME Cryptographic Signature

2023-06-09 14:08:48

by Benjamin Coddington

[permalink] [raw]
Subject: Re: [PATCH] nfs4: don't map EACCESS and EPERM to EIO

On 9 Jun 2023, at 9:30, Mkrtchyan, Tigran wrote:

> Hi Trond,
>
> Obviously, the patch is incorrect. The behavior of the upstream kernel and
> RHEL kernels are different.

RHEL-9 should be ok here.

There's a few things we need to be fixing for RHEL-8.9. This is one of them.
https://bugzilla.redhat.com/show_bug.cgi?id=2213828

Ben


2023-06-13 08:38:27

by Mkrtchyan, Tigran

[permalink] [raw]
Subject: Re: [PATCH] nfs4: don't map EACCESS and EPERM to EIO

Thanks, Ben!

Tigran.

----- Original Message -----
> From: "Benjamin Coddington" <[email protected]>
> To: "Tigran Mkrtchyan" <[email protected]>
> Cc: "Trond Myklebust" <[email protected]>, "anna" <[email protected]>, "linux-nfs" <[email protected]>
> Sent: Friday, 9 June, 2023 16:00:34
> Subject: Re: [PATCH] nfs4: don't map EACCESS and EPERM to EIO

> On 9 Jun 2023, at 9:30, Mkrtchyan, Tigran wrote:
>
>> Hi Trond,
>>
>> Obviously, the patch is incorrect. The behavior of the upstream kernel and
>> RHEL kernels are different.
>
> RHEL-9 should be ok here.
>
> There's a few things we need to be fixing for RHEL-8.9. This is one of them.
> https://bugzilla.redhat.com/show_bug.cgi?id=2213828
>
> Ben


Attachments:
smime.p7s (2.16 kB)
S/MIME Cryptographic Signature