| Message ID | 20210128223638.GE29887@fieldses.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nfs: we don't support removing system.nfs4_acl | expand |
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.
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.
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.
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
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 > >
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.
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.
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 --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))