[PATCH v4 0/4] target: fix NULL pointer dereference

[PATCH v4 0/4] target: fix NULL pointer dereference

[Sudhakar Panneerselvam]

The following set of commits address a NULL pointer dereference and some
refactoring around this issue.

v4:
 - initialize the LUN in transport_init_se_cmd()

v3:
 - fix NULL pointer dereference when cdb initialization fails

v2:
 - new helper is named as target_cmd_init_cdb()
 - existing function, target_setup_cmd_from_cdb is renamed as
   target_cmd_parse_cdb()

Sudhakar Panneerselvam (4):
  target: factor out a new helper, target_cmd_init_cdb()
  target: initialize LUN in transport_init_se_cmd().
  target: fix NULL pointer dereference
  target: rename target_setup_cmd_from_cdb() to target_cmd_parse_cdb()

 drivers/target/iscsi/iscsi_target.c    | 29 ++++++++++--------
 drivers/target/target_core_device.c    | 19 +++++-------
 drivers/target/target_core_tmr.c       |  4 +--
 drivers/target/target_core_transport.c | 55 ++++++++++++++++++++++++++--------
 drivers/target/target_core_xcopy.c     |  9 ++++--
 drivers/usb/gadget/function/f_tcm.c    |  6 ++--
 include/target/target_core_fabric.h    |  9 +++---
 7 files changed, 83 insertions(+), 48 deletions(-)

-- 
1.8.3.1

[PATCH v4 0/4] target: fix NULL pointer dereference

[Sudhakar Panneerselvam]

The following set of commits address a NULL pointer dereference and some
refactoring around this issue.

v4:
 - initialize the LUN in transport_init_se_cmd()

v3:
 - fix NULL pointer dereference when cdb initialization fails

v2:
 - new helper is named as target_cmd_init_cdb()
 - existing function, target_setup_cmd_from_cdb is renamed as
   target_cmd_parse_cdb()

Sudhakar Panneerselvam (4):
  target: factor out a new helper, target_cmd_init_cdb()
  target: initialize LUN in transport_init_se_cmd().
  target: fix NULL pointer dereference
  target: rename target_setup_cmd_from_cdb() to target_cmd_parse_cdb()

 drivers/target/iscsi/iscsi_target.c    | 29 ++++++++++--------
 drivers/target/target_core_device.c    | 19 +++++-------
 drivers/target/target_core_tmr.c       |  4 +--
 drivers/target/target_core_transport.c | 55 ++++++++++++++++++++++++++--------
 drivers/target/target_core_xcopy.c     |  9 ++++--
 drivers/usb/gadget/function/f_tcm.c    |  6 ++--
 include/target/target_core_fabric.h    |  9 +++---
 7 files changed, 83 insertions(+), 48 deletions(-)

-- 
1.8.3.1

[PATCH v4 1/4] target: factor out a new helper, target_cmd_init_cdb()

[Sudhakar Panneerselvam]

target_setup_cmd_from_cdb() is called after a successful call to
transport_lookup_cmd_lun(). The new helper factors out the code that can
be called before the call to transport_lookup_cmd_lun(). This helper
will be used in an upcoming commit to address NULL pointer dereference.

Signed-off-by: Sudhakar Panneerselvam <sudhakar.panneerselvam@oracle.com>
---
 drivers/target/target_core_transport.c | 16 ++++++++++++----
 include/target/target_core_fabric.h    |  1 +
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index e6b448f43071..f2f7c5b818cc 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1410,11 +1410,8 @@ void transport_init_se_cmd(
 }
 
 sense_reason_t
-target_setup_cmd_from_cdb(struct se_cmd *cmd, unsigned char *cdb)
+target_cmd_init_cdb(struct se_cmd *cmd, unsigned char *cdb)
 {
-	struct se_device *dev = cmd->se_dev;
-	sense_reason_t ret;
-
 	/*
 	 * Ensure that the received CDB is less than the max (252 + 8) bytes
 	 * for VARIABLE_LENGTH_CMD
@@ -1448,6 +1445,17 @@ void transport_init_se_cmd(
 	memcpy(cmd->t_task_cdb, cdb, scsi_command_size(cdb));
 
 	trace_target_sequencer_start(cmd);
+	return 0;
+}
+EXPORT_SYMBOL(target_cmd_init_cdb);
+
+sense_reason_t
+target_setup_cmd_from_cdb(struct se_cmd *cmd, unsigned char *cdb)
+{
+	struct se_device *dev = cmd->se_dev;
+	sense_reason_t ret;
+
+	target_cmd_init_cdb(cmd, cdb);
 
 	ret = dev->transport->parse_cdb(cmd);
 	if (ret = TCM_UNSUPPORTED_SCSI_OPCODE)
diff --git a/include/target/target_core_fabric.h b/include/target/target_core_fabric.h
index 063f133e47c2..6a2bfcca0c98 100644
--- a/include/target/target_core_fabric.h
+++ b/include/target/target_core_fabric.h
@@ -152,6 +152,7 @@ void	transport_init_se_cmd(struct se_cmd *,
 		const struct target_core_fabric_ops *,
 		struct se_session *, u32, int, int, unsigned char *);
 sense_reason_t transport_lookup_cmd_lun(struct se_cmd *, u64);
+sense_reason_t target_cmd_init_cdb(struct se_cmd *, unsigned char *);
 sense_reason_t target_setup_cmd_from_cdb(struct se_cmd *, unsigned char *);
 int	target_submit_cmd_map_sgls(struct se_cmd *, struct se_session *,
 		unsigned char *, unsigned char *, u64, u32, int, int, int,
-- 
1.8.3.1

[PATCH v4 1/4] target: factor out a new helper, target_cmd_init_cdb()

[Sudhakar Panneerselvam]

target_setup_cmd_from_cdb() is called after a successful call to
transport_lookup_cmd_lun(). The new helper factors out the code that can
be called before the call to transport_lookup_cmd_lun(). This helper
will be used in an upcoming commit to address NULL pointer dereference.

Signed-off-by: Sudhakar Panneerselvam <sudhakar.panneerselvam@oracle.com>
---
 drivers/target/target_core_transport.c | 16 ++++++++++++----
 include/target/target_core_fabric.h    |  1 +
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index e6b448f43071..f2f7c5b818cc 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1410,11 +1410,8 @@ void transport_init_se_cmd(
 }
 
 sense_reason_t
-target_setup_cmd_from_cdb(struct se_cmd *cmd, unsigned char *cdb)
+target_cmd_init_cdb(struct se_cmd *cmd, unsigned char *cdb)
 {
-	struct se_device *dev = cmd->se_dev;
-	sense_reason_t ret;
-
 	/*
 	 * Ensure that the received CDB is less than the max (252 + 8) bytes
 	 * for VARIABLE_LENGTH_CMD
@@ -1448,6 +1445,17 @@ void transport_init_se_cmd(
 	memcpy(cmd->t_task_cdb, cdb, scsi_command_size(cdb));
 
 	trace_target_sequencer_start(cmd);
+	return 0;
+}
+EXPORT_SYMBOL(target_cmd_init_cdb);
+
+sense_reason_t
+target_setup_cmd_from_cdb(struct se_cmd *cmd, unsigned char *cdb)
+{
+	struct se_device *dev = cmd->se_dev;
+	sense_reason_t ret;
+
+	target_cmd_init_cdb(cmd, cdb);
 
 	ret = dev->transport->parse_cdb(cmd);
 	if (ret == TCM_UNSUPPORTED_SCSI_OPCODE)
diff --git a/include/target/target_core_fabric.h b/include/target/target_core_fabric.h
index 063f133e47c2..6a2bfcca0c98 100644
--- a/include/target/target_core_fabric.h
+++ b/include/target/target_core_fabric.h
@@ -152,6 +152,7 @@ void	transport_init_se_cmd(struct se_cmd *,
 		const struct target_core_fabric_ops *,
 		struct se_session *, u32, int, int, unsigned char *);
 sense_reason_t transport_lookup_cmd_lun(struct se_cmd *, u64);
+sense_reason_t target_cmd_init_cdb(struct se_cmd *, unsigned char *);
 sense_reason_t target_setup_cmd_from_cdb(struct se_cmd *, unsigned char *);
 int	target_submit_cmd_map_sgls(struct se_cmd *, struct se_session *,
 		unsigned char *, unsigned char *, u64, u32, int, int, int,
-- 
1.8.3.1

[PATCH v4 2/4] target: initialize LUN in transport_init_se_cmd().

[Sudhakar Panneerselvam]

Initialization of orig_fe_lun is moved to transport_init_se_cmd() from
transport_lookup_cmd_lun(). This helps for the cases where the scsi request
fails before the call to transport_lookup_cmd_lun() so that
trace_target_cmd_complete() can print the LUN information to the trace
buffer. Due to this change, the lun parameter is removed from
transport_lookup_cmd_lun() and transport_lookup_tmr_lun().

Signed-off-by: Sudhakar Panneerselvam <sudhakar.panneerselvam@oracle.com>
---
 drivers/target/iscsi/iscsi_target.c    | 11 +++++------
 drivers/target/target_core_device.c    | 19 ++++++++-----------
 drivers/target/target_core_tmr.c       |  4 ++--
 drivers/target/target_core_transport.c | 12 +++++++-----
 drivers/target/target_core_xcopy.c     |  4 ++--
 drivers/usb/gadget/function/f_tcm.c    |  6 ++++--
 include/target/target_core_fabric.h    |  6 +++---
 7 files changed, 31 insertions(+), 31 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index 59379d662626..1b453629b516 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -1158,7 +1158,7 @@ int iscsit_setup_scsi_cmd(struct iscsi_conn *conn, struct iscsi_cmd *cmd,
 	transport_init_se_cmd(&cmd->se_cmd, &iscsi_ops,
 			conn->sess->se_sess, be32_to_cpu(hdr->data_length),
 			cmd->data_direction, sam_task_attr,
-			cmd->sense_buffer + 2);
+			cmd->sense_buffer + 2, scsilun_to_int(&hdr->lun));
 
 	pr_debug("Got SCSI Command, ITT: 0x%08x, CmdSN: 0x%08x,"
 		" ExpXferLen: %u, Length: %u, CID: %hu\n", hdr->itt,
@@ -1167,8 +1167,7 @@ int iscsit_setup_scsi_cmd(struct iscsi_conn *conn, struct iscsi_cmd *cmd,
 
 	target_get_sess_cmd(&cmd->se_cmd, true);
 
-	cmd->sense_reason = transport_lookup_cmd_lun(&cmd->se_cmd,
-						     scsilun_to_int(&hdr->lun));
+	cmd->sense_reason = transport_lookup_cmd_lun(&cmd->se_cmd);
 	if (cmd->sense_reason)
 		goto attach_cmd;
 
@@ -2000,7 +1999,8 @@ static enum tcm_tmreq_table iscsit_convert_tmf(u8 iscsi_tmf)
 
 	transport_init_se_cmd(&cmd->se_cmd, &iscsi_ops,
 			      conn->sess->se_sess, 0, DMA_NONE,
-			      TCM_SIMPLE_TAG, cmd->sense_buffer + 2);
+			      TCM_SIMPLE_TAG, cmd->sense_buffer + 2,
+			      scsilun_to_int(&hdr->lun));
 
 	target_get_sess_cmd(&cmd->se_cmd, true);
 
@@ -2038,8 +2038,7 @@ static enum tcm_tmreq_table iscsit_convert_tmf(u8 iscsi_tmf)
 	 * Locate the struct se_lun for all TMRs not related to ERL=2 TASK_REASSIGN
 	 */
 	if (function != ISCSI_TM_FUNC_TASK_REASSIGN) {
-		ret = transport_lookup_tmr_lun(&cmd->se_cmd,
-					       scsilun_to_int(&hdr->lun));
+		ret = transport_lookup_tmr_lun(&cmd->se_cmd);
 		if (ret < 0) {
 			se_tmr->response = ISCSI_TMF_RSP_NO_LUN;
 			goto attach;
diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c
index 46b0e1ceb77f..405d82d44717 100644
--- a/drivers/target/target_core_device.c
+++ b/drivers/target/target_core_device.c
@@ -45,7 +45,7 @@
 struct se_device *g_lun0_dev;
 
 sense_reason_t
-transport_lookup_cmd_lun(struct se_cmd *se_cmd, u64 unpacked_lun)
+transport_lookup_cmd_lun(struct se_cmd *se_cmd)
 {
 	struct se_lun *se_lun = NULL;
 	struct se_session *se_sess = se_cmd->se_sess;
@@ -54,7 +54,7 @@
 	sense_reason_t ret = TCM_NO_SENSE;
 
 	rcu_read_lock();
-	deve = target_nacl_find_deve(nacl, unpacked_lun);
+	deve = target_nacl_find_deve(nacl, se_cmd->orig_fe_lun);
 	if (deve) {
 		atomic_long_inc(&deve->total_cmds);
 
@@ -74,7 +74,6 @@
 
 		se_cmd->se_lun = se_lun;
 		se_cmd->pr_res_key = deve->pr_res_key;
-		se_cmd->orig_fe_lun = unpacked_lun;
 		se_cmd->se_cmd_flags |= SCF_SE_LUN_CMD;
 		se_cmd->lun_ref_active = true;
 
@@ -83,7 +82,7 @@
 			pr_err("TARGET_CORE[%s]: Detected WRITE_PROTECTED LUN"
 				" Access for 0x%08llx\n",
 				se_cmd->se_tfo->fabric_name,
-				unpacked_lun);
+				se_cmd->orig_fe_lun);
 			rcu_read_unlock();
 			ret = TCM_WRITE_PROTECTED;
 			goto ref_dev;
@@ -98,18 +97,17 @@
 		 * REPORT_LUNS, et al to be returned when no active
 		 * MappedLUN=0 exists for this Initiator Port.
 		 */
-		if (unpacked_lun != 0) {
+		if (se_cmd->orig_fe_lun != 0) {
 			pr_err("TARGET_CORE[%s]: Detected NON_EXISTENT_LUN"
 				" Access for 0x%08llx from %s\n",
 				se_cmd->se_tfo->fabric_name,
-				unpacked_lun,
+				se_cmd->orig_fe_lun,
 				nacl->initiatorname);
 			return TCM_NON_EXISTENT_LUN;
 		}
 
 		se_lun = se_sess->se_tpg->tpg_virt_lun0;
 		se_cmd->se_lun = se_sess->se_tpg->tpg_virt_lun0;
-		se_cmd->orig_fe_lun = 0;
 		se_cmd->se_cmd_flags |= SCF_SE_LUN_CMD;
 
 		percpu_ref_get(&se_lun->lun_ref);
@@ -145,7 +143,7 @@
 }
 EXPORT_SYMBOL(transport_lookup_cmd_lun);
 
-int transport_lookup_tmr_lun(struct se_cmd *se_cmd, u64 unpacked_lun)
+int transport_lookup_tmr_lun(struct se_cmd *se_cmd)
 {
 	struct se_dev_entry *deve;
 	struct se_lun *se_lun = NULL;
@@ -155,7 +153,7 @@ int transport_lookup_tmr_lun(struct se_cmd *se_cmd, u64 unpacked_lun)
 	unsigned long flags;
 
 	rcu_read_lock();
-	deve = target_nacl_find_deve(nacl, unpacked_lun);
+	deve = target_nacl_find_deve(nacl, se_cmd->orig_fe_lun);
 	if (deve) {
 		se_lun = rcu_dereference(deve->se_lun);
 
@@ -166,7 +164,6 @@ int transport_lookup_tmr_lun(struct se_cmd *se_cmd, u64 unpacked_lun)
 
 		se_cmd->se_lun = se_lun;
 		se_cmd->pr_res_key = deve->pr_res_key;
-		se_cmd->orig_fe_lun = unpacked_lun;
 		se_cmd->se_cmd_flags |= SCF_SE_LUN_CMD;
 		se_cmd->lun_ref_active = true;
 	}
@@ -177,7 +174,7 @@ int transport_lookup_tmr_lun(struct se_cmd *se_cmd, u64 unpacked_lun)
 		pr_debug("TARGET_CORE[%s]: Detected NON_EXISTENT_LUN"
 			" Access for 0x%08llx for %s\n",
 			se_cmd->se_tfo->fabric_name,
-			unpacked_lun,
+			se_cmd->orig_fe_lun,
 			nacl->initiatorname);
 		return -ENODEV;
 	}
diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c
index afbd492c76a9..89c84d472cd7 100644
--- a/drivers/target/target_core_tmr.c
+++ b/drivers/target/target_core_tmr.c
@@ -148,8 +148,8 @@ void core_tmr_abort_task(
 		 * code.
 		 */
 		if (!tmr->tmr_dev)
-			WARN_ON_ONCE(transport_lookup_tmr_lun(tmr->task_cmd,
-						se_cmd->orig_fe_lun) < 0);
+			WARN_ON_ONCE(transport_lookup_tmr_lun(tmr->task_cmd) <
+					0);
 
 		target_put_cmd_and_wait(se_cmd);
 
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index f2f7c5b818cc..7ea77933e64d 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1364,7 +1364,7 @@ void transport_init_se_cmd(
 	u32 data_length,
 	int data_direction,
 	int task_attr,
-	unsigned char *sense_buffer)
+	unsigned char *sense_buffer, u64 unpacked_lun)
 {
 	INIT_LIST_HEAD(&cmd->se_delayed_node);
 	INIT_LIST_HEAD(&cmd->se_qf_node);
@@ -1383,6 +1383,7 @@ void transport_init_se_cmd(
 	cmd->data_direction = data_direction;
 	cmd->sam_task_attr = task_attr;
 	cmd->sense_buffer = sense_buffer;
+	cmd->orig_fe_lun = unpacked_lun;
 
 	cmd->state_active = false;
 }
@@ -1596,7 +1597,8 @@ int target_submit_cmd_map_sgls(struct se_cmd *se_cmd, struct se_session *se_sess
 	 * target_core_fabric_ops->queue_status() callback
 	 */
 	transport_init_se_cmd(se_cmd, se_tpg->se_tpg_tfo, se_sess,
-				data_length, data_dir, task_attr, sense);
+				data_length, data_dir, task_attr, sense,
+				unpacked_lun);
 
 	if (flags & TARGET_SCF_USE_CPUID)
 		se_cmd->se_cmd_flags |= SCF_USE_CPUID;
@@ -1622,7 +1624,7 @@ int target_submit_cmd_map_sgls(struct se_cmd *se_cmd, struct se_session *se_sess
 	/*
 	 * Locate se_lun pointer and attach it to struct se_cmd
 	 */
-	rc = transport_lookup_cmd_lun(se_cmd, unpacked_lun);
+	rc = transport_lookup_cmd_lun(se_cmd);
 	if (rc) {
 		transport_send_check_condition_and_sense(se_cmd, rc, 0);
 		target_put_sess_cmd(se_cmd);
@@ -1790,7 +1792,7 @@ int target_submit_tmr(struct se_cmd *se_cmd, struct se_session *se_sess,
 	BUG_ON(!se_tpg);
 
 	transport_init_se_cmd(se_cmd, se_tpg->se_tpg_tfo, se_sess,
-			      0, DMA_NONE, TCM_SIMPLE_TAG, sense);
+			      0, DMA_NONE, TCM_SIMPLE_TAG, sense, unpacked_lun);
 	/*
 	 * FIXME: Currently expect caller to handle se_cmd->se_tmr_req
 	 * allocation failure.
@@ -1818,7 +1820,7 @@ int target_submit_tmr(struct se_cmd *se_cmd, struct se_session *se_sess,
 			goto failure;
 	}
 
-	ret = transport_lookup_tmr_lun(se_cmd, unpacked_lun);
+	ret = transport_lookup_tmr_lun(se_cmd);
 	if (ret)
 		goto failure;
 
diff --git a/drivers/target/target_core_xcopy.c b/drivers/target/target_core_xcopy.c
index bd3ed6ce7571..5f1bab37a6bf 100644
--- a/drivers/target/target_core_xcopy.c
+++ b/drivers/target/target_core_xcopy.c
@@ -585,7 +585,7 @@ static int target_xcopy_read_source(
 		(unsigned long long)src_lba, src_sectors, length);
 
 	transport_init_se_cmd(se_cmd, &xcopy_pt_tfo, &xcopy_pt_sess, length,
-			      DMA_FROM_DEVICE, 0, &xpt_cmd.sense_buffer[0]);
+			      DMA_FROM_DEVICE, 0, &xpt_cmd.sense_buffer[0], 0);
 
 	rc = target_xcopy_setup_pt_cmd(&xpt_cmd, xop, src_dev, &cdb[0],
 				remote_port);
@@ -630,7 +630,7 @@ static int target_xcopy_write_destination(
 		(unsigned long long)dst_lba, dst_sectors, length);
 
 	transport_init_se_cmd(se_cmd, &xcopy_pt_tfo, &xcopy_pt_sess, length,
-			      DMA_TO_DEVICE, 0, &xpt_cmd.sense_buffer[0]);
+			      DMA_TO_DEVICE, 0, &xpt_cmd.sense_buffer[0], 0);
 
 	rc = target_xcopy_setup_pt_cmd(&xpt_cmd, xop, dst_dev, &cdb[0],
 				remote_port);
diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index 36504931b2d1..ac049909a8bf 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -1049,7 +1049,8 @@ static void usbg_cmd_work(struct work_struct *work)
 		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,
-				cmd->prio_attr, cmd->sense_iu.sense);
+				cmd->prio_attr, cmd->sense_iu.sense,
+				cmd->unpacked_lun);
 		goto out;
 	}
 
@@ -1179,7 +1180,8 @@ static void bot_cmd_work(struct work_struct *work)
 		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,
-				cmd->prio_attr, cmd->sense_iu.sense);
+				cmd->prio_attr, cmd->sense_iu.sense,
+				cmd->unpacked_lun);
 		goto out;
 	}
 
diff --git a/include/target/target_core_fabric.h b/include/target/target_core_fabric.h
index 6a2bfcca0c98..92f6cb20775e 100644
--- a/include/target/target_core_fabric.h
+++ b/include/target/target_core_fabric.h
@@ -150,8 +150,8 @@ void	transport_register_session(struct se_portal_group *,
 
 void	transport_init_se_cmd(struct se_cmd *,
 		const struct target_core_fabric_ops *,
-		struct se_session *, u32, int, int, unsigned char *);
-sense_reason_t transport_lookup_cmd_lun(struct se_cmd *, u64);
+		struct se_session *, u32, int, int, unsigned char *, u64);
+sense_reason_t transport_lookup_cmd_lun(struct se_cmd *);
 sense_reason_t target_cmd_init_cdb(struct se_cmd *, unsigned char *);
 sense_reason_t target_setup_cmd_from_cdb(struct se_cmd *, unsigned char *);
 int	target_submit_cmd_map_sgls(struct se_cmd *, struct se_session *,
@@ -188,7 +188,7 @@ int	transport_send_check_condition_and_sense(struct se_cmd *,
 void	core_tmr_release_req(struct se_tmr_req *);
 int	transport_generic_handle_tmr(struct se_cmd *);
 void	transport_generic_request_failure(struct se_cmd *, sense_reason_t);
-int	transport_lookup_tmr_lun(struct se_cmd *, u64);
+int	transport_lookup_tmr_lun(struct se_cmd *);
 void	core_allocate_nexus_loss_ua(struct se_node_acl *acl);
 
 struct se_node_acl *core_tpg_get_initiator_node_acl(struct se_portal_group *tpg,
-- 
1.8.3.1

[PATCH v4 2/4] target: initialize LUN in transport_init_se_cmd().

[Sudhakar Panneerselvam]

Initialization of orig_fe_lun is moved to transport_init_se_cmd() from
transport_lookup_cmd_lun(). This helps for the cases where the scsi request
fails before the call to transport_lookup_cmd_lun() so that
trace_target_cmd_complete() can print the LUN information to the trace
buffer. Due to this change, the lun parameter is removed from
transport_lookup_cmd_lun() and transport_lookup_tmr_lun().

Signed-off-by: Sudhakar Panneerselvam <sudhakar.panneerselvam@oracle.com>
---
 drivers/target/iscsi/iscsi_target.c    | 11 +++++------
 drivers/target/target_core_device.c    | 19 ++++++++-----------
 drivers/target/target_core_tmr.c       |  4 ++--
 drivers/target/target_core_transport.c | 12 +++++++-----
 drivers/target/target_core_xcopy.c     |  4 ++--
 drivers/usb/gadget/function/f_tcm.c    |  6 ++++--
 include/target/target_core_fabric.h    |  6 +++---
 7 files changed, 31 insertions(+), 31 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index 59379d662626..1b453629b516 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -1158,7 +1158,7 @@ int iscsit_setup_scsi_cmd(struct iscsi_conn *conn, struct iscsi_cmd *cmd,
 	transport_init_se_cmd(&cmd->se_cmd, &iscsi_ops,
 			conn->sess->se_sess, be32_to_cpu(hdr->data_length),
 			cmd->data_direction, sam_task_attr,
-			cmd->sense_buffer + 2);
+			cmd->sense_buffer + 2, scsilun_to_int(&hdr->lun));
 
 	pr_debug("Got SCSI Command, ITT: 0x%08x, CmdSN: 0x%08x,"
 		" ExpXferLen: %u, Length: %u, CID: %hu\n", hdr->itt,
@@ -1167,8 +1167,7 @@ int iscsit_setup_scsi_cmd(struct iscsi_conn *conn, struct iscsi_cmd *cmd,
 
 	target_get_sess_cmd(&cmd->se_cmd, true);
 
-	cmd->sense_reason = transport_lookup_cmd_lun(&cmd->se_cmd,
-						     scsilun_to_int(&hdr->lun));
+	cmd->sense_reason = transport_lookup_cmd_lun(&cmd->se_cmd);
 	if (cmd->sense_reason)
 		goto attach_cmd;
 
@@ -2000,7 +1999,8 @@ static enum tcm_tmreq_table iscsit_convert_tmf(u8 iscsi_tmf)
 
 	transport_init_se_cmd(&cmd->se_cmd, &iscsi_ops,
 			      conn->sess->se_sess, 0, DMA_NONE,
-			      TCM_SIMPLE_TAG, cmd->sense_buffer + 2);
+			      TCM_SIMPLE_TAG, cmd->sense_buffer + 2,
+			      scsilun_to_int(&hdr->lun));
 
 	target_get_sess_cmd(&cmd->se_cmd, true);
 
@@ -2038,8 +2038,7 @@ static enum tcm_tmreq_table iscsit_convert_tmf(u8 iscsi_tmf)
 	 * Locate the struct se_lun for all TMRs not related to ERL=2 TASK_REASSIGN
 	 */
 	if (function != ISCSI_TM_FUNC_TASK_REASSIGN) {
-		ret = transport_lookup_tmr_lun(&cmd->se_cmd,
-					       scsilun_to_int(&hdr->lun));
+		ret = transport_lookup_tmr_lun(&cmd->se_cmd);
 		if (ret < 0) {
 			se_tmr->response = ISCSI_TMF_RSP_NO_LUN;
 			goto attach;
diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c
index 46b0e1ceb77f..405d82d44717 100644
--- a/drivers/target/target_core_device.c
+++ b/drivers/target/target_core_device.c
@@ -45,7 +45,7 @@
 struct se_device *g_lun0_dev;
 
 sense_reason_t
-transport_lookup_cmd_lun(struct se_cmd *se_cmd, u64 unpacked_lun)
+transport_lookup_cmd_lun(struct se_cmd *se_cmd)
 {
 	struct se_lun *se_lun = NULL;
 	struct se_session *se_sess = se_cmd->se_sess;
@@ -54,7 +54,7 @@
 	sense_reason_t ret = TCM_NO_SENSE;
 
 	rcu_read_lock();
-	deve = target_nacl_find_deve(nacl, unpacked_lun);
+	deve = target_nacl_find_deve(nacl, se_cmd->orig_fe_lun);
 	if (deve) {
 		atomic_long_inc(&deve->total_cmds);
 
@@ -74,7 +74,6 @@
 
 		se_cmd->se_lun = se_lun;
 		se_cmd->pr_res_key = deve->pr_res_key;
-		se_cmd->orig_fe_lun = unpacked_lun;
 		se_cmd->se_cmd_flags |= SCF_SE_LUN_CMD;
 		se_cmd->lun_ref_active = true;
 
@@ -83,7 +82,7 @@
 			pr_err("TARGET_CORE[%s]: Detected WRITE_PROTECTED LUN"
 				" Access for 0x%08llx\n",
 				se_cmd->se_tfo->fabric_name,
-				unpacked_lun);
+				se_cmd->orig_fe_lun);
 			rcu_read_unlock();
 			ret = TCM_WRITE_PROTECTED;
 			goto ref_dev;
@@ -98,18 +97,17 @@
 		 * REPORT_LUNS, et al to be returned when no active
 		 * MappedLUN=0 exists for this Initiator Port.
 		 */
-		if (unpacked_lun != 0) {
+		if (se_cmd->orig_fe_lun != 0) {
 			pr_err("TARGET_CORE[%s]: Detected NON_EXISTENT_LUN"
 				" Access for 0x%08llx from %s\n",
 				se_cmd->se_tfo->fabric_name,
-				unpacked_lun,
+				se_cmd->orig_fe_lun,
 				nacl->initiatorname);
 			return TCM_NON_EXISTENT_LUN;
 		}
 
 		se_lun = se_sess->se_tpg->tpg_virt_lun0;
 		se_cmd->se_lun = se_sess->se_tpg->tpg_virt_lun0;
-		se_cmd->orig_fe_lun = 0;
 		se_cmd->se_cmd_flags |= SCF_SE_LUN_CMD;
 
 		percpu_ref_get(&se_lun->lun_ref);
@@ -145,7 +143,7 @@
 }
 EXPORT_SYMBOL(transport_lookup_cmd_lun);
 
-int transport_lookup_tmr_lun(struct se_cmd *se_cmd, u64 unpacked_lun)
+int transport_lookup_tmr_lun(struct se_cmd *se_cmd)
 {
 	struct se_dev_entry *deve;
 	struct se_lun *se_lun = NULL;
@@ -155,7 +153,7 @@ int transport_lookup_tmr_lun(struct se_cmd *se_cmd, u64 unpacked_lun)
 	unsigned long flags;
 
 	rcu_read_lock();
-	deve = target_nacl_find_deve(nacl, unpacked_lun);
+	deve = target_nacl_find_deve(nacl, se_cmd->orig_fe_lun);
 	if (deve) {
 		se_lun = rcu_dereference(deve->se_lun);
 
@@ -166,7 +164,6 @@ int transport_lookup_tmr_lun(struct se_cmd *se_cmd, u64 unpacked_lun)
 
 		se_cmd->se_lun = se_lun;
 		se_cmd->pr_res_key = deve->pr_res_key;
-		se_cmd->orig_fe_lun = unpacked_lun;
 		se_cmd->se_cmd_flags |= SCF_SE_LUN_CMD;
 		se_cmd->lun_ref_active = true;
 	}
@@ -177,7 +174,7 @@ int transport_lookup_tmr_lun(struct se_cmd *se_cmd, u64 unpacked_lun)
 		pr_debug("TARGET_CORE[%s]: Detected NON_EXISTENT_LUN"
 			" Access for 0x%08llx for %s\n",
 			se_cmd->se_tfo->fabric_name,
-			unpacked_lun,
+			se_cmd->orig_fe_lun,
 			nacl->initiatorname);
 		return -ENODEV;
 	}
diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c
index afbd492c76a9..89c84d472cd7 100644
--- a/drivers/target/target_core_tmr.c
+++ b/drivers/target/target_core_tmr.c
@@ -148,8 +148,8 @@ void core_tmr_abort_task(
 		 * code.
 		 */
 		if (!tmr->tmr_dev)
-			WARN_ON_ONCE(transport_lookup_tmr_lun(tmr->task_cmd,
-						se_cmd->orig_fe_lun) < 0);
+			WARN_ON_ONCE(transport_lookup_tmr_lun(tmr->task_cmd) <
+					0);
 
 		target_put_cmd_and_wait(se_cmd);
 
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index f2f7c5b818cc..7ea77933e64d 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1364,7 +1364,7 @@ void transport_init_se_cmd(
 	u32 data_length,
 	int data_direction,
 	int task_attr,
-	unsigned char *sense_buffer)
+	unsigned char *sense_buffer, u64 unpacked_lun)
 {
 	INIT_LIST_HEAD(&cmd->se_delayed_node);
 	INIT_LIST_HEAD(&cmd->se_qf_node);
@@ -1383,6 +1383,7 @@ void transport_init_se_cmd(
 	cmd->data_direction = data_direction;
 	cmd->sam_task_attr = task_attr;
 	cmd->sense_buffer = sense_buffer;
+	cmd->orig_fe_lun = unpacked_lun;
 
 	cmd->state_active = false;
 }
@@ -1596,7 +1597,8 @@ int target_submit_cmd_map_sgls(struct se_cmd *se_cmd, struct se_session *se_sess
 	 * target_core_fabric_ops->queue_status() callback
 	 */
 	transport_init_se_cmd(se_cmd, se_tpg->se_tpg_tfo, se_sess,
-				data_length, data_dir, task_attr, sense);
+				data_length, data_dir, task_attr, sense,
+				unpacked_lun);
 
 	if (flags & TARGET_SCF_USE_CPUID)
 		se_cmd->se_cmd_flags |= SCF_USE_CPUID;
@@ -1622,7 +1624,7 @@ int target_submit_cmd_map_sgls(struct se_cmd *se_cmd, struct se_session *se_sess
 	/*
 	 * Locate se_lun pointer and attach it to struct se_cmd
 	 */
-	rc = transport_lookup_cmd_lun(se_cmd, unpacked_lun);
+	rc = transport_lookup_cmd_lun(se_cmd);
 	if (rc) {
 		transport_send_check_condition_and_sense(se_cmd, rc, 0);
 		target_put_sess_cmd(se_cmd);
@@ -1790,7 +1792,7 @@ int target_submit_tmr(struct se_cmd *se_cmd, struct se_session *se_sess,
 	BUG_ON(!se_tpg);
 
 	transport_init_se_cmd(se_cmd, se_tpg->se_tpg_tfo, se_sess,
-			      0, DMA_NONE, TCM_SIMPLE_TAG, sense);
+			      0, DMA_NONE, TCM_SIMPLE_TAG, sense, unpacked_lun);
 	/*
 	 * FIXME: Currently expect caller to handle se_cmd->se_tmr_req
 	 * allocation failure.
@@ -1818,7 +1820,7 @@ int target_submit_tmr(struct se_cmd *se_cmd, struct se_session *se_sess,
 			goto failure;
 	}
 
-	ret = transport_lookup_tmr_lun(se_cmd, unpacked_lun);
+	ret = transport_lookup_tmr_lun(se_cmd);
 	if (ret)
 		goto failure;
 
diff --git a/drivers/target/target_core_xcopy.c b/drivers/target/target_core_xcopy.c
index bd3ed6ce7571..5f1bab37a6bf 100644
--- a/drivers/target/target_core_xcopy.c
+++ b/drivers/target/target_core_xcopy.c
@@ -585,7 +585,7 @@ static int target_xcopy_read_source(
 		(unsigned long long)src_lba, src_sectors, length);
 
 	transport_init_se_cmd(se_cmd, &xcopy_pt_tfo, &xcopy_pt_sess, length,
-			      DMA_FROM_DEVICE, 0, &xpt_cmd.sense_buffer[0]);
+			      DMA_FROM_DEVICE, 0, &xpt_cmd.sense_buffer[0], 0);
 
 	rc = target_xcopy_setup_pt_cmd(&xpt_cmd, xop, src_dev, &cdb[0],
 				remote_port);
@@ -630,7 +630,7 @@ static int target_xcopy_write_destination(
 		(unsigned long long)dst_lba, dst_sectors, length);
 
 	transport_init_se_cmd(se_cmd, &xcopy_pt_tfo, &xcopy_pt_sess, length,
-			      DMA_TO_DEVICE, 0, &xpt_cmd.sense_buffer[0]);
+			      DMA_TO_DEVICE, 0, &xpt_cmd.sense_buffer[0], 0);
 
 	rc = target_xcopy_setup_pt_cmd(&xpt_cmd, xop, dst_dev, &cdb[0],
 				remote_port);
diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index 36504931b2d1..ac049909a8bf 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -1049,7 +1049,8 @@ static void usbg_cmd_work(struct work_struct *work)
 		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,
-				cmd->prio_attr, cmd->sense_iu.sense);
+				cmd->prio_attr, cmd->sense_iu.sense,
+				cmd->unpacked_lun);
 		goto out;
 	}
 
@@ -1179,7 +1180,8 @@ static void bot_cmd_work(struct work_struct *work)
 		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,
-				cmd->prio_attr, cmd->sense_iu.sense);
+				cmd->prio_attr, cmd->sense_iu.sense,
+				cmd->unpacked_lun);
 		goto out;
 	}
 
diff --git a/include/target/target_core_fabric.h b/include/target/target_core_fabric.h
index 6a2bfcca0c98..92f6cb20775e 100644
--- a/include/target/target_core_fabric.h
+++ b/include/target/target_core_fabric.h
@@ -150,8 +150,8 @@ void	transport_register_session(struct se_portal_group *,
 
 void	transport_init_se_cmd(struct se_cmd *,
 		const struct target_core_fabric_ops *,
-		struct se_session *, u32, int, int, unsigned char *);
-sense_reason_t transport_lookup_cmd_lun(struct se_cmd *, u64);
+		struct se_session *, u32, int, int, unsigned char *, u64);
+sense_reason_t transport_lookup_cmd_lun(struct se_cmd *);
 sense_reason_t target_cmd_init_cdb(struct se_cmd *, unsigned char *);
 sense_reason_t target_setup_cmd_from_cdb(struct se_cmd *, unsigned char *);
 int	target_submit_cmd_map_sgls(struct se_cmd *, struct se_session *,
@@ -188,7 +188,7 @@ int	transport_send_check_condition_and_sense(struct se_cmd *,
 void	core_tmr_release_req(struct se_tmr_req *);
 int	transport_generic_handle_tmr(struct se_cmd *);
 void	transport_generic_request_failure(struct se_cmd *, sense_reason_t);
-int	transport_lookup_tmr_lun(struct se_cmd *, u64);
+int	transport_lookup_tmr_lun(struct se_cmd *);
 void	core_allocate_nexus_loss_ua(struct se_node_acl *acl);
 
 struct se_node_acl *core_tpg_get_initiator_node_acl(struct se_portal_group *tpg,
-- 
1.8.3.1

[PATCH v4 3/4] target: fix NULL pointer dereference

[Sudhakar Panneerselvam]

NULL pointer dereference happens when the following conditions are met
1) A SCSI command is received for a non-existing LU or cdb
initialization fails in target_setup_cmd_from_cdb().
2) Tracing is enabled.

The following call sequences lead to NULL pointer dereference:

1) iscsit_setup_scsi_cmd
     transport_lookup_cmd_lun <-- lookup fails.
          or
     target_setup_cmd_from_cdb() <-- cdb initialization fails
   iscsit_process_scsi_cmd
     iscsit_sequence_cmd
       transport_send_check_condition_and_sense
         trace_target_cmd_complete <-- NULL dereference

2) target_submit_cmd_map_sgls
     transport_lookup_cmd_lun <-- lookup fails
          or
     target_setup_cmd_from_cdb() <-- cdb initialization fails
       transport_send_check_condition_and_sense
         trace_target_cmd_complete <-- NULL dereference

In the above sequence, cmd->t_task_cdb is uninitialized which when
referenced in trace_target_cmd_complete() causes NULL pointer dereference.

The fix is to use the helper, target_cmd_init_cdb() and call it after
transport_init_se_cmd() is called, so that cmd->t_task_cdb can
be initialized and hence can be referenced in trace_target_cmd_complete().

Signed-off-by: Sudhakar Panneerselvam <sudhakar.panneerselvam@oracle.com>
---
 drivers/target/iscsi/iscsi_target.c    | 18 +++++++++++-------
 drivers/target/target_core_transport.c | 31 +++++++++++++++++++++++++------
 drivers/target/target_core_xcopy.c     |  3 +++
 3 files changed, 39 insertions(+), 13 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index 1b453629b516..93534dcc66c9 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -1167,6 +1167,16 @@ int iscsit_setup_scsi_cmd(struct iscsi_conn *conn, struct iscsi_cmd *cmd,
 
 	target_get_sess_cmd(&cmd->se_cmd, true);
 
+	cmd->sense_reason = target_cmd_init_cdb(&cmd->se_cmd, hdr->cdb);
+	if (cmd->sense_reason) {
+		if (cmd->sense_reason = TCM_OUT_OF_RESOURCES) {
+			return iscsit_add_reject_cmd(cmd,
+				ISCSI_REASON_BOOKMARK_NO_RESOURCES, buf);
+		}
+
+		goto attach_cmd;
+	}
+
 	cmd->sense_reason = transport_lookup_cmd_lun(&cmd->se_cmd);
 	if (cmd->sense_reason)
 		goto attach_cmd;
@@ -1174,14 +1184,8 @@ int iscsit_setup_scsi_cmd(struct iscsi_conn *conn, struct iscsi_cmd *cmd,
 	/* only used for printks or comparing with ->ref_task_tag */
 	cmd->se_cmd.tag = (__force u32)cmd->init_task_tag;
 	cmd->sense_reason = target_setup_cmd_from_cdb(&cmd->se_cmd, hdr->cdb);
-	if (cmd->sense_reason) {
-		if (cmd->sense_reason = TCM_OUT_OF_RESOURCES) {
-			return iscsit_add_reject_cmd(cmd,
-					ISCSI_REASON_BOOKMARK_NO_RESOURCES, buf);
-		}
-
+	if (cmd->sense_reason)
 		goto attach_cmd;
-	}
 
 	if (iscsit_build_pdu_and_seq_lists(cmd, payload_length) < 0) {
 		return iscsit_add_reject_cmd(cmd,
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 7ea77933e64d..0fbb38254535 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1413,6 +1413,9 @@ void transport_init_se_cmd(
 sense_reason_t
 target_cmd_init_cdb(struct se_cmd *cmd, unsigned char *cdb)
 {
+	sense_reason_t ret;
+
+	cmd->t_task_cdb = &cmd->__t_task_cdb[0];
 	/*
 	 * Ensure that the received CDB is less than the max (252 + 8) bytes
 	 * for VARIABLE_LENGTH_CMD
@@ -1421,7 +1424,8 @@ void transport_init_se_cmd(
 		pr_err("Received SCSI CDB with command_size: %d that"
 			" exceeds SCSI_MAX_VARLEN_CDB_SIZE: %d\n",
 			scsi_command_size(cdb), SCSI_MAX_VARLEN_CDB_SIZE);
-		return TCM_INVALID_CDB_FIELD;
+		ret = TCM_INVALID_CDB_FIELD;
+		goto err;
 	}
 	/*
 	 * If the received CDB is larger than TCM_MAX_COMMAND_SIZE,
@@ -1436,10 +1440,10 @@ void transport_init_se_cmd(
 				" %u > sizeof(cmd->__t_task_cdb): %lu ops\n",
 				scsi_command_size(cdb),
 				(unsigned long)sizeof(cmd->__t_task_cdb));
-			return TCM_OUT_OF_RESOURCES;
+			ret = TCM_OUT_OF_RESOURCES;
+			goto err;
 		}
-	} else
-		cmd->t_task_cdb = &cmd->__t_task_cdb[0];
+	}
 	/*
 	 * Copy the original CDB into cmd->
 	 */
@@ -1447,6 +1451,15 @@ void transport_init_se_cmd(
 
 	trace_target_sequencer_start(cmd);
 	return 0;
+
+err:
+	/*
+	 * Copy the CDB here to allow trace_target_cmd_complete() to
+	 * print the cdb to the trace buffers.
+	 */
+	memcpy(cmd->t_task_cdb, cdb, min(scsi_command_size(cdb),
+					 (unsigned int)TCM_MAX_COMMAND_SIZE));
+	return ret;
 }
 EXPORT_SYMBOL(target_cmd_init_cdb);
 
@@ -1456,8 +1469,6 @@ void transport_init_se_cmd(
 	struct se_device *dev = cmd->se_dev;
 	sense_reason_t ret;
 
-	target_cmd_init_cdb(cmd, cdb);
-
 	ret = dev->transport->parse_cdb(cmd);
 	if (ret = TCM_UNSUPPORTED_SCSI_OPCODE)
 		pr_warn_ratelimited("%s/%s: Unsupported SCSI Opcode 0x%02x, sending CHECK_CONDITION.\n",
@@ -1621,6 +1632,14 @@ int target_submit_cmd_map_sgls(struct se_cmd *se_cmd, struct se_session *se_sess
 	 */
 	if (flags & TARGET_SCF_BIDI_OP)
 		se_cmd->se_cmd_flags |= SCF_BIDI;
+
+	rc = target_cmd_init_cdb(se_cmd, cdb);
+	if (rc) {
+		transport_send_check_condition_and_sense(se_cmd, rc, 0);
+		target_put_sess_cmd(se_cmd);
+		return 0;
+	}
+
 	/*
 	 * Locate se_lun pointer and attach it to struct se_cmd
 	 */
diff --git a/drivers/target/target_core_xcopy.c b/drivers/target/target_core_xcopy.c
index 5f1bab37a6bf..7f9c93b5cba6 100644
--- a/drivers/target/target_core_xcopy.c
+++ b/drivers/target/target_core_xcopy.c
@@ -526,6 +526,9 @@ static int target_xcopy_setup_pt_cmd(
 	}
 	cmd->se_cmd_flags |= SCF_SE_LUN_CMD;
 
+	if (target_cmd_init_cdb(cmd, cdb))
+		return -EINVAL;
+
 	cmd->tag = 0;
 	if (target_setup_cmd_from_cdb(cmd, cdb))
 		return -EINVAL;
-- 
1.8.3.1

[PATCH v4 3/4] target: fix NULL pointer dereference

[Sudhakar Panneerselvam]

NULL pointer dereference happens when the following conditions are met
1) A SCSI command is received for a non-existing LU or cdb
initialization fails in target_setup_cmd_from_cdb().
2) Tracing is enabled.

The following call sequences lead to NULL pointer dereference:

1) iscsit_setup_scsi_cmd
     transport_lookup_cmd_lun <-- lookup fails.
          or
     target_setup_cmd_from_cdb() <-- cdb initialization fails
   iscsit_process_scsi_cmd
     iscsit_sequence_cmd
       transport_send_check_condition_and_sense
         trace_target_cmd_complete <-- NULL dereference

2) target_submit_cmd_map_sgls
     transport_lookup_cmd_lun <-- lookup fails
          or
     target_setup_cmd_from_cdb() <-- cdb initialization fails
       transport_send_check_condition_and_sense
         trace_target_cmd_complete <-- NULL dereference

In the above sequence, cmd->t_task_cdb is uninitialized which when
referenced in trace_target_cmd_complete() causes NULL pointer dereference.

The fix is to use the helper, target_cmd_init_cdb() and call it after
transport_init_se_cmd() is called, so that cmd->t_task_cdb can
be initialized and hence can be referenced in trace_target_cmd_complete().

Signed-off-by: Sudhakar Panneerselvam <sudhakar.panneerselvam@oracle.com>
---
 drivers/target/iscsi/iscsi_target.c    | 18 +++++++++++-------
 drivers/target/target_core_transport.c | 31 +++++++++++++++++++++++++------
 drivers/target/target_core_xcopy.c     |  3 +++
 3 files changed, 39 insertions(+), 13 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index 1b453629b516..93534dcc66c9 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -1167,6 +1167,16 @@ int iscsit_setup_scsi_cmd(struct iscsi_conn *conn, struct iscsi_cmd *cmd,
 
 	target_get_sess_cmd(&cmd->se_cmd, true);
 
+	cmd->sense_reason = target_cmd_init_cdb(&cmd->se_cmd, hdr->cdb);
+	if (cmd->sense_reason) {
+		if (cmd->sense_reason == TCM_OUT_OF_RESOURCES) {
+			return iscsit_add_reject_cmd(cmd,
+				ISCSI_REASON_BOOKMARK_NO_RESOURCES, buf);
+		}
+
+		goto attach_cmd;
+	}
+
 	cmd->sense_reason = transport_lookup_cmd_lun(&cmd->se_cmd);
 	if (cmd->sense_reason)
 		goto attach_cmd;
@@ -1174,14 +1184,8 @@ int iscsit_setup_scsi_cmd(struct iscsi_conn *conn, struct iscsi_cmd *cmd,
 	/* only used for printks or comparing with ->ref_task_tag */
 	cmd->se_cmd.tag = (__force u32)cmd->init_task_tag;
 	cmd->sense_reason = target_setup_cmd_from_cdb(&cmd->se_cmd, hdr->cdb);
-	if (cmd->sense_reason) {
-		if (cmd->sense_reason == TCM_OUT_OF_RESOURCES) {
-			return iscsit_add_reject_cmd(cmd,
-					ISCSI_REASON_BOOKMARK_NO_RESOURCES, buf);
-		}
-
+	if (cmd->sense_reason)
 		goto attach_cmd;
-	}
 
 	if (iscsit_build_pdu_and_seq_lists(cmd, payload_length) < 0) {
 		return iscsit_add_reject_cmd(cmd,
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 7ea77933e64d..0fbb38254535 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1413,6 +1413,9 @@ void transport_init_se_cmd(
 sense_reason_t
 target_cmd_init_cdb(struct se_cmd *cmd, unsigned char *cdb)
 {
+	sense_reason_t ret;
+
+	cmd->t_task_cdb = &cmd->__t_task_cdb[0];
 	/*
 	 * Ensure that the received CDB is less than the max (252 + 8) bytes
 	 * for VARIABLE_LENGTH_CMD
@@ -1421,7 +1424,8 @@ void transport_init_se_cmd(
 		pr_err("Received SCSI CDB with command_size: %d that"
 			" exceeds SCSI_MAX_VARLEN_CDB_SIZE: %d\n",
 			scsi_command_size(cdb), SCSI_MAX_VARLEN_CDB_SIZE);
-		return TCM_INVALID_CDB_FIELD;
+		ret = TCM_INVALID_CDB_FIELD;
+		goto err;
 	}
 	/*
 	 * If the received CDB is larger than TCM_MAX_COMMAND_SIZE,
@@ -1436,10 +1440,10 @@ void transport_init_se_cmd(
 				" %u > sizeof(cmd->__t_task_cdb): %lu ops\n",
 				scsi_command_size(cdb),
 				(unsigned long)sizeof(cmd->__t_task_cdb));
-			return TCM_OUT_OF_RESOURCES;
+			ret = TCM_OUT_OF_RESOURCES;
+			goto err;
 		}
-	} else
-		cmd->t_task_cdb = &cmd->__t_task_cdb[0];
+	}
 	/*
 	 * Copy the original CDB into cmd->
 	 */
@@ -1447,6 +1451,15 @@ void transport_init_se_cmd(
 
 	trace_target_sequencer_start(cmd);
 	return 0;
+
+err:
+	/*
+	 * Copy the CDB here to allow trace_target_cmd_complete() to
+	 * print the cdb to the trace buffers.
+	 */
+	memcpy(cmd->t_task_cdb, cdb, min(scsi_command_size(cdb),
+					 (unsigned int)TCM_MAX_COMMAND_SIZE));
+	return ret;
 }
 EXPORT_SYMBOL(target_cmd_init_cdb);
 
@@ -1456,8 +1469,6 @@ void transport_init_se_cmd(
 	struct se_device *dev = cmd->se_dev;
 	sense_reason_t ret;
 
-	target_cmd_init_cdb(cmd, cdb);
-
 	ret = dev->transport->parse_cdb(cmd);
 	if (ret == TCM_UNSUPPORTED_SCSI_OPCODE)
 		pr_warn_ratelimited("%s/%s: Unsupported SCSI Opcode 0x%02x, sending CHECK_CONDITION.\n",
@@ -1621,6 +1632,14 @@ int target_submit_cmd_map_sgls(struct se_cmd *se_cmd, struct se_session *se_sess
 	 */
 	if (flags & TARGET_SCF_BIDI_OP)
 		se_cmd->se_cmd_flags |= SCF_BIDI;
+
+	rc = target_cmd_init_cdb(se_cmd, cdb);
+	if (rc) {
+		transport_send_check_condition_and_sense(se_cmd, rc, 0);
+		target_put_sess_cmd(se_cmd);
+		return 0;
+	}
+
 	/*
 	 * Locate se_lun pointer and attach it to struct se_cmd
 	 */
diff --git a/drivers/target/target_core_xcopy.c b/drivers/target/target_core_xcopy.c
index 5f1bab37a6bf..7f9c93b5cba6 100644
--- a/drivers/target/target_core_xcopy.c
+++ b/drivers/target/target_core_xcopy.c
@@ -526,6 +526,9 @@ static int target_xcopy_setup_pt_cmd(
 	}
 	cmd->se_cmd_flags |= SCF_SE_LUN_CMD;
 
+	if (target_cmd_init_cdb(cmd, cdb))
+		return -EINVAL;
+
 	cmd->tag = 0;
 	if (target_setup_cmd_from_cdb(cmd, cdb))
 		return -EINVAL;
-- 
1.8.3.1

[PATCH v4 4/4] target: rename target_setup_cmd_from_cdb() to target_cmd_parse_cdb()

[Sudhakar Panneerselvam]

This commit also removes the unused argument, cdb that was passed
to this function.

Signed-off-by: Sudhakar Panneerselvam <sudhakar.panneerselvam@oracle.com>
---
 drivers/target/iscsi/iscsi_target.c    | 2 +-
 drivers/target/target_core_transport.c | 6 +++---
 drivers/target/target_core_xcopy.c     | 2 +-
 include/target/target_core_fabric.h    | 2 +-
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index 93534dcc66c9..c9689610e186 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -1183,7 +1183,7 @@ int iscsit_setup_scsi_cmd(struct iscsi_conn *conn, struct iscsi_cmd *cmd,
 
 	/* only used for printks or comparing with ->ref_task_tag */
 	cmd->se_cmd.tag = (__force u32)cmd->init_task_tag;
-	cmd->sense_reason = target_setup_cmd_from_cdb(&cmd->se_cmd, hdr->cdb);
+	cmd->sense_reason = target_cmd_parse_cdb(&cmd->se_cmd);
 	if (cmd->sense_reason)
 		goto attach_cmd;
 
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 0fbb38254535..229407d81613 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1464,7 +1464,7 @@ void transport_init_se_cmd(
 EXPORT_SYMBOL(target_cmd_init_cdb);
 
 sense_reason_t
-target_setup_cmd_from_cdb(struct se_cmd *cmd, unsigned char *cdb)
+target_cmd_parse_cdb(struct se_cmd *cmd)
 {
 	struct se_device *dev = cmd->se_dev;
 	sense_reason_t ret;
@@ -1486,7 +1486,7 @@ void transport_init_se_cmd(
 	atomic_long_inc(&cmd->se_lun->lun_stats.cmd_pdus);
 	return 0;
 }
-EXPORT_SYMBOL(target_setup_cmd_from_cdb);
+EXPORT_SYMBOL(target_cmd_parse_cdb);
 
 /*
  * Used by fabric module frontends to queue tasks directly.
@@ -1650,7 +1650,7 @@ int target_submit_cmd_map_sgls(struct se_cmd *se_cmd, struct se_session *se_sess
 		return 0;
 	}
 
-	rc = target_setup_cmd_from_cdb(se_cmd, cdb);
+	rc = target_cmd_parse_cdb(se_cmd);
 	if (rc != 0) {
 		transport_generic_request_failure(se_cmd, rc);
 		return 0;
diff --git a/drivers/target/target_core_xcopy.c b/drivers/target/target_core_xcopy.c
index 7f9c93b5cba6..0d00ccbeb050 100644
--- a/drivers/target/target_core_xcopy.c
+++ b/drivers/target/target_core_xcopy.c
@@ -530,7 +530,7 @@ static int target_xcopy_setup_pt_cmd(
 		return -EINVAL;
 
 	cmd->tag = 0;
-	if (target_setup_cmd_from_cdb(cmd, cdb))
+	if (target_cmd_parse_cdb(cmd))
 		return -EINVAL;
 
 	if (transport_generic_map_mem_to_cmd(cmd, xop->xop_data_sg,
diff --git a/include/target/target_core_fabric.h b/include/target/target_core_fabric.h
index 92f6cb20775e..6adf4d71acf6 100644
--- a/include/target/target_core_fabric.h
+++ b/include/target/target_core_fabric.h
@@ -153,7 +153,7 @@ void	transport_init_se_cmd(struct se_cmd *,
 		struct se_session *, u32, int, int, unsigned char *, u64);
 sense_reason_t transport_lookup_cmd_lun(struct se_cmd *);
 sense_reason_t target_cmd_init_cdb(struct se_cmd *, unsigned char *);
-sense_reason_t target_setup_cmd_from_cdb(struct se_cmd *, unsigned char *);
+sense_reason_t target_cmd_parse_cdb(struct se_cmd *);
 int	target_submit_cmd_map_sgls(struct se_cmd *, struct se_session *,
 		unsigned char *, unsigned char *, u64, u32, int, int, int,
 		struct scatterlist *, u32, struct scatterlist *, u32,
-- 
1.8.3.1

[PATCH v4 4/4] target: rename target_setup_cmd_from_cdb() to target_cmd_parse_cdb()

[Sudhakar Panneerselvam]

This commit also removes the unused argument, cdb that was passed
to this function.

Signed-off-by: Sudhakar Panneerselvam <sudhakar.panneerselvam@oracle.com>
---
 drivers/target/iscsi/iscsi_target.c    | 2 +-
 drivers/target/target_core_transport.c | 6 +++---
 drivers/target/target_core_xcopy.c     | 2 +-
 include/target/target_core_fabric.h    | 2 +-
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index 93534dcc66c9..c9689610e186 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -1183,7 +1183,7 @@ int iscsit_setup_scsi_cmd(struct iscsi_conn *conn, struct iscsi_cmd *cmd,
 
 	/* only used for printks or comparing with ->ref_task_tag */
 	cmd->se_cmd.tag = (__force u32)cmd->init_task_tag;
-	cmd->sense_reason = target_setup_cmd_from_cdb(&cmd->se_cmd, hdr->cdb);
+	cmd->sense_reason = target_cmd_parse_cdb(&cmd->se_cmd);
 	if (cmd->sense_reason)
 		goto attach_cmd;
 
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 0fbb38254535..229407d81613 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1464,7 +1464,7 @@ void transport_init_se_cmd(
 EXPORT_SYMBOL(target_cmd_init_cdb);
 
 sense_reason_t
-target_setup_cmd_from_cdb(struct se_cmd *cmd, unsigned char *cdb)
+target_cmd_parse_cdb(struct se_cmd *cmd)
 {
 	struct se_device *dev = cmd->se_dev;
 	sense_reason_t ret;
@@ -1486,7 +1486,7 @@ void transport_init_se_cmd(
 	atomic_long_inc(&cmd->se_lun->lun_stats.cmd_pdus);
 	return 0;
 }
-EXPORT_SYMBOL(target_setup_cmd_from_cdb);
+EXPORT_SYMBOL(target_cmd_parse_cdb);
 
 /*
  * Used by fabric module frontends to queue tasks directly.
@@ -1650,7 +1650,7 @@ int target_submit_cmd_map_sgls(struct se_cmd *se_cmd, struct se_session *se_sess
 		return 0;
 	}
 
-	rc = target_setup_cmd_from_cdb(se_cmd, cdb);
+	rc = target_cmd_parse_cdb(se_cmd);
 	if (rc != 0) {
 		transport_generic_request_failure(se_cmd, rc);
 		return 0;
diff --git a/drivers/target/target_core_xcopy.c b/drivers/target/target_core_xcopy.c
index 7f9c93b5cba6..0d00ccbeb050 100644
--- a/drivers/target/target_core_xcopy.c
+++ b/drivers/target/target_core_xcopy.c
@@ -530,7 +530,7 @@ static int target_xcopy_setup_pt_cmd(
 		return -EINVAL;
 
 	cmd->tag = 0;
-	if (target_setup_cmd_from_cdb(cmd, cdb))
+	if (target_cmd_parse_cdb(cmd))
 		return -EINVAL;
 
 	if (transport_generic_map_mem_to_cmd(cmd, xop->xop_data_sg,
diff --git a/include/target/target_core_fabric.h b/include/target/target_core_fabric.h
index 92f6cb20775e..6adf4d71acf6 100644
--- a/include/target/target_core_fabric.h
+++ b/include/target/target_core_fabric.h
@@ -153,7 +153,7 @@ void	transport_init_se_cmd(struct se_cmd *,
 		struct se_session *, u32, int, int, unsigned char *, u64);
 sense_reason_t transport_lookup_cmd_lun(struct se_cmd *);
 sense_reason_t target_cmd_init_cdb(struct se_cmd *, unsigned char *);
-sense_reason_t target_setup_cmd_from_cdb(struct se_cmd *, unsigned char *);
+sense_reason_t target_cmd_parse_cdb(struct se_cmd *);
 int	target_submit_cmd_map_sgls(struct se_cmd *, struct se_session *,
 		unsigned char *, unsigned char *, u64, u32, int, int, int,
 		struct scatterlist *, u32, struct scatterlist *, u32,
-- 
1.8.3.1

Re: [PATCH v4 0/4] target: fix NULL pointer dereference

[Mike Christie]

On 6/7/20 2:58 PM, Sudhakar Panneerselvam wrote:
> The following set of commits address a NULL pointer dereference and some
> refactoring around this issue.
> 
> v4:
>   - initialize the LUN in transport_init_se_cmd()
> 
> v3:
>   - fix NULL pointer dereference when cdb initialization fails
> 
> v2:
>   - new helper is named as target_cmd_init_cdb()
>   - existing function, target_setup_cmd_from_cdb is renamed as
>     target_cmd_parse_cdb()
> 
> Sudhakar Panneerselvam (4):
>    target: factor out a new helper, target_cmd_init_cdb()
>    target: initialize LUN in transport_init_se_cmd().
>    target: fix NULL pointer dereference
>    target: rename target_setup_cmd_from_cdb() to target_cmd_parse_cdb()
> 
>   drivers/target/iscsi/iscsi_target.c    | 29 ++++++++++--------
>   drivers/target/target_core_device.c    | 19 +++++-------
>   drivers/target/target_core_tmr.c       |  4 +--
>   drivers/target/target_core_transport.c | 55 ++++++++++++++++++++++++++--------
>   drivers/target/target_core_xcopy.c     |  9 ++++--
>   drivers/usb/gadget/function/f_tcm.c    |  6 ++--
>   include/target/target_core_fabric.h    |  9 +++---
>   7 files changed, 83 insertions(+), 48 deletions(-)
> 

Reviewed-by: Mike Christie <michael.christie@oracle.com>

Re: [PATCH v4 0/4] target: fix NULL pointer dereference

[Mike Christie]

On 6/7/20 2:58 PM, Sudhakar Panneerselvam wrote:
> The following set of commits address a NULL pointer dereference and some
> refactoring around this issue.
> 
> v4:
>   - initialize the LUN in transport_init_se_cmd()
> 
> v3:
>   - fix NULL pointer dereference when cdb initialization fails
> 
> v2:
>   - new helper is named as target_cmd_init_cdb()
>   - existing function, target_setup_cmd_from_cdb is renamed as
>     target_cmd_parse_cdb()
> 
> Sudhakar Panneerselvam (4):
>    target: factor out a new helper, target_cmd_init_cdb()
>    target: initialize LUN in transport_init_se_cmd().
>    target: fix NULL pointer dereference
>    target: rename target_setup_cmd_from_cdb() to target_cmd_parse_cdb()
> 
>   drivers/target/iscsi/iscsi_target.c    | 29 ++++++++++--------
>   drivers/target/target_core_device.c    | 19 +++++-------
>   drivers/target/target_core_tmr.c       |  4 +--
>   drivers/target/target_core_transport.c | 55 ++++++++++++++++++++++++++--------
>   drivers/target/target_core_xcopy.c     |  9 ++++--
>   drivers/usb/gadget/function/f_tcm.c    |  6 ++--
>   include/target/target_core_fabric.h    |  9 +++---
>   7 files changed, 83 insertions(+), 48 deletions(-)
> 

Reviewed-by: Mike Christie <michael.christie@oracle.com>

Re: [PATCH v4 0/4] target: fix NULL pointer dereference

[Martin K. Petersen]

On Sun, 7 Jun 2020 19:58:29 +0000, Sudhakar Panneerselvam wrote:

> The following set of commits address a NULL pointer dereference and some
> refactoring around this issue.
> 
> v4:
>  - initialize the LUN in transport_init_se_cmd()
> 
> v3:
>  - fix NULL pointer dereference when cdb initialization fails
> 
> [...]

Applied to 5.8/scsi-queue, thanks!

[1/4] scsi: target: Factor out a new helper, target_cmd_init_cdb()
      https://git.kernel.org/mkp/scsi/c/f98c2ddf8ba3
[2/4] scsi: target: Initialize LUN in transport_init_se_cmd()
      https://git.kernel.org/mkp/scsi/c/a36840d80027
[3/4] scsi: target: Fix NULL pointer dereference
      https://git.kernel.org/mkp/scsi/c/9e95fb805dc0
[4/4] scsi: target: Rename target_setup_cmd_from_cdb() to target_cmd_parse_cdb()
      https://git.kernel.org/mkp/scsi/c/987db58737e2

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v4 0/4] target: fix NULL pointer dereference

[Martin K. Petersen]

On Sun, 7 Jun 2020 19:58:29 +0000, Sudhakar Panneerselvam wrote:

> The following set of commits address a NULL pointer dereference and some
> refactoring around this issue.
> 
> v4:
>  - initialize the LUN in transport_init_se_cmd()
> 
> v3:
>  - fix NULL pointer dereference when cdb initialization fails
> 
> [...]

Applied to 5.8/scsi-queue, thanks!

[1/4] scsi: target: Factor out a new helper, target_cmd_init_cdb()
      https://git.kernel.org/mkp/scsi/c/f98c2ddf8ba3
[2/4] scsi: target: Initialize LUN in transport_init_se_cmd()
      https://git.kernel.org/mkp/scsi/c/a36840d80027
[3/4] scsi: target: Fix NULL pointer dereference
      https://git.kernel.org/mkp/scsi/c/9e95fb805dc0
[4/4] scsi: target: Rename target_setup_cmd_from_cdb() to target_cmd_parse_cdb()
      https://git.kernel.org/mkp/scsi/c/987db58737e2

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v4 2/4] target: initialize LUN in transport_init_se_cmd().

[Martin Wilck]

Hello Sudhakar,

On Sun, 2020-06-07 at 19:58 +0000, Sudhakar Panneerselvam wrote:
> Initialization of orig_fe_lun is moved to transport_init_se_cmd()
> from
> transport_lookup_cmd_lun(). This helps for the cases where the scsi
> request
> fails before the call to transport_lookup_cmd_lun() so that
> trace_target_cmd_complete() can print the LUN information to the
> trace
> buffer. Due to this change, the lun parameter is removed from
> transport_lookup_cmd_lun() and transport_lookup_tmr_lun().
> 
> Signed-off-by: Sudhakar Panneerselvam <
> sudhakar.panneerselvam@oracle.com>
> ---
>  drivers/target/iscsi/iscsi_target.c    | 11 +++++------
>  drivers/target/target_core_device.c    | 19 ++++++++-----------
>  drivers/target/target_core_tmr.c       |  4 ++--
>  drivers/target/target_core_transport.c | 12 +++++++-----
>  drivers/target/target_core_xcopy.c     |  4 ++--
>  drivers/usb/gadget/function/f_tcm.c    |  6 ++++--
>  include/target/target_core_fabric.h    |  6 +++---
>  7 files changed, 31 insertions(+), 31 deletions(-)
> 
> [...]
> diff --git a/drivers/target/target_core_transport.c
> b/drivers/target/target_core_transport.c
> index f2f7c5b818cc..7ea77933e64d 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> [...]
> @@ -1790,7 +1792,7 @@ int target_submit_tmr(struct se_cmd *se_cmd,
> struct se_session *se_sess,
>  	BUG_ON(!se_tpg);
>  
>  	transport_init_se_cmd(se_cmd, se_tpg->se_tpg_tfo, se_sess,
> -			      0, DMA_NONE, TCM_SIMPLE_TAG, sense);
> +			      0, DMA_NONE, TCM_SIMPLE_TAG, sense,
> unpacked_lun);
>  	/*
>  	 * FIXME: Currently expect caller to handle se_cmd->se_tmr_req
>  	 * allocation failure.

Between this hunk and the next one in target_submit_tmr(), there
is this code:

        /*
         * If this is ABORT_TASK with no explicit fabric provided LUN,
         * go ahead and search active session tags for a match to figure
         * out unpacked_lun for the original se_cmd.
         */
        if (tm_type = TMR_ABORT_TASK && (flags & TARGET_SCF_LOOKUP_LUN_FROM_TAG)) {
                if (!target_lookup_lun_from_tag(se_sess, tag, &unpacked_lun))
                        goto failure;
        }

> @@ -1818,7 +1820,7 @@ int target_submit_tmr(struct se_cmd *se_cmd,
> struct se_session *se_sess,
>  			goto failure;
>  	}
>  
> -	ret = transport_lookup_tmr_lun(se_cmd, unpacked_lun);
> +	ret = transport_lookup_tmr_lun(se_cmd);
>  	if (ret)
>  		goto failure;
>  

AFAICS, your patch breaks the case where the above code is executed to
derive unpacked_lun from the tag. The updated value of unpacked_lun is 
never used. That would break aborts for the qla2xxx target.

Am I overlooking something? Can you please explain?

Regards
Martin

Re: [PATCH v4 2/4] target: initialize LUN in transport_init_se_cmd().

[Martin Wilck]

Hello Sudhakar,

On Sun, 2020-06-07 at 19:58 +0000, Sudhakar Panneerselvam wrote:
> Initialization of orig_fe_lun is moved to transport_init_se_cmd()
> from
> transport_lookup_cmd_lun(). This helps for the cases where the scsi
> request
> fails before the call to transport_lookup_cmd_lun() so that
> trace_target_cmd_complete() can print the LUN information to the
> trace
> buffer. Due to this change, the lun parameter is removed from
> transport_lookup_cmd_lun() and transport_lookup_tmr_lun().
> 
> Signed-off-by: Sudhakar Panneerselvam <
> sudhakar.panneerselvam@oracle.com>
> ---
>  drivers/target/iscsi/iscsi_target.c    | 11 +++++------
>  drivers/target/target_core_device.c    | 19 ++++++++-----------
>  drivers/target/target_core_tmr.c       |  4 ++--
>  drivers/target/target_core_transport.c | 12 +++++++-----
>  drivers/target/target_core_xcopy.c     |  4 ++--
>  drivers/usb/gadget/function/f_tcm.c    |  6 ++++--
>  include/target/target_core_fabric.h    |  6 +++---
>  7 files changed, 31 insertions(+), 31 deletions(-)
> 
> [...]
> diff --git a/drivers/target/target_core_transport.c
> b/drivers/target/target_core_transport.c
> index f2f7c5b818cc..7ea77933e64d 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> [...]
> @@ -1790,7 +1792,7 @@ int target_submit_tmr(struct se_cmd *se_cmd,
> struct se_session *se_sess,
>  	BUG_ON(!se_tpg);
>  
>  	transport_init_se_cmd(se_cmd, se_tpg->se_tpg_tfo, se_sess,
> -			      0, DMA_NONE, TCM_SIMPLE_TAG, sense);
> +			      0, DMA_NONE, TCM_SIMPLE_TAG, sense,
> unpacked_lun);
>  	/*
>  	 * FIXME: Currently expect caller to handle se_cmd->se_tmr_req
>  	 * allocation failure.

Between this hunk and the next one in target_submit_tmr(), there
is this code:

        /*
         * If this is ABORT_TASK with no explicit fabric provided LUN,
         * go ahead and search active session tags for a match to figure
         * out unpacked_lun for the original se_cmd.
         */
        if (tm_type == TMR_ABORT_TASK && (flags & TARGET_SCF_LOOKUP_LUN_FROM_TAG)) {
                if (!target_lookup_lun_from_tag(se_sess, tag, &unpacked_lun))
                        goto failure;
        }

> @@ -1818,7 +1820,7 @@ int target_submit_tmr(struct se_cmd *se_cmd,
> struct se_session *se_sess,
>  			goto failure;
>  	}
>  
> -	ret = transport_lookup_tmr_lun(se_cmd, unpacked_lun);
> +	ret = transport_lookup_tmr_lun(se_cmd);
>  	if (ret)
>  		goto failure;
>  

AFAICS, your patch breaks the case where the above code is executed to
derive unpacked_lun from the tag. The updated value of unpacked_lun is 
never used. That would break aborts for the qla2xxx target.

Am I overlooking something? Can you please explain?

Regards
Martin

RE: [PATCH v4 2/4] target: initialize LUN in transport_init_se_cmd().

[Sudhakar Panneerselvam]

Hi Martin,

> -----Original Message-----
> From: Martin Wilck [mailto:mwilck@suse.com]
> Sent: Wednesday, September 2, 2020 7:50 AM
> To: Sudhakar Panneerselvam <sudhakar.panneerselvam@oracle.com>; Martin
> Petersen <martin.petersen@oracle.com>; Michael Christie
> <michael.christie@oracle.com>; target-devel@vger.kernel.org; linux-
> scsi@vger.kernel.org
> Cc: Shirley Ma <shirley.ma@oracle.com>; Hannes Reinecke <hare@suse.com>;
> Daniel Wagner <daniel.wagner@suse.com>; Arun Easi <aeasi@marvell.com>
> Subject: Re: [PATCH v4 2/4] target: initialize LUN in transport_init_se_cmd().
> 
> Hello Sudhakar,
> 
> On Sun, 2020-06-07 at 19:58 +0000, Sudhakar Panneerselvam wrote:
> > Initialization of orig_fe_lun is moved to transport_init_se_cmd()
> > from
> > transport_lookup_cmd_lun(). This helps for the cases where the scsi
> > request
> > fails before the call to transport_lookup_cmd_lun() so that
> > trace_target_cmd_complete() can print the LUN information to the
> > trace
> > buffer. Due to this change, the lun parameter is removed from
> > transport_lookup_cmd_lun() and transport_lookup_tmr_lun().
> >
> > Signed-off-by: Sudhakar Panneerselvam <
> > sudhakar.panneerselvam@oracle.com>
> > ---
> >  drivers/target/iscsi/iscsi_target.c    | 11 +++++------
> >  drivers/target/target_core_device.c    | 19 ++++++++-----------
> >  drivers/target/target_core_tmr.c       |  4 ++--
> >  drivers/target/target_core_transport.c | 12 +++++++-----
> >  drivers/target/target_core_xcopy.c     |  4 ++--
> >  drivers/usb/gadget/function/f_tcm.c    |  6 ++++--
> >  include/target/target_core_fabric.h    |  6 +++---
> >  7 files changed, 31 insertions(+), 31 deletions(-)
> >
> > [...]
> > diff --git a/drivers/target/target_core_transport.c
> > b/drivers/target/target_core_transport.c
> > index f2f7c5b818cc..7ea77933e64d 100644
> > --- a/drivers/target/target_core_transport.c
> > +++ b/drivers/target/target_core_transport.c
> > [...]
> > @@ -1790,7 +1792,7 @@ int target_submit_tmr(struct se_cmd *se_cmd,
> > struct se_session *se_sess,
> >  	BUG_ON(!se_tpg);
> >
> >  	transport_init_se_cmd(se_cmd, se_tpg->se_tpg_tfo, se_sess,
> > -			      0, DMA_NONE, TCM_SIMPLE_TAG, sense);
> > +			      0, DMA_NONE, TCM_SIMPLE_TAG, sense,
> > unpacked_lun);
> >  	/*
> >  	 * FIXME: Currently expect caller to handle se_cmd->se_tmr_req
> >  	 * allocation failure.
> 
> Between this hunk and the next one in target_submit_tmr(), there
> is this code:
> 
>         /*
>          * If this is ABORT_TASK with no explicit fabric provided LUN,
>          * go ahead and search active session tags for a match to figure
>          * out unpacked_lun for the original se_cmd.
>          */
>         if (tm_type == TMR_ABORT_TASK && (flags &
> TARGET_SCF_LOOKUP_LUN_FROM_TAG)) {
>                 if (!target_lookup_lun_from_tag(se_sess, tag, &unpacked_lun))
>                         goto failure;
>         }
> 
> > @@ -1818,7 +1820,7 @@ int target_submit_tmr(struct se_cmd *se_cmd,
> > struct se_session *se_sess,
> >  			goto failure;
> >  	}
> >
> > -	ret = transport_lookup_tmr_lun(se_cmd, unpacked_lun);
> > +	ret = transport_lookup_tmr_lun(se_cmd);
> >  	if (ret)
> >  		goto failure;
> >
> 
> AFAICS, your patch breaks the case where the above code is executed to
> derive unpacked_lun from the tag. The updated value of unpacked_lun is
> never used. That would break aborts for the qla2xxx target.
> 
> Am I overlooking something? Can you please explain?
> 

You are right. This change breaks the qlogic abort task code path, since it is the only transport that sets the TARGET_SCF_LOOKUP_LUN_FROM_TAG flag making that condition true. My apologies. I can send out a patch if you have not written one already. Please let me know.

Thanks
Sudhakar

> Regards
> Martin
> 
> 

RE: [PATCH v4 2/4] target: initialize LUN in transport_init_se_cmd().

[Sudhakar Panneerselvam]

Hi Martin,

> -----Original Message-----
> From: Martin Wilck [mailto:mwilck@suse.com]
> Sent: Wednesday, September 2, 2020 7:50 AM
> To: Sudhakar Panneerselvam <sudhakar.panneerselvam@oracle.com>; Martin
> Petersen <martin.petersen@oracle.com>; Michael Christie
> <michael.christie@oracle.com>; target-devel@vger.kernel.org; linux-
> scsi@vger.kernel.org
> Cc: Shirley Ma <shirley.ma@oracle.com>; Hannes Reinecke <hare@suse.com>;
> Daniel Wagner <daniel.wagner@suse.com>; Arun Easi <aeasi@marvell.com>
> Subject: Re: [PATCH v4 2/4] target: initialize LUN in transport_init_se_cmd().
> 
> Hello Sudhakar,
> 
> On Sun, 2020-06-07 at 19:58 +0000, Sudhakar Panneerselvam wrote:
> > Initialization of orig_fe_lun is moved to transport_init_se_cmd()
> > from
> > transport_lookup_cmd_lun(). This helps for the cases where the scsi
> > request
> > fails before the call to transport_lookup_cmd_lun() so that
> > trace_target_cmd_complete() can print the LUN information to the
> > trace
> > buffer. Due to this change, the lun parameter is removed from
> > transport_lookup_cmd_lun() and transport_lookup_tmr_lun().
> >
> > Signed-off-by: Sudhakar Panneerselvam <
> > sudhakar.panneerselvam@oracle.com>
> > ---
> >  drivers/target/iscsi/iscsi_target.c    | 11 +++++------
> >  drivers/target/target_core_device.c    | 19 ++++++++-----------
> >  drivers/target/target_core_tmr.c       |  4 ++--
> >  drivers/target/target_core_transport.c | 12 +++++++-----
> >  drivers/target/target_core_xcopy.c     |  4 ++--
> >  drivers/usb/gadget/function/f_tcm.c    |  6 ++++--
> >  include/target/target_core_fabric.h    |  6 +++---
> >  7 files changed, 31 insertions(+), 31 deletions(-)
> >
> > [...]
> > diff --git a/drivers/target/target_core_transport.c
> > b/drivers/target/target_core_transport.c
> > index f2f7c5b818cc..7ea77933e64d 100644
> > --- a/drivers/target/target_core_transport.c
> > +++ b/drivers/target/target_core_transport.c
> > [...]
> > @@ -1790,7 +1792,7 @@ int target_submit_tmr(struct se_cmd *se_cmd,
> > struct se_session *se_sess,
> >  	BUG_ON(!se_tpg);
> >
> >  	transport_init_se_cmd(se_cmd, se_tpg->se_tpg_tfo, se_sess,
> > -			      0, DMA_NONE, TCM_SIMPLE_TAG, sense);
> > +			      0, DMA_NONE, TCM_SIMPLE_TAG, sense,
> > unpacked_lun);
> >  	/*
> >  	 * FIXME: Currently expect caller to handle se_cmd->se_tmr_req
> >  	 * allocation failure.
> 
> Between this hunk and the next one in target_submit_tmr(), there
> is this code:
> 
>         /*
>          * If this is ABORT_TASK with no explicit fabric provided LUN,
>          * go ahead and search active session tags for a match to figure
>          * out unpacked_lun for the original se_cmd.
>          */
>         if (tm_type == TMR_ABORT_TASK && (flags &
> TARGET_SCF_LOOKUP_LUN_FROM_TAG)) {
>                 if (!target_lookup_lun_from_tag(se_sess, tag, &unpacked_lun))
>                         goto failure;
>         }
> 
> > @@ -1818,7 +1820,7 @@ int target_submit_tmr(struct se_cmd *se_cmd,
> > struct se_session *se_sess,
> >  			goto failure;
> >  	}
> >
> > -	ret = transport_lookup_tmr_lun(se_cmd, unpacked_lun);
> > +	ret = transport_lookup_tmr_lun(se_cmd);
> >  	if (ret)
> >  		goto failure;
> >
> 
> AFAICS, your patch breaks the case where the above code is executed to
> derive unpacked_lun from the tag. The updated value of unpacked_lun is
> never used. That would break aborts for the qla2xxx target.
> 
> Am I overlooking something? Can you please explain?
> 

You are right. This change breaks the qlogic abort task code path, since it is the only transport that sets the TARGET_SCF_LOOKUP_LUN_FROM_TAG flag making that condition true. My apologies. I can send out a patch if you have not written one already. Please let me know.

Thanks
Sudhakar

> Regards
> Martin
> 
> 

Re: [PATCH v4 2/4] target: initialize LUN in transport_init_se_cmd().

[Martin Wilck]

On Wed, 2020-09-02 at 08:14 -0700, Sudhakar Panneerselvam wrote:
> Hi Martin,
> 
> > 
> > AFAICS, your patch breaks the case where the above code is executed
> > to
> > derive unpacked_lun from the tag. The updated value of unpacked_lun
> > is
> > never used. That would break aborts for the qla2xxx target.
> > 
> > Am I overlooking something? Can you please explain?
> > 
> 
> You are right. This change breaks the qlogic abort task code path,
> since it is the only transport that sets the
> TARGET_SCF_LOOKUP_LUN_FROM_TAG flag making that condition true. My
> apologies. I can send out a patch if you have not written one
> already. Please let me know.

Please go ahead. I haven't written a patch - I'm not experienced enough
with the target code to quickly grok whether simply moving the
target_lookup_lun_from_tag() code upward would work, in particular wrt
handling failures and cleaning up.

Regards,
Martin

Re: [PATCH v4 2/4] target: initialize LUN in transport_init_se_cmd().

[Martin Wilck]

On Wed, 2020-09-02 at 08:14 -0700, Sudhakar Panneerselvam wrote:
> Hi Martin,
> 
> > 
> > AFAICS, your patch breaks the case where the above code is executed
> > to
> > derive unpacked_lun from the tag. The updated value of unpacked_lun
> > is
> > never used. That would break aborts for the qla2xxx target.
> > 
> > Am I overlooking something? Can you please explain?
> > 
> 
> You are right. This change breaks the qlogic abort task code path,
> since it is the only transport that sets the
> TARGET_SCF_LOOKUP_LUN_FROM_TAG flag making that condition true. My
> apologies. I can send out a patch if you have not written one
> already. Please let me know.

Please go ahead. I haven't written a patch - I'm not experienced enough
with the target code to quickly grok whether simply moving the
target_lookup_lun_from_tag() code upward would work, in particular wrt
handling failures and cleaning up.

Regards,
Martin

Re: [PATCH v4 2/4] target: initialize LUN in transport_init_se_cmd().

[Martin Wilck]

On Thu, 2020-09-03 at 15:00 +0200, Martin Wilck wrote:
> On Wed, 2020-09-02 at 08:14 -0700, Sudhakar Panneerselvam wrote:
> > Hi Martin,
> > 
> > > AFAICS, your patch breaks the case where the above code is
> > > executed
> > > to
> > > derive unpacked_lun from the tag. The updated value of
> > > unpacked_lun
> > > is
> > > never used. That would break aborts for the qla2xxx target.
> > > 
> > > Am I overlooking something? Can you please explain?
> > > 
> > 
> > You are right. This change breaks the qlogic abort task code path,
> > since it is the only transport that sets the
> > TARGET_SCF_LOOKUP_LUN_FROM_TAG flag making that condition true. My
> > apologies. I can send out a patch if you have not written one
> > already. Please let me know.
> 
> Please go ahead. I haven't written a patch - I'm not experienced
> enough
> with the target code to quickly grok whether simply moving the
> target_lookup_lun_from_tag() code upward would work, in particular
> wrt
> handling failures and cleaning up.

We may want to discuss whether TARGET_SCF_LOOKUP_LUN_FROM_TAG should be
removed entirely. As you said, qla2xxx is the only user of this
feature. And it actually uses it only in a single code path, 
__qlt_24xx_handle_abts() (everywhere else the LUN is known beforehand,
AFAICT).

If you look at the code of __qlt_24xx_handle_abts(), you can see that
it calls tcm_qla2xxx_find_cmd_by_tag(), which does the same thing
as target_lookup_lun_from_tag(): iterate over se_sess->sess_cmd_list
until a matching tag is found. It would clearly be equivalent, and more
efficient, if qla2xxx set the lun directly rather than relying on
common code iterating through the same list again, later.

Martin

Re: [PATCH v4 2/4] target: initialize LUN in transport_init_se_cmd().

[Martin Wilck]

On Thu, 2020-09-03 at 15:00 +0200, Martin Wilck wrote:
> On Wed, 2020-09-02 at 08:14 -0700, Sudhakar Panneerselvam wrote:
> > Hi Martin,
> > 
> > > AFAICS, your patch breaks the case where the above code is
> > > executed
> > > to
> > > derive unpacked_lun from the tag. The updated value of
> > > unpacked_lun
> > > is
> > > never used. That would break aborts for the qla2xxx target.
> > > 
> > > Am I overlooking something? Can you please explain?
> > > 
> > 
> > You are right. This change breaks the qlogic abort task code path,
> > since it is the only transport that sets the
> > TARGET_SCF_LOOKUP_LUN_FROM_TAG flag making that condition true. My
> > apologies. I can send out a patch if you have not written one
> > already. Please let me know.
> 
> Please go ahead. I haven't written a patch - I'm not experienced
> enough
> with the target code to quickly grok whether simply moving the
> target_lookup_lun_from_tag() code upward would work, in particular
> wrt
> handling failures and cleaning up.

We may want to discuss whether TARGET_SCF_LOOKUP_LUN_FROM_TAG should be
removed entirely. As you said, qla2xxx is the only user of this
feature. And it actually uses it only in a single code path, 
__qlt_24xx_handle_abts() (everywhere else the LUN is known beforehand,
AFAICT).

If you look at the code of __qlt_24xx_handle_abts(), you can see that
it calls tcm_qla2xxx_find_cmd_by_tag(), which does the same thing
as target_lookup_lun_from_tag(): iterate over se_sess->sess_cmd_list
until a matching tag is found. It would clearly be equivalent, and more
efficient, if qla2xxx set the lun directly rather than relying on
common code iterating through the same list again, later.

Martin