Re: dm: Fix oops when clone_and_map_rq returns !DM_MAPIO_REMAPPED

[Mike Snitzer <snitzer@redhat.com>]

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