Re: [PATCH 09/10] nfsd: cap decoded POSIX ACL count to bound sort cost

[Chuck Lever]

On 5/29/26 6:48 AM, Jeff Layton wrote:
> On Thu, 2026-05-28 at 20:07 -0400, Chuck Lever wrote:
>>
>> On Thu, May 28, 2026, at 7:11 PM, Chuck Lever wrote:
>>> On Thu, May 28, 2026, at 6:11 PM, Rick Macklem wrote:
>>>> On Thu, May 28, 2026 at 2:56 PM Jeff Layton <jlayton@xxxxxxxxxx> wrote:
>>>>>
>>>>> CAUTION: This email originated from outside of the University of Guelph. Do not click links or open attachments unless you recognize the sender and know the content is safe. If you are unsure, forward the message to ITHelp@xxxxxxxxxxx for review.
>>>>>
>>>>>
>>>>> From: Chris Mason <clm@xxxxxxxx>
>>>>>
>>>>> nfsd4_decode_posixacl() reads a u32 entry count off the wire and passes
>>>>> it straight to posix_acl_alloc() and sort_pacl_range(). The latter is
>>>>> an O(n^2) bubble sort, so a client-chosen count drives unbounded CPU in
>>>>> the server's compound processing path.
>>>>>
>>>>>     nfsd4_decode_posixacl()
>>>>>       xdr_stream_decode_u32(&count)       /* uncapped u32 */
>>>>>       posix_acl_alloc(count, GFP_KERNEL)
>>>>>       sort_pacl_range(*acl, 0, count - 1) /* O(n^2) bubble sort */
>>>>>
>>>>> The encoder side in the same file already rejects ACLs whose a_count
>>>>> exceeds NFS_ACL_MAX_ENTRIES, but the decoder introduced in commit
>>>>> 5fc51dfc2eb1 ("NFSD: Add support for XDR decoding POSIX draft ACLs")
>>>>> omitted the symmetric check.
>>>> My recollection is that Chuck didn't like this limit. He argued that it was
>>>> specific to the NFSv3 ACL protocol and that the limit on the size of a NFSv4
>>>> RPC message was sufficient.  I, personally, think that 1024 is a reasonable
>>>> limit for # of ACEs, but Chuck can jump in here if he doesn't agree.
>>>
>>> The NFSACL protocol limits the number of ACEs in an ACL to NFS_ACL_MAX_ENTRIES
>>> (1024). It’s a limit that is defined in the protocol itself.
>>>
>>> The NFSv4 protocol sets no similar limit. In particular, NFS_ACL_MAX_ENTRIES
>>> is not a constant defined or used by the NFSv4.x family of protocols IIRC.
>>>
>>> Using NFS_ACL_MAX_ENTRIES to cap the number of ACEs in NFSv4 ACLs is a
>>> convenience, but it adds technical debt (slight though it may be).
>>>
>>> So… We can define an implementation limit for NFSv4 ACL support in NFSD.
>>> But it shouldn’t be called NFS_ACL_MAX_ENTRIES, IMHO. For clarity of
>>> documentation, pick a number (could be 1024) and state in a comment that
>>> it is an NFSD implementation constraint, not a protocol limit. Name the
>>> constant something like NFSD4_MAX_yada to make it clear it is an
>>> implementation limit, not a protocol limit.
>>
>> A different take on this might be that we want to ensure that ACLs set
>> via the NFSv4 POSIX ACL extension can always be retrieved via NFSACL.
>> In that case, capping the ACE count at the same number makes sense.
>> As long as the reason for this is clearly mentioned.
>>
> 
> First, I'll note that the encoder already has this limit in place:
> 
> static __be32
> nfsd4_encode_posixacl(struct xdr_stream *xdr, struct svc_rqst *rqstp,
>                       struct posix_acl *acl)
> {
> [...]
>         if (acl->a_count > NFS_ACL_MAX_ENTRIES)
>                 return nfserr_resource;
> [...]
> }
> 
> ...so if you set an ACL that is longer than NFS_ACL_MAX_ENTRIES you
> already can't retrieve it with NFSv4. Given that, I went with the above
> suggestion, and added a comment to the patch. Seem ok?
> 
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index c6c50c376b23..eaf71c65d665 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -448,6 +448,14 @@ nfsd4_decode_posixacl(struct nfsd4_compoundargs *argp, struct posix_acl **acl)
>  
>         if (xdr_stream_decode_u32(argp->xdr, &count) < 0)
>                 return nfserr_bad_xdr;
> +       /*
> +        * RFC8881 doesn't define a max number of ACE's, but the NFSACL spec
> +        * does. For NFSv4, cap the number of entries to the v3 limit, as we
> +        * want to ensure that ACLs set via NFSv4 POSIX ACL extensions are
> +        * retrievable via NFSv3.

Or, more precisely, "retrievable via NFSACL."

Otherwise, LGTM.


> +        */
> +       if (count > NFS_ACL_MAX_ENTRIES)
> +               return nfserr_resource;
>  
>         *acl = posix_acl_alloc(count, GFP_KERNEL);
>         if (*acl == NULL)


-- 
Chuck Lever