Re: dm-mpath: Clear map_context pointer when requeuing

[Hannes Reinecke <hare@suse.de>]

On 12/05/2011 11:49 AM, Jun'ichi Nomura wrote:
> 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?
>
No. Observation here is that
multipath_end_io() absolutely required map_context->ptr to be set to a 
sane value.
But without the fix map_context->ptr in multipath_end_io() will point to 
an uninitialized location, thus causing the error.

But having checked the functions, it really looks as if we'd need 
another patch on top of which to check for NULL mpio in do_end_io().
Probably sheer luck we didn't hit that.

I'll be sending an updated patch.

>> 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?
>
Difference is that dispatch_queued_ios() only deals with queued 
requests, ie where it's already known this request is queued.
For multipath_map() it's not and the block layer might decide to abort 
the request on its own, thus calling multipath_end_io() directly, 
regardless of the return value of multipath_map().

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html