On 06/08/2012 01:17 AM, Hannes Reinecke wrote:
> On 06/06/2012 04:10 PM, Alex Elder wrote:
>> On 06/06/2012 03:03 AM, Yan, Zheng wrote:
>>> From: "Yan, Zheng" <zheng.z.yan@xxxxxxxxx>
>>>
>>> The bug can cause NULL pointer dereference in write_partial_msg_pages
>>
>> Although this looks simple enough, I want to study it a little more
>> before committing it. I've been wanting to walk through this bit
>> of code anyway so I'll do that today.
>>
>> One quick observation though: m->bio_iter really ought to be
>> initialized only within #ifdef CONFIG_BLOCK (although I see it's
>> defined without it in the structure definition). At some point
>> I'll put together a cleanup patch to do that everywhere; feel free
>> to do that yourself if you are so inclined.
>>
>> -Alex
>>
>>> Signed-off-by: Zheng Yan <zheng.z.yan@xxxxxxxxx>
>>> ---
>>> net/ceph/messenger.c | 1 +
>>> 1 files changed, 1 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
>>> index 1a80907..785b953 100644
>>> --- a/net/ceph/messenger.c
>>> +++ b/net/ceph/messenger.c
>>> @@ -598,6 +598,7 @@ static void prepare_write_message(struct ceph_connection *con)
>>> le32_to_cpu(con->out_msg->footer.front_crc),
>>> le32_to_cpu(con->out_msg->footer.middle_crc));
>>>
>>> + m->bio_iter = NULL;
>>> /* is there a data payload? */
>>> if (le32_to_cpu(m->hdr.data_len) > 0) {
>>> /* initialize page iterator */
>>
> Incidentally, we've come across the same issue. First thing which
> struck me was this:
>
> diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
> index 524f4e4..759d4d2 100644
> --- a/net/ceph/messenger.c
> +++ b/net/ceph/messenger.c
> @@ -874,7 +874,7 @@ static int write_partial_msg_pages(struct
> ceph_connection *c
> on)
> page = list_first_entry(&msg->pagelist->head,
> struct page, lru);
> #ifdef CONFIG_BLOCK
> - } else if (msg->bio) {
> + } else if (msg->bio_iter) {
> struct bio_vec *bv;
>
> bv = bio_iovec_idx(msg->bio_iter, msg->bio_seg);
>
> We've called bio_list_init() a few lines above; however, it might
> return with a NULL bio_iter. So for consistency we should be
> checking for ->bio_iter here, as this is what we'll be using
> afterwards anyway.
Zheng is right, the only way bio_iter will be null following the
call to init_bio_iter() is if bio is also null, so it's roughly
equivalent either way. I do think it would be reassuring to have
the check be against bio_iter as you suggest in this case, since
that's the pointer we're then dereferencing.
I'm reworking this code today, and will update it to check the
bio_iter pointer instead if this suggestion still applies.
-Alex
> Cheers,
>
> Hannes
--
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
