On Wed, 2019-06-12 at 21:51 +0800, Yan, Zheng wrote: > On Sat, Jun 8, 2019 at 2:47 AM Jeff Layton <jlayton@xxxxxxxxxx> 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@xxxxxxxxxx> > > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx> > > --- > > 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@xxxxxxx > > To unsubscribe send an email to dev-leave@xxxxxxx Thanks, -- Jeff Layton <jlayton@xxxxxxxxxx>
