Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <Yv1jwsHVWI+lguAT@ZenIV> <CAN-5tyFvV7QOxyAQXu3UM5swQVB2roDpQ5CBRVc64Epp1gj9hg@mail.gmail.com>
 <Yv2BVKuzZdMDY2Td@ZenIV> <CAN-5tyF0ZMX8a6M6Qbbco3EmOzwVnnGZmqak8=t4Cvtzc45g7Q@mail.gmail.com>
 <CAOQ4uxgA8jD6KnbuHDevNLsjD-LbEs_y1W6uYMEY6EG_es0o+Q@mail.gmail.com>
 <Yv3Ti/niVd5ZVPP+@ZenIV> <CAN-5tyHpDHzmo-rSw1X+0oX0xbxR+x13eP57osB0qhFLKbXzVA@mail.gmail.com>
 <b7a77d4f-32de-af24-ed5c-8a3e49947c5a@oracle.com> <CAN-5tyH6=GD_A48PEu0oWZYix4g0=+0FwVgE262Ek0U1qNiwvA@mail.gmail.com>
 <debe59b1-35cc-c3b0-f3ca-76d6a56b826b@oracle.com>
In-Reply-To: <debe59b1-35cc-c3b0-f3ca-76d6a56b826b@oracle.com>
From:   Olga Kornievskaia <aglo@umich.edu>
Date:   Fri, 19 Aug 2022 13:37:33 -0400
Message-ID: <CAN-5tyHdr_RXPcFpa7fsg=jpOyge0C4pB1waj=BdHHzmeaMdPw@mail.gmail.com>
Subject: Re: [RFC] problems with alloc_file_pseudo() use in __nfs42_ssc_open()
To:     Dai Ngo <dai.ngo@oracle.com>
Cc:     Al Viro <viro@zeniv.linux.org.uk>,
        Amir Goldstein <amir73il@gmail.com>,
        linux-nfs <linux-nfs@vger.kernel.org>,
        Olga Kornievskaia <kolga@netapp.com>,
        linux-fsdevel <linux-fsdevel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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;
> >>>           }