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.
> > +
> > 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.
>
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@xxxxxxxxxxxxxxx
