| Message ID | 20190607184749.8333-1-jlayton@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] ceph: fix getxattr return values for vxattrs | expand |
On Fri, Jun 7, 2019 at 11:48 AM Jeff Layton <jlayton@kernel.org> wrote: > > We have several virtual xattrs in cephfs which return various values as > strings. xattrs don't necessarily return strings however, so we need to > include the terminating NULL byte when we return the length. > > Furthermore, the getxattr manpage says that we should return -ERANGE if > the buffer is too small to hold the resulting value. Let's start doing > that here as well. > > URL: https://bugzilla.redhat.com/show_bug.cgi?id=1717454 > Reported-by: Tomas Petr <tpetr@redhat.com> > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > fs/ceph/xattr.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c > index 6621d27e64f5..2a61e02e7166 100644 > --- a/fs/ceph/xattr.c > +++ b/fs/ceph/xattr.c > @@ -803,8 +803,15 @@ ssize_t __ceph_getxattr(struct inode *inode, const char *name, void *value, > if (err) > return err; > err = -ENODATA; > - if (!(vxattr->exists_cb && !vxattr->exists_cb(ci))) > - err = vxattr->getxattr_cb(ci, value, size); > + if (!(vxattr->exists_cb && !vxattr->exists_cb(ci))) { > + /* > + * getxattr_cb returns strlen(value), xattr length must > + * include the NULL. > + */ > + err = vxattr->getxattr_cb(ci, value, size) + 1; > + if (size && size < err) > + err = -ERANGE; > + } > return err; > } Reviewed-by: Patrick Donnelly <pdonnell@redhat.com>
On Sat, Jun 8, 2019 at 2:47 AM Jeff Layton <jlayton@kernel.org> wrote: > > We have several virtual xattrs in cephfs which return various values as > strings. xattrs don't necessarily return strings however, so we need to > include the terminating NULL byte when we return the length. > > Furthermore, the getxattr manpage says that we should return -ERANGE if > the buffer is too small to hold the resulting value. Let's start doing > that here as well. > > URL: https://bugzilla.redhat.com/show_bug.cgi?id=1717454 > Reported-by: Tomas Petr <tpetr@redhat.com> > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > fs/ceph/xattr.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c > index 6621d27e64f5..2a61e02e7166 100644 > --- a/fs/ceph/xattr.c > +++ b/fs/ceph/xattr.c > @@ -803,8 +803,15 @@ ssize_t __ceph_getxattr(struct inode *inode, const char *name, void *value, > if (err) > return err; > err = -ENODATA; > - if (!(vxattr->exists_cb && !vxattr->exists_cb(ci))) > - err = vxattr->getxattr_cb(ci, value, size); > + if (!(vxattr->exists_cb && !vxattr->exists_cb(ci))) { > + /* > + * getxattr_cb returns strlen(value), xattr length must > + * include the NULL. > + */ > + err = vxattr->getxattr_cb(ci, value, size) + 1; This confuses getfattr. also causes failures of our test cases. run getfattr without the patch, we get: [root@kvm2-alpha ceph]# getfattr -n ceph.dir.entries . # file: . ceph.dir.entries="1" with the patch, we get [root@kvm1-alpha ceph]# getfattr -n ceph.dir.entries . # file: . ceph.dir.entries=0sMQA= > + if (size && size < err) > + err = -ERANGE; > + } > return err; > } > > -- > 2.21.0 > _______________________________________________ > Dev mailing list -- dev@ceph.io > To unsubscribe send an email to dev-leave@ceph.io
On Wed, Jun 12, 2019 at 6:52 AM Yan, Zheng <ukernel@gmail.com> wrote: > > On Sat, Jun 8, 2019 at 2:47 AM Jeff Layton <jlayton@kernel.org> wrote: > > > > We have several virtual xattrs in cephfs which return various values as > > strings. xattrs don't necessarily return strings however, so we need to > > include the terminating NULL byte when we return the length. > > > > Furthermore, the getxattr manpage says that we should return -ERANGE if > > the buffer is too small to hold the resulting value. Let's start doing > > that here as well. > > > > URL: https://bugzilla.redhat.com/show_bug.cgi?id=1717454 > > Reported-by: Tomas Petr <tpetr@redhat.com> > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > --- > > fs/ceph/xattr.c | 11 +++++++++-- > > 1 file changed, 9 insertions(+), 2 deletions(-) > > > > diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c > > index 6621d27e64f5..2a61e02e7166 100644 > > --- a/fs/ceph/xattr.c > > +++ b/fs/ceph/xattr.c > > @@ -803,8 +803,15 @@ ssize_t __ceph_getxattr(struct inode *inode, const char *name, void *value, > > if (err) > > return err; > > err = -ENODATA; > > - if (!(vxattr->exists_cb && !vxattr->exists_cb(ci))) > > - err = vxattr->getxattr_cb(ci, value, size); > > + if (!(vxattr->exists_cb && !vxattr->exists_cb(ci))) { > > + /* > > + * getxattr_cb returns strlen(value), xattr length must > > + * include the NULL. > > + */ > > + err = vxattr->getxattr_cb(ci, value, size) + 1; > > This confuses getfattr. also causes failures of our test cases. > > run getfattr without the patch, we get: > [root@kvm2-alpha ceph]# getfattr -n ceph.dir.entries . > # file: . > ceph.dir.entries="1" > > > with the patch, we get > [root@kvm1-alpha ceph]# getfattr -n ceph.dir.entries . > # file: . > ceph.dir.entries=0sMQA= So the trick here is we need the user buffer to be large enough to hold the NUL byte so we can use snprintf in the kernel. So I think if the buffer is not large enough, return ERANGE or size+1 (NUL byte) if value==NULL. If the buffer is large enough (size+1), then we return size and not size+1. Sound reasonable?
On Wed, 2019-06-12 at 21:51 +0800, Yan, Zheng wrote: > On Sat, Jun 8, 2019 at 2:47 AM Jeff Layton <jlayton@kernel.org> wrote: > > We have several virtual xattrs in cephfs which return various values as > > strings. xattrs don't necessarily return strings however, so we need to > > include the terminating NULL byte when we return the length. > > > > Furthermore, the getxattr manpage says that we should return -ERANGE if > > the buffer is too small to hold the resulting value. Let's start doing > > that here as well. > > > > URL: https://bugzilla.redhat.com/show_bug.cgi?id=1717454 > > Reported-by: Tomas Petr <tpetr@redhat.com> > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > --- > > fs/ceph/xattr.c | 11 +++++++++-- > > 1 file changed, 9 insertions(+), 2 deletions(-) > > > > diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c > > index 6621d27e64f5..2a61e02e7166 100644 > > --- a/fs/ceph/xattr.c > > +++ b/fs/ceph/xattr.c > > @@ -803,8 +803,15 @@ ssize_t __ceph_getxattr(struct inode *inode, const char *name, void *value, > > if (err) > > return err; > > err = -ENODATA; > > - if (!(vxattr->exists_cb && !vxattr->exists_cb(ci))) > > - err = vxattr->getxattr_cb(ci, value, size); > > + if (!(vxattr->exists_cb && !vxattr->exists_cb(ci))) { > > + /* > > + * getxattr_cb returns strlen(value), xattr length must > > + * include the NULL. > > + */ > > + err = vxattr->getxattr_cb(ci, value, size) + 1; > > This confuses getfattr. also causes failures of our test cases. > > run getfattr without the patch, we get: > [root@kvm2-alpha ceph]# getfattr -n ceph.dir.entries . > # file: . > ceph.dir.entries="1" > > > with the patch, we get > [root@kvm1-alpha ceph]# getfattr -n ceph.dir.entries . > # file: . > ceph.dir.entries=0sMQA= > > (cc'ing Andreas...) I have to wonder if this is a bug in getfattr. If I do this, then it works: $ getfattr -e text -n ceph.dir.entries . # file: . ceph.dir.entries="1" ...which makes me think that we're running afoul of well_enough_printable() in getfattr command. When I set a "user." xattr to a (short) string, it doesn't include the null terminator. security.selinux however, does include the NULL terminating byte in the returned length. I guess though, that it's returning long enough strings that well_enough_printable returns true. Andreas, we have a number of "virtual" xattrs in cephfs. When we're returning xattr values that are strings, should the returned length include the NULL terminating byte? > > + if (size && size < err) > > + err = -ERANGE; > > + } > > return err; > > } > > > > -- > > 2.21.0 > > _______________________________________________ > > Dev mailing list -- dev@ceph.io > > To unsubscribe send an email to dev-leave@ceph.io Thanks,
diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c index 6621d27e64f5..2a61e02e7166 100644 --- a/fs/ceph/xattr.c +++ b/fs/ceph/xattr.c @@ -803,8 +803,15 @@ ssize_t __ceph_getxattr(struct inode *inode, const char *name, void *value, if (err) return err; err = -ENODATA; - if (!(vxattr->exists_cb && !vxattr->exists_cb(ci))) - err = vxattr->getxattr_cb(ci, value, size); + if (!(vxattr->exists_cb && !vxattr->exists_cb(ci))) { + /* + * getxattr_cb returns strlen(value), xattr length must + * include the NULL. + */ + err = vxattr->getxattr_cb(ci, value, size) + 1; + if (size && size < err) + err = -ERANGE; + } return err; }
We have several virtual xattrs in cephfs which return various values as strings. xattrs don't necessarily return strings however, so we need to include the terminating NULL byte when we return the length. Furthermore, the getxattr manpage says that we should return -ERANGE if the buffer is too small to hold the resulting value. Let's start doing that here as well. URL: https://bugzilla.redhat.com/show_bug.cgi?id=1717454 Reported-by: Tomas Petr <tpetr@redhat.com> Signed-off-by: Jeff Layton <jlayton@kernel.org> --- fs/ceph/xattr.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)