Re: dm-mpath: Clear map_context pointer when requeuing

Re: dm-mpath: Clear map_context pointer when requeuing

[Mike Snitzer]

On Wed, Nov 30 2011 at  9:25am -0500,
Hannes Reinecke <hare@suse.de> wrote:

> When requeing a request we should be clearing the map_context
> pointer, otherwise we might access an invalid memory location.
> 
> Cc: Mike Snitzer <snitzer@redhat.com>
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> Tested-by: Heiko Carstens <heiko.carstens@de.ibm.com>

Acked-by: Mike Snitzer <snitzer@redhat.com>

Should Cc: stable too.

(I was thinking Alasdair would pick this up for 3.2 seeing as it is a
change to dm-mpath.c.  Alasdair, James.. I'll let you guys decide)

Re: dm-mpath: Clear map_context pointer when requeuing

[Jun'ichi Nomura]

Hi Hannes,

On 11/30/11 23:49, Mike Snitzer wrote:
> On Wed, Nov 30 2011 at  9:25am -0500,
> Hannes Reinecke <hare@suse.de> 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?

mpio is known to be non-NULL where it is used. So clearing the pointer
should not make any difference in logic.

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?

>> Cc: Mike Snitzer <snitzer@redhat.com>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> Tested-by: Heiko Carstens <heiko.carstens@de.ibm.com>
> 
> Acked-by: Mike Snitzer <snitzer@redhat.com>
> 
> Should Cc: stable too.
> 
> (I was thinking Alasdair would pick this up for 3.2 seeing as it is a
> change to dm-mpath.c.  Alasdair, James.. I'll let you guys decide)

-- 
Jun'ichi Nomura, NEC Corporation

Re: dm-mpath: Clear map_context pointer when requeuing

[Hannes Reinecke]

Hi Jun'ichi,

On 12/01/2011 01:12 AM, Jun'ichi Nomura wrote:
> Hi Hannes,
> 
> On 11/30/11 23:49, Mike Snitzer wrote:
>> On Wed, Nov 30 2011 at  9:25am -0500,
>> Hannes Reinecke <hare@suse.de> 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 :-)

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.

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.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, 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

Re: 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

Re: dm-mpath: Clear map_context pointer when requeuing

[Hannes Reinecke]

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

Re: dm-mpath: Clear map_context pointer when requeuing

[Jun'ichi Nomura]

Hi Hannes,

On 12/06/11 01:23, Hannes Reinecke wrote:
>>> 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.

multipath_end_io() should not be called in such a case.
If it is, that's the bug we have to fix.

> 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().

See? Since there is no NULL-check, I couldn't understand
why the original patch fix anything.

> Probably sheer luck we didn't hit that.

It is not by luck.

Request status is controlled; clone is either mapped or unmapped.
  * Mapped clone is sent to lower driver and rq->end_io calls back on
    completion. map_context->ptr is valid.
    For termination, multipath_end_io() is called via softirq_done after
    dm_complete_request().
  * Unmapped clone is intermediate state which is under full control of dm.
    map_context->ptr may be invalid.
    It may be terminated by dm_kill_unmapped_request(), that bypasses
    multipath_end_io().
For requeueing, clone is first unmapped then freed
and the original unprep-ed request is requeued.

When block layer directly calls blk_end_request for re-queued request,
multipath_end_io() is not called. So that's fine.

> I'll be sending an updated patch.

If you update the patch, I think we should BUG_ON
if mpio is NULL in multipath_end_io().
Then I can understand the meaning of the patch as enhancement/clean-up.
It's not a bug fix.

>>> 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().

Hmm, I can't follow the reasoning.
What do you mean by "the block layer might decide to abort
the request on its own"?
If block layer aborts an unprep-ed request via softirq_done,
I'm afraid that causes more problems; e.g. breaks SCSI.

Thanks,
-- 
Jun'ichi Nomura, NEC Corporation

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

[Alasdair G Kergon]

OK - I'm not taking this patch in its current form.

If there are still concerns about this, I would accept a BUG_ON() style patch
that consistently checks/clears the value of the field.

Alasdair

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

[Dave Wysochanski]

On Wed, 2011-11-30 at 15:25 +0100, Hannes Reinecke wrote:
> When requeing a request we should be clearing the map_context
> pointer, otherwise we might access an invalid memory location.
> 
> Cc: Mike Snitzer <snitzer@redhat.com>
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> Tested-by: Heiko Carstens <heiko.carstens@de.ibm.com>
> ---
>  drivers/md/dm-mpath.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
> index 5e0090e..e6fad46 100644
> --- a/drivers/md/dm-mpath.c
> +++ b/drivers/md/dm-mpath.c
> @@ -920,8 +920,10 @@ static int multipath_map(struct dm_target *ti, struct request *clone,
>  	map_context->ptr = mpio;
>  	clone->cmd_flags |= REQ_FAILFAST_TRANSPORT;
>  	r = map_io(m, clone, mpio, 0);
> -	if (r < 0 || r == DM_MAPIO_REQUEUE)
> +	if (r < 0 || r == DM_MAPIO_REQUEUE) {
>  		mempool_free(mpio, m->mpio_pool);
> +		map_context->ptr = NULL;
> +	}
>  
>  	return r;
>  }

Ack.