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

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

[J. Bruce Fields]

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

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

[Trond Myklebust]

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.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

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

[bfields]

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.

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

[bfields]

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.

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

[Trond Myklebust]

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



-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

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

[bfields]

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

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

[bfields]

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.

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

[Trond Myklebust]

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.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

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

[bfields]

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.

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

[bfields]

From: "J. Bruce Fields" <bfields@redhat.com>

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@redhat.com>
---
 fs/nfs/nfs4proc.c | 6 ++++++
 1 file changed, 6 insertions(+)

On Mon, Feb 08, 2021 at 05:08:55PM -0500, bfields@fieldses.org 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

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

[Murphy Zhou]

Hi,

On Fri, Feb 12, 2021 at 2:58 AM bfields@fieldses.org
<bfields@fieldses.org> wrote:
>
> From: "J. Bruce Fields" <bfields@redhat.com>
>
> 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@redhat.com>
> ---
>  fs/nfs/nfs4proc.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> On Mon, Feb 08, 2021 at 05:08:55PM -0500, bfields@fieldses.org 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,

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

[Anna Schumaker]

Hi Murphy,

On Wed, Mar 3, 2021 at 9:30 PM Murphy Zhou <jencce.kernel@gmail.com> wrote:
>
> Hi,
>
> On Fri, Feb 12, 2021 at 2:58 AM bfields@fieldses.org
> <bfields@fieldses.org> wrote:
> >
> > From: "J. Bruce Fields" <bfields@redhat.com>
> >
> > 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@redhat.com>
> > ---
> >  fs/nfs/nfs4proc.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > On Mon, Feb 08, 2021 at 05:08:55PM -0500, bfields@fieldses.org 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,