Hi Hannes,
On 12/03/11 01:19, Hannes Reinecke wrote:
>>>> When requeing a request we should be clearing the map_context
>>>> pointer, otherwise we might access an invalid memory location.
>>
>> Could you elaborate on the mechanism how the map_context->ptr
>> (= mpio) is accessed after freeing it?
>>
> In short: No. Pure guesswork :-)
Guesswork is OK :)
But..
> The longer answer here is that 'map_context' is managed by the
> caller for multipath_map().
> So in theory the caller is free to re-use the map_context whenever
> 'clone' is in use.
> So if 'clone' is terminated when it's still requeued the caller
> might be calling multipath_end_io(), at which point map_context->ptr
> will be pointing to an invalid memory location.
With that logic, 'map_context->ptr = NULL' would just replace
the invalid memory access by NULL pointer dereference,
because there is no NULL-check for map_context->ptr.
Right?
> But as I said, this is not a detailed analysis. It's good enough
> for me that it solves the problem :-)
>
>> mpio is known to be non-NULL where it is used. So clearing the pointer
>> should not make any difference in logic.
>>
> It does, see above.
>
>> If this is a preventive change so that we can see NULL dereference
>> instead of random invalid access if anything happens, it should be
>> noted in the patch description and in the code.
>> Otherwise, somebody looking at the code/change in future might be
>> confused: "why we have to clear this pointer?"
>>
>> And there are other places where mpio is freed.
>> (E.g. in dispatch_queued_ios() in dm-mpath.c)
>> Don't we need the same change there?
>>
> I don't think so. It's just from multipath_map() where we need to
> ensure map_context->ptr is correct. All the other places will not
> touch the map_context->ptr again.
For DM_MAPIO_REQUEUE, both multipath_map() and dispatch_queued_ios()
end up with dm_requeue_unmapped_request().
What is the difference?
Thanks,
--
Jun'ichi Nomura, NEC Corporation