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 ? Thanks,
