[PATCH 0/2] target: Fix LUN_RESET active I/O + TMR handling

[PATCH 0/2] target: Fix LUN_RESET active I/O + TMR handling

[Nicholas A. Bellinger]

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

[PATCH 1/2] target: Fix LUN_RESET active I/O handling for ACK_KREF

[Nicholas A. 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

[PATCH 2/2] target: Fix LUN_RESET active TMR descriptor handling

[Nicholas A. 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

Re: [PATCH 1/2] target: Fix LUN_RESET active I/O handling for ACK_KREF

[Christoph Hellwig]

> 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 would be really useful to have all drivers follow that
ACK KREF model instead of needing to deal with driver
differences everywhere..

> 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.

Sounds like that should be a separate patch.

> +/*
> + * Called with se_session->sess_cmd_lock held with irq disabled
> + */

Please enforce this in the code instead of the comments, e.g.

	assert_spin_locked(&se_session->sess_cmd_lock);
	WARN_ON_ONCE(!irqs_disabled());

> +static bool __target_check_io_state(struct se_cmd *se_cmd)
> +{
> +	struct se_session *sess = se_cmd->se_sess;
> +
> +	if (!sess)
> +		return false;

Given that you expect the session lock to be held this doesn't
make sense.

> +		/*
> +		 * 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
> +		 */

This just explains what __target_check_io_state does, but no why.
I'd suggest to remove the comment as it doesn't add any value.

Re: [PATCH 2/2] target: Fix LUN_RESET active TMR descriptor handling

[Christoph Hellwig]

>  	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);

list_del_init on a empty list is fine.

> +		sess = cmd->se_sess;
> +		if (!sess) {
> +			dump_stack();
> +			continue;
> +		}

Why not something like:

		if (WARN_ON_ONCE(!sess))
			continue;

same for the previous patch.

> +		spin_lock(&sess->sess_cmd_lock);
> +		rc = kref_get_unless_zero(&cmd->cmd_kref);
> +		spin_unlock(&sess->sess_cmd_lock);

no need to take a lock around kref_get_unless_zero, it's not going to
help with anything.

Re: [PATCH 1/2] target: Fix LUN_RESET active I/O handling for ACK_KREF

[Bart Van Assche]

On 01/12/2016 07:20 AM, Christoph Hellwig wrote:
>> 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 would be really useful to have all drivers follow that
> ACK KREF model instead of needing to deal with driver
> differences everywhere..

Hello Christoph,

How about eliminating the TARGET_SCF_ACK_KREF flag ? If a command would 
only be freed upon the last target_put_sess_cmd() call then I think that 
the check_stop_free() callback wouldn't have to call 
target_put_sess_cmd() and hence that the TARGET_SCF_ACK_KREF flag could 
be removed.

Bart.

Re: [PATCH 1/2] target: Fix LUN_RESET active I/O handling for ACK_KREF

[Christoph Hellwig]

On Tue, Jan 12, 2016 at 08:32:48AM -0800, Bart Van Assche wrote:
> Hello Christoph,
>
> How about eliminating the TARGET_SCF_ACK_KREF flag ? If a command would 
> only be freed upon the last target_put_sess_cmd() call then I think that 
> the check_stop_free() callback wouldn't have to call target_put_sess_cmd() 
> and hence that the TARGET_SCF_ACK_KREF flag could be removed.

Yes, that's what I meant.  I think it shoul be generally feasibly, but
would require a careful audit of the !TARGET_SCF_ACK_KREF code path
first.

Re: [PATCH 1/2] target: Fix LUN_RESET active I/O handling for ACK_KREF

[Nicholas A. Bellinger]

On Wed, 2016-01-13 at 09:34 +0100, Christoph Hellwig wrote:
> On Tue, Jan 12, 2016 at 08:32:48AM -0800, Bart Van Assche wrote:
> > Hello Christoph,
> >
> > How about eliminating the TARGET_SCF_ACK_KREF flag ? If a command would 
> > only be freed upon the last target_put_sess_cmd() call then I think that 
> > the check_stop_free() callback wouldn't have to call target_put_sess_cmd() 
> > and hence that the TARGET_SCF_ACK_KREF flag could be removed.
> 
> Yes, that's what I meant.  I think it shoul be generally feasibly, but
> would require a careful audit of the !TARGET_SCF_ACK_KREF code path
> first.

No, no, no.

The whole point of TARGET_SCF_ACK_KREF was so the backend driver
completion path calling check_stop_free() for one ->cmd_kref, and the
fabric driver patch calling target_put_sess_cmd() for the second
->cmd_kref both complete before attempting to free se_cmd memory.

The hw fabric drivers that have a hard requirement for
TARGET_SCF_ACK_KREF already use it, but it wouldn't hurt to convert the
remaining ones to use it.

Re: [PATCH 1/2] target: Fix LUN_RESET active I/O handling for ACK_KREF

[Christoph Hellwig]

On Wed, Jan 13, 2016 at 01:14:37AM -0800, Nicholas A. Bellinger wrote:
> > Yes, that's what I meant.  I think it shoul be generally feasibly, but
> > would require a careful audit of the !TARGET_SCF_ACK_KREF code path
> > first.
> 
> No, no, no.
> 
> The whole point of TARGET_SCF_ACK_KREF was so the backend driver
> completion path calling check_stop_free() for one ->cmd_kref, and the
> fabric driver patch calling target_put_sess_cmd() for the second
> ->cmd_kref both complete before attempting to free se_cmd memory.
> 
> The hw fabric drivers that have a hard requirement for
> TARGET_SCF_ACK_KREF already use it, but it wouldn't hurt to convert the
> remaining ones to use it.

Oh, I misread what Bart said above - I though he meant to always
take the ref as well.

Re: [PATCH 0/2] target: Fix LUN_RESET active I/O + TMR handling

[Quinn Tran]

[-- 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 --]

Re: [PATCH 1/2] target: Fix LUN_RESET active I/O handling for ACK_KREF

[Nicholas A. Bellinger]

On Tue, 2016-01-12 at 16:20 +0100, Christoph Hellwig wrote:
> > 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 would be really useful to have all drivers follow that
> ACK KREF model instead of needing to deal with driver
> differences everywhere..
> 

tcm_fc, usb-gadget, sbp-target and xen-scsiback are whom
need to be converted.

It should be easy enough to do for v4.6 code.

> > 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.
> 
> Sounds like that should be a separate patch.
> 

This should to stay with the original bug-fix for stable back-porting
purposes, and it's been kept together with the original for now.

It's time-consuming enough to back-port given the various upstream
changes recently.

> > +/*
> > + * Called with se_session->sess_cmd_lock held with irq disabled
> > + */
> 
> Please enforce this in the code instead of the comments, e.g.
> 
> 	assert_spin_locked(&se_session->sess_cmd_lock);
> 	WARN_ON_ONCE(!irqs_disabled());
> 

Done.

> > +static bool __target_check_io_state(struct se_cmd *se_cmd)
> > +{
> > +	struct se_session *sess = se_cmd->se_sess;
> > +
> > +	if (!sess)
> > +		return false;
> 
> Given that you expect the session lock to be held this doesn't
> make sense.
> 

Dropped.

> > +		/*
> > +		 * 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
> > +		 */
> 
> This just explains what __target_check_io_state does, but no why.
> I'd suggest to remove the comment as it doesn't add any value.

How about the following for __target_check_io_state()..?

       /*
        * If command already reached CMD_T_COMPLETE state within
        * target_complete_cmd(), this se_cmd has been passed to
        * fabric driver and will not be aborted.
        *
        * Otherwise, obtain a local se_cmd->cmd_kref now for TMR
        * ABORT_TASK + LUN_RESET for CMD_T_ABORTED processing as
        * long as se_cmd->cmd_kref is still active unless zero.
        */

Re: [PATCH 2/2] target: Fix LUN_RESET active TMR descriptor handling

[Nicholas A. Bellinger]

On Tue, 2016-01-12 at 16:25 +0100, Christoph Hellwig wrote:
> >  	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);
> 
> list_del_init on a empty list is fine.
> 

Done.

> > +		sess = cmd->se_sess;
> > +		if (!sess) {
> > +			dump_stack();
> > +			continue;
> > +		}
> 
> Why not something like:
> 
> 		if (WARN_ON_ONCE(!sess))
> 			continue;
> 
> same for the previous patch.
> 

Done.

> > +		spin_lock(&sess->sess_cmd_lock);
> > +		rc = kref_get_unless_zero(&cmd->cmd_kref);
> > +		spin_unlock(&sess->sess_cmd_lock);
> 
> no need to take a lock around kref_get_unless_zero, it's not going to
> help with anything.

So for -v2, this lock is being obtained before ->t_state_lock above, to
follow how __target_check_io_state() works with aborted I/O + LUN_RESET
and explicit TMR_ABORT_TASK.

It's been made consistent with the other cases for now, but depending on
how the bug-fix for se_session shutdown plus remote port LUN_RESET works
out, this may or may-not be able to go away for -v3.