All of lore.kernel.org
 help / color / mirror / Atom feed
From: Himanshu Madhani <himanshu.madhani@qlogic.com>
To: target-devel@vger.kernel.org, nab@linux-iscsi.org
Cc: giridhar.malavali@qlogic.com, linux-scsi@vger.kernel.org,
	himanshu.madhani@qlogic.com
Subject: [PATCH 12/20] target/tmr: LUN reset cause cmd premature free.
Date: Mon, 7 Dec 2015 19:48:59 -0500	[thread overview]
Message-ID: <1449535747-2850-13-git-send-email-himanshu.madhani@qlogic.com> (raw)
In-Reply-To: <1449535747-2850-1-git-send-email-himanshu.madhani@qlogic.com>

From: Quinn Tran <quinn.tran@qlogic.com>

During LUN/Target reset, the TMR code attempt to intercept
cmds and try to aborted them.  Current code assume cmds are
always intercepted at the back end device.  The cleanup code
would issue a "queue_status() & check_stop_free()" to terminate
the command.  However, when a cmd is intercepted at the front
end/Fabric layer, current code introduce premature free or
cause Fabric to double free.

When command is intercepted at Fabric layer, it means a
check_stop_free(cmd_kref--) has been called.  The extra
check_stop_free in the Lun Reset cleanup code causes early
free.  When a cmd in the Fabric layer is completed, the normal
free code adds another another free which introduce a double free.

To fix the issue:
- add a new flag/CMD_T_SENT_STATUS to track command that have
 made it down to fabric layer after back end good/bad completion.
- if cmd reach Fabric Layer at Lun Reset time, add an extra
 cmd_kref count to prevent premature free.

Signed-off-by: Quinn Tran <quinn.tran@qlogic.com>
Signed-off-by: Himanshu Madhani <himanshu.madhani@qlogic.com>
---
 drivers/target/target_core_tmr.c       |   33 +++++++++++++++++++++++++++++++-
 drivers/target/target_core_transport.c |   30 +++++++++++++++++++++++++++++
 include/target/target_core_base.h      |    1 +
 3 files changed, 63 insertions(+), 1 deletions(-)

diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c
index 28fb301..41f8b57 100644
--- a/drivers/target/target_core_tmr.c
+++ b/drivers/target/target_core_tmr.c
@@ -243,7 +243,9 @@ static void core_tmr_drain_state_list(
 {
 	LIST_HEAD(drain_task_list);
 	struct se_cmd *cmd, *next;
-	unsigned long flags;
+	unsigned long flags, flags2;
+	int rmkref;
+	struct se_session *se_sess;
 
 	/*
 	 * Complete outstanding commands with TASK_ABORTED SAM status.
@@ -282,6 +284,16 @@ static void core_tmr_drain_state_list(
 		if (prout_cmd == cmd)
 			continue;
 
+		se_sess = cmd->se_sess;
+		/* take an extra kref to prevent cmd free race condition. */
+		spin_lock_irqsave(&se_sess->sess_cmd_lock, flags2);
+		if (!kref_get_unless_zero(&cmd->cmd_kref)) {
+			/* cmd is already in the free process */
+			spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags2);
+			continue;
+		}
+		spin_unlock_irqrestore(&se_sess->sess_cmd_lock,	flags2);
+
 		list_move_tail(&cmd->state_list, &drain_task_list);
 		cmd->state_active = false;
 	}
@@ -320,9 +332,28 @@ static void core_tmr_drain_state_list(
 		target_stop_cmd(cmd, &flags);
 
 		cmd->transport_state |= CMD_T_ABORTED;
+
+		/* CMD_T_SENT_STATUS: cmd is down in fabric layer.
+		 * A check stop has been called.  Keep the extra kref
+		 * from above because core_tmr_handle_tas_abort will
+		 * generate another check_stop.
+		 *
+		 * !CMD_T_SENT_STATUS: cmd intercepted at back end.
+		 * Remove the extra kref from above because only
+		 * 1 check_stop is required or generated by
+		 * core_tmr_handle_tas_abort()
+		 */
+		rmkref = 0;
+		if (!((cmd->t_state == TRANSPORT_COMPLETE) &&
+		    (cmd->transport_state & CMD_T_SENT_STATUS)))
+			rmkref = 1;
+
 		spin_unlock_irqrestore(&cmd->t_state_lock, flags);
 
 		core_tmr_handle_tas_abort(tmr_nacl, cmd, tas);
+
+		if (rmkref)
+			target_put_sess_cmd(cmd);
 	}
 }
 
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 4fdcee2..cdd18bf 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -639,9 +639,14 @@ void transport_cmd_finish_abort(struct se_cmd *cmd, int remove)
 static void target_complete_failure_work(struct work_struct *work)
 {
 	struct se_cmd *cmd = container_of(work, struct se_cmd, work);
+	unsigned long flags;
 
 	transport_generic_request_failure(cmd,
 			TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE);
+
+	spin_lock_irqsave(&cmd->t_state_lock, flags);
+	cmd->transport_state |= CMD_T_SENT_STATUS;
+	spin_unlock_irqrestore(&cmd->t_state_lock, flags);
 }
 
 /*
@@ -1659,6 +1664,7 @@ void transport_generic_request_failure(struct se_cmd *cmd,
 		sense_reason_t sense_reason)
 {
 	int ret = 0, post_ret = 0;
+	unsigned long flags;
 
 	pr_debug("-----[ Storage Engine Exception for cmd: %p ITT: 0x%08llx"
 		" CDB: 0x%02x\n", cmd, cmd->tag, cmd->t_task_cdb[0]);
@@ -1670,6 +1676,10 @@ void transport_generic_request_failure(struct se_cmd *cmd,
 		(cmd->transport_state & CMD_T_STOP) != 0,
 		(cmd->transport_state & CMD_T_SENT) != 0);
 
+	spin_lock_irqsave(&cmd->t_state_lock, flags);
+	cmd->transport_state |= CMD_T_SENT_STATUS;
+	spin_unlock_irqrestore(&cmd->t_state_lock, flags);
+
 	/*
 	 * For SAM Task Attribute emulation for failed struct se_cmd
 	 */
@@ -1951,6 +1961,7 @@ static void transport_complete_task_attr(struct se_cmd *cmd)
 static void transport_complete_qf(struct se_cmd *cmd)
 {
 	int ret = 0;
+	unsigned long flags;
 
 	transport_complete_task_attr(cmd);
 
@@ -1986,6 +1997,10 @@ out:
 	}
 	transport_lun_remove_cmd(cmd);
 	transport_cmd_check_stop_to_fabric(cmd);
+
+	spin_lock_irqsave(&cmd->t_state_lock, flags);
+	cmd->transport_state |= CMD_T_SENT_STATUS;
+	spin_unlock_irqrestore(&cmd->t_state_lock, flags);
 }
 
 static void transport_handle_queue_full(
@@ -2032,6 +2047,7 @@ static void target_complete_ok_work(struct work_struct *work)
 {
 	struct se_cmd *cmd = container_of(work, struct se_cmd, work);
 	int ret;
+	unsigned long flags;
 
 	/*
 	 * Check if we need to move delayed/dormant tasks from cmds on the
@@ -2060,6 +2076,10 @@ static void target_complete_ok_work(struct work_struct *work)
 
 		transport_lun_remove_cmd(cmd);
 		transport_cmd_check_stop_to_fabric(cmd);
+
+		spin_lock_irqsave(&cmd->t_state_lock, flags);
+		cmd->transport_state |= CMD_T_SENT_STATUS;
+		spin_unlock_irqrestore(&cmd->t_state_lock, flags);
 		return;
 	}
 	/*
@@ -2086,6 +2106,11 @@ static void target_complete_ok_work(struct work_struct *work)
 
 			transport_lun_remove_cmd(cmd);
 			transport_cmd_check_stop_to_fabric(cmd);
+
+			spin_lock_irqsave(&cmd->t_state_lock, flags);
+			cmd->transport_state |= CMD_T_SENT_STATUS;
+			spin_unlock_irqrestore(&cmd->t_state_lock, flags);
+
 			return;
 		}
 	}
@@ -2136,6 +2161,7 @@ queue_rsp:
 		ret = cmd->se_tfo->queue_status(cmd);
 		if (ret == -EAGAIN || ret == -ENOMEM)
 			goto queue_full;
+
 		break;
 	default:
 		break;
@@ -2143,6 +2169,10 @@ queue_rsp:
 
 	transport_lun_remove_cmd(cmd);
 	transport_cmd_check_stop_to_fabric(cmd);
+
+	spin_lock_irqsave(&cmd->t_state_lock, flags);
+	cmd->transport_state |= CMD_T_SENT_STATUS;
+	spin_unlock_irqrestore(&cmd->t_state_lock, flags);
 	return;
 
 queue_full:
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index aabf0ac..efccd71 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -490,6 +490,7 @@ struct se_cmd {
 #define CMD_T_DEV_ACTIVE	(1 << 7)
 #define CMD_T_REQUEST_STOP	(1 << 8)
 #define CMD_T_BUSY		(1 << 9)
+#define CMD_T_SENT_STATUS (1 << 10)
 	spinlock_t		t_state_lock;
 	struct kref		cmd_kref;
 	struct completion	t_transport_stop_comp;
-- 
1.7.7

  parent reply	other threads:[~2015-12-08  0:48 UTC|newest]

Thread overview: 51+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-12-08  0:48 [PATCH 00/20] qla2xxx: Patches for target-pending branch Himanshu Madhani
2015-12-08  0:48 ` [PATCH 01/20] qla2xxx: Enable Extended Login support Himanshu Madhani
2015-12-08 15:51   ` Hannes Reinecke
2015-12-08 19:35     ` Himanshu Madhani
2015-12-08  0:48 ` [PATCH 02/20] qla2xxx: Enable Exchange offload support Himanshu Madhani
2015-12-08 15:52   ` Hannes Reinecke
2015-12-08  0:48 ` [PATCH 03/20] qla2xxx: Enable Target counters in DebugFS Himanshu Madhani
2015-12-08 15:52   ` Hannes Reinecke
2015-12-08  0:48 ` [PATCH 04/20] qla2xxx: Add FW resource count " Himanshu Madhani
2015-12-08 15:53   ` Hannes Reinecke
2015-12-08  0:48 ` [PATCH 05/20] qla2xxx: Added interface to send ELS commands from driver Himanshu Madhani
2015-12-08  2:10   ` kbuild test robot
2015-12-08  2:10   ` [PATCH] qla2xxx: fix ifnullfree.cocci warnings kbuild test robot
2015-12-08 15:54   ` [PATCH 05/20] qla2xxx: Added interface to send ELS commands from driver Hannes Reinecke
2015-12-08  0:48 ` [PATCH 06/20] qla2xxx: Delete session if initiator is gone from FW Himanshu Madhani
2015-12-08  1:41   ` kbuild test robot
2015-12-08 15:58   ` Hannes Reinecke
2015-12-08  0:48 ` [PATCH 07/20] qla2xxx: Wait for all conflicts before ack'ing PLOGI Himanshu Madhani
2015-12-08 16:00   ` Hannes Reinecke
2015-12-08  0:48 ` [PATCH 08/20] qla2xxx: Replace QLA_TGT_STATE_ABORTED with a bit Himanshu Madhani
2015-12-08 16:01   ` Hannes Reinecke
2015-12-08  0:48 ` [PATCH 09/20] qla2xxx: Change check_stop_free to always return 1 Himanshu Madhani
2015-12-08  2:33   ` Christoph Hellwig
2015-12-09  6:56   ` Hannes Reinecke
2015-12-10  1:06     ` Quinn Tran
2015-12-08  0:48 ` [PATCH 10/20] qla2xxx: Fix interaction issue between qla2xxx and Target Core Module Himanshu Madhani
2015-12-08  2:37   ` Christoph Hellwig
2015-12-09 22:07     ` Quinn Tran
2015-12-14 10:34       ` Christoph Hellwig
2015-12-14 21:59         ` Quinn Tran
2015-12-09  7:01   ` Hannes Reinecke
2015-12-09 22:41     ` Quinn Tran
2015-12-08  0:48 ` [PATCH 11/20] qla2xxx: Add TAS detection for kernel 3.15 n newer Himanshu Madhani
2015-12-08  2:48   ` Christoph Hellwig
2015-12-09 20:24     ` Quinn Tran
2015-12-14 10:37       ` Christoph Hellwig
2015-12-14 22:00         ` Quinn Tran
2015-12-09  7:02   ` Hannes Reinecke
2015-12-08  0:48 ` Himanshu Madhani [this message]
2015-12-08  2:48   ` [PATCH 12/20] target/tmr: LUN reset cause cmd premature free Christoph Hellwig
2015-12-09 20:11     ` Quinn Tran
2016-01-04  7:44     ` Bart Van Assche
2015-12-09  7:03   ` Hannes Reinecke
2015-12-08  0:49 ` [PATCH 13/20] qla2xxx: Remove dependency on hardware_lock to reduce lock contention Himanshu Madhani
2015-12-08  0:49 ` [PATCH 14/20] qla2xxx: Add irq affinity notification Himanshu Madhani
2015-12-08  0:49 ` [PATCH 15/20] qla2xxx: Add selective command queuing Himanshu Madhani
2015-12-08  0:49 ` [PATCH 16/20] qla2xxx: Move atioq to a different lock to reduce lock contention Himanshu Madhani
2015-12-08  0:49 ` [PATCH 17/20] qla2xxx: Disable ZIO at start time Himanshu Madhani
2015-12-08  0:49 ` [PATCH 18/20] qla2xxx: Set all queues to 4k Himanshu Madhani
2015-12-08  0:49 ` [PATCH 19/20] qla2xxx: Add bulk send for atio & ctio completion paths Himanshu Madhani
2015-12-08  0:49 ` [PATCH 20/20] qla2xxx: Check for online flag instead of active reset when transmitting responses Himanshu Madhani

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1449535747-2850-13-git-send-email-himanshu.madhani@qlogic.com \
    --to=himanshu.madhani@qlogic.com \
    --cc=giridhar.malavali@qlogic.com \
    --cc=linux-scsi@vger.kernel.org \
    --cc=nab@linux-iscsi.org \
    --cc=target-devel@vger.kernel.org \
    --subject='Re: [PATCH 12/20] target/tmr: LUN reset cause cmd premature free.' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.