[PATCH 0/7] series to fix qla2xxx use-after-free

[PATCH 0/7] series to fix qla2xxx use-after-free

[Roland Dreier]

From: Roland Dreier <roland@purestorage.com>

Hi Nic,

Here's a series that's fundamentally about fixing a use-after-free in
qla_target code.  It ends up being seven patches because I wanted to
make each step easy to review, and several of these are just cleanups
that stand on their own.

We have a few more qla2xxx-related fixes that I need to clean up and
merge with the latest.  I'll send them soon.

Roland Dreier (7):
  qla2xxx: Get rid of redundant qla_tgt_sess.tearing_down
  target: Un-export target_get_sess_cmd()
  sbp-target: Consolidate duplicated error path code in
    sbp_handle_command()
  target: Allow for target_submit_cmd() returning errors
  target: Check sess_tearing_down in target_get_sess_cmd()
  qla2xxx: Remove racy, now-redundant check of sess_tearing_down
  target: Remove se_session.sess_wait_list

 drivers/scsi/qla2xxx/qla_target.c      |   16 ++--------
 drivers/scsi/qla2xxx/qla_target.h      |    1 -
 drivers/scsi/qla2xxx/tcm_qla2xxx.c     |   10 +++---
 drivers/target/sbp/sbp_target.c        |   36 ++++++++++-----------
 drivers/target/target_core_transport.c |   55 ++++++++++++++++++++------------
 drivers/target/tcm_fc/tfc_cmd.c        |    8 +++--
 drivers/usb/gadget/tcm_usb_gadget.c    |   20 +++++-------
 include/target/target_core_base.h      |    1 -
 include/target/target_core_fabric.h    |    5 ++-
 9 files changed, 73 insertions(+), 79 deletions(-)

-- 
1.7.10.4

[PATCH 1/7] qla2xxx: Get rid of redundant qla_tgt_sess.tearing_down

[Roland Dreier]

From: Roland Dreier <roland@purestorage.com>

The only place that sets qla_tgt_sess.tearing_down calls
target_splice_sess_cmd_list() immediately afterwards, without dropping
the lock it holds.  That function sets se_session.sess_tearing_down,
so we can get rid of the qla_target-specific flag, and in the one
place that looks at the qla_tgt_sess.tearing_down flag just test
se_session.sess_tearing_down instead.

Cc: Chad Dupuis <chad.dupuis@qlogic.com>
Cc: Arun Easi <arun.easi@qlogic.com>
Signed-off-by: Roland Dreier <roland@purestorage.com>
---
 drivers/scsi/qla2xxx/qla_target.c  |    2 +-
 drivers/scsi/qla2xxx/qla_target.h  |    1 -
 drivers/scsi/qla2xxx/tcm_qla2xxx.c |    1 -
 3 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_target.c b/drivers/scsi/qla2xxx/qla_target.c
index 77759c7..87b5a330 100644
--- a/drivers/scsi/qla2xxx/qla_target.c
+++ b/drivers/scsi/qla2xxx/qla_target.c
@@ -2644,7 +2644,7 @@ static void qlt_do_work(struct work_struct *work)
 	sess = ha->tgt.tgt_ops->find_sess_by_s_id(vha,
 	    atio->u.isp24.fcp_hdr.s_id);
 	if (sess) {
-		if (unlikely(sess->tearing_down)) {
+		if (unlikely(sess->se_sess->sess_tearing_down)) {
 			sess = NULL;
 			spin_unlock_irqrestore(&ha->hardware_lock, flags);
 			goto out_term;
diff --git a/drivers/scsi/qla2xxx/qla_target.h b/drivers/scsi/qla2xxx/qla_target.h
index 9f9ef16..07aa265 100644
--- a/drivers/scsi/qla2xxx/qla_target.h
+++ b/drivers/scsi/qla2xxx/qla_target.h
@@ -813,7 +813,6 @@ struct qla_tgt_sess {
 	unsigned int conf_compl_supported:1;
 	unsigned int deleted:1;
 	unsigned int local:1;
-	unsigned int tearing_down:1;
 
 	struct se_session *se_sess;
 	struct scsi_qla_host *vha;
diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
index 6e64314..3cb2b97 100644
--- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
+++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
@@ -466,7 +466,6 @@ static int tcm_qla2xxx_shutdown_session(struct se_session *se_sess)
 	vha = sess->vha;
 
 	spin_lock_irqsave(&vha->hw->hardware_lock, flags);
-	sess->tearing_down = 1;
 	target_splice_sess_cmd_list(se_sess);
 	spin_unlock_irqrestore(&vha->hw->hardware_lock, flags);
 
-- 
1.7.10.4

[PATCH 2/7] target: Un-export target_get_sess_cmd()

[Roland Dreier]

From: Roland Dreier <roland@purestorage.com>

There are no in-tree users of target_get_sess_cmd() outside of
target_core_transport.c.  Any new code should use the higher-level
target_submit_cmd() interface.  So let's un-export target_get_sess_cmd()
and make it static to the one file where it's actually used.

Signed-off-by: Roland Dreier <roland@purestorage.com>
---
 drivers/target/target_core_transport.c |    6 +++---
 include/target/target_core_fabric.h    |    1 -
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 634d0f3..82e40e1 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -73,6 +73,7 @@ static void transport_complete_task_attr(struct se_cmd *cmd);
 static void transport_handle_queue_full(struct se_cmd *cmd,
 		struct se_device *dev);
 static int transport_generic_get_mem(struct se_cmd *cmd);
+static void target_get_sess_cmd(struct se_session *, struct se_cmd *, bool);
 static void transport_put_cmd(struct se_cmd *cmd);
 static void transport_remove_cmd_from_queue(struct se_cmd *cmd);
 static int transport_set_sense_codes(struct se_cmd *cmd, u8 asc, u8 ascq);
@@ -3648,8 +3649,8 @@ EXPORT_SYMBOL(transport_generic_free_cmd);
  * @se_cmd:	command descriptor to add
  * @ack_kref:	Signal that fabric will perform an ack target_put_sess_cmd()
  */
-void target_get_sess_cmd(struct se_session *se_sess, struct se_cmd *se_cmd,
-			bool ack_kref)
+static void target_get_sess_cmd(struct se_session *se_sess, struct se_cmd *se_cmd,
+				bool ack_kref)
 {
 	unsigned long flags;
 
@@ -3669,7 +3670,6 @@ void target_get_sess_cmd(struct se_session *se_sess, struct se_cmd *se_cmd,
 	se_cmd->check_release = 1;
 	spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
 }
-EXPORT_SYMBOL(target_get_sess_cmd);
 
 static void target_release_cmd_kref(struct kref *kref)
 {
diff --git a/include/target/target_core_fabric.h b/include/target/target_core_fabric.h
index c78a233..1e68882 100644
--- a/include/target/target_core_fabric.h
+++ b/include/target/target_core_fabric.h
@@ -129,7 +129,6 @@ bool	transport_wait_for_tasks(struct se_cmd *);
 int	transport_check_aborted_status(struct se_cmd *, int);
 int	transport_send_check_condition_and_sense(struct se_cmd *, u8, int);
 
-void	target_get_sess_cmd(struct se_session *, struct se_cmd *, bool);
 int	target_put_sess_cmd(struct se_session *, struct se_cmd *);
 void	target_splice_sess_cmd_list(struct se_session *);
 void	target_wait_for_sess_cmds(struct se_session *, int);
-- 
1.7.10.4

[PATCH 3/7] sbp-target: Consolidate duplicated error path code in sbp_handle_command()

[Roland Dreier]

From: Roland Dreier <roland@purestorage.com>

Cc: Chris Boot <bootc@bootc.net>
Cc: Stefan Richter <stefanr@s5r6.in-berlin.de>
Signed-off-by: Roland Dreier <roland@purestorage.com>
---
 drivers/target/sbp/sbp_target.c |   28 ++++++++++++----------------
 1 file changed, 12 insertions(+), 16 deletions(-)

diff --git a/drivers/target/sbp/sbp_target.c b/drivers/target/sbp/sbp_target.c
index 7e6136e..2425ca4 100644
--- a/drivers/target/sbp/sbp_target.c
+++ b/drivers/target/sbp/sbp_target.c
@@ -1219,28 +1219,14 @@ static void sbp_handle_command(struct sbp_target_request *req)
 	ret = sbp_fetch_command(req);
 	if (ret) {
 		pr_debug("sbp_handle_command: fetch command failed: %d\n", ret);
-		req->status.status |= cpu_to_be32(
-			STATUS_BLOCK_RESP(STATUS_RESP_TRANSPORT_FAILURE) |
-			STATUS_BLOCK_DEAD(0) |
-			STATUS_BLOCK_LEN(1) |
-			STATUS_BLOCK_SBP_STATUS(SBP_STATUS_UNSPECIFIED_ERROR));
-		sbp_send_status(req);
-		sbp_free_request(req);
-		return;
+		goto err;
 	}
 
 	ret = sbp_fetch_page_table(req);
 	if (ret) {
 		pr_debug("sbp_handle_command: fetch page table failed: %d\n",
 			ret);
-		req->status.status |= cpu_to_be32(
-			STATUS_BLOCK_RESP(STATUS_RESP_TRANSPORT_FAILURE) |
-			STATUS_BLOCK_DEAD(0) |
-			STATUS_BLOCK_LEN(1) |
-			STATUS_BLOCK_SBP_STATUS(SBP_STATUS_UNSPECIFIED_ERROR));
-		sbp_send_status(req);
-		sbp_free_request(req);
-		return;
+		goto err;
 	}
 
 	unpacked_lun = req->login->lun->unpacked_lun;
@@ -1252,6 +1238,16 @@ static void sbp_handle_command(struct sbp_target_request *req)
 	target_submit_cmd(&req->se_cmd, sess->se_sess, req->cmd_buf,
 			req->sense_buf, unpacked_lun, data_length,
 			MSG_SIMPLE_TAG, data_dir, 0);
+	return;
+
+err:
+	req->status.status |= cpu_to_be32(
+		STATUS_BLOCK_RESP(STATUS_RESP_TRANSPORT_FAILURE) |
+		STATUS_BLOCK_DEAD(0) |
+		STATUS_BLOCK_LEN(1) |
+		STATUS_BLOCK_SBP_STATUS(SBP_STATUS_UNSPECIFIED_ERROR));
+	sbp_send_status(req);
+	sbp_free_request(req);
 }
 
 /*
-- 
1.7.10.4

[PATCH 4/7] target: Allow for target_submit_cmd() returning errors

[Roland Dreier]

From: Roland Dreier <roland@purestorage.com>

We want it to be possible for target_submit_cmd() to return errors up
to its fabric module callers.  For now just update the prototype to
return an int, and update all callers to handle non-zero return values
as an error.

Cc: Chad Dupuis <chad.dupuis@qlogic.com>
Cc: Arun Easi <arun.easi@qlogic.com>
Cc: Chris Boot <bootc@bootc.net>
Cc: Stefan Richter <stefanr@s5r6.in-berlin.de>
Cc: Mark Rustad <mark.d.rustad@intel.com>
Cc: Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Felipe Balbi <balbi@ti.com>
Signed-off-by: Roland Dreier <roland@purestorage.com>
---
 drivers/scsi/qla2xxx/tcm_qla2xxx.c     |    7 +++----
 drivers/target/sbp/sbp_target.c        |    8 +++++---
 drivers/target/target_core_transport.c |    9 +++++----
 drivers/target/tcm_fc/tfc_cmd.c        |    8 +++++---
 drivers/usb/gadget/tcm_usb_gadget.c    |   20 ++++++++------------
 include/target/target_core_fabric.h    |    2 +-
 6 files changed, 27 insertions(+), 27 deletions(-)

diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
index 3cb2b97..7db7ca7 100644
--- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
+++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
@@ -599,10 +599,9 @@ static int tcm_qla2xxx_handle_cmd(scsi_qla_host_t *vha, struct qla_tgt_cmd *cmd,
 		return -EINVAL;
 	}
 
-	target_submit_cmd(se_cmd, se_sess, cdb, &cmd->sense_buffer[0],
-				cmd->unpacked_lun, data_length, fcp_task_attr,
-				data_dir, flags);
-	return 0;
+	return target_submit_cmd(se_cmd, se_sess, cdb, &cmd->sense_buffer[0],
+				 cmd->unpacked_lun, data_length, fcp_task_attr,
+				 data_dir, flags);
 }
 
 static void tcm_qla2xxx_do_rsp(struct work_struct *work)
diff --git a/drivers/target/sbp/sbp_target.c b/drivers/target/sbp/sbp_target.c
index 2425ca4..fb75d05 100644
--- a/drivers/target/sbp/sbp_target.c
+++ b/drivers/target/sbp/sbp_target.c
@@ -1235,9 +1235,11 @@ static void sbp_handle_command(struct sbp_target_request *req)
 	pr_debug("sbp_handle_command ORB:0x%llx unpacked_lun:%d data_len:%d data_dir:%d\n",
 			req->orb_pointer, unpacked_lun, data_length, data_dir);
 
-	target_submit_cmd(&req->se_cmd, sess->se_sess, req->cmd_buf,
-			req->sense_buf, unpacked_lun, data_length,
-			MSG_SIMPLE_TAG, data_dir, 0);
+	if (target_submit_cmd(&req->se_cmd, sess->se_sess, req->cmd_buf,
+			      req->sense_buf, unpacked_lun, data_length,
+			      MSG_SIMPLE_TAG, data_dir, 0))
+		goto err;
+
 	return;
 
 err:
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 82e40e1..34ca821 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1544,7 +1544,7 @@ EXPORT_SYMBOL(transport_handle_cdb_direct);
  * This may only be called from process context, and also currently
  * assumes internal allocation of fabric payload buffer by target-core.
  **/
-void target_submit_cmd(struct se_cmd *se_cmd, struct se_session *se_sess,
+int target_submit_cmd(struct se_cmd *se_cmd, struct se_session *se_sess,
 		unsigned char *cdb, unsigned char *sense, u32 unpacked_lun,
 		u32 data_length, int task_attr, int data_dir, int flags)
 {
@@ -1583,7 +1583,7 @@ void target_submit_cmd(struct se_cmd *se_cmd, struct se_session *se_sess,
 		transport_send_check_condition_and_sense(se_cmd,
 				se_cmd->scsi_sense_reason, 0);
 		target_put_sess_cmd(se_sess, se_cmd);
-		return;
+		return 0;
 	}
 	/*
 	 * Sanitize CDBs via transport_generic_cmd_sequencer() and
@@ -1592,7 +1592,7 @@ void target_submit_cmd(struct se_cmd *se_cmd, struct se_session *se_sess,
 	rc = target_setup_cmd_from_cdb(se_cmd, cdb);
 	if (rc != 0) {
 		transport_generic_request_failure(se_cmd);
-		return;
+		return 0;
 	}
 
 	/*
@@ -1608,7 +1608,8 @@ void target_submit_cmd(struct se_cmd *se_cmd, struct se_session *se_sess,
 	 * when fabric has filled the incoming buffer.
 	 */
 	transport_handle_cdb_direct(se_cmd);
-	return;
+
+	return 0;
 }
 EXPORT_SYMBOL(target_submit_cmd);
 
diff --git a/drivers/target/tcm_fc/tfc_cmd.c b/drivers/target/tcm_fc/tfc_cmd.c
index f03fb97..c7c90aa 100644
--- a/drivers/target/tcm_fc/tfc_cmd.c
+++ b/drivers/target/tcm_fc/tfc_cmd.c
@@ -541,9 +541,11 @@ static void ft_send_work(struct work_struct *work)
 	 * Use a single se_cmd->cmd_kref as we expect to release se_cmd
 	 * directly from ft_check_stop_free callback in response path.
 	 */
-	target_submit_cmd(&cmd->se_cmd, cmd->sess->se_sess, fcp->fc_cdb,
-			&cmd->ft_sense_buffer[0], scsilun_to_int(&fcp->fc_lun),
-			ntohl(fcp->fc_dl), task_attr, data_dir, 0);
+	if (target_submit_cmd(&cmd->se_cmd, cmd->sess->se_sess, fcp->fc_cdb,
+			      &cmd->ft_sense_buffer[0], scsilun_to_int(&fcp->fc_lun),
+			      ntohl(fcp->fc_dl), task_attr, data_dir, 0))
+		goto err;
+
 	pr_debug("r_ctl %x alloc target_submit_cmd\n", fh->fh_r_ctl);
 	return;
 
diff --git a/drivers/usb/gadget/tcm_usb_gadget.c b/drivers/usb/gadget/tcm_usb_gadget.c
index c46439c..53aafd2 100644
--- a/drivers/usb/gadget/tcm_usb_gadget.c
+++ b/drivers/usb/gadget/tcm_usb_gadget.c
@@ -1060,7 +1060,10 @@ static void usbg_cmd_work(struct work_struct *work)
 	tpg = cmd->fu->tpg;
 	tv_nexus = tpg->tpg_nexus;
 	dir = get_cmd_dir(cmd->cmd_buf);
-	if (dir < 0) {
+	if (dir < 0 ||
+	    target_submit_cmd(se_cmd, tv_nexus->tvn_se_sess,
+			      cmd->cmd_buf, cmd->sense_iu.sense, cmd->unpacked_lun,
+			      0, cmd->prio_attr, dir, TARGET_SCF_UNKNOWN_SIZE)) {
 		transport_init_se_cmd(se_cmd,
 				tv_nexus->tvn_se_sess->se_tpg->se_tpg_tfo,
 				tv_nexus->tvn_se_sess, cmd->data_len, DMA_NONE,
@@ -1069,12 +1072,7 @@ static void usbg_cmd_work(struct work_struct *work)
 		transport_send_check_condition_and_sense(se_cmd,
 				TCM_UNSUPPORTED_SCSI_OPCODE, 1);
 		usbg_cleanup_cmd(cmd);
-		return;
 	}
-
-	target_submit_cmd(se_cmd, tv_nexus->tvn_se_sess,
-			cmd->cmd_buf, cmd->sense_iu.sense, cmd->unpacked_lun,
-			0, cmd->prio_attr, dir, TARGET_SCF_UNKNOWN_SIZE);
 }
 
 static int usbg_submit_command(struct f_uas *fu,
@@ -1172,7 +1170,10 @@ static void bot_cmd_work(struct work_struct *work)
 	tpg = cmd->fu->tpg;
 	tv_nexus = tpg->tpg_nexus;
 	dir = get_cmd_dir(cmd->cmd_buf);
-	if (dir < 0) {
+	if (dir < 0 ||
+	    target_submit_cmd(se_cmd, tv_nexus->tvn_se_sess,
+			      cmd->cmd_buf, cmd->sense_iu.sense, cmd->unpacked_lun,
+			      cmd->data_len, cmd->prio_attr, dir, 0)) {
 		transport_init_se_cmd(se_cmd,
 				tv_nexus->tvn_se_sess->se_tpg->se_tpg_tfo,
 				tv_nexus->tvn_se_sess, cmd->data_len, DMA_NONE,
@@ -1181,12 +1182,7 @@ static void bot_cmd_work(struct work_struct *work)
 		transport_send_check_condition_and_sense(se_cmd,
 				TCM_UNSUPPORTED_SCSI_OPCODE, 1);
 		usbg_cleanup_cmd(cmd);
-		return;
 	}
-
-	target_submit_cmd(se_cmd, tv_nexus->tvn_se_sess,
-			cmd->cmd_buf, cmd->sense_iu.sense, cmd->unpacked_lun,
-			cmd->data_len, cmd->prio_attr, dir, 0);
 }
 
 static int bot_submit_command(struct f_uas *fu,
diff --git a/include/target/target_core_fabric.h b/include/target/target_core_fabric.h
index 1e68882..4f8e515 100644
--- a/include/target/target_core_fabric.h
+++ b/include/target/target_core_fabric.h
@@ -108,7 +108,7 @@ void	transport_init_se_cmd(struct se_cmd *, struct target_core_fabric_ops *,
 		struct se_session *, u32, int, int, unsigned char *);
 int	transport_lookup_cmd_lun(struct se_cmd *, u32);
 int	target_setup_cmd_from_cdb(struct se_cmd *, unsigned char *);
-void	target_submit_cmd(struct se_cmd *, struct se_session *, unsigned char *,
+int	target_submit_cmd(struct se_cmd *, struct se_session *, unsigned char *,
 		unsigned char *, u32, u32, int, int, int);
 int	target_submit_tmr(struct se_cmd *se_cmd, struct se_session *se_sess,
 		unsigned char *sense, u32 unpacked_lun,
-- 
1.7.10.4

[PATCH 5/7] target: Check sess_tearing_down in target_get_sess_cmd()

[Roland Dreier]

From: Roland Dreier <roland@purestorage.com>

Target core code assumes that target_splice_sess_cmd_list() has set
sess_tearing_down and moved the list of pending commands to
sess_wait_list, no more commands will be added to the session; if any
are added, nothing keeps the se_session from being freed while the
command is still in flight, which e.g. leads to use-after-free of
se_cmd->se_sess in target_release_cmd_kref().

To enforce this invariant, put a check of sess_tearing_down inside of
sess_cmd_lock in target_get_sess_cmd(); any checks before this are
racy and can lead to the use-after-free described above.  For example,
the qla_target check in qlt_do_work() checks sess_tearing_down from
work thread context but then drops all locks before calling
target_submit_cmd() (as it must, since that is a sleeping function).

However, since no locks are held, anything can happen with respect to
the session it has looked up -- although it does correctly get
sess_kref within its lock, so the memory won't be freed while
target_submit_cmd() is actually running, nothing stops eg an ACL from
being dropped and calling ->shutdown_session() (which calls into
target_splice_sess_cmd_list()) before we get to target_get_sess_cmd().
Once this happens, the se_session memory can be freed as soon as
target_submit_cmd() returns and qlt_do_work() drops its reference,
even though we've just added a command to sess_cmd_list.

To prevent this use-after-free, check sess_tearing_down inside of
sess_cmd_lock right before target_get_sess_cmd() adds a command to
sess_cmd_list; this is synchronized with target_splice_sess_cmd_list()
so that every command is either waited for or not added to the queue.

Signed-off-by: Roland Dreier <roland@purestorage.com>
---
 drivers/target/target_core_transport.c |   24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 34ca821..23e6e2d 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -73,7 +73,7 @@ static void transport_complete_task_attr(struct se_cmd *cmd);
 static void transport_handle_queue_full(struct se_cmd *cmd,
 		struct se_device *dev);
 static int transport_generic_get_mem(struct se_cmd *cmd);
-static void target_get_sess_cmd(struct se_session *, struct se_cmd *, bool);
+static int target_get_sess_cmd(struct se_session *, struct se_cmd *, bool);
 static void transport_put_cmd(struct se_cmd *cmd);
 static void transport_remove_cmd_from_queue(struct se_cmd *cmd);
 static int transport_set_sense_codes(struct se_cmd *cmd, u8 asc, u8 ascq);
@@ -1570,7 +1570,9 @@ int target_submit_cmd(struct se_cmd *se_cmd, struct se_session *se_sess,
 	 * for fabrics using TARGET_SCF_ACK_KREF that expect a second
 	 * kref_put() to happen during fabric packet acknowledgement.
 	 */
-	target_get_sess_cmd(se_sess, se_cmd, (flags & TARGET_SCF_ACK_KREF));
+	rc = target_get_sess_cmd(se_sess, se_cmd, (flags & TARGET_SCF_ACK_KREF));
+	if (rc)
+		return rc;
 	/*
 	 * Signal bidirectional data payloads to target-core
 	 */
@@ -1664,7 +1666,11 @@ int target_submit_tmr(struct se_cmd *se_cmd, struct se_session *se_sess,
 		se_cmd->se_tmr_req->ref_task_tag = tag;
 
 	/* See target_submit_cmd for commentary */
-	target_get_sess_cmd(se_sess, se_cmd, (flags & TARGET_SCF_ACK_KREF));
+	ret = target_get_sess_cmd(se_sess, se_cmd, (flags & TARGET_SCF_ACK_KREF));
+	if (ret) {
+		core_tmr_release_req(se_cmd->se_tmr_req);
+		return ret;
+	}
 
 	ret = transport_lookup_tmr_lun(se_cmd, unpacked_lun);
 	if (ret) {
@@ -3650,10 +3656,11 @@ EXPORT_SYMBOL(transport_generic_free_cmd);
  * @se_cmd:	command descriptor to add
  * @ack_kref:	Signal that fabric will perform an ack target_put_sess_cmd()
  */
-static void target_get_sess_cmd(struct se_session *se_sess, struct se_cmd *se_cmd,
-				bool ack_kref)
+static int target_get_sess_cmd(struct se_session *se_sess, struct se_cmd *se_cmd,
+			       bool ack_kref)
 {
 	unsigned long flags;
+	int ret = 0;
 
 	kref_init(&se_cmd->cmd_kref);
 	/*
@@ -3667,9 +3674,16 @@ static void target_get_sess_cmd(struct se_session *se_sess, struct se_cmd *se_cm
 	}
 
 	spin_lock_irqsave(&se_sess->sess_cmd_lock, flags);
+	if (se_sess->sess_tearing_down) {
+		ret = -ESHUTDOWN;
+		goto out;
+	}
 	list_add_tail(&se_cmd->se_cmd_list, &se_sess->sess_cmd_list);
 	se_cmd->check_release = 1;
+
+out:
 	spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
+	return ret;
 }
 
 static void target_release_cmd_kref(struct kref *kref)
-- 
1.7.10.4

[PATCH 6/7] qla2xxx: Remove racy, now-redundant check of sess_tearing_down

[Roland Dreier]

From: Roland Dreier <roland@purestorage.com>

Now that target_submit_cmd() / target_get_sess_cmd() check
sess_tearing_down before adding commands to the list, we no longer
need the check in qlt_do_work().  In fact this check is racy anyway
(and that race is what inspired the change to add the check of
sess_tearing_down to the target core).

Cc: Chad Dupuis <chad.dupuis@qlogic.com>
Cc: Arun Easi <arun.easi@qlogic.com>
Signed-off-by: Roland Dreier <roland@purestorage.com>
---
 drivers/scsi/qla2xxx/qla_target.c |   16 +++-------------
 1 file changed, 3 insertions(+), 13 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_target.c b/drivers/scsi/qla2xxx/qla_target.c
index 87b5a330..5b30132 100644
--- a/drivers/scsi/qla2xxx/qla_target.c
+++ b/drivers/scsi/qla2xxx/qla_target.c
@@ -2643,19 +2643,9 @@ static void qlt_do_work(struct work_struct *work)
 	spin_lock_irqsave(&ha->hardware_lock, flags);
 	sess = ha->tgt.tgt_ops->find_sess_by_s_id(vha,
 	    atio->u.isp24.fcp_hdr.s_id);
-	if (sess) {
-		if (unlikely(sess->se_sess->sess_tearing_down)) {
-			sess = NULL;
-			spin_unlock_irqrestore(&ha->hardware_lock, flags);
-			goto out_term;
-		} else {
-			/*
-			 * Do the extra kref_get() before dropping
-			 * qla_hw_data->hardware_lock.
-			 */
-			kref_get(&sess->se_sess->sess_kref);
-		}
-	}
+	/* Do kref_get() before dropping qla_hw_data->hardware_lock. */
+	if (sess)
+		kref_get(&sess->se_sess->sess_kref);
 	spin_unlock_irqrestore(&ha->hardware_lock, flags);
 
 	if (unlikely(!sess)) {
-- 
1.7.10.4

[PATCH 7/7] target: Remove se_session.sess_wait_list

[Roland Dreier]

From: Roland Dreier <roland@purestorage.com>

Since we set se_session.sess_tearing_down and stop new commands from
being added to se_session.sess_cmd_list before we wait for commands to
finish when freeing a session, there's no need for a separate
sess_wait_list -- if we let new commands be added to sess_cmd_list
after setting sess_tearing_down, that would be a bug that breaks the
logic of waiting in-flight commands.

Also rename target_splice_sess_cmd_list() to
target_sess_cmd_list_set_waiting(), since we are no longer splicing
onto a separate list.

Signed-off-by: Roland Dreier <roland@purestorage.com>
---
 drivers/scsi/qla2xxx/tcm_qla2xxx.c     |    2 +-
 drivers/target/target_core_transport.c |   22 ++++++++++------------
 include/target/target_core_base.h      |    1 -
 include/target/target_core_fabric.h    |    2 +-
 4 files changed, 12 insertions(+), 15 deletions(-)

diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
index 7db7ca7..72f3557 100644
--- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
+++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
@@ -466,7 +466,7 @@ static int tcm_qla2xxx_shutdown_session(struct se_session *se_sess)
 	vha = sess->vha;
 
 	spin_lock_irqsave(&vha->hw->hardware_lock, flags);
-	target_splice_sess_cmd_list(se_sess);
+	target_sess_cmd_list_set_waiting(se_sess);
 	spin_unlock_irqrestore(&vha->hw->hardware_lock, flags);
 
 	return 1;
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 23e6e2d..7b3359e 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -244,7 +244,6 @@ struct se_session *transport_init_session(void)
 	INIT_LIST_HEAD(&se_sess->sess_list);
 	INIT_LIST_HEAD(&se_sess->sess_acl_list);
 	INIT_LIST_HEAD(&se_sess->sess_cmd_list);
-	INIT_LIST_HEAD(&se_sess->sess_wait_list);
 	spin_lock_init(&se_sess->sess_cmd_lock);
 	kref_init(&se_sess->sess_kref);
 
@@ -3719,28 +3718,27 @@ int target_put_sess_cmd(struct se_session *se_sess, struct se_cmd *se_cmd)
 }
 EXPORT_SYMBOL(target_put_sess_cmd);
 
-/* target_splice_sess_cmd_list - Split active cmds into sess_wait_list
- * @se_sess:	session to split
+/* target_sess_cmd_list_set_waiting - Flag all commands in
+ *         sess_cmd_list to complete cmd_wait_comp.  Set
+ *         sess_tearing_down so no more commands are queued.
+ * @se_sess:	session to flag
  */
-void target_splice_sess_cmd_list(struct se_session *se_sess)
+void target_sess_cmd_list_set_waiting(struct se_session *se_sess)
 {
 	struct se_cmd *se_cmd;
 	unsigned long flags;
 
-	WARN_ON(!list_empty(&se_sess->sess_wait_list));
-	INIT_LIST_HEAD(&se_sess->sess_wait_list);
-
 	spin_lock_irqsave(&se_sess->sess_cmd_lock, flags);
-	se_sess->sess_tearing_down = 1;
 
-	list_splice_init(&se_sess->sess_cmd_list, &se_sess->sess_wait_list);
+	WARN_ON(se_sess->sess_tearing_down);
+	se_sess->sess_tearing_down = 1;
 
-	list_for_each_entry(se_cmd, &se_sess->sess_wait_list, se_cmd_list)
+	list_for_each_entry(se_cmd, &se_sess->sess_cmd_list, se_cmd_list)
 		se_cmd->cmd_wait_set = 1;
 
 	spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
 }
-EXPORT_SYMBOL(target_splice_sess_cmd_list);
+EXPORT_SYMBOL(target_sess_cmd_list_set_waiting);
 
 /* target_wait_for_sess_cmds - Wait for outstanding descriptors
  * @se_sess:    session to wait for active I/O
@@ -3754,7 +3752,7 @@ void target_wait_for_sess_cmds(
 	bool rc = false;
 
 	list_for_each_entry_safe(se_cmd, tmp_cmd,
-				&se_sess->sess_wait_list, se_cmd_list) {
+				&se_sess->sess_cmd_list, se_cmd_list) {
 		list_del(&se_cmd->se_cmd_list);
 
 		pr_debug("Waiting for se_cmd: %p t_state: %d, fabric state:"
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index dc35d86..5d8907e 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -633,7 +633,6 @@ struct se_session {
 	struct list_head	sess_list;
 	struct list_head	sess_acl_list;
 	struct list_head	sess_cmd_list;
-	struct list_head	sess_wait_list;
 	spinlock_t		sess_cmd_lock;
 	struct kref		sess_kref;
 };
diff --git a/include/target/target_core_fabric.h b/include/target/target_core_fabric.h
index 4f8e515..86d8a4a 100644
--- a/include/target/target_core_fabric.h
+++ b/include/target/target_core_fabric.h
@@ -130,7 +130,7 @@ int	transport_check_aborted_status(struct se_cmd *, int);
 int	transport_send_check_condition_and_sense(struct se_cmd *, u8, int);
 
 int	target_put_sess_cmd(struct se_session *, struct se_cmd *);
-void	target_splice_sess_cmd_list(struct se_session *);
+void	target_sess_cmd_list_set_waiting(struct se_session *);
 void	target_wait_for_sess_cmds(struct se_session *, int);
 
 int	core_alua_check_nonop_delay(struct se_cmd *);
-- 
1.7.10.4

Re: [PATCH 1/7] qla2xxx: Get rid of redundant qla_tgt_sess.tearing_down

[Nicholas A. Bellinger]

On Mon, 2012-07-16 at 11:04 -0700, Roland Dreier wrote:
> From: Roland Dreier <roland@purestorage.com>
> 
> The only place that sets qla_tgt_sess.tearing_down calls
> target_splice_sess_cmd_list() immediately afterwards, without dropping
> the lock it holds.  That function sets se_session.sess_tearing_down,
> so we can get rid of the qla_target-specific flag, and in the one
> place that looks at the qla_tgt_sess.tearing_down flag just test
> se_session.sess_tearing_down instead.
> 

Looks fine.  Applied to for-next.

Thanks Roland!

> Cc: Chad Dupuis <chad.dupuis@qlogic.com>
> Cc: Arun Easi <arun.easi@qlogic.com>
> Signed-off-by: Roland Dreier <roland@purestorage.com>
> ---
>  drivers/scsi/qla2xxx/qla_target.c  |    2 +-
>  drivers/scsi/qla2xxx/qla_target.h  |    1 -
>  drivers/scsi/qla2xxx/tcm_qla2xxx.c |    1 -
>  3 files changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/qla2xxx/qla_target.c b/drivers/scsi/qla2xxx/qla_target.c
> index 77759c7..87b5a330 100644
> --- a/drivers/scsi/qla2xxx/qla_target.c
> +++ b/drivers/scsi/qla2xxx/qla_target.c
> @@ -2644,7 +2644,7 @@ static void qlt_do_work(struct work_struct *work)
>  	sess = ha->tgt.tgt_ops->find_sess_by_s_id(vha,
>  	    atio->u.isp24.fcp_hdr.s_id);
>  	if (sess) {
> -		if (unlikely(sess->tearing_down)) {
> +		if (unlikely(sess->se_sess->sess_tearing_down)) {
>  			sess = NULL;
>  			spin_unlock_irqrestore(&ha->hardware_lock, flags);
>  			goto out_term;
> diff --git a/drivers/scsi/qla2xxx/qla_target.h b/drivers/scsi/qla2xxx/qla_target.h
> index 9f9ef16..07aa265 100644
> --- a/drivers/scsi/qla2xxx/qla_target.h
> +++ b/drivers/scsi/qla2xxx/qla_target.h
> @@ -813,7 +813,6 @@ struct qla_tgt_sess {
>  	unsigned int conf_compl_supported:1;
>  	unsigned int deleted:1;
>  	unsigned int local:1;
> -	unsigned int tearing_down:1;
>  
>  	struct se_session *se_sess;
>  	struct scsi_qla_host *vha;
> diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
> index 6e64314..3cb2b97 100644
> --- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
> +++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
> @@ -466,7 +466,6 @@ static int tcm_qla2xxx_shutdown_session(struct se_session *se_sess)
>  	vha = sess->vha;
>  
>  	spin_lock_irqsave(&vha->hw->hardware_lock, flags);
> -	sess->tearing_down = 1;
>  	target_splice_sess_cmd_list(se_sess);
>  	spin_unlock_irqrestore(&vha->hw->hardware_lock, flags);
>  

Re: [PATCH 2/7] target: Un-export target_get_sess_cmd()

[Nicholas A. Bellinger]

On Mon, 2012-07-16 at 11:04 -0700, Roland Dreier wrote:
> From: Roland Dreier <roland@purestorage.com>
> 
> There are no in-tree users of target_get_sess_cmd() outside of
> target_core_transport.c.  Any new code should use the higher-level
> target_submit_cmd() interface.  So let's un-export target_get_sess_cmd()
> and make it static to the one file where it's actually used.
> 
> Signed-off-by: Roland Dreier <roland@purestorage.com>
> ---
>  drivers/target/target_core_transport.c |    6 +++---
>  include/target/target_core_fabric.h    |    1 -
>  2 files changed, 3 insertions(+), 4 deletions(-)
> 

Applied with some minor fuzz against for-next.

Thanks!

Re: [PATCH 3/7] sbp-target: Consolidate duplicated error path code in sbp_handle_command()

[Nicholas A. Bellinger]

On Mon, 2012-07-16 at 11:04 -0700, Roland Dreier wrote:
> From: Roland Dreier <roland@purestorage.com>
> 
> Cc: Chris Boot <bootc@bootc.net>
> Cc: Stefan Richter <stefanr@s5r6.in-berlin.de>
> Signed-off-by: Roland Dreier <roland@purestorage.com>
> ---
>  drivers/target/sbp/sbp_target.c |   28 ++++++++++++----------------
>  1 file changed, 12 insertions(+), 16 deletions(-)
> 

Looks like a reasonable 'de-dupe'.  Applied to for-next.

> diff --git a/drivers/target/sbp/sbp_target.c b/drivers/target/sbp/sbp_target.c
> index 7e6136e..2425ca4 100644
> --- a/drivers/target/sbp/sbp_target.c
> +++ b/drivers/target/sbp/sbp_target.c
> @@ -1219,28 +1219,14 @@ static void sbp_handle_command(struct sbp_target_request *req)
>  	ret = sbp_fetch_command(req);
>  	if (ret) {
>  		pr_debug("sbp_handle_command: fetch command failed: %d\n", ret);
> -		req->status.status |= cpu_to_be32(
> -			STATUS_BLOCK_RESP(STATUS_RESP_TRANSPORT_FAILURE) |
> -			STATUS_BLOCK_DEAD(0) |
> -			STATUS_BLOCK_LEN(1) |
> -			STATUS_BLOCK_SBP_STATUS(SBP_STATUS_UNSPECIFIED_ERROR));
> -		sbp_send_status(req);
> -		sbp_free_request(req);
> -		return;
> +		goto err;
>  	}
>  
>  	ret = sbp_fetch_page_table(req);
>  	if (ret) {
>  		pr_debug("sbp_handle_command: fetch page table failed: %d\n",
>  			ret);
> -		req->status.status |= cpu_to_be32(
> -			STATUS_BLOCK_RESP(STATUS_RESP_TRANSPORT_FAILURE) |
> -			STATUS_BLOCK_DEAD(0) |
> -			STATUS_BLOCK_LEN(1) |
> -			STATUS_BLOCK_SBP_STATUS(SBP_STATUS_UNSPECIFIED_ERROR));
> -		sbp_send_status(req);
> -		sbp_free_request(req);
> -		return;
> +		goto err;
>  	}
>  
>  	unpacked_lun = req->login->lun->unpacked_lun;
> @@ -1252,6 +1238,16 @@ static void sbp_handle_command(struct sbp_target_request *req)
>  	target_submit_cmd(&req->se_cmd, sess->se_sess, req->cmd_buf,
>  			req->sense_buf, unpacked_lun, data_length,
>  			MSG_SIMPLE_TAG, data_dir, 0);
> +	return;
> +
> +err:
> +	req->status.status |= cpu_to_be32(
> +		STATUS_BLOCK_RESP(STATUS_RESP_TRANSPORT_FAILURE) |
> +		STATUS_BLOCK_DEAD(0) |
> +		STATUS_BLOCK_LEN(1) |
> +		STATUS_BLOCK_SBP_STATUS(SBP_STATUS_UNSPECIFIED_ERROR));
> +	sbp_send_status(req);
> +	sbp_free_request(req);
>  }
>  
>  /*

Re: [PATCH 4/7] target: Allow for target_submit_cmd() returning errors

[Nicholas A. Bellinger]

On Mon, 2012-07-16 at 11:04 -0700, Roland Dreier wrote:
> From: Roland Dreier <roland@purestorage.com>
> 
> We want it to be possible for target_submit_cmd() to return errors up
> to its fabric module callers.  For now just update the prototype to
> return an int, and update all callers to handle non-zero return values
> as an error.
> 

Mmmm.  The original target_submit_cmd() code had been propagating up a
return value, but then we decided (via Agrover's patch) that it made
more sense for target_submit_cmd() to always handle exceptions via
normal TFO callbacks, and not have the fabric worry about the return
here..

Also, I'm not sure if the error paths that this patch now accesses after
target_submit_cmd() failure are going to deal with different types of
target_submit_cmd() failures correctly.

So NACK for the moment, as I don't really see why this is necessary in
the first place..?

> Cc: Chad Dupuis <chad.dupuis@qlogic.com>
> Cc: Arun Easi <arun.easi@qlogic.com>
> Cc: Chris Boot <bootc@bootc.net>
> Cc: Stefan Richter <stefanr@s5r6.in-berlin.de>
> Cc: Mark Rustad <mark.d.rustad@intel.com>
> Cc: Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Cc: Felipe Balbi <balbi@ti.com>
> Signed-off-by: Roland Dreier <roland@purestorage.com>
> ---
>  drivers/scsi/qla2xxx/tcm_qla2xxx.c     |    7 +++----
>  drivers/target/sbp/sbp_target.c        |    8 +++++---
>  drivers/target/target_core_transport.c |    9 +++++----
>  drivers/target/tcm_fc/tfc_cmd.c        |    8 +++++---
>  drivers/usb/gadget/tcm_usb_gadget.c    |   20 ++++++++------------
>  include/target/target_core_fabric.h    |    2 +-
>  6 files changed, 27 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
> index 3cb2b97..7db7ca7 100644
> --- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
> +++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
> @@ -599,10 +599,9 @@ static int tcm_qla2xxx_handle_cmd(scsi_qla_host_t *vha, struct qla_tgt_cmd *cmd,
>  		return -EINVAL;
>  	}
>  
> -	target_submit_cmd(se_cmd, se_sess, cdb, &cmd->sense_buffer[0],
> -				cmd->unpacked_lun, data_length, fcp_task_attr,
> -				data_dir, flags);
> -	return 0;
> +	return target_submit_cmd(se_cmd, se_sess, cdb, &cmd->sense_buffer[0],
> +				 cmd->unpacked_lun, data_length, fcp_task_attr,
> +				 data_dir, flags);
>  }
>  
>  static void tcm_qla2xxx_do_rsp(struct work_struct *work)
> diff --git a/drivers/target/sbp/sbp_target.c b/drivers/target/sbp/sbp_target.c
> index 2425ca4..fb75d05 100644
> --- a/drivers/target/sbp/sbp_target.c
> +++ b/drivers/target/sbp/sbp_target.c
> @@ -1235,9 +1235,11 @@ static void sbp_handle_command(struct sbp_target_request *req)
>  	pr_debug("sbp_handle_command ORB:0x%llx unpacked_lun:%d data_len:%d data_dir:%d\n",
>  			req->orb_pointer, unpacked_lun, data_length, data_dir);
>  
> -	target_submit_cmd(&req->se_cmd, sess->se_sess, req->cmd_buf,
> -			req->sense_buf, unpacked_lun, data_length,
> -			MSG_SIMPLE_TAG, data_dir, 0);
> +	if (target_submit_cmd(&req->se_cmd, sess->se_sess, req->cmd_buf,
> +			      req->sense_buf, unpacked_lun, data_length,
> +			      MSG_SIMPLE_TAG, data_dir, 0))
> +		goto err;
> +
>  	return;
>  
>  err:
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index 82e40e1..34ca821 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -1544,7 +1544,7 @@ EXPORT_SYMBOL(transport_handle_cdb_direct);
>   * This may only be called from process context, and also currently
>   * assumes internal allocation of fabric payload buffer by target-core.
>   **/
> -void target_submit_cmd(struct se_cmd *se_cmd, struct se_session *se_sess,
> +int target_submit_cmd(struct se_cmd *se_cmd, struct se_session *se_sess,
>  		unsigned char *cdb, unsigned char *sense, u32 unpacked_lun,
>  		u32 data_length, int task_attr, int data_dir, int flags)
>  {
> @@ -1583,7 +1583,7 @@ void target_submit_cmd(struct se_cmd *se_cmd, struct se_session *se_sess,
>  		transport_send_check_condition_and_sense(se_cmd,
>  				se_cmd->scsi_sense_reason, 0);
>  		target_put_sess_cmd(se_sess, se_cmd);
> -		return;
> +		return 0;
>  	}
>  	/*
>  	 * Sanitize CDBs via transport_generic_cmd_sequencer() and
> @@ -1592,7 +1592,7 @@ void target_submit_cmd(struct se_cmd *se_cmd, struct se_session *se_sess,
>  	rc = target_setup_cmd_from_cdb(se_cmd, cdb);
>  	if (rc != 0) {
>  		transport_generic_request_failure(se_cmd);
> -		return;
> +		return 0;
>  	}
>  
>  	/*
> @@ -1608,7 +1608,8 @@ void target_submit_cmd(struct se_cmd *se_cmd, struct se_session *se_sess,
>  	 * when fabric has filled the incoming buffer.
>  	 */
>  	transport_handle_cdb_direct(se_cmd);
> -	return;
> +
> +	return 0;
>  }
>  EXPORT_SYMBOL(target_submit_cmd);
>  
> diff --git a/drivers/target/tcm_fc/tfc_cmd.c b/drivers/target/tcm_fc/tfc_cmd.c
> index f03fb97..c7c90aa 100644
> --- a/drivers/target/tcm_fc/tfc_cmd.c
> +++ b/drivers/target/tcm_fc/tfc_cmd.c
> @@ -541,9 +541,11 @@ static void ft_send_work(struct work_struct *work)
>  	 * Use a single se_cmd->cmd_kref as we expect to release se_cmd
>  	 * directly from ft_check_stop_free callback in response path.
>  	 */
> -	target_submit_cmd(&cmd->se_cmd, cmd->sess->se_sess, fcp->fc_cdb,
> -			&cmd->ft_sense_buffer[0], scsilun_to_int(&fcp->fc_lun),
> -			ntohl(fcp->fc_dl), task_attr, data_dir, 0);
> +	if (target_submit_cmd(&cmd->se_cmd, cmd->sess->se_sess, fcp->fc_cdb,
> +			      &cmd->ft_sense_buffer[0], scsilun_to_int(&fcp->fc_lun),
> +			      ntohl(fcp->fc_dl), task_attr, data_dir, 0))
> +		goto err;
> +
>  	pr_debug("r_ctl %x alloc target_submit_cmd\n", fh->fh_r_ctl);
>  	return;
>  
> diff --git a/drivers/usb/gadget/tcm_usb_gadget.c b/drivers/usb/gadget/tcm_usb_gadget.c
> index c46439c..53aafd2 100644
> --- a/drivers/usb/gadget/tcm_usb_gadget.c
> +++ b/drivers/usb/gadget/tcm_usb_gadget.c
> @@ -1060,7 +1060,10 @@ static void usbg_cmd_work(struct work_struct *work)
>  	tpg = cmd->fu->tpg;
>  	tv_nexus = tpg->tpg_nexus;
>  	dir = get_cmd_dir(cmd->cmd_buf);
> -	if (dir < 0) {
> +	if (dir < 0 ||
> +	    target_submit_cmd(se_cmd, tv_nexus->tvn_se_sess,
> +			      cmd->cmd_buf, cmd->sense_iu.sense, cmd->unpacked_lun,
> +			      0, cmd->prio_attr, dir, TARGET_SCF_UNKNOWN_SIZE)) {
>  		transport_init_se_cmd(se_cmd,
>  				tv_nexus->tvn_se_sess->se_tpg->se_tpg_tfo,
>  				tv_nexus->tvn_se_sess, cmd->data_len, DMA_NONE,
> @@ -1069,12 +1072,7 @@ static void usbg_cmd_work(struct work_struct *work)
>  		transport_send_check_condition_and_sense(se_cmd,
>  				TCM_UNSUPPORTED_SCSI_OPCODE, 1);
>  		usbg_cleanup_cmd(cmd);
> -		return;
>  	}
> -
> -	target_submit_cmd(se_cmd, tv_nexus->tvn_se_sess,
> -			cmd->cmd_buf, cmd->sense_iu.sense, cmd->unpacked_lun,
> -			0, cmd->prio_attr, dir, TARGET_SCF_UNKNOWN_SIZE);
>  }
>  
>  static int usbg_submit_command(struct f_uas *fu,
> @@ -1172,7 +1170,10 @@ static void bot_cmd_work(struct work_struct *work)
>  	tpg = cmd->fu->tpg;
>  	tv_nexus = tpg->tpg_nexus;
>  	dir = get_cmd_dir(cmd->cmd_buf);
> -	if (dir < 0) {
> +	if (dir < 0 ||
> +	    target_submit_cmd(se_cmd, tv_nexus->tvn_se_sess,
> +			      cmd->cmd_buf, cmd->sense_iu.sense, cmd->unpacked_lun,
> +			      cmd->data_len, cmd->prio_attr, dir, 0)) {
>  		transport_init_se_cmd(se_cmd,
>  				tv_nexus->tvn_se_sess->se_tpg->se_tpg_tfo,
>  				tv_nexus->tvn_se_sess, cmd->data_len, DMA_NONE,
> @@ -1181,12 +1182,7 @@ static void bot_cmd_work(struct work_struct *work)
>  		transport_send_check_condition_and_sense(se_cmd,
>  				TCM_UNSUPPORTED_SCSI_OPCODE, 1);
>  		usbg_cleanup_cmd(cmd);
> -		return;
>  	}
> -
> -	target_submit_cmd(se_cmd, tv_nexus->tvn_se_sess,
> -			cmd->cmd_buf, cmd->sense_iu.sense, cmd->unpacked_lun,
> -			cmd->data_len, cmd->prio_attr, dir, 0);
>  }
>  
>  static int bot_submit_command(struct f_uas *fu,
> diff --git a/include/target/target_core_fabric.h b/include/target/target_core_fabric.h
> index 1e68882..4f8e515 100644
> --- a/include/target/target_core_fabric.h
> +++ b/include/target/target_core_fabric.h
> @@ -108,7 +108,7 @@ void	transport_init_se_cmd(struct se_cmd *, struct target_core_fabric_ops *,
>  		struct se_session *, u32, int, int, unsigned char *);
>  int	transport_lookup_cmd_lun(struct se_cmd *, u32);
>  int	target_setup_cmd_from_cdb(struct se_cmd *, unsigned char *);
> -void	target_submit_cmd(struct se_cmd *, struct se_session *, unsigned char *,
> +int	target_submit_cmd(struct se_cmd *, struct se_session *, unsigned char *,
>  		unsigned char *, u32, u32, int, int, int);
>  int	target_submit_tmr(struct se_cmd *se_cmd, struct se_session *se_sess,
>  		unsigned char *sense, u32 unpacked_lun,

Re: [PATCH 4/7] target: Allow for target_submit_cmd() returning errors

[Roland Dreier]

On Mon, Jul 16, 2012 at 4:00 PM, Nicholas A. Bellinger
<nab@linux-iscsi.org> wrote:
> Mmmm.  The original target_submit_cmd() code had been propagating up a
> return value, but then we decided (via Agrover's patch) that it made
> more sense for target_submit_cmd() to always handle exceptions via
> normal TFO callbacks, and not have the fabric worry about the return
> here..
>
> Also, I'm not sure if the error paths that this patch now accesses after
> target_submit_cmd() failure are going to deal with different types of
> target_submit_cmd() failures correctly.
>
> So NACK for the moment, as I don't really see why this is necessary in
> the first place..?

Read on in the series to see why this is needed; in short, for qla2xxx
at least, we need a race-free way to check for sess_tearing_down
atomically with actually adding the command to sess_cmd_list.

I'm OK with returning failure via callback, but

 a) I'm not sure we can use the normal TFO callbacks in case
    we can't add the command to sess_cmd_list (seems like it
    opens the door to other use-after-frees in qla2xxx at least)
 b) Maybe it's OK if we say that failure to add the command to
    the sess_cmd_list is the only time submit cmd fails?

The qla2xxx race/use-after-free is definitely real, we hit it in testing
here with active IO across ACL changes.

 - R.

Re: [PATCH 5/7] target: Check sess_tearing_down in target_get_sess_cmd()

[Nicholas A. Bellinger]

On Mon, 2012-07-16 at 11:04 -0700, Roland Dreier wrote:
> From: Roland Dreier <roland@purestorage.com>
> 
> Target core code assumes that target_splice_sess_cmd_list() has set
> sess_tearing_down and moved the list of pending commands to
> sess_wait_list, no more commands will be added to the session; if any
> are added, nothing keeps the se_session from being freed while the
> command is still in flight, which e.g. leads to use-after-free of
> se_cmd->se_sess in target_release_cmd_kref().
> 
> To enforce this invariant, put a check of sess_tearing_down inside of
> sess_cmd_lock in target_get_sess_cmd(); any checks before this are
> racy and can lead to the use-after-free described above.  For example,
> the qla_target check in qlt_do_work() checks sess_tearing_down from
> work thread context but then drops all locks before calling
> target_submit_cmd() (as it must, since that is a sleeping function).
> 
> However, since no locks are held, anything can happen with respect to
> the session it has looked up -- although it does correctly get
> sess_kref within its lock, so the memory won't be freed while
> target_submit_cmd() is actually running, nothing stops eg an ACL from
> being dropped and calling ->shutdown_session() (which calls into
> target_splice_sess_cmd_list()) before we get to target_get_sess_cmd().
> Once this happens, the se_session memory can be freed as soon as
> target_submit_cmd() returns and qlt_do_work() drops its reference,
> even though we've just added a command to sess_cmd_list.
> 
> To prevent this use-after-free, check sess_tearing_down inside of
> sess_cmd_lock right before target_get_sess_cmd() adds a command to
> sess_cmd_list; this is synchronized with target_splice_sess_cmd_list()
> so that every command is either waited for or not added to the queue.
> 
> Signed-off-by: Roland Dreier <roland@purestorage.com>
> ---
>  drivers/target/target_core_transport.c |   24 +++++++++++++++++++-----
>  1 file changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index 34ca821..23e6e2d 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -73,7 +73,7 @@ static void transport_complete_task_attr(struct se_cmd *cmd);
>  static void transport_handle_queue_full(struct se_cmd *cmd,
>  		struct se_device *dev);
>  static int transport_generic_get_mem(struct se_cmd *cmd);
> -static void target_get_sess_cmd(struct se_session *, struct se_cmd *, bool);
> +static int target_get_sess_cmd(struct se_session *, struct se_cmd *, bool);
>  static void transport_put_cmd(struct se_cmd *cmd);
>  static void transport_remove_cmd_from_queue(struct se_cmd *cmd);
>  static int transport_set_sense_codes(struct se_cmd *cmd, u8 asc, u8 ascq);
> @@ -1570,7 +1570,9 @@ int target_submit_cmd(struct se_cmd *se_cmd, struct se_session *se_sess,
>  	 * for fabrics using TARGET_SCF_ACK_KREF that expect a second
>  	 * kref_put() to happen during fabric packet acknowledgement.
>  	 */
> -	target_get_sess_cmd(se_sess, se_cmd, (flags & TARGET_SCF_ACK_KREF));
> +	rc = target_get_sess_cmd(se_sess, se_cmd, (flags & TARGET_SCF_ACK_KREF));
> +	if (rc)
> +		return rc;

OK, I see where you where going with the target_submit_cmd() failure
here now..

However, I'm still leaning towards a way to do this for tcm_qla2xxx that
does not require all fabric callers to handle target_submit_cmd()
exceptions directly..

How about a special target_get_sess_cmd() failure callback to free
se_cmd when target_get_sess_cmd() fails, but not actually send a SCSI
response back out to the fabric here during session shutdown..?

That said, I'm going to commit this patch (slightly changed to keep
target_submit_cmd() returning void for now) but I'd still like a better
way of handling this particular failure without propagating up the
return value.

>  	/*
>  	 * Signal bidirectional data payloads to target-core
>  	 */
> @@ -1664,7 +1666,11 @@ int target_submit_tmr(struct se_cmd *se_cmd, struct se_session *se_sess,
>  		se_cmd->se_tmr_req->ref_task_tag = tag;
>  
>  	/* See target_submit_cmd for commentary */
> -	target_get_sess_cmd(se_sess, se_cmd, (flags & TARGET_SCF_ACK_KREF));
> +	ret = target_get_sess_cmd(se_sess, se_cmd, (flags & TARGET_SCF_ACK_KREF));
> +	if (ret) {
> +		core_tmr_release_req(se_cmd->se_tmr_req);
> +		return ret;
> +	}
>  
>  	ret = transport_lookup_tmr_lun(se_cmd, unpacked_lun);
>  	if (ret) {
> @@ -3650,10 +3656,11 @@ EXPORT_SYMBOL(transport_generic_free_cmd);
>   * @se_cmd:	command descriptor to add
>   * @ack_kref:	Signal that fabric will perform an ack target_put_sess_cmd()
>   */
> -static void target_get_sess_cmd(struct se_session *se_sess, struct se_cmd *se_cmd,
> -				bool ack_kref)
> +static int target_get_sess_cmd(struct se_session *se_sess, struct se_cmd *se_cmd,
> +			       bool ack_kref)
>  {
>  	unsigned long flags;
> +	int ret = 0;
>  
>  	kref_init(&se_cmd->cmd_kref);
>  	/*
> @@ -3667,9 +3674,16 @@ static void target_get_sess_cmd(struct se_session *se_sess, struct se_cmd *se_cm
>  	}
>  
>  	spin_lock_irqsave(&se_sess->sess_cmd_lock, flags);
> +	if (se_sess->sess_tearing_down) {
> +		ret = -ESHUTDOWN;
> +		goto out;
> +	}
>  	list_add_tail(&se_cmd->se_cmd_list, &se_sess->sess_cmd_list);
>  	se_cmd->check_release = 1;
> +
> +out:
>  	spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
> +	return ret;
>  }
>  
>  static void target_release_cmd_kref(struct kref *kref)

Re: [PATCH 6/7] qla2xxx: Remove racy, now-redundant check of sess_tearing_down

[Nicholas A. Bellinger]

On Mon, 2012-07-16 at 11:04 -0700, Roland Dreier wrote:
> From: Roland Dreier <roland@purestorage.com>
> 
> Now that target_submit_cmd() / target_get_sess_cmd() check
> sess_tearing_down before adding commands to the list, we no longer
> need the check in qlt_do_work().  In fact this check is racy anyway
> (and that race is what inspired the change to add the check of
> sess_tearing_down to the target core).
> 
> Cc: Chad Dupuis <chad.dupuis@qlogic.com>
> Cc: Arun Easi <arun.easi@qlogic.com>
> Signed-off-by: Roland Dreier <roland@purestorage.com>
> ---
>  drivers/scsi/qla2xxx/qla_target.c |   16 +++-------------
>  1 file changed, 3 insertions(+), 13 deletions(-)
> 

Glad to see this one finally go.  ;)

Applied to for-next

> diff --git a/drivers/scsi/qla2xxx/qla_target.c b/drivers/scsi/qla2xxx/qla_target.c
> index 87b5a330..5b30132 100644
> --- a/drivers/scsi/qla2xxx/qla_target.c
> +++ b/drivers/scsi/qla2xxx/qla_target.c
> @@ -2643,19 +2643,9 @@ static void qlt_do_work(struct work_struct *work)
>  	spin_lock_irqsave(&ha->hardware_lock, flags);
>  	sess = ha->tgt.tgt_ops->find_sess_by_s_id(vha,
>  	    atio->u.isp24.fcp_hdr.s_id);
> -	if (sess) {
> -		if (unlikely(sess->se_sess->sess_tearing_down)) {
> -			sess = NULL;
> -			spin_unlock_irqrestore(&ha->hardware_lock, flags);
> -			goto out_term;
> -		} else {
> -			/*
> -			 * Do the extra kref_get() before dropping
> -			 * qla_hw_data->hardware_lock.
> -			 */
> -			kref_get(&sess->se_sess->sess_kref);
> -		}
> -	}
> +	/* Do kref_get() before dropping qla_hw_data->hardware_lock. */
> +	if (sess)
> +		kref_get(&sess->se_sess->sess_kref);
>  	spin_unlock_irqrestore(&ha->hardware_lock, flags);
>  
>  	if (unlikely(!sess)) {

Re: [PATCH 7/7] target: Remove se_session.sess_wait_list

[Nicholas A. Bellinger]

On Mon, 2012-07-16 at 11:04 -0700, Roland Dreier wrote:
> From: Roland Dreier <roland@purestorage.com>
> 
> Since we set se_session.sess_tearing_down and stop new commands from
> being added to se_session.sess_cmd_list before we wait for commands to
> finish when freeing a session, there's no need for a separate
> sess_wait_list -- if we let new commands be added to sess_cmd_list
> after setting sess_tearing_down, that would be a bug that breaks the
> logic of waiting in-flight commands.
> 
> Also rename target_splice_sess_cmd_list() to
> target_sess_cmd_list_set_waiting(), since we are no longer splicing
> onto a separate list.
> 

So it looks reasonable to drop this cruft now, aside from the
target_get_sess_cmd() failure bit that still needs to be worked out
for-3.6 code.

Nice work Roland & Pure folks!

> 
> 
> Signed-off-by: Roland Dreier <roland@purestorage.com>
> ---
>  drivers/scsi/qla2xxx/tcm_qla2xxx.c     |    2 +-
>  drivers/target/target_core_transport.c |   22 ++++++++++------------
>  include/target/target_core_base.h      |    1 -
>  include/target/target_core_fabric.h    |    2 +-
>  4 files changed, 12 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
> index 7db7ca7..72f3557 100644
> --- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
> +++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
> @@ -466,7 +466,7 @@ static int tcm_qla2xxx_shutdown_session(struct se_session *se_sess)
>  	vha = sess->vha;
>  
>  	spin_lock_irqsave(&vha->hw->hardware_lock, flags);
> -	target_splice_sess_cmd_list(se_sess);
> +	target_sess_cmd_list_set_waiting(se_sess);
>  	spin_unlock_irqrestore(&vha->hw->hardware_lock, flags);
>  
>  	return 1;
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index 23e6e2d..7b3359e 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -244,7 +244,6 @@ struct se_session *transport_init_session(void)
>  	INIT_LIST_HEAD(&se_sess->sess_list);
>  	INIT_LIST_HEAD(&se_sess->sess_acl_list);
>  	INIT_LIST_HEAD(&se_sess->sess_cmd_list);
> -	INIT_LIST_HEAD(&se_sess->sess_wait_list);
>  	spin_lock_init(&se_sess->sess_cmd_lock);
>  	kref_init(&se_sess->sess_kref);
>  
> @@ -3719,28 +3718,27 @@ int target_put_sess_cmd(struct se_session *se_sess, struct se_cmd *se_cmd)
>  }
>  EXPORT_SYMBOL(target_put_sess_cmd);
>  
> -/* target_splice_sess_cmd_list - Split active cmds into sess_wait_list
> - * @se_sess:	session to split
> +/* target_sess_cmd_list_set_waiting - Flag all commands in
> + *         sess_cmd_list to complete cmd_wait_comp.  Set
> + *         sess_tearing_down so no more commands are queued.
> + * @se_sess:	session to flag
>   */
> -void target_splice_sess_cmd_list(struct se_session *se_sess)
> +void target_sess_cmd_list_set_waiting(struct se_session *se_sess)
>  {
>  	struct se_cmd *se_cmd;
>  	unsigned long flags;
>  
> -	WARN_ON(!list_empty(&se_sess->sess_wait_list));
> -	INIT_LIST_HEAD(&se_sess->sess_wait_list);
> -
>  	spin_lock_irqsave(&se_sess->sess_cmd_lock, flags);
> -	se_sess->sess_tearing_down = 1;
>  
> -	list_splice_init(&se_sess->sess_cmd_list, &se_sess->sess_wait_list);
> +	WARN_ON(se_sess->sess_tearing_down);
> +	se_sess->sess_tearing_down = 1;
>  
> -	list_for_each_entry(se_cmd, &se_sess->sess_wait_list, se_cmd_list)
> +	list_for_each_entry(se_cmd, &se_sess->sess_cmd_list, se_cmd_list)
>  		se_cmd->cmd_wait_set = 1;
>  
>  	spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
>  }
> -EXPORT_SYMBOL(target_splice_sess_cmd_list);
> +EXPORT_SYMBOL(target_sess_cmd_list_set_waiting);
>  
>  /* target_wait_for_sess_cmds - Wait for outstanding descriptors
>   * @se_sess:    session to wait for active I/O
> @@ -3754,7 +3752,7 @@ void target_wait_for_sess_cmds(
>  	bool rc = false;
>  
>  	list_for_each_entry_safe(se_cmd, tmp_cmd,
> -				&se_sess->sess_wait_list, se_cmd_list) {
> +				&se_sess->sess_cmd_list, se_cmd_list) {
>  		list_del(&se_cmd->se_cmd_list);
>  
>  		pr_debug("Waiting for se_cmd: %p t_state: %d, fabric state:"
> diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
> index dc35d86..5d8907e 100644
> --- a/include/target/target_core_base.h
> +++ b/include/target/target_core_base.h
> @@ -633,7 +633,6 @@ struct se_session {
>  	struct list_head	sess_list;
>  	struct list_head	sess_acl_list;
>  	struct list_head	sess_cmd_list;
> -	struct list_head	sess_wait_list;
>  	spinlock_t		sess_cmd_lock;
>  	struct kref		sess_kref;
>  };
> diff --git a/include/target/target_core_fabric.h b/include/target/target_core_fabric.h
> index 4f8e515..86d8a4a 100644
> --- a/include/target/target_core_fabric.h
> +++ b/include/target/target_core_fabric.h
> @@ -130,7 +130,7 @@ int	transport_check_aborted_status(struct se_cmd *, int);
>  int	transport_send_check_condition_and_sense(struct se_cmd *, u8, int);
>  
>  int	target_put_sess_cmd(struct se_session *, struct se_cmd *);
> -void	target_splice_sess_cmd_list(struct se_session *);
> +void	target_sess_cmd_list_set_waiting(struct se_session *);
>  void	target_wait_for_sess_cmds(struct se_session *, int);
>  
>  int	core_alua_check_nonop_delay(struct se_cmd *);

Re: [PATCH 5/7] target: Check sess_tearing_down in target_get_sess_cmd()

[Roland Dreier]

On Mon, Jul 16, 2012 at 4:08 PM, Nicholas A. Bellinger
<nab@linux-iscsi.org> wrote:
> However, I'm still leaning towards a way to do this for tcm_qla2xxx that
> does not require all fabric callers to handle target_submit_cmd()
> exceptions directly..
>
> How about a special target_get_sess_cmd() failure callback to free
> se_cmd when target_get_sess_cmd() fails, but not actually send a SCSI
> response back out to the fabric here during session shutdown..?

I guess that's OK, although I'm not sure it ends up being cleaner.  If
you look at my 4/7 patch, you see that all target_submit_cmd callers
have an error path where they handle terminating commands that
fail processing just before calling submit_cmd anyway.

eg for qla2xxx we'll need a callback to call qlt_send_term_exchange()
in addition to the error path call that qlt_do_work() has anyway.
Similarly tcm_fc ends up with a second call to ft_send_resp_code_and_free().
etc.

But I don't really have a strong feeling about this, if the view is that
submit_cmd() should never return failure directly then I'm OK with that.

> That said, I'm going to commit this patch (slightly changed to keep
> target_submit_cmd() returning void for now) but I'd still like a better
> way of handling this particular failure without propagating up the
> return value.

OK, I'll take a look at how you handle this...

 - R.

Re: [PATCH 4/7] target: Allow for target_submit_cmd() returning errors

[Nicholas A. Bellinger]

On Mon, 2012-07-16 at 16:05 -0700, Roland Dreier wrote:
> On Mon, Jul 16, 2012 at 4:00 PM, Nicholas A. Bellinger
> <nab@linux-iscsi.org> wrote:
> > Mmmm.  The original target_submit_cmd() code had been propagating up a
> > return value, but then we decided (via Agrover's patch) that it made
> > more sense for target_submit_cmd() to always handle exceptions via
> > normal TFO callbacks, and not have the fabric worry about the return
> > here..
> >
> > Also, I'm not sure if the error paths that this patch now accesses after
> > target_submit_cmd() failure are going to deal with different types of
> > target_submit_cmd() failures correctly.
> >
> > So NACK for the moment, as I don't really see why this is necessary in
> > the first place..?
> 
> Read on in the series to see why this is needed; in short, for qla2xxx
> at least, we need a race-free way to check for sess_tearing_down
> atomically with actually adding the command to sess_cmd_list.
> 

<nod>

> I'm OK with returning failure via callback, but
> 
>  a) I'm not sure we can use the normal TFO callbacks in case
>     we can't add the command to sess_cmd_list (seems like it
>     opens the door to other use-after-frees in qla2xxx at least)

The TFO callback to release qla_tgt_cmd memory would be the same here,
so it would be a internal release for target_get_sess_cmd() failures
that is careful with items have not been setup in the passed
fabric_tgt_cmd->se_cmd due to the session shutdown exception.

As commands are received during session shutdown, dispatching their
handling to tcm_qla2xxx_free_wq to do extra HW descriptor cleanup would
be nice if we can get away without actually sending a proper SCSI
response during session shutdown..  

>  b) Maybe it's OK if we say that failure to add the command to
>     the sess_cmd_list is the only time submit cmd fails?
> 

I'd still like to avoid (re)propagating up a return value of
target_submit_cmd() if at all possible..

> The qla2xxx race/use-after-free is definitely real, we hit it in testing
> here with active IO across ACL changes.
> 
>  - R.
> --
> To unsubscribe from this list: send the line "unsubscribe target-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/7] target: Check sess_tearing_down in target_get_sess_cmd()

[Roland Dreier]

> OK, I'll take a look at how you handle this...

So looking at commit bc187ea6c3b3 in the tree you just pushed out
("target: Check sess_tearing_down in target_get_sess_cmd()") it looks
like you just return from target_submit_cmd() if we fail to add the
command to sess_cmd_list -- doesn't this mean we just leak those
commands and possibly leave the HW sitting there with open exchanges?

Do you have a plan for how to handle this?  Do we really want to plumb
through another callback to tell the fabric driver to free the command
in this case?

 - R.

Re: [PATCH 5/7] target: Check sess_tearing_down in target_get_sess_cmd()

[Nicholas A. Bellinger]

On Mon, 2012-07-16 at 18:40 -0700, Roland Dreier wrote:
> > OK, I'll take a look at how you handle this...
> 
> So looking at commit bc187ea6c3b3 in the tree you just pushed out
> ("target: Check sess_tearing_down in target_get_sess_cmd()") it looks
> like you just return from target_submit_cmd() if we fail to add the
> command to sess_cmd_list -- doesn't this mean we just leak those
> commands and possibly leave the HW sitting there with open exchanges?
> 

With dropping patch #5 for now, I assume that would be the case..

> Do you have a plan for how to handle this?  Do we really want to plumb
> through another callback to tell the fabric driver to free the command
> in this case?
> 

I need to think more about this ahead of changing it back again for-3.6
now that other fabrics have the assumption of target_submit_cmd() would
not fail.

There is a clear case with qla2xxx for just changing it back (we might
end up doing that just yet) but I wanted to get the other important bits
into for-next into place first..

Re: [PATCH 5/7] target: Check sess_tearing_down in target_get_sess_cmd()

[Nicholas A. Bellinger]

On Mon, 2012-07-16 at 18:56 -0700, Nicholas A. Bellinger wrote:
> On Mon, 2012-07-16 at 18:40 -0700, Roland Dreier wrote:
> > > OK, I'll take a look at how you handle this...
> > 
> > So looking at commit bc187ea6c3b3 in the tree you just pushed out
> > ("target: Check sess_tearing_down in target_get_sess_cmd()") it looks
> > like you just return from target_submit_cmd() if we fail to add the
> > command to sess_cmd_list -- doesn't this mean we just leak those
> > commands and possibly leave the HW sitting there with open exchanges?
> > 
> 
> With dropping patch #5 for now, I assume that would be the case..
> 

Or with patch #4 even..

Re: [PATCH 5/7] target: Check sess_tearing_down in target_get_sess_cmd()

[Roland Dreier]

On Mon, Jul 16, 2012 at 6:56 PM, Nicholas A. Bellinger
<nab@linux-iscsi.org> wrote:
>> Do you have a plan for how to handle this?  Do we really want to plumb
>> through another callback to tell the fabric driver to free the command
>> in this case?

> I need to think more about this ahead of changing it back again for-3.6
> now that other fabrics have the assumption of target_submit_cmd() would
> not fail.

> There is a clear case with qla2xxx for just changing it back (we might
> end up doing that just yet) but I wanted to get the other important bits
> into for-next into place first..

Sleeping on things, I now feel pretty strongly that having target_submit_cmd
return an error value for "immediate" errors where the command does not
make it into the target core is the right approach.

Freeing commands is already one of the most confusing and complex parts
of the target code, with "check_stop_free," "cmd_wait_comp" and
"SCF_ACK_KREF."  Adding another code flow back to the fabric driver
with yet another set of semantics around freeing a command seems like
it's making things even harder to maintain.

On the other hand, every caller of target_submit_cmd() in the tree already
has an error path where it handles problems it detects right before calling
target_submit_cmd(), and so it's trivial to share that for problems detected
inside target_submit_cmd().  If you look at patch 4/7, pretty much the
only changes are like going from

    target_submit_cmd(...);

to

    if (target_submit_cmd(...))
        goto err;

and in fact the ->handle_cmd() wrapper that qla_target uses to call
target_submit_cmd via tcm_qla2xxx already has a return value that
qla_target handled properly!

I guess another approach would be to split target_submit_cmd() into
the target_get_sess_cmd() part and the rest of it, and have fabric
drivers handle errors queueing commands but not have to worry about
the submit cmd part failing.  But I'm not sure that's any better.

Andy, any feelings about this one way or another?  Christoph?

Thanks,
  Roland

Re: [PATCH 5/7] target: Check sess_tearing_down in target_get_sess_cmd()

[Christoph Hellwig]

On Tue, Jul 17, 2012 at 09:34:49AM -0700, Roland Dreier wrote:
> Sleeping on things, I now feel pretty strongly that having target_submit_cmd
> return an error value for "immediate" errors where the command does not
> make it into the target core is the right approach.

I think it is.  When I tried to convert other drivers to
target_submit_cmd while doing the target processing thread removal that
would have simplified a lot of things.

> Freeing commands is already one of the most confusing and complex parts
> of the target code, with "check_stop_free," "cmd_wait_comp" and
> "SCF_ACK_KREF."  Adding another code flow back to the fabric driver
> with yet another set of semantics around freeing a command seems like
> it's making things even harder to maintain.

True.  And the above mess really needs to be simplified, too.

Re: [PATCH 5/7] target: Check sess_tearing_down in target_get_sess_cmd()

[Andy Grover]

On 07/17/2012 09:39 AM, Christoph Hellwig wrote:
> On Tue, Jul 17, 2012 at 09:34:49AM -0700, Roland Dreier wrote:
>> Sleeping on things, I now feel pretty strongly that having target_submit_cmd
>> return an error value for "immediate" errors where the command does not
>> make it into the target core is the right approach.
> 
> I think it is.  When I tried to convert other drivers to
> target_submit_cmd while doing the target processing thread removal that
> would have simplified a lot of things.
> 
>> Freeing commands is already one of the most confusing and complex parts
>> of the target code, with "check_stop_free," "cmd_wait_comp" and
>> "SCF_ACK_KREF."  Adding another code flow back to the fabric driver
>> with yet another set of semantics around freeing a command seems like
>> it's making things even harder to maintain.
> 
> True.  And the above mess really needs to be simplified, too.

I'm ok with submit_cmd returning a value for now. Maybe in the end it
doesn't, but getting the code working comes first.

This is one of those areas that needs a complete rewrite, so who cares
if there's some dirt on the floor when the whole joint is due for
remodeling.

-- Andy

Re: [PATCH 5/7] target: Check sess_tearing_down in target_get_sess_cmd()

[Nicholas A. Bellinger]

On Tue, 2012-07-17 at 09:34 -0700, Roland Dreier wrote:
> On Mon, Jul 16, 2012 at 6:56 PM, Nicholas A. Bellinger
> <nab@linux-iscsi.org> wrote:
> >> Do you have a plan for how to handle this?  Do we really want to plumb
> >> through another callback to tell the fabric driver to free the command
> >> in this case?
> 
> > I need to think more about this ahead of changing it back again for-3.6
> > now that other fabrics have the assumption of target_submit_cmd() would
> > not fail.
> 
> > There is a clear case with qla2xxx for just changing it back (we might
> > end up doing that just yet) but I wanted to get the other important bits
> > into for-next into place first..
> 
> Sleeping on things, I now feel pretty strongly that having target_submit_cmd
> return an error value for "immediate" errors where the command does not
> make it into the target core is the right approach.
> 
> Freeing commands is already one of the most confusing and complex parts
> of the target code, with "check_stop_free," "cmd_wait_comp" and
> "SCF_ACK_KREF."  Adding another code flow back to the fabric driver
> with yet another set of semantics around freeing a command seems like
> it's making things even harder to maintain.
> 

I wasn't talking about adding another release path.  I was thinking more
about a TFO call to optionally abort HW/SW exchange if we can't accept
the command in question.

Not to mention, this could end up being useful for other purposes beyond
the initial target_submit_cmd() failure case due to active I/O shutdown.
(aborts + active I/O shutdown with active SRP WRITEs with ib_srpt come
to mind needing something like this..)

> On the other hand, every caller of target_submit_cmd() in the tree already
> has an error path where it handles problems it detects right before calling
> target_submit_cmd(), and so it's trivial to share that for problems detected
> inside target_submit_cmd().  If you look at patch 4/7, pretty much the
> only changes are like going from
> 
>     target_submit_cmd(...);
> 
> to
> 
>     if (target_submit_cmd(...))
>         goto err;
> 
> and in fact the ->handle_cmd() wrapper that qla_target uses to call
> target_submit_cmd via tcm_qla2xxx already has a return value that
> qla_target handled properly!
> 

Sure, there is no doubting tcm_qla2xxx's usage of this return value back
into qla_target.c code.. 

> I guess another approach would be to split target_submit_cmd() into
> the target_get_sess_cmd() part and the rest of it, and have fabric
> drivers handle errors queueing commands but not have to worry about
> the submit cmd part failing.  But I'm not sure that's any better.
> 
> Andy, any feelings about this one way or another?  Christoph?
> 

Ok, so for-3.6 it makes the most sense to just convert this back to
propagate up the return code, but only for target_get_sess_cmd() failure
case..

That said, I'll go ahead and respin patch #4 on top of for-next, and
post the updated patch shortly with the same CC's and ask for ACKs from
the necessary folks.

--nab