[PATCH 0/2] rbd_img_obj_exists_submit() fixes

[PATCH 0/2] rbd_img_obj_exists_submit() fixes

[David Disseldorp]

Hi,

This patchset fixes a couple of bugs in the rbd_img_obj_exists_submit()
code-path which appear to have been there since the function was first
added.

Feedback appreciated.

Cheers, David

 drivers/block/rbd.c | 31 ++++++++++++++++++++-----------
 1 file changed, 20 insertions(+), 11 deletions(-)

[PATCH 1/2] rbd: fix invalid img_request dereference

[David Disseldorp]

To obtain the snap_id, rbd_osd_req_format_read() dereferences
rbd_obj_request->img_request if non-null. This should never happen in
the rbd_img_obj_exists_submit() call path, as stat_request->img_request
is invalid - it's overwritten by stat_request->obj_request, which is
stored in the same union.

Fixes: c5b5ef6c5 ("rbd: issue stat request before layered write")

Signed-off-by: David Disseldorp <ddiss@suse.de>
---
 drivers/block/rbd.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 6c6519f..93d6200 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -1875,9 +1875,9 @@ static void rbd_osd_req_callback(struct ceph_osd_request *osd_req)
 		rbd_obj_request_complete(obj_request);
 }
 
-static void rbd_osd_req_format_read(struct rbd_obj_request *obj_request)
+static void rbd_osd_req_format_read(struct rbd_obj_request *obj_request,
+				    struct rbd_img_request *img_request)
 {
-	struct rbd_img_request *img_request = obj_request->img_request;
 	struct ceph_osd_request *osd_req = obj_request->osd_req;
 
 	if (img_request)
@@ -2420,7 +2420,7 @@ static void rbd_img_obj_request_fill(struct rbd_obj_request *obj_request,
 	if (op_type == OBJ_OP_WRITE || op_type == OBJ_OP_DISCARD)
 		rbd_osd_req_format_write(obj_request);
 	else
-		rbd_osd_req_format_read(obj_request);
+		rbd_osd_req_format_read(obj_request, img_request);
 }
 
 /*
@@ -2877,7 +2877,11 @@ static int rbd_img_obj_exists_submit(struct rbd_obj_request *obj_request)
 	osd_req_op_init(stat_request->osd_req, 0, CEPH_OSD_OP_STAT, 0);
 	osd_req_op_raw_data_in_pages(stat_request->osd_req, 0, pages, size, 0,
 					false, false);
-	rbd_osd_req_format_read(stat_request);
+	/*
+	 * Use img_request from original obj_request - stat_request->img_request
+	 * is invalid (in a union with stat_request->obj_request).
+	 */
+	rbd_osd_req_format_read(stat_request, obj_request->img_request);
 
 	osdc = &rbd_dev->rbd_client->client->osdc;
 	ret = rbd_obj_request_submit(osdc, stat_request);
@@ -3242,7 +3246,7 @@ static int rbd_obj_method_sync(struct rbd_device *rbd_dev,
 	osd_req_op_cls_response_data_pages(obj_request->osd_req, 0,
 					obj_request->pages, inbound_size,
 					0, false, false);
-	rbd_osd_req_format_read(obj_request);
+	rbd_osd_req_format_read(obj_request, obj_request->img_request);
 
 	ret = rbd_obj_request_submit(osdc, obj_request);
 	if (ret)
@@ -3448,7 +3452,7 @@ static int rbd_obj_read_sync(struct rbd_device *rbd_dev,
 					obj_request->length,
 					obj_request->offset & ~PAGE_MASK,
 					false, false);
-	rbd_osd_req_format_read(obj_request);
+	rbd_osd_req_format_read(obj_request, obj_request->img_request);
 
 	ret = rbd_obj_request_submit(osdc, obj_request);
 	if (ret)
-- 
2.6.6

[PATCH 2/2] rbd: fix rbd_img_obj_exists_submit() error path

[David Disseldorp]

A couple of issues addressed:
- obj_request put before get.
- stat_request leaked, including payload pages.

Fixes: c5b5ef6c5 ("rbd: issue stat request before layered write")

Signed-off-by: David Disseldorp <ddiss@suse.de>
---
 drivers/block/rbd.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 93d6200..970fea6 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -2858,8 +2858,10 @@ static int rbd_img_obj_exists_submit(struct rbd_obj_request *obj_request)
 	ret = -ENOMEM;
 	stat_request = rbd_obj_request_create(obj_request->object_name, 0, 0,
 							OBJ_REQUEST_PAGES);
-	if (!stat_request)
+	if (!stat_request) {
+		ceph_release_page_vector(pages, page_count);
 		goto out;
+	}
 
 	rbd_obj_request_get(obj_request);
 	stat_request->obj_request = obj_request;
@@ -2871,7 +2873,7 @@ static int rbd_img_obj_exists_submit(struct rbd_obj_request *obj_request)
 	stat_request->osd_req = rbd_osd_req_create(rbd_dev, OBJ_OP_READ, 1,
 						   stat_request);
 	if (!stat_request->osd_req)
-		goto out;
+		goto out_cleanup;
 	stat_request->callback = rbd_img_obj_exists_callback;
 
 	osd_req_op_init(stat_request->osd_req, 0, CEPH_OSD_OP_STAT, 0);
@@ -2885,10 +2887,13 @@ static int rbd_img_obj_exists_submit(struct rbd_obj_request *obj_request)
 
 	osdc = &rbd_dev->rbd_client->client->osdc;
 	ret = rbd_obj_request_submit(osdc, stat_request);
-out:
-	if (ret)
+out_cleanup:
+	if (ret) {
 		rbd_obj_request_put(obj_request);
-
+		stat_request->img_request = NULL;
+		rbd_obj_request_put(stat_request);
+	}
+out:
 	return ret;
 }
 
-- 
2.6.6

Re: [PATCH 0/2] rbd_img_obj_exists_submit() fixes

[David Disseldorp]

On Thu,  1 Sep 2016 16:59:17 +0200, David Disseldorp wrote:

> Hi,
> 
> This patchset fixes a couple of bugs in the rbd_img_obj_exists_submit()
> code-path which appear to have been there since the function was first
> added.
> 
> Feedback appreciated.

Did anyone get a chance to look at these changes?

Cheers, David

Re: [PATCH 0/2] rbd_img_obj_exists_submit() fixes

[Ilya Dryomov]

On Thu, Sep 15, 2016 at 12:48 PM, David Disseldorp <ddiss@suse.de> wrote:
> On Thu,  1 Sep 2016 16:59:17 +0200, David Disseldorp wrote:
>
>> Hi,
>>
>> This patchset fixes a couple of bugs in the rbd_img_obj_exists_submit()
>> code-path which appear to have been there since the function was first
>> added.
>>
>> Feedback appreciated.
>
> Did anyone get a chance to look at these changes?

Hi David,

I was waiting for Alex to take a look, but then realized that there is
more to fix: img_request is also getting leaked and we can ignore
ceph_osdc_start_request return value, simplifying things.

For the invalid union field use issue, I think we can just skip calling
rbd_osd_req_format_read() for !img_data requests, but I need to double
check.

I'll put together a branch my version of these fixes and ping you.

Thanks,

                Ilya

Re: [PATCH 0/2] rbd_img_obj_exists_submit() fixes

[Alex Elder]

On 09/15/2016 06:53 AM, Ilya Dryomov wrote:
> On Thu, Sep 15, 2016 at 12:48 PM, David Disseldorp <ddiss@suse.de> wrote:
>> On Thu,  1 Sep 2016 16:59:17 +0200, David Disseldorp wrote:
>>
>>> Hi,
>>>
>>> This patchset fixes a couple of bugs in the rbd_img_obj_exists_submit()
>>> code-path which appear to have been there since the function was first
>>> added.
>>>
>>> Feedback appreciated.
>>
>> Did anyone get a chance to look at these changes?
> 
> Hi David,
> 
> I was waiting for Alex to take a look, but then realized that there is
> more to fix: img_request is also getting leaked and we can ignore
> ceph_osdc_start_request return value, simplifying things.

Yeah, it has been my intention to look at these, sorry I
haven't gotten to it yet.  I'll try to look today.  I can
comment on yours even knowing Ilya plans to send out a
different version.

					-Alex

> 
> For the invalid union field use issue, I think we can just skip calling
> rbd_osd_req_format_read() for !img_data requests, but I need to double
> check.
> 
> I'll put together a branch my version of these fixes and ping you.
> 
> Thanks,
> 
>                 Ilya
> 

Re: [PATCH 0/2] rbd_img_obj_exists_submit() fixes

[Ilya Dryomov]

On Thu, Sep 15, 2016 at 1:53 PM, Ilya Dryomov <idryomov@gmail.com> wrote:
> On Thu, Sep 15, 2016 at 12:48 PM, David Disseldorp <ddiss@suse.de> wrote:
>> On Thu,  1 Sep 2016 16:59:17 +0200, David Disseldorp wrote:
>>
>>> Hi,
>>>
>>> This patchset fixes a couple of bugs in the rbd_img_obj_exists_submit()
>>> code-path which appear to have been there since the function was first
>>> added.
>>>
>>> Feedback appreciated.
>>
>> Did anyone get a chance to look at these changes?
>
> Hi David,
>
> I was waiting for Alex to take a look, but then realized that there is
> more to fix: img_request is also getting leaked and we can ignore
> ceph_osdc_start_request return value, simplifying things.
>
> For the invalid union field use issue, I think we can just skip calling
> rbd_osd_req_format_read() for !img_data requests, but I need to double
> check.
>
> I'll put together a branch my version of these fixes and ping you.

Hi David,

I've pushed to testing @ ceph-client.git and posted to the list.
See patches 6 and 7.

Thanks,

                Ilya

Re: [PATCH 0/2] rbd_img_obj_exists_submit() fixes

[David Disseldorp]

On Mon, 19 Sep 2016 19:06:45 +0200, Ilya Dryomov wrote:

> Hi David,
> 
> I've pushed to testing @ ceph-client.git and posted to the list.
> See patches 6 and 7.

Thanks Ilya. I'll put these on my test/review queue.

Cheers, David