On Wed, 2021-02-03 at 15:07 -0500, bfields@xxxxxxxxxxxx wrote:
> On Sun, Jan 31, 2021 at 04:58:43PM -0500, bfields@xxxxxxxxxxxx wrote:
> > On Sun, Jan 31, 2021 at 08:41:37PM +0000, Trond Myklebust wrote:
> > > On Thu, 2021-01-28 at 21:50 -0500, bfields@xxxxxxxxxxxx wrote:
> > > > On Thu, Jan 28, 2021 at 09:35:27PM -0500,
> > > > bfields@xxxxxxxxxxxx wrote:
> > > > > Note that this patch doesn't prevent an application from
> > > > > setting a
> > > > > zero-length ACL. The xattr format is XDR with the first four
> > > > > bytes
> > > > > representing the number of ACEs, so you'd set a zero-length
> > > > > ACL by
> > > > > passing down a 4-byte all-zero buffer as the new value of the
> > > > > system.nfs4_acl xattr.
> > > > >
> > > > > A zero-length NULL buffer is what's used to implement
> > > > > removexattr:
> > > > >
> > > > > int
> > > > > __vfs_removexattr(struct dentry *dentry, const char *name)
> > > > > {
> > > > > ...
> > > > > return handler->set(handler, dentry, inode, name,
> > > > > NULL, 0,
> > > > > XATTR_REPLACE);
> > > > > }
> > > > >
> > > > > That's the case this patch covers.
> > > >
> > > > So, I should have said in the changelog, apologies--the
> > > > behavior
> > > > without
> > > > this patch is that when it gets a removexattr, the client sends
> > > > a
> > > > SETATTR with a bitmap claiming there's an ACL attribute, but a
> > > > zero-length attribute value list, and the server (correctly)
> > > > returns
> > > > BADXDR.
> > > >
> > >
> > > I don't see anything in the spec that prohibits a zero length
> > > array
> > > size for nfs41_aces<> or states that should return
> > > NFS4ERR_BADXDR. Why
> > > shouldn't we allow that?
> >
> > Again: I agree. And we do allow that, both before and after this
> > patch.
> >
> > There's a difference between a SETATTR with a zero-length body and
> > a
> > SETATTR with a body containing a zero-length ACL. The former is
> > bad
> > protocol, the latter is, I agree, fine.
>
> Are we on the same page now? Or should I update the changelog and
> resend?
>
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.
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@xxxxxxxxxxxxxxx
