Re: [PATCH v2] ceph: fix getxattr return values for vxattrs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Jun 12, 2019 at 6:52 AM Yan, Zheng <ukernel@xxxxxxxxx> 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=

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?

-- 
Patrick Donnelly, Ph.D.
He / Him / His
Senior Software Engineer
Red Hat Sunnyvale, CA
GPG: 19F28A586F808C2402351B93C3301A3E258DD79D



[Index of Archives]     [CEPH Users]     [Ceph Large]     [Information on CEPH]     [Linux BTRFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux