From: Roland Dreier <roland@xxxxxxxxxxxxxxx>
Commit 56182e1a9af1 ("target: Replace embedded struct se_queue_req
with a list_head") changed transport_add_cmd_to_queue() to link
commands using cmd->se_queue_node instead of cmd->se_qr.qr_list, but
the matching conversion of transport_remove_cmd_from_queue() didn't
happen.
This leads to use-after-free problems when we hit some exception
paths, because the command doesn't actually get freed from the queue:
qr->cmd is bogus -- we never had a qr struct to begin with, so we're
just looking at some random offset from cmd->se_queue_node, and so we
never find a matching command, but then we free the underlying
command, leaving a freed list head on the list.
The patch below fixes the remaining dev_queue_obj.qobj_list handling
to always use cmd->se_queue_node, which fixes at least the following
reproducible crash with tcm_loop LUN reset handling for me:
test_bit(BIO_UPTODATE) failed for bio: ffff88003d07ca40, err: -12
LUN_RESET: TMR caller fabric: loopback initiator port naa.6001405c39ca4d29
LUN_RESET: TMR starting for [iblock], tas: 1
LUN_RESET: cmd: ffff88003db904c0 task: ffff88003fbf7da8 ITT/CmdSN: 0x00000001/0x00000000, i_state: 0, t_state/def_t_state: 7/0 cdb: 0x28
LUN_RESET: ITT[0x00000001] - pr_res_key: 0x0000000000000000 t_task_cdbs: 1 t_task_cdbs_left: 0 t_task_cdbs_sent: 1 -- t_transport_active: 1 t_transport_stop: 0 t_transport_sent: 0
LUN_RESET: got t_transport_active = 1 for task: ffff88003fbf7da8, t_fe_count: 1 dev: ffff88003bc41090
tcm_loop_queue_status() called for scsi_cmnd: ffff88003d292640 cdb: 0x28
ITT: 0x00000001 t_transport_queue_active: 1
LUN_RESET: cmd: ffff88003db90940 task: ffff88003d047488 ITT/CmdSN: 0x00000001/0x00000000, i_state: 0, t_state/def_t_state: 7/0 cdb: 0x28
LUN_RESET: ITT[0x00000001] - pr_res_key: 0x0000000000000000 t_task_cdbs: 1 t_task_cdbs_left: 0 t_task_cdbs_sent: 1 -- t_transport_active: 1 t_transport_stop: 0 t_transport_sent: 0
LUN_RESET: got t_transport_active = 1 for task: ffff88003d047488, t_fe_count: 1 dev: ffff88003bc41090
tcm_loop_queue_status() called for scsi_cmnd: ffff88003d292500 cdb: 0x28
general protection fault: 0000 [#1] SMP
CPU 1
Pid: 932, comm: LIO_iblock Not tainted 3.0.0-rc4+ #19 Bochs Bochs
RIP: 0010:[<ffffffff812a4691>] [<ffffffff812a4691>] transport_remove_cmd_from_queue+0x71/0x110
RSP: 0018:ffff88003dbc5c90 EFLAGS: 00010007
RAX: 0000000000000286 RBX: ffff88003db90940 RCX: ffff88003db90578
RDX: ffff88003db90ab4 RSI: 1030000000000000 RDI: 6b6b6b6b6b6b6b6b
RBP: ffff88003dbc5cd0 R08: fffffffff7e80206 R09: 0000000000000206
R10: ffff88003dba6088 R11: 0000000000000000 R12: ffff88003bc41258
R13: ffff88003bc41250 R14: 6b6b6b6b6b6b6b5b R15: ffff88003bc41290
FS: 0000000000000000(0000) GS:ffff88003fd00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 00007fff2acab620 CR3: 000000003f9c3000 CR4: 00000000000006a0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process LIO_iblock (pid: 932, threadinfo ffff88003dbc4000, task ffff88003dba5b20)
Stack:
ffff88003dbc5cb0 ffffffff8127ab76 ffff88003d292500 ffff88003db90940
0000000000000000 0000000000000000 ffff88003d047488 ffff88003bc416c8
ffff88003dbc5cf0 ffffffff812a934f ffff88003db90940 ffff88003bc41588
Call Trace:
[<ffffffff8127ab76>] ? scsi_done+0x26/0x60
[<ffffffff812a934f>] transport_cmd_finish_abort+0x2f/0x70
[<ffffffff812a1f55>] core_tmr_handle_tas_abort+0x35/0x70
[<ffffffff812a2711>] core_tmr_lun_reset+0x781/0x860
[<ffffffff812a45fc>] transport_generic_do_tmr+0x8c/0xb0
[<ffffffff812ac15c>] transport_processing_thread+0x1ec/0x300
[<ffffffff812abf70>] ? transport_handle_cdb_direct+0x70/0x70
[<ffffffff81063b20>] ? wake_up_bit+0x40/0x40
[<ffffffff812abf70>] ? transport_handle_cdb_direct+0x70/0x70
[<ffffffff810635f6>] kthread+0x96/0xa0
[<ffffffff8137d444>] kernel_thread_helper+0x4/0x10
[<ffffffff8137bd54>] ? retint_restore_args+0x13/0x13
[<ffffffff81063560>] ? __init_kthread_worker+0x70/0x70
[<ffffffff8137d440>] ? gs_change+0x13/0x13
Code: 8d 4f f0 4c 39 ff 4c 8b 71 10 74 28 49 83 ee 10 eb 0f 0f 1f 84 00 00 00 00 00 4c 89 f1 4c 8d 76 f0 48 39 59 08 74 33 49 8d 7e 10
8b 76 10 4c 39 ff 75 e6 48 89 c6 4c 89 e7 e8 ab 72 0d 00 8b
RIP [<ffffffff812a4691>] transport_remove_cmd_from_queue+0x71/0x110
RSP <ffff88003dbc5c90>
---[ end trace 73378292d89d3399 ]---
Signed-off-by: Roland Dreier <roland@xxxxxxxxxxxxxxx>
---
drivers/target/target_core_tmr.c | 27 +++++----------------------
drivers/target/target_core_transport.c | 30 +++++++++++++-----------------
2 files changed, 18 insertions(+), 39 deletions(-)
diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c
index b95f090..0c7877b 100644
--- a/drivers/target/target_core_tmr.c
+++ b/drivers/target/target_core_tmr.c
@@ -112,15 +112,14 @@ int core_tmr_lun_reset(
struct list_head *preempt_and_abort_list,
struct se_cmd *prout_cmd)
{
- struct se_cmd *cmd;
- struct se_queue_req *qr, *qr_tmp;
+ struct se_cmd *cmd, *tcmd;
struct se_node_acl *tmr_nacl = NULL;
struct se_portal_group *tmr_tpg = NULL;
struct se_queue_obj *qobj = &dev->dev_queue_obj;
struct se_tmr_req *tmr_p, *tmr_pp;
struct se_task *task, *task_tmp;
unsigned long flags;
- int fe_count, state, tas;
+ int fe_count, tas;
/*
* TASK_ABORTED status bit, this is configurable via ConfigFS
* struct se_device attributes. spc4r17 section 7.4.6 Control mode page
@@ -330,20 +329,7 @@ int core_tmr_lun_reset(
* reference, otherwise the struct se_cmd is released.
*/
spin_lock_irqsave(&qobj->cmd_queue_lock, flags);
- list_for_each_entry_safe(qr, qr_tmp, &qobj->qobj_list, qr_list) {
- cmd = (struct se_cmd *)qr->cmd;
- if (!(cmd)) {
- /*
- * Skip these for non PREEMPT_AND_ABORT usage..
- */
- if (preempt_and_abort_list != NULL)
- continue;
-
- atomic_dec(&qobj->queue_cnt);
- list_del(&qr->qr_list);
- kfree(qr);
- continue;
- }
+ list_for_each_entry_safe(cmd, tcmd, &qobj->qobj_list, se_queue_node) {
/*
* For PREEMPT_AND_ABORT usage, only process commands
* with a matching reservation key.
@@ -360,15 +346,12 @@ int core_tmr_lun_reset(
atomic_dec(&cmd->t_transport_queue_active);
atomic_dec(&qobj->queue_cnt);
- list_del(&qr->qr_list);
+ list_del(&cmd->se_queue_node);
spin_unlock_irqrestore(&qobj->cmd_queue_lock, flags);
- state = qr->state;
- kfree(qr);
-
DEBUG_LR("LUN_RESET: %s from Device Queue: cmd: %p t_state:"
" %d t_fe_count: %d\n", (preempt_and_abort_list) ?
- "Preempt" : "", cmd, state,
+ "Preempt" : "", cmd, cmd->t_state,
atomic_read(&cmd->t_fe_count));
/*
* Signal that the command has failed via cmd->se_cmd_flags,
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 3288cef..124fa14 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -809,7 +809,7 @@ transport_get_cmd_from_queue(struct se_queue_obj *qobj)
static void transport_remove_cmd_from_queue(struct se_cmd *cmd,
struct se_queue_obj *qobj)
{
- struct se_queue_req *qr = NULL, *qr_p = NULL;
+ struct se_cmd *t;
unsigned long flags;
spin_lock_irqsave(&qobj->cmd_queue_lock, flags);
@@ -818,14 +818,13 @@ static void transport_remove_cmd_from_queue(struct se_cmd *cmd,
return;
}
- list_for_each_entry_safe(qr, qr_p, &qobj->qobj_list, qr_list) {
- if (qr->cmd != cmd)
- continue;
-
- atomic_dec(&qr->cmd->t_transport_queue_active);
- atomic_dec(&qobj->queue_cnt);
- list_del(&qr->qr_list);
- }
+ list_for_each_entry(t, &qobj->qobj_list, se_queue_node)
+ if (t == cmd) {
+ atomic_dec(&cmd->t_transport_queue_active);
+ atomic_dec(&qobj->queue_cnt);
+ list_del(&cmd->se_queue_node);
+ break;
+ }
spin_unlock_irqrestore(&qobj->cmd_queue_lock, flags);
if (atomic_read(&cmd->t_transport_queue_active)) {
@@ -1202,18 +1201,15 @@ void transport_dump_dev_state(
*/
static void transport_release_all_cmds(struct se_device *dev)
{
- struct se_cmd *cmd = NULL;
- struct se_queue_req *qr = NULL, *qr_p = NULL;
+ struct se_cmd *cmd, *tcmd;
int bug_out = 0, t_state;
unsigned long flags;
spin_lock_irqsave(&dev->dev_queue_obj.cmd_queue_lock, flags);
- list_for_each_entry_safe(qr, qr_p, &dev->dev_queue_obj.qobj_list,
- qr_list) {
-
- cmd = qr->cmd;
- t_state = qr->state;
- list_del(&qr->qr_list);
+ list_for_each_entry_safe(cmd, tcmd, &dev->dev_queue_obj.qobj_list,
+ se_queue_node) {
+ t_state = cmd->t_state;
+ list_del(&cmd->se_queue_node);
spin_unlock_irqrestore(&dev->dev_queue_obj.cmd_queue_lock,
flags);
--
1.7.5.4
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
