Oh my. -- Andy
Acked-by: Andy Grover <agrover@xxxxxxxxxx>
On 07/13/2011 06:05 PM, Roland Dreier wrote:
> 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);
>
--
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
