| Message ID | 1474304608-17958-9-git-send-email-idryomov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 09/19/2016 12:03 PM, Ilya Dryomov wrote: > Move the check into rbd_obj_request_destroy(), to avoid use-after-free > on errors in rbd_img_request_fill(). > > Signed-off-by: Ilya Dryomov <idryomov@gmail.com> Is this because an error occurring in rbd_img_request_fill() causes rbd_img_obj_request_del() to be called, which leads to rbd_obj_request_destroy(), which (by this time) has not yet had its obj_request->pages pointer set to NULL because the object request is still outstanding? (Your explanation was a little brief...) The change in rbd_obj_request_destroy() looks good for that case. The code deleted in rbd_img_obj_end_request() could still stay, however. Almost everywhere, pointers are reassigned to NULL when it's known the thing referred to is no longer needed. It's useful in post-mortem understanding of what's occurred. I guess it's fine with me either way. Reviewed-by: Alex Elder <elder@linaro.org> > --- > drivers/block/rbd.c | 11 +++-------- > 1 file changed, 3 insertions(+), 8 deletions(-) > > diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c > index 8907ee6342ac..d305b9ebe2cf 100644 > --- a/drivers/block/rbd.c > +++ b/drivers/block/rbd.c > @@ -2139,7 +2139,9 @@ static void rbd_obj_request_destroy(struct kref *kref) > bio_chain_put(obj_request->bio_list); > break; > case OBJ_REQUEST_PAGES: > - if (obj_request->pages) > + /* img_data requests don't own their page array */ > + if (obj_request->pages && > + !obj_request_img_data_test(obj_request)) > ceph_release_page_vector(obj_request->pages, > obj_request->page_count); > break; > @@ -2360,13 +2362,6 @@ static bool rbd_img_obj_end_request(struct rbd_obj_request *obj_request) > xferred = obj_request->length; > } > > - /* Image object requests don't own their page array */ > - > - if (obj_request->type == OBJ_REQUEST_PAGES) { > - obj_request->pages = NULL; > - obj_request->page_count = 0; > - } > - > if (img_request_child_test(img_request)) { > rbd_assert(img_request->obj_request != NULL); > more = obj_request->which < img_request->obj_request_count - 1; > -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, 25 Sep 2016 12:06:34 -0500, Alex Elder wrote: > On 09/19/2016 12:03 PM, Ilya Dryomov wrote: > > Move the check into rbd_obj_request_destroy(), to avoid use-after-free > > on errors in rbd_img_request_fill(). > > > > Signed-off-by: Ilya Dryomov <idryomov@gmail.com> > > Is this because an error occurring in rbd_img_request_fill() > causes rbd_img_obj_request_del() to be called, which leads to > rbd_obj_request_destroy(), which (by this time) has not yet > had its obj_request->pages pointer set to NULL because the > object request is still outstanding? (Your explanation was > a little brief...) I agree, the commit message could be improved... I think the use after free is in the rbd_img_obj_parent_read_full() call path - if rbd_img_request_fill() fails after adding an obj_request to the img_request->obj_requests list, then the corresponding page(s) will be freed in the rbd_img_request_fill() out_unwind error path *and* the rbd_img_obj_parent_read_full() error path. > The change in rbd_obj_request_destroy() looks good for that > case. > > The code deleted in rbd_img_obj_end_request() could still stay, > however. Almost everywhere, pointers are reassigned to NULL > when it's known the thing referred to is no longer needed. > It's useful in post-mortem understanding of what's occurred. Agreed. > I guess it's fine with me either way. > > Reviewed-by: Alex Elder <elder@linaro.org> Looks fine to me too. Reviewed-by: David Disseldorp <ddiss@suse.de> -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Sep 26, 2016 at 5:33 PM, David Disseldorp <ddiss@suse.de> wrote: > On Sun, 25 Sep 2016 12:06:34 -0500, Alex Elder wrote: > >> On 09/19/2016 12:03 PM, Ilya Dryomov wrote: >> > Move the check into rbd_obj_request_destroy(), to avoid use-after-free >> > on errors in rbd_img_request_fill(). >> > >> > Signed-off-by: Ilya Dryomov <idryomov@gmail.com> >> >> Is this because an error occurring in rbd_img_request_fill() >> causes rbd_img_obj_request_del() to be called, which leads to >> rbd_obj_request_destroy(), which (by this time) has not yet >> had its obj_request->pages pointer set to NULL because the >> object request is still outstanding? (Your explanation was >> a little brief...) > > I agree, the commit message could be improved... > > I think the use after free is in the rbd_img_obj_parent_read_full() > call path - if rbd_img_request_fill() fails after adding an obj_request > to the img_request->obj_requests list, then the corresponding page(s) > will be freed in the rbd_img_request_fill() out_unwind error path *and* > the rbd_img_obj_parent_read_full() error path. Correct. The same goes for rbd_img_request_fill(OBJ_REQUEST_PAGES) call in rbd_img_parent_read(). I'll update the commit message. > >> The change in rbd_obj_request_destroy() looks good for that >> case. >> >> The code deleted in rbd_img_obj_end_request() could still stay, >> however. Almost everywhere, pointers are reassigned to NULL >> when it's known the thing referred to is no longer needed. >> It's useful in post-mortem understanding of what's occurred. > > Agreed. I disagree. Yes, it sometimes helps if you do it systematically and all you have is a panic message. But if you have a vmcore and trying to do an actual post-mortem, NULLed out pointers tend to work against you, especially if you NULL out something that hasn't just been freed. Thanks, Ilya -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c index 8907ee6342ac..d305b9ebe2cf 100644 --- a/drivers/block/rbd.c +++ b/drivers/block/rbd.c @@ -2139,7 +2139,9 @@ static void rbd_obj_request_destroy(struct kref *kref) bio_chain_put(obj_request->bio_list); break; case OBJ_REQUEST_PAGES: - if (obj_request->pages) + /* img_data requests don't own their page array */ + if (obj_request->pages && + !obj_request_img_data_test(obj_request)) ceph_release_page_vector(obj_request->pages, obj_request->page_count); break; @@ -2360,13 +2362,6 @@ static bool rbd_img_obj_end_request(struct rbd_obj_request *obj_request) xferred = obj_request->length; } - /* Image object requests don't own their page array */ - - if (obj_request->type == OBJ_REQUEST_PAGES) { - obj_request->pages = NULL; - obj_request->page_count = 0; - } - if (img_request_child_test(img_request)) { rbd_assert(img_request->obj_request != NULL); more = obj_request->which < img_request->obj_request_count - 1;
Move the check into rbd_obj_request_destroy(), to avoid use-after-free on errors in rbd_img_request_fill(). Signed-off-by: Ilya Dryomov <idryomov@gmail.com> --- drivers/block/rbd.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-)