Re: [PATCH] nfs: we don't support removing system.nfs4_acl

[Murphy Zhou <jencce.kernel@gmail.com>]

Hi,

On Fri, Feb 12, 2021 at 2:58 AM bfields@fieldses.org
<bfields@fieldses.org> wrote:
>
> From: "J. Bruce Fields" <bfields@redhat.com>
>
> The contents of the system.nfs4_acl xattr are literally just the
> xdr-encoded ACL attribute.  That attribute starts with a 4-byte integer
> representing the number of ACEs in the ACL.  So even a zero-ACE ACL will
> be at least 4 bytes.
>
> We've never actually bothered to sanity-check the ACL encoding that
> userspace gives us.  The only problem that causes is that we return an
> error that's probably wrong.  (The server will return BADXDR, which
> we'll translate to EIO, when EINVAL would make more sense.)
>
> It's not much a problem in practice since the standard utilities give us
> well-formed XDR.  The one case we're likely to see from userspace in
> practice is a set of a zero-length xattr since that's how
>
>         removexattr(path, "system.nfs4_acl")
>
> is implemented.  It's worth trying to give a better error for that case.
>
> Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> ---
>  fs/nfs/nfs4proc.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> On Mon, Feb 08, 2021 at 05:08:55PM -0500, bfields@fieldses.org wrote:
> > On Mon, Feb 08, 2021 at 07:31:38PM +0000, Trond Myklebust wrote:
> > > OK. So you're not really saying that the SETATTR has a zero length
> > > body, but that the ACL attribute in this case has a zero length body,
> > > whereas in the 'empty acl' case, it is supposed to have a body
> > > containing a zero-length nfsace4<> array. Fair enough.
> >
> > Yep!  I'll see if I can think of a helpful concise comment, and resend.
>
> Oops, forgot about this, here you go.--b.
>
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 2f4679a62712..86e87f7d7686 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -5895,6 +5895,12 @@ static int __nfs4_proc_set_acl(struct inode *inode, const void *buf, size_t bufl
>         unsigned int npages = DIV_ROUND_UP(buflen, PAGE_SIZE);
>         int ret, i;
>
> +       /*
> +        * We don't support removing system.nfs4_acl, and even a
> +        * 0-length ACL needs at least 4 bytes for the number of ACEs:
> +        */
> +       if (buflen < 4)
> +               return -EINVAL;
>         if (!nfs4_server_supports_acls(server))
>                 return -EOPNOTSUPP;
>         if (npages > ARRAY_SIZE(pages))
> --
> 2.29.2
>

Has this queued up for the next RC ?


Thanks,