On Tue, 2025-06-10 at 12:05 -0400, Chuck Lever wrote:
> From: Chuck Lever <chuck.lever@xxxxxxxxxx>
>
> This limit has always been a sanity check; in nearly all cases a
> large COMPOUND is a sign of a malfunctioning client. The only real
> limit on COMPOUND size and complexity is the size of NFSD's send
> and receive buffers.
>
> However, there are a few cases where a large COMPOUND is sane. For
> example, when a client implementation wants to walk down a long file
> pathname in a single round trip.
>
> A small risk is that now a client can construct a COMPOUND request
> that can keep a single nfsd thread busy for quite some time.
>
You're right about the risk there. I wonder what we could do to
mitigate that?
Maybe get a timestamp at the start of the compound and then check vs.
that after every operation? If the compound is taking longer than a
some timeout, give up and return an error on the next operation?
Also, while I did suggest it, we should consider not removing this
limit altogether, and rather just increase it to something like a max
practical limit:
For instance, we have limits in the channel_attrs for ca_maxrequestsize
and ca_maxresponsesize. What's the smallest operation? If we had a
compound comprised of just those operations, how many would fit?
That would at least act as a sanity check against compounds that are
clearly nonsensical.
> Suggested-by: Jeff Layton <jlayton@xxxxxxxxxx>
> Signed-off-by: Chuck Lever <chuck.lever@xxxxxxxxxx>
> ---
> fs/nfsd/nfs4proc.c | 14 ++------------
> fs/nfsd/nfs4state.c | 1 -
> fs/nfsd/nfs4xdr.c | 4 +---
> fs/nfsd/nfsd.h | 3 ---
> fs/nfsd/xdr4.h | 1 -
> 5 files changed, 3 insertions(+), 20 deletions(-)
>
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index f13abbb13b38..f4edf222e00e 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -2842,20 +2842,10 @@ nfsd4_proc_compound(struct svc_rqst *rqstp)
>
> rqstp->rq_lease_breaker = (void **)&cstate->clp;
>
> - trace_nfsd_compound(rqstp, args->tag, args->taglen, args->client_opcnt);
> + trace_nfsd_compound(rqstp, args->tag, args->taglen, args->opcnt);
> while (!status && resp->opcnt < args->opcnt) {
> op = &args->ops[resp->opcnt++];
>
> - if (unlikely(resp->opcnt == NFSD_MAX_OPS_PER_COMPOUND)) {
> - /* If there are still more operations to process,
> - * stop here and report NFS4ERR_RESOURCE. */
> - if (cstate->minorversion == 0 &&
> - args->client_opcnt > resp->opcnt) {
> - op->status = nfserr_resource;
> - goto encode_op;
> - }
> - }
> -
> /*
> * The XDR decode routines may have pre-set op->status;
> * for example, if there is a miscellaneous XDR error
> @@ -2932,7 +2922,7 @@ nfsd4_proc_compound(struct svc_rqst *rqstp)
> status = op->status;
> }
>
> - trace_nfsd_compound_status(args->client_opcnt, resp->opcnt,
> + trace_nfsd_compound_status(args->opcnt, resp->opcnt,
> status, nfsd4_op_name(op->opnum));
>
> nfsd4_cstate_clear_replay(cstate);
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index d5694987f86f..4b6ae8e54cd2 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -3872,7 +3872,6 @@ static __be32 check_forechannel_attrs(struct nfsd4_channel_attrs *ca, struct nfs
> ca->headerpadsz = 0;
> ca->maxreq_sz = min_t(u32, ca->maxreq_sz, maxrpc);
> ca->maxresp_sz = min_t(u32, ca->maxresp_sz, maxrpc);
> - ca->maxops = min_t(u32, ca->maxops, NFSD_MAX_OPS_PER_COMPOUND);
> ca->maxresp_cached = min_t(u32, ca->maxresp_cached,
> NFSD_SLOT_CACHE_SIZE + NFSD_MIN_HDR_SEQ_SZ);
> ca->maxreqs = min_t(u32, ca->maxreqs, NFSD_MAX_SLOTS_PER_SESSION);
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index 3afcdbed6e14..ea91bad4eee2 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -2500,10 +2500,8 @@ nfsd4_decode_compound(struct nfsd4_compoundargs *argp)
>
> if (xdr_stream_decode_u32(argp->xdr, &argp->minorversion) < 0)
> return false;
> - if (xdr_stream_decode_u32(argp->xdr, &argp->client_opcnt) < 0)
> + if (xdr_stream_decode_u32(argp->xdr, &argp->opcnt) < 0)
> return false;
> - argp->opcnt = min_t(u32, argp->client_opcnt,
> - NFSD_MAX_OPS_PER_COMPOUND);
>
> if (argp->opcnt > ARRAY_SIZE(argp->iops)) {
> argp->ops = vcalloc(argp->opcnt, sizeof(*argp->ops));
> diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h
> index 570065285e67..54a96042f5ac 100644
> --- a/fs/nfsd/nfsd.h
> +++ b/fs/nfsd/nfsd.h
> @@ -57,9 +57,6 @@ struct readdir_cd {
> __be32 err; /* 0, nfserr, or nfserr_eof */
> };
>
> -/* Maximum number of operations per session compound */
> -#define NFSD_MAX_OPS_PER_COMPOUND 50
> -
> struct nfsd_genl_rqstp {
> struct sockaddr rq_daddr;
> struct sockaddr rq_saddr;
> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> index aa2a356da784..a23bc56051ca 100644
> --- a/fs/nfsd/xdr4.h
> +++ b/fs/nfsd/xdr4.h
> @@ -870,7 +870,6 @@ struct nfsd4_compoundargs {
> char * tag;
> u32 taglen;
> u32 minorversion;
> - u32 client_opcnt;
> u32 opcnt;
> bool splice_ok;
> struct nfsd4_op *ops;
--
Jeff Layton <jlayton@xxxxxxxxxx>
