On Mon, Sep 26, 2016 at 5:33 PM, David Disseldorp <ddiss@xxxxxxx> 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@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.
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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
