[PATCH] rbd: do not return -ERANGE on auth failure

[PATCH] rbd: do not return -ERANGE on auth failure

[Ilya Dryomov]

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>
---
 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..fe3726c62a37 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 > (int)sizeof(u32)) {
 		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) {
-- 
1.7.10.4

Re: [PATCH] rbd: do not return -ERANGE on auth failure

[Alex Elder]

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.

In any case, this looks good to 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..fe3726c62a37 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 > (int)sizeof(u32)) {
>  		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) {
> 

Re: [PATCH] rbd: do not return -ERANGE on auth failure

[Ilya Dryomov]

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

Re: [PATCH] rbd: do not return -ERANGE on auth failure

[Alex Elder]

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

Re: [PATCH] rbd: do not return -ERANGE on auth failure

[Ilya Dryomov]

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

Re: [PATCH] rbd: do not return -ERANGE on auth failure

[Alex Elder]

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

Re: [PATCH] rbd: do not return -ERANGE on auth failure

[Alex Elder]

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