> On Jan 31, 2022, at 1:47 PM, Trond Myklebust <trondmy@xxxxxxxxxxxxxxx> wrote: > > On Mon, 2022-01-31 at 13:37 -0500, Trond Myklebust 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; > > BTW: This EFBIG / EOVERFLOW case could only possibly happen due to an > internal server error. > > EFBIG See EOVERFLOW. > > EOVERFLOW > pathname refers to a regular file that is too large to be > opened. The usual scenario here is that an application compiled > on a 32-bit platform without -D_FILE_OFFSET_BITS=64 tried to > open a file whose size exceeds (1<<31)-1 bytes; see also > O_LARGEFILE above. This is the error specified by POSIX.1; in > kernels before 2.6.24, Linux gave the error EFBIG for this case. What if the client has sent a CREATE with attributes that has a filesize that is smaller than OFFSET_MAX but larger than the filesystem's s_maxbytes? I believe notify_change() will return -EFBIG in this case, and correctly so. NFSD's NFSv3 SETATTR implementation will leak FBIG in some cases. If that's going to be a problem for certain important clients, then I'd like it not to do that. -- Chuck Lever
