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!
(Are the checks really necessary? I can't tell what vfs_fsync_range()
does with weird values.)
--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,
