[dm-devel] dm-mpath: Clear map_context pointer when requeuing

[Jun'ichi Nomura]

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