dm: Fix oops when clone_and_map_rq returns !DM_MAPIO_REMAPPED

PATCH In review Junichi Nomura 2 participants 4 messages May 27, 2015 Device mapper

Share & tools

Junichi Nomura

author

2015-05-27 04:22 UTC #0 lore raw

PATCH 1/1 dm: Fix oops when clone_and_map_rq returns !DM_MAPIO_REMAPPED

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 <[email protected]>

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

drivers/md/dm.c +2 -2

index 0bf79a0..1c62ed8 100644
@@ -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);

Mike Snitzer

2015-05-27 13:22 UTC #1 lore raw

Quoted 39 lines · 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 <[email protected]>
--
This patch is a minimal fix.
map_request() function could be refactored in better shape.
drivers/md/dm.c +0 -0
index 0bf79a0..1c62ed8 100644

drivers/md/dm.c static int map_request(struct dm_rq_target_io *tio, struct request *rq, · line 1979

Quoted 10 lines

drivers/md/dm.c +2 -2

@@ -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

Mike Snitzer

2015-05-27 13:50 UTC #2 lore raw

Quoted 62 lines · Mike Snitzer

Quoted 39 lines · 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 <[email protected]>
--
This patch is a minimal fix.
map_request() function could be refactored in better shape.
drivers/md/dm.c +0 -0
index 0bf79a0..1c62ed8 100644
drivers/md/dm.c static int map_request(struct dm_rq_target_io *tio, struct request *rq, · line 1979
Quoted 10 lines
drivers/md/dm.c +2 -2
@@ -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().

Quoted 4 lines

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

Junichi Nomura

author

2015-05-27 22:48 UTC #3 lore raw

Quoted 8 lines · Mike Snitzer

Quoted 8 lines
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.

Quoted 7 lines

Quoted 4 lines
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.

Signature · 1 line

Jun'ichi Nomura, NEC Corporation