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

[dai.ngo@oracle.com]

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