Re: [PATCH AUTOSEL for 4.15 168/189] nfsd: return RESOURCE not GARBAGE_ARGS on too many ops

[J. Bruce Fields]

What's your default on these patches on these AUTOSEL patches if you
don't get an ACK or NACK?  Do you apply them anyway?

(I'd skip this one as it doesn't meet the "It must fix a real bug that
bothers people" criterion.  But I don't recall it *causing* any bugs
either, so the stakes are low, I'm mainly just curious.)

--b.

On Mon, Apr 09, 2018 at 12:19:02AM +0000, Sasha Levin wrote:
> From: "J. Bruce Fields" <bfields@xxxxxxxxxx>
> 
> [ Upstream commit 0078117c6d9160031b866cfa1853514d4f6865d2 ]
> 
> A client that sends more than a hundred ops in a single compound
> currently gets an rpc-level GARBAGE_ARGS error.
> 
> It would be more helpful to return NFS4ERR_RESOURCE, since that gives
> the client a better idea how to recover (for example by splitting up the
> compound into smaller compounds).
> 
> This is all a bit academic since we've never actually seen a reason for
> clients to send such long compounds, but we may as well fix it.
> 
> While we're there, just use NFSD4_MAX_OPS_PER_COMPOUND == 16, the
> constant we already use in the 4.1 case, instead of hard-coding 100.
> Chances anyone actually uses even 16 ops per compound are small enough
> that I think there's a neglible risk or any regression.
> 
> This fixes pynfs test COMP6.
> 
> Reported-by: "Lu, Xinyu" <luxy.fnst@xxxxxxxxxxxxxx>
> Signed-off-by: J. Bruce Fields <bfields@xxxxxxxxxx>
> Signed-off-by: Sasha Levin <alexander.levin@xxxxxxxxxxxxx>
> ---
>  fs/nfsd/nfs4proc.c | 3 +++
>  fs/nfsd/nfs4xdr.c  | 9 +++++++--
>  2 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index effeeb4f556f..a0bed2b2004d 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -1703,6 +1703,9 @@ nfsd4_proc_compound(struct svc_rqst *rqstp)
>  	status = nfserr_minor_vers_mismatch;
>  	if (nfsd_minorversion(args->minorversion, NFSD_TEST) <= 0)
>  		goto out;
> +	status = nfserr_resource;
> +	if (args->opcnt > NFSD_MAX_OPS_PER_COMPOUND)
> +		goto out;
>  
>  	status = nfs41_check_op_ordering(args);
>  	if (status) {
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index 2c61c6b8ae09..5dcd7cb45b2d 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -1918,8 +1918,13 @@ nfsd4_decode_compound(struct nfsd4_compoundargs *argp)
>  
>  	if (argp->taglen > NFSD4_MAX_TAGLEN)
>  		goto xdr_error;
> -	if (argp->opcnt > 100)
> -		goto xdr_error;
> +	/*
> +	 * NFS4ERR_RESOURCE is a more helpful error than GARBAGE_ARGS
> +	 * here, so we return success at the xdr level so that
> +	 * nfsd4_proc can handle this is an NFS-level error.
> +	 */
> +	if (argp->opcnt > NFSD_MAX_OPS_PER_COMPOUND)
> +		return 0;
>  
>  	if (argp->opcnt > ARRAY_SIZE(argp->iops)) {
>  		argp->ops = kzalloc(argp->opcnt * sizeof(*argp->ops), GFP_KERNEL);
> -- 
> 2.15.1