[RFC] problems with alloc_file_pseudo() use in __nfs42_ssc_open()

[RFC] problems with alloc_file_pseudo() use in __nfs42_ssc_open()

[Al Viro]

	My apologies for having missed that back when the SSC
patchset had been done (and missing the problems after it got
merged, actually).

1) if this
        r_ino = nfs_fhget(ss_mnt->mnt_sb, src_fh, fattr);
in __nfs42_ssc_open() yields a directory inode, we are screwed
as soon as it's passed to alloc_file_pseudo() - a *lot* of places
in dcache handling would break if we do that.  It's not too
nice for a regular file from non-cooperating filesystem, but for
directory ones it's deadly.

2) if alloc_file_pseudo() fails there, we get an inode leak.  It
needs an iput() for that case.  As in
        if (IS_ERR(filep)) {
		res = ERR_CAST(filep);
		iput(r_ino);
		goto out_free_name;
	}

But I'd like to point out that alloc_file_pseudo() is not inteded for
use on normal filesystem's inodes - the use here *mostly* works
(directories aside), but...  Use it on filesystem with non-trivial
default dentry_operations and things will get interesting, etc.

Re: [RFC] problems with alloc_file_pseudo() use in __nfs42_ssc_open()

[Olga Kornievskaia]

On Wed, Aug 17, 2022 at 6:18 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
>         My apologies for having missed that back when the SSC
> patchset had been done (and missing the problems after it got
> merged, actually).
>
> 1) if this
>         r_ino = nfs_fhget(ss_mnt->mnt_sb, src_fh, fattr);
> in __nfs42_ssc_open() yields a directory inode, we are screwed
> as soon as it's passed to alloc_file_pseudo() - a *lot* of places
> in dcache handling would break if we do that.  It's not too
> nice for a regular file from non-cooperating filesystem, but for
> directory ones it's deadly.

This inode is created to make an appearance of an opened file to do
(an NFS) read, it's never a directory.

> 2) if alloc_file_pseudo() fails there, we get an inode leak.  It
> needs an iput() for that case.  As in
>         if (IS_ERR(filep)) {
>                 res = ERR_CAST(filep);
>                 iput(r_ino);
>                 goto out_free_name;
>         }
>
> But I'd like to point out that alloc_file_pseudo() is not inteded for
> use on normal filesystem's inodes - the use here *mostly* works
> (directories aside), but...  Use it on filesystem with non-trivial
> default dentry_operations and things will get interesting, etc.

Re: [RFC] problems with alloc_file_pseudo() use in __nfs42_ssc_open()

[Al Viro]

On Wed, Aug 17, 2022 at 06:32:15PM -0400, Olga Kornievskaia wrote:
> On Wed, Aug 17, 2022 at 6:18 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> >         My apologies for having missed that back when the SSC
> > patchset had been done (and missing the problems after it got
> > merged, actually).
> >
> > 1) if this
> >         r_ino = nfs_fhget(ss_mnt->mnt_sb, src_fh, fattr);
> > in __nfs42_ssc_open() yields a directory inode, we are screwed
> > as soon as it's passed to alloc_file_pseudo() - a *lot* of places
> > in dcache handling would break if we do that.  It's not too
> > nice for a regular file from non-cooperating filesystem, but for
> > directory ones it's deadly.
> 
> This inode is created to make an appearance of an opened file to do
> (an NFS) read, it's never a directory.

Er...  Where does the fhandle come from?  From my reading it's a client-sent
data; I don't know what trust model do you assume, but the price of
getting multiple dentries over the same directory inode is high.
Bogus or compromised client should not be able to cause severe corruption
of kernel data structures...

Re: [RFC] problems with alloc_file_pseudo() use in __nfs42_ssc_open()

[Olga Kornievskaia]

On Wed, Aug 17, 2022 at 8:01 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Wed, Aug 17, 2022 at 06:32:15PM -0400, Olga Kornievskaia wrote:
> > On Wed, Aug 17, 2022 at 6:18 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
> > >
> > >         My apologies for having missed that back when the SSC
> > > patchset had been done (and missing the problems after it got
> > > merged, actually).
> > >
> > > 1) if this
> > >         r_ino = nfs_fhget(ss_mnt->mnt_sb, src_fh, fattr);
> > > in __nfs42_ssc_open() yields a directory inode, we are screwed
> > > as soon as it's passed to alloc_file_pseudo() - a *lot* of places
> > > in dcache handling would break if we do that.  It's not too
> > > nice for a regular file from non-cooperating filesystem, but for
> > > directory ones it's deadly.
> >
> > This inode is created to make an appearance of an opened file to do
> > (an NFS) read, it's never a directory.
>
> Er...  Where does the fhandle come from?  From my reading it's a client-sent
> data; I don't know what trust model do you assume, but the price of
> getting multiple dentries over the same directory inode is high.
> Bogus or compromised client should not be able to cause severe corruption
> of kernel data structures...

This is an NFS spec specified operation. The (source file's)
filehandle comes from the COPY operation compound that the destination
server gets and then uses -- creates an inode from using the code you
are looking at now -- to access from the source server. Security is
all described in the spec. The uniqueness of the filehandle is
provided by the source server that created it.

Re: [RFC] problems with alloc_file_pseudo() use in __nfs42_ssc_open()

[Al Viro]

On Wed, Aug 17, 2022 at 08:12:27PM -0400, Olga Kornievskaia wrote:
> On Wed, Aug 17, 2022 at 8:01 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > On Wed, Aug 17, 2022 at 06:32:15PM -0400, Olga Kornievskaia wrote:
> > > On Wed, Aug 17, 2022 at 6:18 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
> > > >
> > > >         My apologies for having missed that back when the SSC
> > > > patchset had been done (and missing the problems after it got
> > > > merged, actually).
> > > >
> > > > 1) if this
> > > >         r_ino = nfs_fhget(ss_mnt->mnt_sb, src_fh, fattr);
> > > > in __nfs42_ssc_open() yields a directory inode, we are screwed
> > > > as soon as it's passed to alloc_file_pseudo() - a *lot* of places
> > > > in dcache handling would break if we do that.  It's not too
> > > > nice for a regular file from non-cooperating filesystem, but for
> > > > directory ones it's deadly.
> > >
> > > This inode is created to make an appearance of an opened file to do
> > > (an NFS) read, it's never a directory.
> >
> > Er...  Where does the fhandle come from?  From my reading it's a client-sent
> > data; I don't know what trust model do you assume, but the price of
> > getting multiple dentries over the same directory inode is high.
> > Bogus or compromised client should not be able to cause severe corruption
> > of kernel data structures...
> 
> This is an NFS spec specified operation. The (source file's)
> filehandle comes from the COPY operation compound that the destination
> server gets and then uses -- creates an inode from using the code you
> are looking at now -- to access from the source server. Security is
> all described in the spec. The uniqueness of the filehandle is
> provided by the source server that created it.

Do we assume that compromise of source server is escalatable at least
to kernel panics on the destination server anyway?  Confused...

Re: [RFC] problems with alloc_file_pseudo() use in __nfs42_ssc_open()

[Amir Goldstein]

On Thu, Aug 18, 2022 at 3:23 AM Olga Kornievskaia <aglo@umich.edu> wrote:
>
> On Wed, Aug 17, 2022 at 8:01 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > On Wed, Aug 17, 2022 at 06:32:15PM -0400, Olga Kornievskaia wrote:
> > > On Wed, Aug 17, 2022 at 6:18 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
> > > >
> > > >         My apologies for having missed that back when the SSC
> > > > patchset had been done (and missing the problems after it got
> > > > merged, actually).
> > > >
> > > > 1) if this
> > > >         r_ino = nfs_fhget(ss_mnt->mnt_sb, src_fh, fattr);
> > > > in __nfs42_ssc_open() yields a directory inode, we are screwed
> > > > as soon as it's passed to alloc_file_pseudo() - a *lot* of places
> > > > in dcache handling would break if we do that.  It's not too
> > > > nice for a regular file from non-cooperating filesystem, but for
> > > > directory ones it's deadly.
> > >
> > > This inode is created to make an appearance of an opened file to do
> > > (an NFS) read, it's never a directory.
> >
> > Er...  Where does the fhandle come from?  From my reading it's a client-sent
> > data; I don't know what trust model do you assume, but the price of
> > getting multiple dentries over the same directory inode is high.
> > Bogus or compromised client should not be able to cause severe corruption
> > of kernel data structures...
>
> This is an NFS spec specified operation. The (source file's)
> filehandle comes from the COPY operation compound that the destination
> server gets and then uses -- creates an inode from using the code you
> are looking at now -- to access from the source server. Security is
> all described in the spec. The uniqueness of the filehandle is
> provided by the source server that created it.

Olga,

NFS spec does not guarantee the safety of the server.
It's like saying that the Law makes Crime impossible.
The law needs to be enforced, so if server gets a request
to COPY from/to an fhandle that resolves as a non-regular file
(from a rogue or buggy NFS client) the server should return an
error and not continue to alloc_file_pseudo().

Thanks,
Amir.

Re: [RFC] problems with alloc_file_pseudo() use in __nfs42_ssc_open()

[Al Viro]

On Thu, Aug 18, 2022 at 08:19:54AM +0300, Amir Goldstein wrote:

> NFS spec does not guarantee the safety of the server.
> It's like saying that the Law makes Crime impossible.
> The law needs to be enforced, so if server gets a request
> to COPY from/to an fhandle that resolves as a non-regular file
> (from a rogue or buggy NFS client) the server should return an
> error and not continue to alloc_file_pseudo().

FWIW, my preference would be to have alloc_file_pseudo() reject
directory inodes if it ever gets such.

I'm still not sure that my (and yours, apparently) interpretation
of what Olga said is correct, though.

Re: [RFC] problems with alloc_file_pseudo() use in __nfs42_ssc_open()

[Olga Kornievskaia]

On Thu, Aug 18, 2022 at 1:52 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Thu, Aug 18, 2022 at 08:19:54AM +0300, Amir Goldstein wrote:
>
> > NFS spec does not guarantee the safety of the server.
> > It's like saying that the Law makes Crime impossible.
> > The law needs to be enforced, so if server gets a request
> > to COPY from/to an fhandle that resolves as a non-regular file
> > (from a rogue or buggy NFS client) the server should return an
> > error and not continue to alloc_file_pseudo().
>
> FWIW, my preference would be to have alloc_file_pseudo() reject
> directory inodes if it ever gets such.
>
> I'm still not sure that my (and yours, apparently) interpretation
> of what Olga said is correct, though.

Would it be appropriate to do the following then:

diff --git a/fs/nfs/nfs4file.c b/fs/nfs/nfs4file.c
index e88f6b18445e..112134b6438d 100644
--- a/fs/nfs/nfs4file.c
+++ b/fs/nfs/nfs4file.c
@@ -340,6 +340,11 @@ static struct file *__nfs42_ssc_open(struct
vfsmount *ss_mnt,
                goto out;
        }

+       if (S_ISDIR(fattr->mode)) {
+               res = ERR_PTR(-EBADF);
+               goto out;
+       }
+
        res = ERR_PTR(-ENOMEM);
        len = strlen(SSC_READ_NAME_BODY) + 16;
        read_name = kzalloc(len, GFP_KERNEL);
@@ -357,6 +362,7 @@ static struct file *__nfs42_ssc_open(struct
vfsmount *ss_mnt,
                                     r_ino->i_fop);
        if (IS_ERR(filep)) {
                res = ERR_CAST(filep);
+               iput(r_ino);
                goto out_free_name;
        }

Re: [RFC] problems with alloc_file_pseudo() use in __nfs42_ssc_open()

[Trond Myklebust]

On Thu, 2022-08-18 at 09:13 -0400, Olga Kornievskaia wrote:
> On Thu, Aug 18, 2022 at 1:52 AM Al Viro <viro@zeniv.linux.org.uk>
> wrote:
> > 
> > On Thu, Aug 18, 2022 at 08:19:54AM +0300, Amir Goldstein wrote:
> > 
> > > NFS spec does not guarantee the safety of the server.
> > > It's like saying that the Law makes Crime impossible.
> > > The law needs to be enforced, so if server gets a request
> > > to COPY from/to an fhandle that resolves as a non-regular file
> > > (from a rogue or buggy NFS client) the server should return an
> > > error and not continue to alloc_file_pseudo().
> > 
> > FWIW, my preference would be to have alloc_file_pseudo() reject
> > directory inodes if it ever gets such.
> > 
> > I'm still not sure that my (and yours, apparently) interpretation
> > of what Olga said is correct, though.
> 
> Would it be appropriate to do the following then:
> 
> diff --git a/fs/nfs/nfs4file.c b/fs/nfs/nfs4file.c
> index e88f6b18445e..112134b6438d 100644
> --- a/fs/nfs/nfs4file.c
> +++ b/fs/nfs/nfs4file.c
> @@ -340,6 +340,11 @@ static struct file *__nfs42_ssc_open(struct
> vfsmount *ss_mnt,
>                 goto out;
>         }
> 
> +       if (S_ISDIR(fattr->mode)) {
> +               res = ERR_PTR(-EBADF);
> +               goto out;
> +       }

You're better off going with 'if (!S_ISREG())' so that we also reject
symlinks, pipes, devices, etc. The requirement for the NFSv4.2 copy
offload protocol is that both the source and destination be regular
files.

> +
>         res = ERR_PTR(-ENOMEM);
>         len = strlen(SSC_READ_NAME_BODY) + 16;
>         read_name = kzalloc(len, GFP_KERNEL);
> @@ -357,6 +362,7 @@ static struct file *__nfs42_ssc_open(struct
> vfsmount *ss_mnt,
>                                      r_ino->i_fop);
>         if (IS_ERR(filep)) {
>                 res = ERR_CAST(filep);
> +               iput(r_ino);
>                 goto out_free_name;
>         }

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

Re: [RFC] problems with alloc_file_pseudo() use in __nfs42_ssc_open()

[dai.ngo]

On 8/18/22 6:13 AM, Olga Kornievskaia wrote:
> On Thu, Aug 18, 2022 at 1:52 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
>> On Thu, Aug 18, 2022 at 08:19:54AM +0300, Amir Goldstein wrote:
>>
>>> NFS spec does not guarantee the safety of the server.
>>> It's like saying that the Law makes Crime impossible.
>>> The law needs to be enforced, so if server gets a request
>>> to COPY from/to an fhandle that resolves as a non-regular file
>>> (from a rogue or buggy NFS client) the server should return an
>>> error and not continue to alloc_file_pseudo().
>> FWIW, my preference would be to have alloc_file_pseudo() reject
>> directory inodes if it ever gets such.
>>
>> I'm still not sure that my (and yours, apparently) interpretation
>> of what Olga said is correct, though.
> Would it be appropriate to do the following then:
>
> diff --git a/fs/nfs/nfs4file.c b/fs/nfs/nfs4file.c
> index e88f6b18445e..112134b6438d 100644
> --- a/fs/nfs/nfs4file.c
> +++ b/fs/nfs/nfs4file.c
> @@ -340,6 +340,11 @@ static struct file *__nfs42_ssc_open(struct
> vfsmount *ss_mnt,
>                  goto out;
>          }
>
> +       if (S_ISDIR(fattr->mode)) {
> +               res = ERR_PTR(-EBADF);
> +               goto out;
> +       }
> +

Can we also enhance nfsd4_do_async_copy to check for
-EBADF and returns nfserr_wrong_type? perhaps adding
an error mapping function to handle other errors also.

-Dai

>          res = ERR_PTR(-ENOMEM);
>          len = strlen(SSC_READ_NAME_BODY) + 16;
>          read_name = kzalloc(len, GFP_KERNEL);
> @@ -357,6 +362,7 @@ static struct file *__nfs42_ssc_open(struct
> vfsmount *ss_mnt,
>                                       r_ino->i_fop);
>          if (IS_ERR(filep)) {
>                  res = ERR_CAST(filep);
> +               iput(r_ino);
>                  goto out_free_name;
>          }

Re: [RFC] problems with alloc_file_pseudo() use in __nfs42_ssc_open()

[Olga Kornievskaia]

On Thu, Aug 18, 2022 at 10:52 PM <dai.ngo@oracle.com> wrote:
>
>
> On 8/18/22 6:13 AM, Olga Kornievskaia wrote:
> > On Thu, Aug 18, 2022 at 1:52 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
> >> On Thu, Aug 18, 2022 at 08:19:54AM +0300, Amir Goldstein wrote:
> >>
> >>> NFS spec does not guarantee the safety of the server.
> >>> It's like saying that the Law makes Crime impossible.
> >>> The law needs to be enforced, so if server gets a request
> >>> to COPY from/to an fhandle that resolves as a non-regular file
> >>> (from a rogue or buggy NFS client) the server should return an
> >>> error and not continue to alloc_file_pseudo().
> >> FWIW, my preference would be to have alloc_file_pseudo() reject
> >> directory inodes if it ever gets such.
> >>
> >> I'm still not sure that my (and yours, apparently) interpretation
> >> of what Olga said is correct, though.
> > Would it be appropriate to do the following then:
> >
> > diff --git a/fs/nfs/nfs4file.c b/fs/nfs/nfs4file.c
> > index e88f6b18445e..112134b6438d 100644
> > --- a/fs/nfs/nfs4file.c
> > +++ b/fs/nfs/nfs4file.c
> > @@ -340,6 +340,11 @@ static struct file *__nfs42_ssc_open(struct
> > vfsmount *ss_mnt,
> >                  goto out;
> >          }
> >
> > +       if (S_ISDIR(fattr->mode)) {
> > +               res = ERR_PTR(-EBADF);
> > +               goto out;
> > +       }
> > +
>
> Can we also enhance nfsd4_do_async_copy to check for
> -EBADF and returns nfserr_wrong_type? perhaps adding
> an error mapping function to handle other errors also.

On the server side, if the open fails that's already translated into
the appropriate error -- err_off_load_denied.

>
> -Dai
>
> >          res = ERR_PTR(-ENOMEM);
> >          len = strlen(SSC_READ_NAME_BODY) + 16;
> >          read_name = kzalloc(len, GFP_KERNEL);
> > @@ -357,6 +362,7 @@ static struct file *__nfs42_ssc_open(struct
> > vfsmount *ss_mnt,
> >                                       r_ino->i_fop);
> >          if (IS_ERR(filep)) {
> >                  res = ERR_CAST(filep);
> > +               iput(r_ino);
> >                  goto out_free_name;
> >          }

Re: [RFC] problems with alloc_file_pseudo() use in __nfs42_ssc_open()

[dai.ngo]

On 8/19/22 7:22 AM, Olga Kornievskaia wrote:
> On Thu, Aug 18, 2022 at 10:52 PM <dai.ngo@oracle.com> wrote:
>>
>> On 8/18/22 6:13 AM, Olga Kornievskaia wrote:
>>> On Thu, Aug 18, 2022 at 1:52 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
>>>> On Thu, Aug 18, 2022 at 08:19:54AM +0300, Amir Goldstein wrote:
>>>>
>>>>> NFS spec does not guarantee the safety of the server.
>>>>> It's like saying that the Law makes Crime impossible.
>>>>> The law needs to be enforced, so if server gets a request
>>>>> to COPY from/to an fhandle that resolves as a non-regular file
>>>>> (from a rogue or buggy NFS client) the server should return an
>>>>> error and not continue to alloc_file_pseudo().
>>>> FWIW, my preference would be to have alloc_file_pseudo() reject
>>>> directory inodes if it ever gets such.
>>>>
>>>> I'm still not sure that my (and yours, apparently) interpretation
>>>> of what Olga said is correct, though.
>>> Would it be appropriate to do the following then:
>>>
>>> diff --git a/fs/nfs/nfs4file.c b/fs/nfs/nfs4file.c
>>> index e88f6b18445e..112134b6438d 100644
>>> --- a/fs/nfs/nfs4file.c
>>> +++ b/fs/nfs/nfs4file.c
>>> @@ -340,6 +340,11 @@ static struct file *__nfs42_ssc_open(struct
>>> vfsmount *ss_mnt,
>>>                   goto out;
>>>           }
>>>
>>> +       if (S_ISDIR(fattr->mode)) {
>>> +               res = ERR_PTR(-EBADF);
>>> +               goto out;
>>> +       }
>>> +
>> Can we also enhance nfsd4_do_async_copy to check for
>> -EBADF and returns nfserr_wrong_type? perhaps adding
>> an error mapping function to handle other errors also.
> On the server side, if the open fails that's already translated into
> the appropriate error -- err_off_load_denied.

Currently the server returns nfserr_offload_denied if the open
fails for any reasons. I'm wondering whether the server should
return more accurate error code such as if the source file handle
is a wrong type then the server should return nfserr_wrong_type,
instead of nfserr_offload_denied, to match the spec:

    Both SAVED_FH and CURRENT_FH must be regular files.  If either
    SAVED_FH or CURRENT_FH is not a regular file, the operation MUST fail
    and return NFS4ERR_WRONG_TYPE.

-Dai

>
>> -Dai
>>
>>>           res = ERR_PTR(-ENOMEM);
>>>           len = strlen(SSC_READ_NAME_BODY) + 16;
>>>           read_name = kzalloc(len, GFP_KERNEL);
>>> @@ -357,6 +362,7 @@ static struct file *__nfs42_ssc_open(struct
>>> vfsmount *ss_mnt,
>>>                                        r_ino->i_fop);
>>>           if (IS_ERR(filep)) {
>>>                   res = ERR_CAST(filep);
>>> +               iput(r_ino);
>>>                   goto out_free_name;
>>>           }

Re: [RFC] problems with alloc_file_pseudo() use in __nfs42_ssc_open()

[Olga Kornievskaia]

On Fri, Aug 19, 2022 at 11:42 AM <dai.ngo@oracle.com> wrote:
>
>
> On 8/19/22 7:22 AM, Olga Kornievskaia wrote:
> > On Thu, Aug 18, 2022 at 10:52 PM <dai.ngo@oracle.com> wrote:
> >>
> >> On 8/18/22 6:13 AM, Olga Kornievskaia wrote:
> >>> On Thu, Aug 18, 2022 at 1:52 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
> >>>> On Thu, Aug 18, 2022 at 08:19:54AM +0300, Amir Goldstein wrote:
> >>>>
> >>>>> NFS spec does not guarantee the safety of the server.
> >>>>> It's like saying that the Law makes Crime impossible.
> >>>>> The law needs to be enforced, so if server gets a request
> >>>>> to COPY from/to an fhandle that resolves as a non-regular file
> >>>>> (from a rogue or buggy NFS client) the server should return an
> >>>>> error and not continue to alloc_file_pseudo().
> >>>> FWIW, my preference would be to have alloc_file_pseudo() reject
> >>>> directory inodes if it ever gets such.
> >>>>
> >>>> I'm still not sure that my (and yours, apparently) interpretation
> >>>> of what Olga said is correct, though.
> >>> Would it be appropriate to do the following then:
> >>>
> >>> diff --git a/fs/nfs/nfs4file.c b/fs/nfs/nfs4file.c
> >>> index e88f6b18445e..112134b6438d 100644
> >>> --- a/fs/nfs/nfs4file.c
> >>> +++ b/fs/nfs/nfs4file.c
> >>> @@ -340,6 +340,11 @@ static struct file *__nfs42_ssc_open(struct
> >>> vfsmount *ss_mnt,
> >>>                   goto out;
> >>>           }
> >>>
> >>> +       if (S_ISDIR(fattr->mode)) {
> >>> +               res = ERR_PTR(-EBADF);
> >>> +               goto out;
> >>> +       }
> >>> +
> >> Can we also enhance nfsd4_do_async_copy to check for
> >> -EBADF and returns nfserr_wrong_type? perhaps adding
> >> an error mapping function to handle other errors also.
> > On the server side, if the open fails that's already translated into
> > the appropriate error -- err_off_load_denied.
>
> Currently the server returns nfserr_offload_denied if the open
> fails for any reasons. I'm wondering whether the server should
> return more accurate error code such as if the source file handle
> is a wrong type then the server should return nfserr_wrong_type,
> instead of nfserr_offload_denied, to match the spec:
>
>     Both SAVED_FH and CURRENT_FH must be regular files.  If either
>     SAVED_FH or CURRENT_FH is not a regular file, the operation MUST fail
>     and return NFS4ERR_WRONG_TYPE.

Ok sure. That's a relevant but a separate patch.

>
> -Dai
>
> >
> >> -Dai
> >>
> >>>           res = ERR_PTR(-ENOMEM);
> >>>           len = strlen(SSC_READ_NAME_BODY) + 16;
> >>>           read_name = kzalloc(len, GFP_KERNEL);
> >>> @@ -357,6 +362,7 @@ static struct file *__nfs42_ssc_open(struct
> >>> vfsmount *ss_mnt,
> >>>                                        r_ino->i_fop);
> >>>           if (IS_ERR(filep)) {
> >>>                   res = ERR_CAST(filep);
> >>> +               iput(r_ino);
> >>>                   goto out_free_name;
> >>>           }

Re: [RFC] problems with alloc_file_pseudo() use in __nfs42_ssc_open()

[dai.ngo]

On 8/19/22 10:37 AM, Olga Kornievskaia wrote:
> On Fri, Aug 19, 2022 at 11:42 AM <dai.ngo@oracle.com> wrote:
>>
>> On 8/19/22 7:22 AM, Olga Kornievskaia wrote:
>>> On Thu, Aug 18, 2022 at 10:52 PM <dai.ngo@oracle.com> wrote:
>>>> On 8/18/22 6:13 AM, Olga Kornievskaia wrote:
>>>>> On Thu, Aug 18, 2022 at 1:52 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
>>>>>> On Thu, Aug 18, 2022 at 08:19:54AM +0300, Amir Goldstein wrote:
>>>>>>
>>>>>>> NFS spec does not guarantee the safety of the server.
>>>>>>> It's like saying that the Law makes Crime impossible.
>>>>>>> The law needs to be enforced, so if server gets a request
>>>>>>> to COPY from/to an fhandle that resolves as a non-regular file
>>>>>>> (from a rogue or buggy NFS client) the server should return an
>>>>>>> error and not continue to alloc_file_pseudo().
>>>>>> FWIW, my preference would be to have alloc_file_pseudo() reject
>>>>>> directory inodes if it ever gets such.
>>>>>>
>>>>>> I'm still not sure that my (and yours, apparently) interpretation
>>>>>> of what Olga said is correct, though.
>>>>> Would it be appropriate to do the following then:
>>>>>
>>>>> diff --git a/fs/nfs/nfs4file.c b/fs/nfs/nfs4file.c
>>>>> index e88f6b18445e..112134b6438d 100644
>>>>> --- a/fs/nfs/nfs4file.c
>>>>> +++ b/fs/nfs/nfs4file.c
>>>>> @@ -340,6 +340,11 @@ static struct file *__nfs42_ssc_open(struct
>>>>> vfsmount *ss_mnt,
>>>>>                    goto out;
>>>>>            }
>>>>>
>>>>> +       if (S_ISDIR(fattr->mode)) {
>>>>> +               res = ERR_PTR(-EBADF);
>>>>> +               goto out;
>>>>> +       }
>>>>> +
>>>> Can we also enhance nfsd4_do_async_copy to check for
>>>> -EBADF and returns nfserr_wrong_type? perhaps adding
>>>> an error mapping function to handle other errors also.
>>> On the server side, if the open fails that's already translated into
>>> the appropriate error -- err_off_load_denied.
>> Currently the server returns nfserr_offload_denied if the open
>> fails for any reasons. I'm wondering whether the server should
>> return more accurate error code such as if the source file handle
>> is a wrong type then the server should return nfserr_wrong_type,
>> instead of nfserr_offload_denied, to match the spec:
>>
>>      Both SAVED_FH and CURRENT_FH must be regular files.  If either
>>      SAVED_FH or CURRENT_FH is not a regular file, the operation MUST fail
>>      and return NFS4ERR_WRONG_TYPE.
> Ok sure. That's a relevant but a separate patch.

Thank you Olga!

-Dai

>
>> -Dai
>>
>>>> -Dai
>>>>
>>>>>            res = ERR_PTR(-ENOMEM);
>>>>>            len = strlen(SSC_READ_NAME_BODY) + 16;
>>>>>            read_name = kzalloc(len, GFP_KERNEL);
>>>>> @@ -357,6 +362,7 @@ static struct file *__nfs42_ssc_open(struct
>>>>> vfsmount *ss_mnt,
>>>>>                                         r_ino->i_fop);
>>>>>            if (IS_ERR(filep)) {
>>>>>                    res = ERR_CAST(filep);
>>>>> +               iput(r_ino);
>>>>>                    goto out_free_name;
>>>>>            }