* [PATCH 0/2] rbd_img_obj_exists_submit() fixes
@ 2016-09-01 14:59 David Disseldorp
2016-09-01 14:59 ` [PATCH 1/2] rbd: fix invalid img_request dereference David Disseldorp
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: David Disseldorp @ 2016-09-01 14:59 UTC (permalink / raw)
To: ceph-devel; +Cc: elder
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(-)
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH 1/2] rbd: fix invalid img_request dereference 2016-09-01 14:59 [PATCH 0/2] rbd_img_obj_exists_submit() fixes David Disseldorp @ 2016-09-01 14:59 ` David Disseldorp 2016-09-01 14:59 ` [PATCH 2/2] rbd: fix rbd_img_obj_exists_submit() error path David Disseldorp 2016-09-15 10:48 ` [PATCH 0/2] rbd_img_obj_exists_submit() fixes David Disseldorp 2 siblings, 0 replies; 8+ messages in thread From: David Disseldorp @ 2016-09-01 14:59 UTC (permalink / raw) To: ceph-devel; +Cc: elder, 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/2] rbd: fix rbd_img_obj_exists_submit() error path 2016-09-01 14:59 [PATCH 0/2] rbd_img_obj_exists_submit() fixes David Disseldorp 2016-09-01 14:59 ` [PATCH 1/2] rbd: fix invalid img_request dereference David Disseldorp @ 2016-09-01 14:59 ` David Disseldorp 2016-09-15 10:48 ` [PATCH 0/2] rbd_img_obj_exists_submit() fixes David Disseldorp 2 siblings, 0 replies; 8+ messages in thread From: David Disseldorp @ 2016-09-01 14:59 UTC (permalink / raw) To: ceph-devel; +Cc: elder, 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 0/2] rbd_img_obj_exists_submit() fixes 2016-09-01 14:59 [PATCH 0/2] rbd_img_obj_exists_submit() fixes David Disseldorp 2016-09-01 14:59 ` [PATCH 1/2] rbd: fix invalid img_request dereference David Disseldorp 2016-09-01 14:59 ` [PATCH 2/2] rbd: fix rbd_img_obj_exists_submit() error path David Disseldorp @ 2016-09-15 10:48 ` David Disseldorp 2016-09-15 11:53 ` Ilya Dryomov 2 siblings, 1 reply; 8+ messages in thread From: David Disseldorp @ 2016-09-15 10:48 UTC (permalink / raw) To: ceph-devel; +Cc: elder, Ilya Dryomov 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 0/2] rbd_img_obj_exists_submit() fixes 2016-09-15 10:48 ` [PATCH 0/2] rbd_img_obj_exists_submit() fixes David Disseldorp @ 2016-09-15 11:53 ` Ilya Dryomov 2016-09-15 12:25 ` Alex Elder 2016-09-19 17:06 ` Ilya Dryomov 0 siblings, 2 replies; 8+ messages in thread From: Ilya Dryomov @ 2016-09-15 11:53 UTC (permalink / raw) To: David Disseldorp; +Cc: Ceph Development, Alex Elder 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 0/2] rbd_img_obj_exists_submit() fixes 2016-09-15 11:53 ` Ilya Dryomov @ 2016-09-15 12:25 ` Alex Elder 2016-09-19 17:06 ` Ilya Dryomov 1 sibling, 0 replies; 8+ messages in thread From: Alex Elder @ 2016-09-15 12:25 UTC (permalink / raw) To: Ilya Dryomov, David Disseldorp; +Cc: Ceph Development 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 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 0/2] rbd_img_obj_exists_submit() fixes 2016-09-15 11:53 ` Ilya Dryomov 2016-09-15 12:25 ` Alex Elder @ 2016-09-19 17:06 ` Ilya Dryomov 2016-09-19 22:25 ` David Disseldorp 1 sibling, 1 reply; 8+ messages in thread From: Ilya Dryomov @ 2016-09-19 17:06 UTC (permalink / raw) To: David Disseldorp; +Cc: Ceph Development, Alex Elder 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 0/2] rbd_img_obj_exists_submit() fixes 2016-09-19 17:06 ` Ilya Dryomov @ 2016-09-19 22:25 ` David Disseldorp 0 siblings, 0 replies; 8+ messages in thread From: David Disseldorp @ 2016-09-19 22:25 UTC (permalink / raw) To: Ilya Dryomov; +Cc: Ceph Development, Alex Elder 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-09-19 22:25 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-09-01 14:59 [PATCH 0/2] rbd_img_obj_exists_submit() fixes David Disseldorp 2016-09-01 14:59 ` [PATCH 1/2] rbd: fix invalid img_request dereference David Disseldorp 2016-09-01 14:59 ` [PATCH 2/2] rbd: fix rbd_img_obj_exists_submit() error path David Disseldorp 2016-09-15 10:48 ` [PATCH 0/2] rbd_img_obj_exists_submit() fixes David Disseldorp 2016-09-15 11:53 ` Ilya Dryomov 2016-09-15 12:25 ` Alex Elder 2016-09-19 17:06 ` Ilya Dryomov 2016-09-19 22:25 ` David Disseldorp
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.