On 6/10/25 1:01 PM, Jeff Layton wrote:
> 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?
I'm open to thinking about additional guard rails.
The problem with a timeout is that any single operation can take a long
time -- if the underlying media is malfunctioning or if the remote NFS
server for a re-export is unreachable, for example.
> 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.
Relying on the size of the COMPOUND itself should be sufficient. If the
whole COMPOUND can't fit in ca_maxrequestsize, that's effectively the
same thing as limiting the number of ops based on the maxrequestsize
value.
>> 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;
>
--
Chuck Lever
