> On Jan 22, 2022, at 2:01 PM, Dan Aloni <dan.aloni@xxxxxxxxxxxx> wrote:
>
> On Sat, Jan 22, 2022 at 05:05:49PM +0000, Chuck Lever III wrote:
>>>>> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
>>>>> index 738d564ca4ce..754f4e9ff4a2 100644
>>>>> --- a/fs/nfsd/vfs.c
>>>>> +++ b/fs/nfsd/vfs.c
>>>>> @@ -1046,6 +1046,10 @@ __be32 nfsd_read(struct svc_rqst *rqstp, struct svc_fh *fhp,
>>>>> __be32 err;
>>>>>
>>>>> trace_nfsd_read_start(rqstp, fhp, offset, *count);
>>>>> +
>>>>> + if (unlikely(offset + *count > NFS_OFFSET_MAX))
>>>>> + *count = NFS_OFFSET_MAX - offset;
>>>>
>>>> Can @offset ever be larger than NFS_OFFSET_MAX?
>>>
>>> We have this check in `nfsd4_read`, `(read->rd_offset >= OFFSET_MAX)`.
>>> (should it have been `>` rather?).
>>
>> Don't think so, a zero-byte READ should be valid.
>
> Make sense. BTW, we have a `(argp->offset > NFS_OFFSET_MAX)` check
> resulting in EINVAL under `nfsd3_proc_commit`. Does it apply to writes
> as well?
Geez, that's whole 'nother can of worms.
RFC 1813 section 3.3.21 does not list NFS3ERR_INVAL, and does
not discuss what to do if the commit argument values are
outside the range which the server or local filesystem
supports.
RFC 8881 section 15.2 (Table 6) does not list NFS4ERR_INVAL
as a valid status code for the COMMIT operation, and likewise
section 18.3 does not discuss how the server should respond
when the commit argument values are invalid.
Aside from nfsd3_proc_commit, nfsd_commit() is used by NFSv3
and NFSv4, and it has:
1129 __be32 err = nfserr_inval;
1130
1131 if (offset < 0)
1132 goto out;
1133 if (count != 0) {
1134 end = offset + (loff_t)count - 1;
1135 if (end < offset)
1136 goto out;
1137 }
1138
which I think is going to be problematic. But no-one has
complained, so it's safe to defer changes here to another
patch, IMO.
>> However it's rather interesting that it does not use
>> NFS_OFFSET_MAX here. Does anyone know why NFSv3 uses
>> NFS_OFFSET_MAX but NFSv4 and NLM use OFFSET_MAX?
>
> NFS_OFFSET_MAX introduced in v2.3.31, which is before `OFFSET_MAX` was
> moved to a header file, which explains the comment on top of it,
> outdated for quite awhile:
>
> /*
> * This is really a general kernel constant, but since nothing like
> * this is defined in the kernel headers, I have to do it here.
> */
> #define NFS_OFFSET_MAX ((__s64)((~(__u64)0) >> 1))
>
> And `OFFSET_MAX` in linux/fs.h was introduced in v2.3.99pre4. Seems
> `OFFSET_MAX` always corresponds to 64-bit loff_t, so they seem
> inter-changeable to me.
For now, add OFFSET_MAX in the NFSv4 paths, and use NFS_OFFSET_MAX
in the NFSv3 paths, and at some point someone can propose a clean
up to replace NFS_OFFSET_MAX with OFFSET_MAX.
--
Chuck Lever
