Skip to content

Commit febe562

Browse files
author
Nicholas Bellinger
committed
target: Fix LUN_RESET active I/O handling for ACK_KREF
This patch fixes a NULL pointer se_cmd->cmd_kref < 0 refcount bug during TMR LUN_RESET with active se_cmd I/O, that can be triggered during se_cmd descriptor shutdown + release via core_tmr_drain_state_list() code. To address this bug, add common __target_check_io_state() helper for ABORT_TASK + LUN_RESET w/ CMD_T_COMPLETE checking, and set CMD_T_ABORTED + obtain ->cmd_kref for both cases ahead of last target_put_sess_cmd() after TFO->aborted_task() -> transport_cmd_finish_abort() callback has completed. It also introduces SCF_ACK_KREF to determine when transport_cmd_finish_abort() needs to drop the second extra reference, ahead of calling target_put_sess_cmd() for the final kref_put(&se_cmd->cmd_kref). It also updates transport_cmd_check_stop() to avoid holding se_cmd->t_state_lock while dropping se_cmd device state via target_remove_from_state_list(), now that core_tmr_drain_state_list() is holding the se_device lock while checking se_cmd state from within TMR logic. Finally, move transport_put_cmd() release of SGL + TMR + extended CDB memory into target_free_cmd_mem() in order to avoid potential resource leaks in TMR ABORT_TASK + LUN_RESET code-paths. Also update target_release_cmd_kref() accordingly. Reviewed-by: Quinn Tran <quinn.tran@qlogic.com> Cc: Himanshu Madhani <himanshu.madhani@qlogic.com> Cc: Sagi Grimberg <sagig@mellanox.com> Cc: Christoph Hellwig <hch@lst.de> Cc: Hannes Reinecke <hare@suse.de> Cc: Andy Grover <agrover@redhat.com> Cc: Mike Christie <mchristi@redhat.com> Cc: stable@vger.kernel.org # 3.10+ Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
1 parent a07100e commit febe562

File tree

3 files changed

+76
-61
lines changed

3 files changed

+76
-61
lines changed

‎drivers/target/target_core_tmr.c

Lines changed: 46 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -107,6 +107,34 @@ static int target_check_cdb_and_preempt(struct list_head *list,
107107
return 1;
108108
}
109109

110+
static bool __target_check_io_state(struct se_cmd *se_cmd)
111+
{
112+
struct se_session *sess = se_cmd->se_sess;
113+
114+
assert_spin_locked(&sess->sess_cmd_lock);
115+
WARN_ON_ONCE(!irqs_disabled());
116+
/*
117+
* If command already reached CMD_T_COMPLETE state within
118+
* target_complete_cmd(), this se_cmd has been passed to
119+
* fabric driver and will not be aborted.
120+
*
121+
* Otherwise, obtain a local se_cmd->cmd_kref now for TMR
122+
* ABORT_TASK + LUN_RESET for CMD_T_ABORTED processing as
123+
* long as se_cmd->cmd_kref is still active unless zero.
124+
*/
125+
spin_lock(&se_cmd->t_state_lock);
126+
if (se_cmd->transport_state & CMD_T_COMPLETE) {
127+
pr_debug("Attempted to abort io tag: %llu already complete,"
128+
" skipping\n", se_cmd->tag);
129+
spin_unlock(&se_cmd->t_state_lock);
130+
return false;
131+
}
132+
se_cmd->transport_state |= CMD_T_ABORTED;
133+
spin_unlock(&se_cmd->t_state_lock);
134+
135+
return kref_get_unless_zero(&se_cmd->cmd_kref);
136+
}
137+
110138
void core_tmr_abort_task(
111139
struct se_device *dev,
112140
struct se_tmr_req *tmr,
@@ -130,34 +158,22 @@ void core_tmr_abort_task(
130158
if (tmr->ref_task_tag != ref_tag)
131159
continue;
132160

133-
if (!kref_get_unless_zero(&se_cmd->cmd_kref))
134-
continue;
135-
136161
printk("ABORT_TASK: Found referenced %s task_tag: %llu\n",
137162
se_cmd->se_tfo->get_fabric_name(), ref_tag);
138163

139-
spin_lock(&se_cmd->t_state_lock);
140-
if (se_cmd->transport_state & CMD_T_COMPLETE) {
141-
printk("ABORT_TASK: ref_tag: %llu already complete,"
142-
" skipping\n", ref_tag);
143-
spin_unlock(&se_cmd->t_state_lock);
164+
if (!__target_check_io_state(se_cmd)) {
144165
spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
145-
146166
target_put_sess_cmd(se_cmd);
147-
148167
goto out;
149168
}
150-
se_cmd->transport_state |= CMD_T_ABORTED;
151-
spin_unlock(&se_cmd->t_state_lock);
152-
153169
list_del_init(&se_cmd->se_cmd_list);
154170
spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
155171

156172
cancel_work_sync(&se_cmd->work);
157173
transport_wait_for_tasks(se_cmd);
158174

159-
target_put_sess_cmd(se_cmd);
160175
transport_cmd_finish_abort(se_cmd, true);
176+
target_put_sess_cmd(se_cmd);
161177

162178
printk("ABORT_TASK: Sending TMR_FUNCTION_COMPLETE for"
163179
" ref_tag: %llu\n", ref_tag);
@@ -242,8 +258,10 @@ static void core_tmr_drain_state_list(
242258
struct list_head *preempt_and_abort_list)
243259
{
244260
LIST_HEAD(drain_task_list);
261+
struct se_session *sess;
245262
struct se_cmd *cmd, *next;
246263
unsigned long flags;
264+
int rc;
247265

248266
/*
249267
* Complete outstanding commands with TASK_ABORTED SAM status.
@@ -282,14 +300,24 @@ static void core_tmr_drain_state_list(
282300
if (prout_cmd == cmd)
283301
continue;
284302

303+
sess = cmd->se_sess;
304+
if (WARN_ON_ONCE(!sess))
305+
continue;
306+
307+
spin_lock(&sess->sess_cmd_lock);
308+
rc = __target_check_io_state(cmd);
309+
spin_unlock(&sess->sess_cmd_lock);
310+
if (!rc)
311+
continue;
312+
285313
list_move_tail(&cmd->state_list, &drain_task_list);
286314
cmd->state_active = false;
287315
}
288316
spin_unlock_irqrestore(&dev->execute_task_lock, flags);
289317

290318
while (!list_empty(&drain_task_list)) {
291319
cmd = list_entry(drain_task_list.next, struct se_cmd, state_list);
292-
list_del(&cmd->state_list);
320+
list_del_init(&cmd->state_list);
293321

294322
pr_debug("LUN_RESET: %s cmd: %p"
295323
" ITT/CmdSN: 0x%08llx/0x%08x, i_state: %d, t_state: %d"
@@ -313,16 +341,11 @@ static void core_tmr_drain_state_list(
313341
* loop above, but we do it down here given that
314342
* cancel_work_sync may block.
315343
*/
316-
if (cmd->t_state == TRANSPORT_COMPLETE)
317-
cancel_work_sync(&cmd->work);
318-
319-
spin_lock_irqsave(&cmd->t_state_lock, flags);
320-
target_stop_cmd(cmd, &flags);
321-
322-
cmd->transport_state |= CMD_T_ABORTED;
323-
spin_unlock_irqrestore(&cmd->t_state_lock, flags);
344+
cancel_work_sync(&cmd->work);
345+
transport_wait_for_tasks(cmd);
324346

325347
core_tmr_handle_tas_abort(tmr_nacl, cmd, tas);
348+
target_put_sess_cmd(cmd);
326349
}
327350
}
328351

‎drivers/target/target_core_transport.c

Lines changed: 29 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -534,9 +534,6 @@ void transport_deregister_session(struct se_session *se_sess)
534534
}
535535
EXPORT_SYMBOL(transport_deregister_session);
536536

537-
/*
538-
* Called with cmd->t_state_lock held.
539-
*/
540537
static void target_remove_from_state_list(struct se_cmd *cmd)
541538
{
542539
struct se_device *dev = cmd->se_dev;
@@ -561,10 +558,6 @@ static int transport_cmd_check_stop(struct se_cmd *cmd, bool remove_from_lists,
561558
{
562559
unsigned long flags;
563560

564-
spin_lock_irqsave(&cmd->t_state_lock, flags);
565-
if (write_pending)
566-
cmd->t_state = TRANSPORT_WRITE_PENDING;
567-
568561
if (remove_from_lists) {
569562
target_remove_from_state_list(cmd);
570563

@@ -574,6 +567,10 @@ static int transport_cmd_check_stop(struct se_cmd *cmd, bool remove_from_lists,
574567
cmd->se_lun = NULL;
575568
}
576569

570+
spin_lock_irqsave(&cmd->t_state_lock, flags);
571+
if (write_pending)
572+
cmd->t_state = TRANSPORT_WRITE_PENDING;
573+
577574
/*
578575
* Determine if frontend context caller is requesting the stopping of
579576
* this command for frontend exceptions.
@@ -627,6 +624,8 @@ static void transport_lun_remove_cmd(struct se_cmd *cmd)
627624

628625
void transport_cmd_finish_abort(struct se_cmd *cmd, int remove)
629626
{
627+
bool ack_kref = (cmd->se_cmd_flags & SCF_ACK_KREF);
628+
630629
if (cmd->se_cmd_flags & SCF_SE_LUN_CMD)
631630
transport_lun_remove_cmd(cmd);
632631
/*
@@ -638,7 +637,7 @@ void transport_cmd_finish_abort(struct se_cmd *cmd, int remove)
638637

639638
if (transport_cmd_check_stop_to_fabric(cmd))
640639
return;
641-
if (remove)
640+
if (remove && ack_kref)
642641
transport_put_cmd(cmd);
643642
}
644643

@@ -706,7 +705,7 @@ void target_complete_cmd(struct se_cmd *cmd, u8 scsi_status)
706705
* Check for case where an explicit ABORT_TASK has been received
707706
* and transport_wait_for_tasks() will be waiting for completion..
708707
*/
709-
if (cmd->transport_state & CMD_T_ABORTED &&
708+
if (cmd->transport_state & CMD_T_ABORTED ||
710709
cmd->transport_state & CMD_T_STOP) {
711710
spin_unlock_irqrestore(&cmd->t_state_lock, flags);
712711
complete_all(&cmd->t_transport_stop_comp);
@@ -2222,39 +2221,21 @@ static inline void transport_free_pages(struct se_cmd *cmd)
22222221
}
22232222

22242223
/**
2225-
* transport_release_cmd - free a command
2226-
* @cmd: command to free
2224+
* transport_put_cmd - release a reference to a command
2225+
* @cmd: command to release
22272226
*
2228-
* This routine unconditionally frees a command, and reference counting
2229-
* or list removal must be done in the caller.
2227+
* This routine releases our reference to the command and frees it if possible.
22302228
*/
2231-
static int transport_release_cmd(struct se_cmd *cmd)
2229+
static int transport_put_cmd(struct se_cmd *cmd)
22322230
{
22332231
BUG_ON(!cmd->se_tfo);
2234-
2235-
if (cmd->se_cmd_flags & SCF_SCSI_TMR_CDB)
2236-
core_tmr_release_req(cmd->se_tmr_req);
2237-
if (cmd->t_task_cdb != cmd->__t_task_cdb)
2238-
kfree(cmd->t_task_cdb);
22392232
/*
22402233
* If this cmd has been setup with target_get_sess_cmd(), drop
22412234
* the kref and call ->release_cmd() in kref callback.
22422235
*/
22432236
return target_put_sess_cmd(cmd);
22442237
}
22452238

2246-
/**
2247-
* transport_put_cmd - release a reference to a command
2248-
* @cmd: command to release
2249-
*
2250-
* This routine releases our reference to the command and frees it if possible.
2251-
*/
2252-
static int transport_put_cmd(struct se_cmd *cmd)
2253-
{
2254-
transport_free_pages(cmd);
2255-
return transport_release_cmd(cmd);
2256-
}
2257-
22582239
void *transport_kmap_data_sg(struct se_cmd *cmd)
22592240
{
22602241
struct scatterlist *sg = cmd->t_data_sg;
@@ -2452,14 +2433,13 @@ static void transport_write_pending_qf(struct se_cmd *cmd)
24522433

24532434
int transport_generic_free_cmd(struct se_cmd *cmd, int wait_for_tasks)
24542435
{
2455-
unsigned long flags;
24562436
int ret = 0;
24572437

24582438
if (!(cmd->se_cmd_flags & SCF_SE_LUN_CMD)) {
24592439
if (wait_for_tasks && (cmd->se_cmd_flags & SCF_SCSI_TMR_CDB))
2460-
transport_wait_for_tasks(cmd);
2440+
transport_wait_for_tasks(cmd);
24612441

2462-
ret = transport_release_cmd(cmd);
2442+
ret = transport_put_cmd(cmd);
24632443
} else {
24642444
if (wait_for_tasks)
24652445
transport_wait_for_tasks(cmd);
@@ -2468,11 +2448,8 @@ int transport_generic_free_cmd(struct se_cmd *cmd, int wait_for_tasks)
24682448
* has already added se_cmd to state_list, but fabric has
24692449
* failed command before I/O submission.
24702450
*/
2471-
if (cmd->state_active) {
2472-
spin_lock_irqsave(&cmd->t_state_lock, flags);
2451+
if (cmd->state_active)
24732452
target_remove_from_state_list(cmd);
2474-
spin_unlock_irqrestore(&cmd->t_state_lock, flags);
2475-
}
24762453

24772454
if (cmd->se_lun)
24782455
transport_lun_remove_cmd(cmd);
@@ -2517,6 +2494,16 @@ int target_get_sess_cmd(struct se_cmd *se_cmd, bool ack_kref)
25172494
}
25182495
EXPORT_SYMBOL(target_get_sess_cmd);
25192496

2497+
static void target_free_cmd_mem(struct se_cmd *cmd)
2498+
{
2499+
transport_free_pages(cmd);
2500+
2501+
if (cmd->se_cmd_flags & SCF_SCSI_TMR_CDB)
2502+
core_tmr_release_req(cmd->se_tmr_req);
2503+
if (cmd->t_task_cdb != cmd->__t_task_cdb)
2504+
kfree(cmd->t_task_cdb);
2505+
}
2506+
25202507
static void target_release_cmd_kref(struct kref *kref)
25212508
{
25222509
struct se_cmd *se_cmd = container_of(kref, struct se_cmd, cmd_kref);
@@ -2526,17 +2513,20 @@ static void target_release_cmd_kref(struct kref *kref)
25262513
spin_lock_irqsave(&se_sess->sess_cmd_lock, flags);
25272514
if (list_empty(&se_cmd->se_cmd_list)) {
25282515
spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
2516+
target_free_cmd_mem(se_cmd);
25292517
se_cmd->se_tfo->release_cmd(se_cmd);
25302518
return;
25312519
}
25322520
if (se_sess->sess_tearing_down && se_cmd->cmd_wait_set) {
25332521
spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
2522+
target_free_cmd_mem(se_cmd);
25342523
complete(&se_cmd->cmd_wait_comp);
25352524
return;
25362525
}
25372526
list_del(&se_cmd->se_cmd_list);
25382527
spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
25392528

2529+
target_free_cmd_mem(se_cmd);
25402530
se_cmd->se_tfo->release_cmd(se_cmd);
25412531
}
25422532

@@ -2548,6 +2538,7 @@ int target_put_sess_cmd(struct se_cmd *se_cmd)
25482538
struct se_session *se_sess = se_cmd->se_sess;
25492539

25502540
if (!se_sess) {
2541+
target_free_cmd_mem(se_cmd);
25512542
se_cmd->se_tfo->release_cmd(se_cmd);
25522543
return 1;
25532544
}

‎include/target/target_core_base.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -140,6 +140,7 @@ enum se_cmd_flags_table {
140140
SCF_COMPARE_AND_WRITE = 0x00080000,
141141
SCF_COMPARE_AND_WRITE_POST = 0x00100000,
142142
SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC = 0x00200000,
143+
SCF_ACK_KREF = 0x00400000,
143144
};
144145

145146
/* struct se_dev_entry->lun_flags and struct se_lun->lun_access */

0 commit comments

Comments
 (0)