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