On Mon, 2022-01-31 at 18:49 +0000, Chuck Lever III wrote: > > > > 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. > > Please return NFS3ERR_FBIG in the setattr case, and just drop the create change (do_nfsd_create() can never return EFBIG given that nfsd always opens the file with O_LARGEFILE). There is no document other than the Solaris and Linux NFS code. RFC1813 was never intended as an IETF standard, and never saw any follow up. Nothing else was published following the Connectathon testing events which determined the wire protocol. -- Trond Myklebust Linux NFS client maintainer, Hammerspace trond.myklebust@xxxxxxxxxxxxxxx
