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 -- dm-devel mailing list dm-devel@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/dm-devel
