* [PATCH 1/2] target: Fix LUN_RESET active I/O handling for ACK_KREF
2016-01-12 10:24 [PATCH 0/2] target: Fix LUN_RESET active I/O + TMR handling Nicholas A. Bellinger
@ 2016-01-12 10:24 ` Nicholas A. Bellinger
2016-01-12 15:20 ` Christoph Hellwig
2016-01-12 10:24 ` [PATCH 2/2] target: Fix LUN_RESET active TMR descriptor handling Nicholas A. Bellinger
2016-01-13 20:26 ` [PATCH 0/2] target: Fix LUN_RESET active I/O + TMR handling Quinn Tran
2 siblings, 1 reply; 12+ messages in thread
From: Nicholas A. Bellinger @ 2016-01-12 10:24 UTC (permalink / raw)
To: target-devel
Cc: linux-scsi, Quinn Tran, Himanshu Madhani, Sagi Grimberg,
Christoph Hellwig, Hannes Reinecke, Andy Grover,
Nicholas Bellinger
From: Nicholas Bellinger <nab@linux-iscsi.org>
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).
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.
Cc: 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>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
drivers/target/target_core_tmr.c | 72 +++++++++++++++++++++++++---------
drivers/target/target_core_transport.c | 49 ++++++++++++-----------
include/target/target_core_base.h | 1 +
3 files changed, 78 insertions(+), 44 deletions(-)
diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c
index 7137042..af4adb6 100644
--- a/drivers/target/target_core_tmr.c
+++ b/drivers/target/target_core_tmr.c
@@ -107,6 +107,29 @@ static int target_check_cdb_and_preempt(struct list_head *list,
return 1;
}
+/*
+ * Called with se_session->sess_cmd_lock held with irq disabled
+ */
+static bool __target_check_io_state(struct se_cmd *se_cmd)
+{
+ struct se_session *sess = se_cmd->se_sess;
+
+ if (!sess)
+ return false;
+
+ spin_lock(&se_cmd->t_state_lock);
+ if (se_cmd->transport_state & CMD_T_COMPLETE) {
+ printk("Attempted to abort io tag: %llu already complete,"
+ " skipping\n", se_cmd->tag);
+ spin_unlock(&se_cmd->t_state_lock);
+ return false;
+ }
+ se_cmd->transport_state |= CMD_T_ABORTED;
+ spin_unlock(&se_cmd->t_state_lock);
+
+ return kref_get_unless_zero(&se_cmd->cmd_kref);
+}
+
void core_tmr_abort_task(
struct se_device *dev,
struct se_tmr_req *tmr,
@@ -132,27 +155,23 @@ void core_tmr_abort_task(
printk("ABORT_TASK: Found referenced %s task_tag: %llu\n",
se_cmd->se_tfo->get_fabric_name(), ref_tag);
-
- spin_lock(&se_cmd->t_state_lock);
- if (se_cmd->transport_state & CMD_T_COMPLETE) {
- printk("ABORT_TASK: ref_tag: %llu already complete,"
- " skipping\n", ref_tag);
- spin_unlock(&se_cmd->t_state_lock);
+ /*
+ * Obtain cmd_kref and move to list for shutdown processing
+ * if se_cmd->cmd_kref is still active, the command has not
+ * already reached CMD_T_COMPLETE
+ */
+ if (!__target_check_io_state(se_cmd)) {
spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
goto out;
}
- se_cmd->transport_state |= CMD_T_ABORTED;
- spin_unlock(&se_cmd->t_state_lock);
-
list_del_init(&se_cmd->se_cmd_list);
- kref_get(&se_cmd->cmd_kref);
spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
cancel_work_sync(&se_cmd->work);
transport_wait_for_tasks(se_cmd);
- target_put_sess_cmd(se_cmd);
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);
@@ -237,8 +256,10 @@ static void core_tmr_drain_state_list(
struct list_head *preempt_and_abort_list)
{
LIST_HEAD(drain_task_list);
+ struct se_session *sess;
struct se_cmd *cmd, *next;
unsigned long flags;
+ int rc;
/*
* Complete outstanding commands with TASK_ABORTED SAM status.
@@ -277,6 +298,24 @@ static void core_tmr_drain_state_list(
if (prout_cmd == cmd)
continue;
+ sess = cmd->se_sess;
+ if (!sess) {
+ pr_err("NULL cmd->sess for tag: %llu\n", cmd->tag);
+ dump_stack();
+ continue;
+ }
+ /*
+ * Obtain cmd_kref and move to list for shutdown processing
+ * if se_cmd->cmd_kref is still active, the command has not
+ * already reached CMD_T_COMPLETE
+ */
+ spin_lock(&sess->sess_cmd_lock);
+ rc = __target_check_io_state(cmd);
+ spin_unlock(&sess->sess_cmd_lock);
+ if (!rc) {
+ printk("LUN_RESET I/O: non-zero kref_get_unless_zero\n");
+ continue;
+ }
list_move_tail(&cmd->state_list, &drain_task_list);
cmd->state_active = false;
}
@@ -308,16 +347,11 @@ static void core_tmr_drain_state_list(
* loop above, but we do it down here given that
* cancel_work_sync may block.
*/
- if (cmd->t_state == TRANSPORT_COMPLETE)
- cancel_work_sync(&cmd->work);
-
- spin_lock_irqsave(&cmd->t_state_lock, flags);
- target_stop_cmd(cmd, &flags);
-
- cmd->transport_state |= CMD_T_ABORTED;
- spin_unlock_irqrestore(&cmd->t_state_lock, flags);
+ cancel_work_sync(&cmd->work);
+ transport_wait_for_tasks(cmd);
core_tmr_handle_tas_abort(tmr_nacl, cmd, tas);
+ target_put_sess_cmd(cmd);
}
}
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index c5035b9..f2c596b 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -626,6 +626,8 @@ static void transport_lun_remove_cmd(struct se_cmd *cmd)
void transport_cmd_finish_abort(struct se_cmd *cmd, int remove)
{
+ bool ack_kref = (cmd->se_cmd_flags & SCF_ACK_KREF);
+
if (cmd->se_cmd_flags & SCF_SE_LUN_CMD)
transport_lun_remove_cmd(cmd);
/*
@@ -637,7 +639,7 @@ void transport_cmd_finish_abort(struct se_cmd *cmd, int remove)
if (transport_cmd_check_stop_to_fabric(cmd))
return;
- if (remove)
+ if (remove && ack_kref)
transport_put_cmd(cmd);
}
@@ -2219,20 +2221,14 @@ static inline void transport_free_pages(struct se_cmd *cmd)
}
/**
- * transport_release_cmd - free a command
- * @cmd: command to free
+ * transport_put_cmd - release a reference to a command
+ * @cmd: command to release
*
- * This routine unconditionally frees a command, and reference counting
- * or list removal must be done in the caller.
+ * This routine releases our reference to the command and frees it if possible.
*/
-static int transport_release_cmd(struct se_cmd *cmd)
+static int transport_put_cmd(struct se_cmd *cmd)
{
BUG_ON(!cmd->se_tfo);
-
- if (cmd->se_cmd_flags & SCF_SCSI_TMR_CDB)
- core_tmr_release_req(cmd->se_tmr_req);
- if (cmd->t_task_cdb != cmd->__t_task_cdb)
- kfree(cmd->t_task_cdb);
/*
* If this cmd has been setup with target_get_sess_cmd(), drop
* the kref and call ->release_cmd() in kref callback.
@@ -2240,18 +2236,6 @@ static int transport_release_cmd(struct se_cmd *cmd)
return target_put_sess_cmd(cmd);
}
-/**
- * transport_put_cmd - release a reference to a command
- * @cmd: command to release
- *
- * This routine releases our reference to the command and frees it if possible.
- */
-static int transport_put_cmd(struct se_cmd *cmd)
-{
- transport_free_pages(cmd);
- return transport_release_cmd(cmd);
-}
-
void *transport_kmap_data_sg(struct se_cmd *cmd)
{
struct scatterlist *sg = cmd->t_data_sg;
@@ -2456,7 +2440,7 @@ int transport_generic_free_cmd(struct se_cmd *cmd, int wait_for_tasks)
if (wait_for_tasks && (cmd->se_cmd_flags & SCF_SCSI_TMR_CDB))
transport_wait_for_tasks(cmd);
- ret = transport_release_cmd(cmd);
+ ret = transport_put_cmd(cmd);
} else {
if (wait_for_tasks)
transport_wait_for_tasks(cmd);
@@ -2495,8 +2479,10 @@ int target_get_sess_cmd(struct se_cmd *se_cmd, bool ack_kref)
* fabric acknowledgement that requires two target_put_sess_cmd()
* invocations before se_cmd descriptor release.
*/
- if (ack_kref)
+ if (ack_kref) {
+ se_cmd->se_cmd_flags |= SCF_ACK_KREF;
kref_get(&se_cmd->cmd_kref);
+ }
spin_lock_irqsave(&se_sess->sess_cmd_lock, flags);
if (se_sess->sess_tearing_down) {
@@ -2514,6 +2500,16 @@ out:
}
EXPORT_SYMBOL(target_get_sess_cmd);
+static void target_free_cmd_mem(struct se_cmd *cmd)
+{
+ transport_free_pages(cmd);
+
+ if (cmd->se_cmd_flags & SCF_SCSI_TMR_CDB)
+ core_tmr_release_req(cmd->se_tmr_req);
+ if (cmd->t_task_cdb != cmd->__t_task_cdb)
+ kfree(cmd->t_task_cdb);
+}
+
static void target_release_cmd_kref(struct kref *kref)
__releases(&se_cmd->se_sess->sess_cmd_lock)
{
@@ -2522,17 +2518,20 @@ static void target_release_cmd_kref(struct kref *kref)
if (list_empty(&se_cmd->se_cmd_list)) {
spin_unlock(&se_sess->sess_cmd_lock);
+ target_free_cmd_mem(se_cmd);
se_cmd->se_tfo->release_cmd(se_cmd);
return;
}
if (se_sess->sess_tearing_down && se_cmd->cmd_wait_set) {
spin_unlock(&se_sess->sess_cmd_lock);
+ target_free_cmd_mem(se_cmd);
complete(&se_cmd->cmd_wait_comp);
return;
}
list_del(&se_cmd->se_cmd_list);
spin_unlock(&se_sess->sess_cmd_lock);
+ target_free_cmd_mem(se_cmd);
se_cmd->se_tfo->release_cmd(se_cmd);
}
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index a4bed07..1060c52 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -140,6 +140,7 @@ enum se_cmd_flags_table {
SCF_COMPARE_AND_WRITE = 0x00080000,
SCF_COMPARE_AND_WRITE_POST = 0x00100000,
SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC = 0x00200000,
+ SCF_ACK_KREF = 0x00400000,
};
/* struct se_dev_entry->lun_flags and struct se_lun->lun_access */
--
1.9.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/2] target: Fix LUN_RESET active TMR descriptor handling
2016-01-12 10:24 [PATCH 0/2] target: Fix LUN_RESET active I/O + TMR handling Nicholas A. Bellinger
2016-01-12 10:24 ` [PATCH 1/2] target: Fix LUN_RESET active I/O handling for ACK_KREF Nicholas A. Bellinger
@ 2016-01-12 10:24 ` Nicholas A. Bellinger
2016-01-12 15:25 ` Christoph Hellwig
2016-01-13 20:26 ` [PATCH 0/2] target: Fix LUN_RESET active I/O + TMR handling Quinn Tran
2 siblings, 1 reply; 12+ messages in thread
From: Nicholas A. Bellinger @ 2016-01-12 10:24 UTC (permalink / raw)
To: target-devel
Cc: linux-scsi, Quinn Tran, Himanshu Madhani, Sagi Grimberg,
Christoph Hellwig, Hannes Reinecke, Andy Grover,
Nicholas Bellinger
From: Nicholas Bellinger <nab@linux-iscsi.org>
This patch fixes a NULL pointer se_cmd->cmd_kref < 0
refcount bug during TMR LUN_RESET with TMR se_cmd,
triggered during se_cmd + se_tmr_req descriptor
shutdown + release via core_tmr_drain_tmr_list().
It obtains kref_get_unless_zero(&se_cmd->cmd_kref)
following __target_check_io_state() for active I/O
CMD_T_ABORTED, and adds transport_wait_for_tasks()
followed by the final target_put_sess_cmd() to
release TMR logic locally obtained ->cmd_kref.
Also add two new checks within target_tmr_work() to
avoid CMD_T_ABORTED -> TFO->queue_tm_rsp() callbacks
ahead of invoking the backend -> fabric put in
transport_cmd_check_stop_to_fabric().
For good measure, also change core_tmr_release_req()
to use !list_empty() + list_del_init() ahead of
se_tmr_req memory free.
Cc: 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>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
drivers/target/target_core_tmr.c | 24 +++++++++++++++++++++++-
drivers/target/target_core_transport.c | 17 +++++++++++++++++
2 files changed, 40 insertions(+), 1 deletion(-)
diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c
index af4adb6..894a0b0 100644
--- a/drivers/target/target_core_tmr.c
+++ b/drivers/target/target_core_tmr.c
@@ -68,7 +68,8 @@ void core_tmr_release_req(struct se_tmr_req *tmr)
if (dev) {
spin_lock_irqsave(&dev->se_tmr_lock, flags);
- list_del(&tmr->tmr_list);
+ if (!list_empty(&tmr->tmr_list))
+ list_del_init(&tmr->tmr_list);
spin_unlock_irqrestore(&dev->se_tmr_lock, flags);
}
@@ -192,9 +193,12 @@ static void core_tmr_drain_tmr_list(
struct list_head *preempt_and_abort_list)
{
LIST_HEAD(drain_tmr_list);
+ struct se_session *sess;
struct se_tmr_req *tmr_p, *tmr_pp;
struct se_cmd *cmd;
unsigned long flags;
+ bool rc;
+
/*
* Release all pending and outgoing TMRs aside from the received
* LUN_RESET tmr..
@@ -220,6 +224,12 @@ static void core_tmr_drain_tmr_list(
if (target_check_cdb_and_preempt(preempt_and_abort_list, cmd))
continue;
+ sess = cmd->se_sess;
+ if (!sess) {
+ dump_stack();
+ continue;
+ }
+
spin_lock(&cmd->t_state_lock);
if (!(cmd->transport_state & CMD_T_ACTIVE)) {
spin_unlock(&cmd->t_state_lock);
@@ -229,8 +239,16 @@ static void core_tmr_drain_tmr_list(
spin_unlock(&cmd->t_state_lock);
continue;
}
+ cmd->transport_state |= CMD_T_ABORTED;
spin_unlock(&cmd->t_state_lock);
+ spin_lock(&sess->sess_cmd_lock);
+ rc = kref_get_unless_zero(&cmd->cmd_kref);
+ spin_unlock(&sess->sess_cmd_lock);
+ if (!rc) {
+ printk("LUN_RESET TMR: non-zero kref_get_unless_zero\n");
+ continue;
+ }
list_move_tail(&tmr_p->tmr_list, &drain_tmr_list);
}
spin_unlock_irqrestore(&dev->se_tmr_lock, flags);
@@ -244,7 +262,11 @@ static void core_tmr_drain_tmr_list(
(preempt_and_abort_list) ? "Preempt" : "", tmr_p,
tmr_p->function, tmr_p->response, cmd->t_state);
+ cancel_work_sync(&cmd->work);
+ transport_wait_for_tasks(cmd);
+
transport_cmd_finish_abort(cmd, 1);
+ target_put_sess_cmd(cmd);
}
}
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index f2c596b..f6ecb60 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -2905,8 +2905,17 @@ static void target_tmr_work(struct work_struct *work)
struct se_cmd *cmd = container_of(work, struct se_cmd, work);
struct se_device *dev = cmd->se_dev;
struct se_tmr_req *tmr = cmd->se_tmr_req;
+ unsigned long flags;
int ret;
+ spin_lock_irqsave(&cmd->t_state_lock, flags);
+ if (cmd->transport_state & CMD_T_ABORTED) {
+ tmr->response = TMR_FUNCTION_REJECTED;
+ spin_unlock_irqrestore(&cmd->t_state_lock, flags);
+ goto check_stop;
+ }
+ spin_unlock_irqrestore(&cmd->t_state_lock, flags);
+
switch (tmr->function) {
case TMR_ABORT_TASK:
core_tmr_abort_task(dev, tmr, cmd->se_sess);
@@ -2939,9 +2948,17 @@ static void target_tmr_work(struct work_struct *work)
break;
}
+ spin_lock_irqsave(&cmd->t_state_lock, flags);
+ if (cmd->transport_state & CMD_T_ABORTED) {
+ spin_unlock_irqrestore(&cmd->t_state_lock, flags);
+ goto check_stop;
+ }
cmd->t_state = TRANSPORT_ISTATE_PROCESSING;
+ spin_unlock_irqrestore(&cmd->t_state_lock, flags);
+
cmd->se_tfo->queue_tm_rsp(cmd);
+check_stop:
transport_cmd_check_stop_to_fabric(cmd);
}
--
1.9.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 0/2] target: Fix LUN_RESET active I/O + TMR handling
2016-01-12 10:24 [PATCH 0/2] target: Fix LUN_RESET active I/O + TMR handling Nicholas A. Bellinger
2016-01-12 10:24 ` [PATCH 1/2] target: Fix LUN_RESET active I/O handling for ACK_KREF Nicholas A. Bellinger
2016-01-12 10:24 ` [PATCH 2/2] target: Fix LUN_RESET active TMR descriptor handling Nicholas A. Bellinger
@ 2016-01-13 20:26 ` Quinn Tran
2 siblings, 0 replies; 12+ messages in thread
From: Quinn Tran @ 2016-01-13 20:26 UTC (permalink / raw)
To: Nicholas A. Bellinger, target-devel
Cc: linux-scsi, Himanshu Madhani, Sagi Grimberg, Christoph Hellwig,
Hannes Reinecke, Andy Grover, Nicholas Bellinger
[-- Attachment #1: Type: text/plain, Size: 1574 bytes --]
On 1/12/16, 2:24 AM, "Nicholas A. Bellinger" <nab@daterainc.com> wrote:
>From: Nicholas Bellinger <nab@linux-iscsi.org>
>
>Hi all,
>
>This -v1 series is to address two LUN_RESET active
>I/O + TMR se_cmd->cmd_kref < 0 bugs as reported
>recently by Quinn & Co, that can occur during
>active I/O LUN_RESET with single / multiple LIO
>export port configs.
>
>To address this bug, add __target_check_io_state()
>common handler for ABORT_TASK + LUN_RESET I/O abort
>cases, and move remaining se_cmd SGL page + release
>into target_free_cmd_mem() to now be called directly
>from the final target_release_cmd_kref() callback.
>
>Following __target_check_io_state() code, also obtain
>kref_get_unless_zero(&se_cmd->cmd_kref) for LUN_RESET
>TMR abort in core_tmr_drain_tmr_list(), in order to
>utilize common transport_wait_for_tasks() code
>when se_tmr_req descriptors are being shutdown.
>
>Note there are a few more cleanups to be had but
>have been left off for the moment, ahead of these
>specific fixes getting merged for v4.5-rc code.
>
>Please review,
>
>--nab
>
>Nicholas Bellinger (2):
> target: Fix LUN_RESET active I/O handling for ACK_KREF
> target: Fix LUN_RESET active TMR descriptor handling
>
> drivers/target/target_core_tmr.c | 96 +++++++++++++++++++++++++++-------
> drivers/target/target_core_transport.c | 66 ++++++++++++++---------
> include/target/target_core_base.h | 1 +
> 3 files changed, 118 insertions(+), 45 deletions(-)
>
>--
>1.9.1
>
QT: Nicholas, both patches looks good. Thanks.
>
[-- Attachment #2: winmail.dat --]
[-- Type: application/ms-tnef, Size: 4592 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread