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

dai.ngo
2022-08-19T15:44:33Z

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