Re: [PATCH v2 2/5] NFSD: Fix NFSv3 SETATTR/CREATE's handling of large file sizes

[Trond Myklebust <trondmy@hammerspace.com>]

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@oracle.com>
> ---
>  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.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com