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@xxxxxxxxx> > > 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@xxxxxxxxxx> Looks fine to me too. Reviewed-by: David Disseldorp <ddiss@xxxxxxx> -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
