Re: [PATCH] rbd: Clear ceph_msg->bio_iter for retransmitted message

[Alex Elder <elder@dreamhost.com>]

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@intel.com>
>>>
>>> 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@intel.com>
>>> ---
>>>  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