> On Jan 31, 2022, at 1:37 PM, Trond Myklebust <trondmy@xxxxxxxxxxxxxxx> wrote: > > On Mon, 2022-01-31 at 13:24 -0500, Chuck Lever wrote: >> iattr::ia_size is a loff_t, so these NFSv3 procedures must be >> careful to deal with incoming client size values that are larger >> than s64_max without corrupting the value. >> >> Silently capping the value results in storing a different value >> than the client passed in which is unexpected behavior, so remove >> the min_t() check in decode_sattr3(). >> >> Moreover, a large file size is not an XDR error, since anything up >> to U64_MAX is permitted for NFSv3 file size values. So it has to be >> dealt with in nfs3proc.c, not in the XDR decoder. >> >> Size comparisons like in inode_newsize_ok should now work as >> expected -- the VFS returns -EFBIG if the new size is larger than >> the underlying filesystem's s_maxbytes. >> >> However, RFC 1813 permits only the WRITE procedure to return >> NFS3ERR_FBIG. Extra checks are needed to prevent NFSv3 SETATTR and >> CREATE from returning FBIG. Unfortunately RFC 1813 does not provide >> a specific status code for either procedure to indicate this >> specific failure, so I've chosen NFS3ERR_INVAL for SETATTR and >> NFS3ERR_IO for CREATE. >> >> Applications and NFS clients might be better served if the server >> stuck with NFS3ERR_FBIG despite what RFC 1813 says. >> >> Signed-off-by: Chuck Lever <chuck.lever@xxxxxxxxxx> >> --- >> fs/nfsd/nfs3proc.c | 9 +++++++++ >> fs/nfsd/nfs3xdr.c | 2 +- >> 2 files changed, 10 insertions(+), 1 deletion(-) >> >> diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c >> index 8ef53f6726ec..02edc7074d06 100644 >> --- a/fs/nfsd/nfs3proc.c >> +++ b/fs/nfsd/nfs3proc.c >> @@ -73,6 +73,10 @@ nfsd3_proc_setattr(struct svc_rqst *rqstp) >> fh_copy(&resp->fh, &argp->fh); >> resp->status = nfsd_setattr(rqstp, &resp->fh, &argp->attrs, >> argp->check_guard, argp- >>> guardtime); >> + >> + if (resp->status == nfserr_fbig) >> + resp->status = nfserr_inval; >> + >> return rpc_success; >> } >> >> @@ -245,6 +249,11 @@ nfsd3_proc_create(struct svc_rqst *rqstp) >> resp->status = do_nfsd_create(rqstp, dirfhp, argp->name, >> argp->len, >> attr, newfhp, argp->createmode, >> (u32 *)argp->verf, NULL, NULL); >> + >> + /* CREATE must not return NFS3ERR_FBIG */ >> + if (resp->status == nfserr_fbig) >> + resp->status = nfserr_io; >> + >> return rpc_success; >> } >> >> diff --git a/fs/nfsd/nfs3xdr.c b/fs/nfsd/nfs3xdr.c >> index 7c45ba4db61b..2e47a07029f1 100644 >> --- a/fs/nfsd/nfs3xdr.c >> +++ b/fs/nfsd/nfs3xdr.c >> @@ -254,7 +254,7 @@ svcxdr_decode_sattr3(struct svc_rqst *rqstp, >> struct xdr_stream *xdr, >> if (xdr_stream_decode_u64(xdr, &newsize) < 0) >> return false; >> iap->ia_valid |= ATTR_SIZE; >> - iap->ia_size = min_t(u64, newsize, NFS_OFFSET_MAX); >> + iap->ia_size = newsize; >> } >> if (xdr_stream_decode_u32(xdr, &set_it) < 0)0 >> return false; >> >> > > NACK. > > Unlike NFSV4, NFSv3 has reference implementations, not a reference > specification document. There is no need to change those > implementations to deal with the fact that RFC1813 is underspecified. > > This change would just serve to break client behaviour, for no good > reason. So, I _have_ been asking around. This is not a change that I'm proposing blithely. Which part of the change is wrong, and which clients would break? Solaris NFSv3 server is supposed to return NFS3ERR_FBIG in this case, I believe. NFSD could return NFS3ERR_FBIG in these cases instead. Is there somewhere that the behavior of the reference implementation is documented? If the current XDR decoder behavior is a de facto standard, that should be noted in a comment here. -- Chuck Lever
