On Wed, May 27 2015 at 12:22am -0400, Junichi Nomura <j-nomura@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> > > -- > 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 -- dm-devel mailing list dm-devel@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/dm-devel