3.16.48-rc1 review patch. If anyone has any objections, please let me know.
------------------ From: Nicholas Bellinger <[email protected]> commit 73d4e580ccc5c3e05cea002f18111f66c9c07034 upstream. 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 <[email protected]> Tested-by: Bart Van Assche <[email protected]> Cc: Mike Christie <[email protected]> Cc: Hannes Reinecke <[email protected]> Cc: Christoph Hellwig <[email protected]> Cc: Himanshu Madhani <[email protected]> Cc: Sagi Grimberg <[email protected]> Tested-by: Gary Guo <[email protected]> Tested-by: Chu Yuan Lin <[email protected]> Signed-off-by: Nicholas Bellinger <[email protected]> [bwh: Backported to 3.16: adjust context] Signed-off-by: Ben Hutchings <[email protected]> --- drivers/target/target_core_internal.h | 2 +- drivers/target/target_core_tmr.c | 16 ++++++++-------- drivers/target/target_core_transport.c | 9 ++++++--- 3 files changed, 15 insertions(+), 12 deletions(-) --- a/drivers/target/target_core_internal.h +++ b/drivers/target/target_core_internal.h @@ -92,7 +92,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 *, --- a/drivers/target/target_core_tmr.c +++ b/drivers/target/target_core_tmr.c @@ -82,7 +82,7 @@ void core_tmr_release_req( 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; @@ -98,7 +98,7 @@ static void core_tmr_handle_tas_abort(st 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, @@ -195,8 +195,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_sess, se_cmd); + if (!transport_cmd_finish_abort(se_cmd, true)) + target_put_sess_cmd(se_sess, se_cmd); printk("ABORT_TASK: Sending TMR_FUNCTION_COMPLETE for" " ref_tag: %d\n", ref_tag); @@ -296,8 +296,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->se_sess, cmd); + if (!transport_cmd_finish_abort(cmd, 1)) + target_put_sess_cmd(cmd->se_sess, cmd); } } @@ -395,8 +395,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->se_sess, cmd); + if (!core_tmr_handle_tas_abort(cmd, tas)) + target_put_sess_cmd(cmd->se_sess, cmd); } } --- a/drivers/target/target_core_transport.c +++ b/drivers/target/target_core_transport.c @@ -600,9 +600,10 @@ static void transport_lun_remove_cmd(str 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); @@ -614,9 +615,11 @@ void transport_cmd_finish_abort(struct s 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)
