target: Fix kref->refcount underflow in transport_cmd_finish_abort

This patch fixes a se_cmd->cmd_kref underflow during CMD_T_ABORTED
when a fabric driver drops it's second reference from below the
target_core_tmr.c based callers of transport_cmd_finish_abort().

Recently with the conversion of kref to refcount_t, this bug was
manifesting itself as:

[705519.601034] refcount_t: underflow; use-after-free.
[705519.604034] INFO: NMI handler (kgdb_nmi_handler) took too long to run: 20116.512 msecs
[705539.719111] ------------[ cut here ]------------
[705539.719117] WARNING: CPU: 3 PID: 26510 at lib/refcount.c:184 refcount_sub_and_test+0x33/0x51

Since the original kref atomic_t based kref_put() didn't check for
underflow and only invoked the final callback when zero was reached,
this bug did not manifest in practice since all se_cmd memory is
using preallocated tags.

To address this, go ahead and propigate the existing return from
transport_put_cmd() up via transport_cmd_finish_abort(), and
change transport_cmd_finish_abort() + core_tmr_handle_tas_abort()
callers to only do their local target_put_sess_cmd() if necessary.

Reported-by: Bart Van Assche <bart.vanassche@sandisk.com>
Tested-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Mike Christie <mchristi@redhat.com>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Himanshu Madhani <himanshu.madhani@qlogic.com>
Cc: Sagi Grimberg <sagig@mellanox.com>
Cc: stable@vger.kernel.org # 3.14+
Tested-by: Gary Guo <ghg@datera.io>
Tested-by: Chu Yuan Lin <cyl@datera.io>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>

diff --git a/drivers/target/target_core_internal.h b/drivers/target/target_core_internal.h
index 9ab7090f7c839c6900cb30ddf7db1b8be4bc78cf..0912de7c0cf8f3ade048de694b4e75f77fe27acd 100644 (file)
--- a/drivers/target/target_core_internal.h
+++ b/drivers/target/target_core_internal.h
@@ -136,7 +136,7 @@ int init_se_kmem_caches(void);
 void   release_se_kmem_caches(void);
 u32    scsi_get_new_index(scsi_index_t);
 void   transport_subsystem_check_init(void);
-void   transport_cmd_finish_abort(struct se_cmd *, int);
+int    transport_cmd_finish_abort(struct se_cmd *, int);
 unsigned char *transport_dump_cmd_direction(struct se_cmd *);
 void   transport_dump_dev_state(struct se_device *, char *, int *);
 void   transport_dump_dev_info(struct se_device *, struct se_lun *,

diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c
index dce1e1b47316173329292f90276843d26d32407b..13f47bf4d16b1d790ab470b92127254835e76078 100644 (file)
--- a/drivers/target/target_core_tmr.c
+++ b/drivers/target/target_core_tmr.c
@@ -75,7 +75,7 @@ void core_tmr_release_req(struct se_tmr_req *tmr)
        kfree(tmr);
 }
 
-static void core_tmr_handle_tas_abort(struct se_cmd *cmd, int tas)
+static int core_tmr_handle_tas_abort(struct se_cmd *cmd, int tas)
 {
        unsigned long flags;
        bool remove = true, send_tas;
@@ -91,7 +91,7 @@ static void core_tmr_handle_tas_abort(struct se_cmd *cmd, int tas)
                transport_send_task_abort(cmd);
        }
 
-       transport_cmd_finish_abort(cmd, remove);
+       return transport_cmd_finish_abort(cmd, remove);
 }
 
 static int target_check_cdb_and_preempt(struct list_head *list,
@@ -184,8 +184,8 @@ void core_tmr_abort_task(
                cancel_work_sync(&se_cmd->work);
                transport_wait_for_tasks(se_cmd);
 
-               transport_cmd_finish_abort(se_cmd, true);
-               target_put_sess_cmd(se_cmd);
+               if (!transport_cmd_finish_abort(se_cmd, true))
+                       target_put_sess_cmd(se_cmd);
 
                printk("ABORT_TASK: Sending TMR_FUNCTION_COMPLETE for"
                                " ref_tag: %llu\n", ref_tag);
@@ -281,8 +281,8 @@ static void core_tmr_drain_tmr_list(
                cancel_work_sync(&cmd->work);
                transport_wait_for_tasks(cmd);
 
-               transport_cmd_finish_abort(cmd, 1);
-               target_put_sess_cmd(cmd);
+               if (!transport_cmd_finish_abort(cmd, 1))
+                       target_put_sess_cmd(cmd);
        }
 }
 
@@ -380,8 +380,8 @@ static void core_tmr_drain_state_list(
                cancel_work_sync(&cmd->work);
                transport_wait_for_tasks(cmd);
 
-               core_tmr_handle_tas_abort(cmd, tas);
-               target_put_sess_cmd(cmd);
+               if (!core_tmr_handle_tas_abort(cmd, tas))
+                       target_put_sess_cmd(cmd);
        }
 }
 

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 6025935036c976edeeee0d7a91df79a66aa84a2b..f1b3a46bdcaffaf8a301569ef7e328ad2c47087f 100644 (file)
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -651,9 +651,10 @@ static void transport_lun_remove_cmd(struct se_cmd *cmd)
                percpu_ref_put(&lun->lun_ref);
 }
 
-void transport_cmd_finish_abort(struct se_cmd *cmd, int remove)
+int transport_cmd_finish_abort(struct se_cmd *cmd, int remove)
 {
        bool ack_kref = (cmd->se_cmd_flags & SCF_ACK_KREF);
+       int ret = 0;
 
        if (cmd->se_cmd_flags & SCF_SE_LUN_CMD)
                transport_lun_remove_cmd(cmd);
@@ -665,9 +666,11 @@ void transport_cmd_finish_abort(struct se_cmd *cmd, int remove)
                cmd->se_tfo->aborted_task(cmd);
 
        if (transport_cmd_check_stop_to_fabric(cmd))
-               return;
+               return 1;
        if (remove && ack_kref)
-               transport_put_cmd(cmd);
+               ret = transport_put_cmd(cmd);
+
+       return ret;
 }
 
 static void target_complete_failure_work(struct work_struct *work)