> On Jan 25, 2022, at 11:46 AM, J. Bruce Fields <bfields@xxxxxxxxxxxx> wrote:
>
> On Tue, Jan 25, 2022 at 10:15:18AM -0500, Chuck Lever wrote:
>> Since, well, forever, the Linux NFS server's nfsd_commit() function
>> has returned nfserr_inval when the passed-in byte range arguments
>> were non-sensical.
>>
>> However, according to RFC 1813 section 3.3.21, NFSv3 COMMIT requests
>> are permitted to return only the following non-zero status codes:
>>
>> NFS3ERR_IO
>> NFS3ERR_STALE
>> NFS3ERR_BADHANDLE
>> NFS3ERR_SERVERFAULT
>>
>> NFS3ERR_INVAL is not included in that list. Likewise, NFS4ERR_INVAL
>> is not listed in the COMMIT row of Table 6 in RFC 8881.
>>
>> Instead of dropping or failing a COMMIT request in a byte range that
>> is not supported, turn it into a valid request by treating one or
>> both arguments as zero.
>
> Offset 0 means start-of-file, count 0 means until-end-of-file, so at
> worst you're extending the range, and committing data you don't need to.
> Since committing is something the server's free to do at any time,
> that's harmless. OK!
Right, committing more than requested is allowed.
> (Are the checks really necessary? I can't tell what vfs_fsync_range()
> does with weird values.)
In general we want to ensure the math doesn't overflow.
But I can have a closer look at vfs_fsync_range().
> --b.
>
>>
>> As a clean-up, I replaced the signed v. unsigned integer comparisons
>> because I found that logic difficult to reason about.
>>
>> Reported-by: Dan Aloni <dan.aloni@xxxxxxxxxxxx>
>> Signed-off-by: Chuck Lever <chuck.lever@xxxxxxxxxx>
>> ---
>> fs/nfsd/nfs3proc.c | 6 ------
>> fs/nfsd/vfs.c | 43 ++++++++++++++++++++++++++++---------------
>> fs/nfsd/vfs.h | 4 ++--
>> 3 files changed, 30 insertions(+), 23 deletions(-)
>>
>> diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
>> index 8ef53f6726ec..8cd2953f53c7 100644
>> --- a/fs/nfsd/nfs3proc.c
>> +++ b/fs/nfsd/nfs3proc.c
>> @@ -651,15 +651,9 @@ nfsd3_proc_commit(struct svc_rqst *rqstp)
>> argp->count,
>> (unsigned long long) argp->offset);
>>
>> - if (argp->offset > NFS_OFFSET_MAX) {
>> - resp->status = nfserr_inval;
>> - goto out;
>> - }
>> -
>> fh_copy(&resp->fh, &argp->fh);
>> resp->status = nfsd_commit(rqstp, &resp->fh, argp->offset,
>> argp->count, resp->verf);
>> -out:
>> return rpc_success;
>> }
>>
>> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
>> index 99c2b9dfbb10..384c62591f45 100644
>> --- a/fs/nfsd/vfs.c
>> +++ b/fs/nfsd/vfs.c
>> @@ -1110,42 +1110,55 @@ nfsd_write(struct svc_rqst *rqstp, struct svc_fh *fhp, loff_t offset,
>> }
>>
>> #ifdef CONFIG_NFSD_V3
>> -/*
>> - * Commit all pending writes to stable storage.
>> +/**
>> + * nfsd_commit - Commit pending writes to stable storage
>> + * @rqstp: RPC request being processed
>> + * @fhp: NFS filehandle
>> + * @offset: offset from beginning of file
>> + * @count: count of bytes to sync
>> + * @verf: filled in with the server's current write verifier
>> *
>> * Note: we only guarantee that data that lies within the range specified
>> * by the 'offset' and 'count' parameters will be synced.
>> *
>> * Unfortunately we cannot lock the file to make sure we return full WCC
>> * data to the client, as locking happens lower down in the filesystem.
>> + *
>> + * Return values:
>> + * An nfsstat value in network byte order.
>> */
>> __be32
>> -nfsd_commit(struct svc_rqst *rqstp, struct svc_fh *fhp,
>> - loff_t offset, unsigned long count, __be32 *verf)
>> +nfsd_commit(struct svc_rqst *rqstp, struct svc_fh *fhp, u64 offset,
>> + u32 count, __be32 *verf)
>> {
>> + u64 maxbytes;
>> + loff_t start, end;
>> struct nfsd_net *nn;
>> struct nfsd_file *nf;
>> - loff_t end = LLONG_MAX;
>> - __be32 err = nfserr_inval;
>> -
>> - if (offset < 0)
>> - goto out;
>> - if (count != 0) {
>> - end = offset + (loff_t)count - 1;
>> - if (end < offset)
>> - goto out;
>> - }
>> + __be32 err;
>>
>> err = nfsd_file_acquire(rqstp, fhp,
>> NFSD_MAY_WRITE|NFSD_MAY_NOT_BREAK_LEASE, &nf);
>> if (err)
>> goto out;
>> +
>> + start = 0;
>> + end = LLONG_MAX;
>> + /* NB: s_maxbytes is a (signed) loff_t, thus @maxbytes always
>> + * contains a value that is less than LLONG_MAX. */
>> + maxbytes = fhp->fh_dentry->d_sb->s_maxbytes;
>> + if (offset < maxbytes) {
>> + start = offset;
>> + if (count && (offset + count - 1 < maxbytes))
>> + end = offset + count - 1;
>> + }
>> +
>> nn = net_generic(nf->nf_net, nfsd_net_id);
>> if (EX_ISSYNC(fhp->fh_export)) {
>> errseq_t since = READ_ONCE(nf->nf_file->f_wb_err);
>> int err2;
>>
>> - err2 = vfs_fsync_range(nf->nf_file, offset, end, 0);
>> + err2 = vfs_fsync_range(nf->nf_file, start, end, 0);
>> switch (err2) {
>> case 0:
>> nfsd_copy_write_verifier(verf, nn);
>> diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
>> index 9f56dcb22ff7..2c43d10e3cab 100644
>> --- a/fs/nfsd/vfs.h
>> +++ b/fs/nfsd/vfs.h
>> @@ -74,8 +74,8 @@ __be32 do_nfsd_create(struct svc_rqst *, struct svc_fh *,
>> char *name, int len, struct iattr *attrs,
>> struct svc_fh *res, int createmode,
>> u32 *verifier, bool *truncp, bool *created);
>> -__be32 nfsd_commit(struct svc_rqst *, struct svc_fh *,
>> - loff_t, unsigned long, __be32 *verf);
>> +__be32 nfsd_commit(struct svc_rqst *rqst, struct svc_fh *fhp,
>> + u64 offset, u32 count, __be32 *verf);
>> #endif /* CONFIG_NFSD_V3 */
>> #ifdef CONFIG_NFSD_V4
>> __be32 nfsd_getxattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
--
Chuck Lever
