[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 <***@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) {

[Alex Elder]

Post by 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).

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.

Post by Ilya Dryomov
---
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) {

--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Ilya Dryomov]

Post by Alex Elder

Post by 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).

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?

Post by Alex Elder
From 3ded0a7fee82f2204c58b4fc00fc74f05331514d Mon Sep 17 00:00:00 2001

From: 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 <***@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) {

[Alex Elder]

Post by Ilya Dryomov

Post by Alex Elder

Post by 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).

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 ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Ilya Dryomov]

Post by Alex Elder

Post by Ilya Dryomov

Post by Alex Elder

Post by 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).

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 ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Alex Elder]

Post by Ilya Dryomov

Post by Alex Elder
I should have asked this before. Why is a permission error

Post by Alex Elder
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 ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Alex Elder]

Post by Ilya Dryomov

Post by Alex Elder

Post by 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).

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

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...

Post by Ilya Dryomov
---
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 ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html