Skip to content

Commit d374be7

Browse files
Nicholas Bellingergregkh
authored andcommitted
target: Fix kref->refcount underflow in transport_cmd_finish_abort
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 <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> Tested-by: Gary Guo <ghg@datera.io> Tested-by: Chu Yuan Lin <cyl@datera.io> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
1 parent 1fecf39 commit d374be7

3 files changed

Lines changed: 15 additions & 12 deletions

File tree

drivers/target/target_core_internal.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -132,7 +132,7 @@ int init_se_kmem_caches(void);
132132
void release_se_kmem_caches(void);
133133
u32 scsi_get_new_index(scsi_index_t);
134134
void transport_subsystem_check_init(void);
135-
void transport_cmd_finish_abort(struct se_cmd *, int);
135+
int transport_cmd_finish_abort(struct se_cmd *, int);
136136
unsigned char *transport_dump_cmd_direction(struct se_cmd *);
137137
void transport_dump_dev_state(struct se_device *, char *, int *);
138138
void transport_dump_dev_info(struct se_device *, struct se_lun *,

drivers/target/target_core_tmr.c

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,7 @@ void core_tmr_release_req(struct se_tmr_req *tmr)
7575
kfree(tmr);
7676
}
7777

78-
static void core_tmr_handle_tas_abort(struct se_cmd *cmd, int tas)
78+
static int core_tmr_handle_tas_abort(struct se_cmd *cmd, int tas)
7979
{
8080
unsigned long flags;
8181
bool remove = true, send_tas;
@@ -91,7 +91,7 @@ static void core_tmr_handle_tas_abort(struct se_cmd *cmd, int tas)
9191
transport_send_task_abort(cmd);
9292
}
9393

94-
transport_cmd_finish_abort(cmd, remove);
94+
return transport_cmd_finish_abort(cmd, remove);
9595
}
9696

9797
static int target_check_cdb_and_preempt(struct list_head *list,
@@ -185,8 +185,8 @@ void core_tmr_abort_task(
185185
cancel_work_sync(&se_cmd->work);
186186
transport_wait_for_tasks(se_cmd);
187187

188-
transport_cmd_finish_abort(se_cmd, true);
189-
target_put_sess_cmd(se_cmd);
188+
if (!transport_cmd_finish_abort(se_cmd, true))
189+
target_put_sess_cmd(se_cmd);
190190

191191
printk("ABORT_TASK: Sending TMR_FUNCTION_COMPLETE for"
192192
" ref_tag: %llu\n", ref_tag);
@@ -286,8 +286,8 @@ static void core_tmr_drain_tmr_list(
286286
cancel_work_sync(&cmd->work);
287287
transport_wait_for_tasks(cmd);
288288

289-
transport_cmd_finish_abort(cmd, 1);
290-
target_put_sess_cmd(cmd);
289+
if (!transport_cmd_finish_abort(cmd, 1))
290+
target_put_sess_cmd(cmd);
291291
}
292292
}
293293

@@ -385,8 +385,8 @@ static void core_tmr_drain_state_list(
385385
cancel_work_sync(&cmd->work);
386386
transport_wait_for_tasks(cmd);
387387

388-
core_tmr_handle_tas_abort(cmd, tas);
389-
target_put_sess_cmd(cmd);
388+
if (!core_tmr_handle_tas_abort(cmd, tas))
389+
target_put_sess_cmd(cmd);
390390
}
391391
}
392392

drivers/target/target_core_transport.c

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -639,9 +639,10 @@ static void transport_lun_remove_cmd(struct se_cmd *cmd)
639639
percpu_ref_put(&lun->lun_ref);
640640
}
641641

642-
void transport_cmd_finish_abort(struct se_cmd *cmd, int remove)
642+
int transport_cmd_finish_abort(struct se_cmd *cmd, int remove)
643643
{
644644
bool ack_kref = (cmd->se_cmd_flags & SCF_ACK_KREF);
645+
int ret = 0;
645646

646647
if (cmd->se_cmd_flags & SCF_SE_LUN_CMD)
647648
transport_lun_remove_cmd(cmd);
@@ -653,9 +654,11 @@ void transport_cmd_finish_abort(struct se_cmd *cmd, int remove)
653654
cmd->se_tfo->aborted_task(cmd);
654655

655656
if (transport_cmd_check_stop_to_fabric(cmd))
656-
return;
657+
return 1;
657658
if (remove && ack_kref)
658-
transport_put_cmd(cmd);
659+
ret = transport_put_cmd(cmd);
660+
661+
return ret;
659662
}
660663

661664
static void target_complete_failure_work(struct work_struct *work)

0 commit comments

Comments
 (0)