[PATCH] dm: Fix oops when clone_and_map_rq returns !DM_MAPIO_REMAPPED

[PATCH] dm: Fix oops when clone_and_map_rq returns !DM_MAPIO_REMAPPED

[Junichi Nomura]

When stacking request-based dm on blk_mq device,
request cloning and remapping are done in a single call to
target's clone_and_map_rq(). Only when the return value is
DM_MAPIO_REMAPPED, the clone is valid and owned by dm core.

"IS_ERR(clone)" does not cover all the !DM_MAPIO_REMAPPED cases.
E.g. if underlying devices are not ready or unavailable, the
function may return DM_MAPIO_REQUEUE.

Without this fix, dm core may call setup_clone() for NULL clone
and oops like this:

   BUG: unable to handle kernel NULL pointer dereference at 0000000000000068
   IP: [<ffffffff81227525>] blk_rq_prep_clone+0x7d/0x137
   ...
   CPU: 2 PID: 5793 Comm: kdmwork-253:3 Not tainted 4.0.0-nm #1
   ...
   Call Trace:
    [<ffffffffa01d1c09>] map_tio_request+0xa9/0x258 [dm_mod]
    [<ffffffff81071de9>] kthread_worker_fn+0xfd/0x150
    [<ffffffff81071cec>] ? kthread_parkme+0x24/0x24
    [<ffffffff81071cec>] ? kthread_parkme+0x24/0x24
    [<ffffffff81071fdd>] kthread+0xe6/0xee
    [<ffffffff81093a59>] ? put_lock_stats+0xe/0x20
    [<ffffffff81071ef7>] ? __init_kthread_worker+0x5b/0x5b
    [<ffffffff814c2d98>] ret_from_fork+0x58/0x90
    [<ffffffff81071ef7>] ? __init_kthread_worker+0x5b/0x5b

Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>

--
This patch is a minimal fix.
map_request() function could be refactored in better shape.

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 0bf79a0..1c62ed8 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -1972,8 +1972,8 @@ static int map_request(struct dm_rq_target_io *tio, struct request *rq,
 			dm_kill_unmapped_request(rq, r);
 			return r;
 		}
-		if (IS_ERR(clone))
-			return DM_MAPIO_REQUEUE;
+		if (r != DM_MAPIO_REMAPPED)
+			return r;
 		if (setup_clone(clone, rq, tio, GFP_ATOMIC)) {
 			/* -ENOMEM */
 			ti->type->release_clone_rq(clone);

Re: dm: Fix oops when clone_and_map_rq returns !DM_MAPIO_REMAPPED

[Mike Snitzer]

On Wed, May 27 2015 at 12:22am -0400,
Junichi Nomura <j-nomura@ce.jp.nec.com> wrote:

> When stacking request-based dm on blk_mq device,
> request cloning and remapping are done in a single call to
> target's clone_and_map_rq(). Only when the return value is
> DM_MAPIO_REMAPPED, the clone is valid and owned by dm core.
> 
> "IS_ERR(clone)" does not cover all the !DM_MAPIO_REMAPPED cases.
> E.g. if underlying devices are not ready or unavailable, the
> function may return DM_MAPIO_REQUEUE.
> 
> Without this fix, dm core may call setup_clone() for NULL clone
> and oops like this:
> 
>    BUG: unable to handle kernel NULL pointer dereference at 0000000000000068
>    IP: [<ffffffff81227525>] blk_rq_prep_clone+0x7d/0x137
>    ...
>    CPU: 2 PID: 5793 Comm: kdmwork-253:3 Not tainted 4.0.0-nm #1
>    ...
>    Call Trace:
>     [<ffffffffa01d1c09>] map_tio_request+0xa9/0x258 [dm_mod]
>     [<ffffffff81071de9>] kthread_worker_fn+0xfd/0x150
>     [<ffffffff81071cec>] ? kthread_parkme+0x24/0x24
>     [<ffffffff81071cec>] ? kthread_parkme+0x24/0x24
>     [<ffffffff81071fdd>] kthread+0xe6/0xee
>     [<ffffffff81093a59>] ? put_lock_stats+0xe/0x20
>     [<ffffffff81071ef7>] ? __init_kthread_worker+0x5b/0x5b
>     [<ffffffff814c2d98>] ret_from_fork+0x58/0x90
>     [<ffffffff81071ef7>] ? __init_kthread_worker+0x5b/0x5b
> 
> Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
> 
> --
> This patch is a minimal fix.
> map_request() function could be refactored in better shape.
> 
> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> index 0bf79a0..1c62ed8 100644
> --- a/drivers/md/dm.c
> +++ b/drivers/md/dm.c
> @@ -1972,8 +1972,8 @@ static int map_request(struct dm_rq_target_io *tio, struct request *rq,
>  			dm_kill_unmapped_request(rq, r);
>  			return r;
>  		}
> -		if (IS_ERR(clone))
> -			return DM_MAPIO_REQUEUE;
> +		if (r != DM_MAPIO_REMAPPED)
> +			return r;
>  		if (setup_clone(clone, rq, tio, GFP_ATOMIC)) {
>  			/* -ENOMEM */
>  			ti->type->release_clone_rq(clone);

Hi Junichi,

In reviewing this patch I wondered if it better to xplicitly check for a
return of DM_MAPIO_REQUEUE in map_request() since that is the only other
return that is possible.  I'm still on the fence but your patch is more
conservative and at least we won't go on to try to setup_clone, etc if
for some reason in the future a new DM_MAPIO_* were invented and
returned from clone_and_map_rq().

I do intend to revise the header slightly to make explicit references to
function names in some places to improve clarity.  I'll have to double
check but I _think_ this should cc stable@ too since blk-mq support was
added in Linux 4.0 (IIRC).

All said, thanks for fixing this issue!
Mike

Re: dm: Fix oops when clone_and_map_rq returns !DM_MAPIO_REMAPPED

[Mike Snitzer]

On Wed, May 27 2015 at  9:22am -0400,
Mike Snitzer <snitzer@redhat.com> wrote:

> On Wed, May 27 2015 at 12:22am -0400,
> Junichi Nomura <j-nomura@ce.jp.nec.com> wrote:
> 
> > When stacking request-based dm on blk_mq device,
> > request cloning and remapping are done in a single call to
> > target's clone_and_map_rq(). Only when the return value is
> > DM_MAPIO_REMAPPED, the clone is valid and owned by dm core.
> > 
> > "IS_ERR(clone)" does not cover all the !DM_MAPIO_REMAPPED cases.
> > E.g. if underlying devices are not ready or unavailable, the
> > function may return DM_MAPIO_REQUEUE.
> > 
> > Without this fix, dm core may call setup_clone() for NULL clone
> > and oops like this:
> > 
> >    BUG: unable to handle kernel NULL pointer dereference at 0000000000000068
> >    IP: [<ffffffff81227525>] blk_rq_prep_clone+0x7d/0x137
> >    ...
> >    CPU: 2 PID: 5793 Comm: kdmwork-253:3 Not tainted 4.0.0-nm #1
> >    ...
> >    Call Trace:
> >     [<ffffffffa01d1c09>] map_tio_request+0xa9/0x258 [dm_mod]
> >     [<ffffffff81071de9>] kthread_worker_fn+0xfd/0x150
> >     [<ffffffff81071cec>] ? kthread_parkme+0x24/0x24
> >     [<ffffffff81071cec>] ? kthread_parkme+0x24/0x24
> >     [<ffffffff81071fdd>] kthread+0xe6/0xee
> >     [<ffffffff81093a59>] ? put_lock_stats+0xe/0x20
> >     [<ffffffff81071ef7>] ? __init_kthread_worker+0x5b/0x5b
> >     [<ffffffff814c2d98>] ret_from_fork+0x58/0x90
> >     [<ffffffff81071ef7>] ? __init_kthread_worker+0x5b/0x5b
> > 
> > Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
> > 
> > --
> > This patch is a minimal fix.
> > map_request() function could be refactored in better shape.
> > 
> > diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> > index 0bf79a0..1c62ed8 100644
> > --- a/drivers/md/dm.c
> > +++ b/drivers/md/dm.c
> > @@ -1972,8 +1972,8 @@ static int map_request(struct dm_rq_target_io *tio, struct request *rq,
> >  			dm_kill_unmapped_request(rq, r);
> >  			return r;
> >  		}
> > -		if (IS_ERR(clone))
> > -			return DM_MAPIO_REQUEUE;
> > +		if (r != DM_MAPIO_REMAPPED)
> > +			return r;
> >  		if (setup_clone(clone, rq, tio, GFP_ATOMIC)) {
> >  			/* -ENOMEM */
> >  			ti->type->release_clone_rq(clone);
> 
> Hi Junichi,
> 
> In reviewing this patch I wondered if it better to xplicitly check for a
> return of DM_MAPIO_REQUEUE in map_request() since that is the only other
> return that is possible.  I'm still on the fence but your patch is more
> conservative and at least we won't go on to try to setup_clone, etc if
> for some reason in the future a new DM_MAPIO_* were invented and
> returned from clone_and_map_rq().
> 
> I do intend to revise the header slightly to make explicit references to
> function names in some places to improve clarity.  I'll have to double
> check but I _think_ this should cc stable@ too since blk-mq support was
> added in Linux 4.0 (IIRC).

FYI, here is the revised header:
https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-4.1&id=3a1407559a593d4360af12dd2df5296bf8eb0d28

Re: dm: Fix oops when clone_and_map_rq returns !DM_MAPIO_REMAPPED

[Junichi Nomura]

On 05/27/15 22:50, Mike Snitzer wrote:
>> Hi Junichi,
>>
>> In reviewing this patch I wondered if it better to xplicitly check for a
>> return of DM_MAPIO_REQUEUE in map_request() since that is the only other
>> return that is possible.  I'm still on the fence but your patch is more
>> conservative and at least we won't go on to try to setup_clone, etc if
>> for some reason in the future a new DM_MAPIO_* were invented and
>> returned from clone_and_map_rq().

Either way should work. But I wanted to make it explicit
to call setup_clone() only when DM_MAPIO_REMAPPED is returned.

>> I do intend to revise the header slightly to make explicit references to
>> function names in some places to improve clarity.  I'll have to double
>> check but I _think_ this should cc stable@ too since blk-mq support was
>> added in Linux 4.0 (IIRC).
> 
> FYI, here is the revised header:
> https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-4.1&id=3a1407559a593d4360af12dd2df5296bf8eb0d28

Thanks for the nice revision.

-- 
Jun'ichi Nomura, NEC Corporation