| Message ID | CALFYKtAH_9nQWWyOEcEectWzWQxsNBVHRSPVt_QbU8rfG1_YFA@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 09/11/2014 11:17 AM, Ilya Dryomov wrote: > On Thu, Sep 11, 2014 at 7:23 PM, Alex Elder <elder@ieee.org> wrote: >> On 09/11/2014 10:10 AM, Ilya Dryomov wrote: >>> Trying to map an image out of a pool for which we don't have an 'x' >>> permission bit fails with -ERANGE from ceph_extract_encoded_string() >>> due to an unsigned vs signed bug. Fix it and get rid of the -ENIVAL >>> sink, thus exposing rbd::get_id cls method return value. (I've seen >>> a bunch of unexplained -ERANGE reports, I bet this is it). >>> >>> Signed-off-by: Ilya Dryomov <ilya.dryomov@inktank.com> >> >> I often think people are annoyed by my explicit type casts >> all over the place. This (missed) one matters a lot. >> >> I think the -EINVAL was to ensure an error code that was >> expected by a write() call would be returned. > > Yeah, the way it's written it's possible in theory to get a positive > return value from rbd_dev_image_id(). Looking deeper, this sizeof() is > not needed at all - ceph_extract_encoded_string() deals with short > buffers as it should. As for the ret == sizeof(u32) (i.e. an empty > string), neither userspace nor us check against empty strings in > similar cases (object prefix, snapshot name, etc). I should have asked this before. Why is a permission error leading to ceph_extract_encoded_string() finding a short buffer? I didn't take the time to trace the error path you're talking about here all the way back. (I'm looking at your new patch in the mean time.) -Alex -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Sep 11, 2014 at 8:24 PM, Alex Elder <elder@ieee.org> wrote: > On 09/11/2014 11:17 AM, Ilya Dryomov wrote: >> On Thu, Sep 11, 2014 at 7:23 PM, Alex Elder <elder@ieee.org> wrote: >>> On 09/11/2014 10:10 AM, Ilya Dryomov wrote: >>>> Trying to map an image out of a pool for which we don't have an 'x' >>>> permission bit fails with -ERANGE from ceph_extract_encoded_string() >>>> due to an unsigned vs signed bug. Fix it and get rid of the -ENIVAL >>>> sink, thus exposing rbd::get_id cls method return value. (I've seen >>>> a bunch of unexplained -ERANGE reports, I bet this is it). >>>> >>>> Signed-off-by: Ilya Dryomov <ilya.dryomov@inktank.com> >>> >>> I often think people are annoyed by my explicit type casts >>> all over the place. This (missed) one matters a lot. >>> >>> I think the -EINVAL was to ensure an error code that was >>> expected by a write() call would be returned. >> >> Yeah, the way it's written it's possible in theory to get a positive >> return value from rbd_dev_image_id(). Looking deeper, this sizeof() is >> not needed at all - ceph_extract_encoded_string() deals with short >> buffers as it should. As for the ret == sizeof(u32) (i.e. an empty >> string), neither userspace nor us check against empty strings in >> similar cases (object prefix, snapshot name, etc). > > I should have asked this before. Why is a permission error > leading to ceph_extract_encoded_string() finding a short > buffer? I didn't take the time to trace the error path > you're talking about here all the way back. rbd_obj_method_sync() returns -EPERM, which when compared with size_t from sizeof() ends up a big positive. ceph_extract_encoded_string() is then called and the safe decode macro kicks in.. Thanks, Ilya -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/11/2014 11:27 AM, Ilya Dryomov wrote: >> I should have asked this before. Why is a permission error >> > leading to ceph_extract_encoded_string() finding a short >> > buffer? I didn't take the time to trace the error path >> > you're talking about here all the way back. > rbd_obj_method_sync() returns -EPERM, which when compared with size_t > from sizeof() ends up a big positive. ceph_extract_encoded_string() is > then called and the safe decode macro kicks in.. I somehow got lost along the way and thought rbd_obj_method_sync() was returning -ERANGE. The problem starts because rbd_obj_method_sync() returns an error other than -ENOENT, and that led me astray I think. Sorry for the confusion. -Alex -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/11/2014 11:17 AM, Ilya Dryomov wrote: > On Thu, Sep 11, 2014 at 7:23 PM, Alex Elder <elder@ieee.org> wrote: >> On 09/11/2014 10:10 AM, Ilya Dryomov wrote: >>> Trying to map an image out of a pool for which we don't have an 'x' >>> permission bit fails with -ERANGE from ceph_extract_encoded_string() >>> due to an unsigned vs signed bug. Fix it and get rid of the -ENIVAL >>> sink, thus exposing rbd::get_id cls method return value. (I've seen >>> a bunch of unexplained -ERANGE reports, I bet this is it). >>> >>> Signed-off-by: Ilya Dryomov <ilya.dryomov@inktank.com> >> >> I often think people are annoyed by my explicit type casts >> all over the place. This (missed) one matters a lot. >> >> I think the -EINVAL was to ensure an error code that was >> expected by a write() call would be returned. > > Yeah, the way it's written it's possible in theory to get a positive > return value from rbd_dev_image_id(). Looking deeper, this sizeof() is > not needed at all - ceph_extract_encoded_string() deals with short > buffers as it should. As for the ret == sizeof(u32) (i.e. an empty > string), neither userspace nor us check against empty strings in > similar cases (object prefix, snapshot name, etc). > > With the above in mind, how about this? > > From 3ded0a7fee82f2204c58b4fc00fc74f05331514d Mon Sep 17 00:00:00 2001 > From: Ilya Dryomov <ilya.dryomov@inktank.com> > Date: Thu, 11 Sep 2014 18:49:18 +0400 > Subject: [PATCH] rbd: do not return -ERANGE on auth failures > > Trying to map an image out of a pool for which we don't have an 'x' > permission bit fails with -ERANGE from ceph_extract_encoded_string() > due to an unsigned vs signed bug. Fix it and get rid of the -EINVAL > sink, thus propagating rbd::get_id cls method errors. (I've seen > a bunch of unexplained -ERANGE reports, I bet this is it). > > Signed-off-by: Ilya Dryomov <ilya.dryomov@inktank.com> So now we know that the value returned by rbd_dev_image_id() will be either 0 or a negative errno. It could still return something that write(2) isn't defined to return, but at least it's an error. That's OK with me... Reviewed-by: Alex Elder <elder@linaro.org> > --- > drivers/block/rbd.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c > index 4b97baf8afa3..ce457db5d847 100644 > --- a/drivers/block/rbd.c > +++ b/drivers/block/rbd.c > @@ -4924,7 +4924,7 @@ static int rbd_dev_image_id(struct rbd_device *rbd_dev) > ret = image_id ? 0 : -ENOMEM; > if (!ret) > rbd_dev->image_format = 1; > - } else if (ret > sizeof (__le32)) { > + } else if (ret >= 0) { > void *p = response; > > image_id = ceph_extract_encoded_string(&p, p + ret, > @@ -4932,8 +4932,6 @@ static int rbd_dev_image_id(struct rbd_device *rbd_dev) > ret = PTR_ERR_OR_ZERO(image_id); > if (!ret) > rbd_dev->image_format = 2; > - } else { > - ret = -EINVAL; > } > > if (!ret) { > -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c index 4b97baf8afa3..ce457db5d847 100644 --- a/drivers/block/rbd.c +++ b/drivers/block/rbd.c @@ -4924,7 +4924,7 @@ static int rbd_dev_image_id(struct rbd_device *rbd_dev) ret = image_id ? 0 : -ENOMEM; if (!ret) rbd_dev->image_format = 1; - } else if (ret > sizeof (__le32)) { + } else if (ret >= 0) { void *p = response; image_id = ceph_extract_encoded_string(&p, p + ret, @@ -4932,8 +4932,6 @@ static int rbd_dev_image_id(struct rbd_device *rbd_dev) ret = PTR_ERR_OR_ZERO(image_id); if (!ret) rbd_dev->image_format = 2; - } else { - ret = -EINVAL; } if (!ret) {