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@xxxxxxx +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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
