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

["bfields@fieldses.org" <bfields@fieldses.org>]

On Fri, Jan 29, 2021 at 01:37:10AM +0000, Trond Myklebust wrote:
> On Fri, 2021-01-29 at 01:34 +0200, guy keren wrote:
> > On 1/29/21 12:36 AM, J. Bruce Fields wrote:
> > From: "J. Bruce Fields" <bfields@redhat.com>
> > 
> > The NFSv4 protocol doesn't have any notion of reomoving an attribute,
> > so
> > removexattr(path,"system.nfs4_acl") doesn't make sense.
> > 
> > There's no documented return value.  Arguably it could be EOPNOTSUPP
> > but
> > I'm a little worried an application might take that to mean that we
> > don't support ACLs or xattrs.  How about EINVAL?
> > 
> > Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> > ---
> >  fs/nfs/nfs4proc.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> > index 2f4679a62712..d50dea5f5723 100644
> > --- a/fs/nfs/nfs4proc.c
> > +++ b/fs/nfs/nfs4proc.c
> > @@ -5895,6 +5895,9 @@ 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;
> >  
> > +	/* You can't remove system.nfs4_acl: */
> > +	if (buflen == 0)
> > +		return -EINVAL;
> >  	if (!nfs4_server_supports_acls(server))
> >  		return -EOPNOTSUPP;
> >  	if (npages > ARRAY_SIZE(pages))
> > 
> > question: what happens if someone is attempting to create an empty
> > ACL on a file? as far as i know, this is legal.
> > won't you arrive into this position with a buflen of 0? it should be
> > similar to 'chmod 0 <file>'.
> > 
> 
> Agreed. If the server doesn't support removing the ACL then it should
> be up to it to enforce that condition. I see nothing in the NFS
> protocol that says it is up to the NFS client to act as the enforcer
> here.

Agreed.

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.

--b.