diff mbox series

nfs: we don't support removing system.nfs4_acl

Message ID 20210128223638.GE29887@fieldses.org (mailing list archive)
State New
Headers show
Series nfs: we don't support removing system.nfs4_acl | expand

Commit Message

bfields@fieldses.org Jan. 28, 2021, 10:36 p.m. UTC
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(+)

Comments

Trond Myklebust Jan. 29, 2021, 1:37 a.m. UTC | #1
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.
bfields@fieldses.org Jan. 29, 2021, 2:35 a.m. UTC | #2
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.
bfields@fieldses.org Jan. 29, 2021, 2:50 a.m. UTC | #3
On Thu, Jan 28, 2021 at 09:35:27PM -0500, bfields@fieldses.org 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.

--b.
Trond Myklebust Jan. 31, 2021, 8:41 p.m. UTC | #4
On Thu, 2021-01-28 at 21:50 -0500, bfields@fieldses.org wrote:
> On Thu, Jan 28, 2021 at 09:35:27PM -0500, bfields@fieldses.org 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?

Windows, for instance has explicit support for such an ACL:
https://docs.microsoft.com/en-us/windows/win32/secauthz/null-dacls-and-empty-dacls
bfields@fieldses.org Jan. 31, 2021, 9:58 p.m. UTC | #5
On Sun, Jan 31, 2021 at 08:41:37PM +0000, Trond Myklebust wrote:
> On Thu, 2021-01-28 at 21:50 -0500, bfields@fieldses.org wrote:
> > On Thu, Jan 28, 2021 at 09:35:27PM -0500, bfields@fieldses.org 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.

--b.

> 
> Windows, for instance has explicit support for such an ACL:
> https://docs.microsoft.com/en-us/windows/win32/secauthz/null-dacls-and-empty-dacls
> 
> 
> 
> -- 
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> trond.myklebust@hammerspace.com
> 
>
bfields@fieldses.org Feb. 3, 2021, 8:07 p.m. UTC | #6
On Sun, Jan 31, 2021 at 04:58:43PM -0500, bfields@fieldses.org wrote:
> On Sun, Jan 31, 2021 at 08:41:37PM +0000, Trond Myklebust wrote:
> > On Thu, 2021-01-28 at 21:50 -0500, bfields@fieldses.org wrote:
> > > On Thu, Jan 28, 2021 at 09:35:27PM -0500, bfields@fieldses.org 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?

--b.
Trond Myklebust Feb. 8, 2021, 7:31 p.m. UTC | #7
On Wed, 2021-02-03 at 15:07 -0500, bfields@fieldses.org wrote:
> On Sun, Jan 31, 2021 at 04:58:43PM -0500, bfields@fieldses.org wrote:
> > On Sun, Jan 31, 2021 at 08:41:37PM +0000, Trond Myklebust wrote:
> > > On Thu, 2021-01-28 at 21:50 -0500, bfields@fieldses.org wrote:
> > > > On Thu, Jan 28, 2021 at 09:35:27PM -0500,
> > > > bfields@fieldses.org 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.
bfields@fieldses.org Feb. 8, 2021, 10:08 p.m. UTC | #8
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.

--b.
diff mbox series

Patch

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))