rbd: do not return -ERANGE on auth failure

Commit Message

Ilya Dryomov Sept. 11, 2014, 4:17 p.m. UTC

On Thu, Sep 11, 2014 at 7:23 PM, Alex Elder <[e-mail]> 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 <[e-mail]>
>
> 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 <[e-mail]>
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 <[e-mail]>
---
 drivers/block/rbd.c |    4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

Comments

Alex Elder Sept. 11, 2014, 4:24 p.m. UTC | #1

On 09/11/2014 11:17 AM, Ilya Dryomov wrote:
> On Thu, Sep 11, 2014 at 7:23 PM, Alex Elder <[e-mail]> 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 <[e-mail]>
>>
>> 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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ilya Dryomov Sept. 11, 2014, 4:27 p.m. UTC | #2

On Thu, Sep 11, 2014 at 8:24 PM, Alex Elder <[e-mail]> wrote:
> On 09/11/2014 11:17 AM, Ilya Dryomov wrote:
>> On Thu, Sep 11, 2014 at 7:23 PM, Alex Elder <[e-mail]> 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 <[e-mail]>
>>>
>>> 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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Alex Elder Sept. 11, 2014, 4:43 p.m. UTC | #3

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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Alex Elder Sept. 11, 2014, 4:46 p.m. UTC | #4

On 09/11/2014 11:17 AM, Ilya Dryomov wrote:
> On Thu, Sep 11, 2014 at 7:23 PM, Alex Elder <[e-mail]> 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 <[e-mail]>
>>
>> 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 <[e-mail]>
> 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 <[e-mail]>

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 <[e-mail]>

> ---
>  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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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) {