> On Jan 31, 2022, at 1:58 PM, Trond Myklebust <trondmy@xxxxxxxxxxxxxxx> wrote: > > 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. So to make sure I understand you: Drop the hunks that modify nfsd3_proc_setattr() and nfsd3_proc_create(). I'm fine with that. -- Chuck Lever
