Hi Murphy, On Wed, Mar 3, 2021 at 9:30 PM Murphy Zhou <jencce.kernel@xxxxxxxxx> wrote: > > Hi, > > On Fri, Feb 12, 2021 at 2:58 AM bfields@xxxxxxxxxxxx > <bfields@xxxxxxxxxxxx> wrote: > > > > From: "J. Bruce Fields" <bfields@xxxxxxxxxx> > > > > 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@xxxxxxxxxx> > > --- > > fs/nfs/nfs4proc.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > On Mon, Feb 08, 2021 at 05:08:55PM -0500, bfields@xxxxxxxxxxxx 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 ? Yeah, I have this queued up for the next bugfixes pull request. Anna > > > Thanks,
