linux-scsi.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/7] target: misc fixes for 5.9
@ 2020-07-02  1:43 Mike Christie
  2020-07-02  1:43 ` [PATCH 1/7] target: check enforce_pr_isids during registration Mike Christie
                   ` (7 more replies)
  0 siblings, 8 replies; 9+ messages in thread
From: Mike Christie @ 2020-07-02  1:43 UTC (permalink / raw)
  To: martin.petersen, bstroesser, linux-scsi, target-devel

The following patches were made over Martin's 5.9 queue branch. They fix
some bugs that intersect with the sysfs/configfs patchset I've been
posting

These patches are not critical so are best for 5.9 or later. They fix
the SPEC_I_PT handling and how we report the iscsi inititor transport id
which seems to have always been broken and I do not think anyone uses.
There is also a fix for a leak during target_core_mod rmmod which is
rare.


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 1/7] target: check enforce_pr_isids during registration
  2020-07-02  1:43 [PATCH 0/7] target: misc fixes for 5.9 Mike Christie
@ 2020-07-02  1:43 ` Mike Christie
  2020-07-02  1:43 ` [PATCH 2/7] target: fix xcopy sess release leak Mike Christie
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Mike Christie @ 2020-07-02  1:43 UTC (permalink / raw)
  To: martin.petersen, bstroesser, linux-scsi, target-devel

Move the check for enforce_pr_isids to the registration code where we
can fail at the time an initiator tries to register a path without an
isid. In its current place in __core_scsi3_locate_pr_reg, it is too
late because it can be registered and be reported in PR in commands and
it is stuck in this state because we cannot unregister it.

Reviewed-by: Hannes Reinecke <hare@suse.de>
Reviewed-by: Lee Duncan <lduncan@suse.com>
Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/target/target_core_pr.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
index 91e41cc..293f518 100644
--- a/drivers/target/target_core_pr.c
+++ b/drivers/target/target_core_pr.c
@@ -1176,15 +1176,6 @@ static struct t10_pr_registration *__core_scsi3_locate_pr_reg(
 		 * ISID, then we have found a match.
 		 */
 		if (!pr_reg->isid_present_at_reg) {
-			/*
-			 * Determine if this SCSI device server requires that
-			 * SCSI Intiatior TransportID w/ ISIDs is enforced
-			 * for fabric modules (iSCSI) requiring them.
-			 */
-			if (tpg->se_tpg_tfo->sess_get_initiator_sid != NULL) {
-				if (dev->dev_attrib.enforce_pr_isids)
-					continue;
-			}
 			atomic_inc_mb(&pr_reg->pr_res_holders);
 			spin_unlock(&pr_tmpl->registration_lock);
 			return pr_reg;
@@ -1591,10 +1582,25 @@ static void core_scsi3_lunacl_undepend_item(struct se_dev_entry *se_deve)
 				continue;
 			dest_rtpi = tmp_lun->lun_rtpi;
 
+			iport_ptr = NULL;
 			i_str = target_parse_pr_out_transport_id(tmp_tpg,
 					ptr, &tid_len, &iport_ptr);
 			if (!i_str)
 				continue;
+			/*
+			 * Determine if this SCSI device server requires that
+			 * SCSI Intiatior TransportID w/ ISIDs is enforced
+			 * for fabric modules (iSCSI) requiring them.
+			 */
+			if (tpg->se_tpg_tfo->sess_get_initiator_sid &&
+                            dev->dev_attrib.enforce_pr_isids &&
+			    !iport_ptr) {
+				pr_warn("SPC-PR: enforce_pr_isids is set but a isid has not been sent in the SPEC_I_PT data for %s.",
+					i_str);
+				ret = TCM_INVALID_PARAMETER_LIST;
+				spin_unlock(&dev->se_port_lock);
+				goto out_unmap;
+			}
 
 			atomic_inc_mb(&tmp_tpg->tpg_pr_ref_count);
 			spin_unlock(&dev->se_port_lock);
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 2/7] target: fix xcopy sess release leak
  2020-07-02  1:43 [PATCH 0/7] target: misc fixes for 5.9 Mike Christie
  2020-07-02  1:43 ` [PATCH 1/7] target: check enforce_pr_isids during registration Mike Christie
@ 2020-07-02  1:43 ` Mike Christie
  2020-07-02  1:43 ` [PATCH 3/7] target: fix crash during SPEC_I_PT handling Mike Christie
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Mike Christie @ 2020-07-02  1:43 UTC (permalink / raw)
  To: martin.petersen, bstroesser, linux-scsi, target-devel

transport_init_session can allocate memory via percpu_ref_init, and
target_xcopy_release_pt never frees it. This adds a
transport_uninit_session function to handle cleanup of resources
allocated in the init function.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/target/target_core_internal.h  |  1 +
 drivers/target/target_core_transport.c |  7 ++++++-
 drivers/target/target_core_xcopy.c     | 11 +++++++++--
 3 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/target/target_core_internal.h b/drivers/target/target_core_internal.h
index 8533444..e7b3c6e 100644
--- a/drivers/target/target_core_internal.h
+++ b/drivers/target/target_core_internal.h
@@ -138,6 +138,7 @@ struct se_node_acl *core_tpg_add_initiator_node_acl(struct se_portal_group *tpg,
 void	release_se_kmem_caches(void);
 u32	scsi_get_new_index(scsi_index_t);
 void	transport_subsystem_check_init(void);
+void	transport_uninit_session(struct se_session *);
 unsigned char *transport_dump_cmd_direction(struct se_cmd *);
 void	transport_dump_dev_state(struct se_device *, char *, int *);
 void	transport_dump_dev_info(struct se_device *, struct se_lun *,
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 90ecdd7..e6e1fa6 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -236,6 +236,11 @@ int transport_init_session(struct se_session *se_sess)
 }
 EXPORT_SYMBOL(transport_init_session);
 
+void transport_uninit_session(struct se_session *se_sess)
+{
+	percpu_ref_exit(&se_sess->cmd_count);
+}
+
 /**
  * transport_alloc_session - allocate a session object and initialize it
  * @sup_prot_ops: bitmask that defines which T10-PI modes are supported.
@@ -579,7 +584,7 @@ void transport_free_session(struct se_session *se_sess)
 		sbitmap_queue_free(&se_sess->sess_tag_pool);
 		kvfree(se_sess->sess_cmd_map);
 	}
-	percpu_ref_exit(&se_sess->cmd_count);
+	transport_uninit_session(se_sess);
 	kmem_cache_free(se_sess_cache, se_sess);
 }
 EXPORT_SYMBOL(transport_free_session);
diff --git a/drivers/target/target_core_xcopy.c b/drivers/target/target_core_xcopy.c
index 0d00ccb..44e15d7 100644
--- a/drivers/target/target_core_xcopy.c
+++ b/drivers/target/target_core_xcopy.c
@@ -474,7 +474,7 @@ int target_xcopy_setup_pt(void)
 	memset(&xcopy_pt_sess, 0, sizeof(struct se_session));
 	ret = transport_init_session(&xcopy_pt_sess);
 	if (ret < 0)
-		return ret;
+		goto destroy_wq;
 
 	xcopy_pt_nacl.se_tpg = &xcopy_pt_tpg;
 	xcopy_pt_nacl.nacl_sess = &xcopy_pt_sess;
@@ -483,12 +483,19 @@ int target_xcopy_setup_pt(void)
 	xcopy_pt_sess.se_node_acl = &xcopy_pt_nacl;
 
 	return 0;
+
+destroy_wq:
+	destroy_workqueue(xcopy_wq);
+	xcopy_wq = NULL;
+	return ret;
 }
 
 void target_xcopy_release_pt(void)
 {
-	if (xcopy_wq)
+	if (xcopy_wq) {
 		destroy_workqueue(xcopy_wq);
+		transport_uninit_session(&xcopy_pt_sess);
+	}
 }
 
 /*
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 3/7] target: fix crash during SPEC_I_PT handling
  2020-07-02  1:43 [PATCH 0/7] target: misc fixes for 5.9 Mike Christie
  2020-07-02  1:43 ` [PATCH 1/7] target: check enforce_pr_isids during registration Mike Christie
  2020-07-02  1:43 ` [PATCH 2/7] target: fix xcopy sess release leak Mike Christie
@ 2020-07-02  1:43 ` Mike Christie
  2020-07-02  1:43 ` [PATCH 4/7] target: fix iscsi transport id parsing Mike Christie
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Mike Christie @ 2020-07-02  1:43 UTC (permalink / raw)
  To: martin.petersen, bstroesser, linux-scsi, target-devel

__core_scsi3_add_registration clears the t10_pr_registration
pr_reg_deve and does a core_scsi3_lunacl_undepend_item which does an
undepend and also does a kref_put from the get done in
__core_scsi3_alloc_registration. So when we get to the bottom of
core_scsi3_decode_spec_i_port the pr_reg_deve is NULL and we crash when
trying to access the local_pr_reg's pr_reg_deve. We've also done an extra
undepend for local_pr_reg and if we didn't crash on the NULL we would
have done an extra kref_put too.

This patch has us do a core_scsi3_lunacl_depend_item for local_pr_reg
and then let __core_scsi3_add_registration handle the cleanup for the
pr_reg_deve. We then just skip the undepend for the acl and tpg for the
local pr_reg.

The erorr path then works in a similar way, but we always do the
core_scsi3_lunacl_undepend_item since we never call
__core_scsi3_add_registration in that code path.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/target/target_core_pr.c | 35 +++++++++++++++++++----------------
 1 file changed, 19 insertions(+), 16 deletions(-)

diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
index 293f518..d5e6344 100644
--- a/drivers/target/target_core_pr.c
+++ b/drivers/target/target_core_pr.c
@@ -1521,13 +1521,16 @@ static void core_scsi3_lunacl_undepend_item(struct se_dev_entry *se_deve)
 		kfree(tidh_new);
 		return TCM_INSUFFICIENT_REGISTRATION_RESOURCES;
 	}
+
+	if (core_scsi3_lunacl_depend_item(local_pr_reg->pr_reg_deve)) {
+		kfree(tidh_new);
+		kref_put(&local_pr_reg->pr_reg_deve->pr_kref,
+			 target_pr_kref_release);
+		kmem_cache_free(t10_pr_reg_cache, local_pr_reg);
+		return TCM_INSUFFICIENT_REGISTRATION_RESOURCES;
+	}
+
 	tidh_new->dest_pr_reg = local_pr_reg;
-	/*
-	 * The local I_T nexus does not hold any configfs dependances,
-	 * so we set tidh_new->dest_se_deve to NULL to prevent the
-	 * configfs_undepend_item() calls in the tid_dest_list loops below.
-	 */
-	tidh_new->dest_se_deve = NULL;
 	list_add_tail(&tidh_new->dest_list, &tid_dest_list);
 
 	if (cmd->data_length < 28) {
@@ -1816,12 +1819,9 @@ static void core_scsi3_lunacl_undepend_item(struct se_dev_entry *se_deve)
 			dest_node_acl->initiatorname, i_buf, (dest_se_deve) ?
 			dest_se_deve->mapped_lun : 0);
 
-		if (!dest_se_deve) {
-			kref_put(&local_pr_reg->pr_reg_deve->pr_kref,
-				 target_pr_kref_release);
+		if (dest_pr_reg == local_pr_reg)
 			continue;
-		}
-		core_scsi3_lunacl_undepend_item(dest_se_deve);
+
 		core_scsi3_nodeacl_undepend_item(dest_node_acl);
 		core_scsi3_tpg_undepend_item(dest_tpg);
 	}
@@ -1835,11 +1835,16 @@ static void core_scsi3_lunacl_undepend_item(struct se_dev_entry *se_deve)
 	 * including *dest_pr_reg and the configfs dependances..
 	 */
 	list_for_each_entry_safe(tidh, tidh_tmp, &tid_dest_list, dest_list) {
+		bool is_local = false;
+
 		dest_tpg = tidh->dest_tpg;
 		dest_node_acl = tidh->dest_node_acl;
 		dest_se_deve = tidh->dest_se_deve;
 		dest_pr_reg = tidh->dest_pr_reg;
 
+		if (dest_pr_reg == local_pr_reg)
+			is_local = true;
+
 		list_del(&tidh->dest_list);
 		kfree(tidh);
 		/*
@@ -1855,13 +1860,11 @@ static void core_scsi3_lunacl_undepend_item(struct se_dev_entry *se_deve)
 		}
 
 		kmem_cache_free(t10_pr_reg_cache, dest_pr_reg);
+		core_scsi3_lunacl_undepend_item(dest_se_deve);
 
-		if (!dest_se_deve) {
-			kref_put(&local_pr_reg->pr_reg_deve->pr_kref,
-				 target_pr_kref_release);
+		if (is_local)
 			continue;
-		}
-		core_scsi3_lunacl_undepend_item(dest_se_deve);
+
 		core_scsi3_nodeacl_undepend_item(dest_node_acl);
 		core_scsi3_tpg_undepend_item(dest_tpg);
 	}
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 4/7] target: fix iscsi transport id parsing
  2020-07-02  1:43 [PATCH 0/7] target: misc fixes for 5.9 Mike Christie
                   ` (2 preceding siblings ...)
  2020-07-02  1:43 ` [PATCH 3/7] target: fix crash during SPEC_I_PT handling Mike Christie
@ 2020-07-02  1:43 ` Mike Christie
  2020-07-02  1:43 ` [PATCH 5/7] target: fix iscsi transport id buffer setup Mike Christie
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Mike Christie @ 2020-07-02  1:43 UTC (permalink / raw)
  To: martin.petersen, bstroesser, linux-scsi, target-devel

The length passed in the ADDITIONAL LENGTH field includes padding and
the terminating NULL for the last field (name or isid depending on the
format), so we should not also try to calculate that and then double add
that to the returned length.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/target/target_core_fabric_lib.c | 22 ++++------------------
 1 file changed, 4 insertions(+), 18 deletions(-)

diff --git a/drivers/target/target_core_fabric_lib.c b/drivers/target/target_core_fabric_lib.c
index 1e031d8..81bc8ec 100644
--- a/drivers/target/target_core_fabric_lib.c
+++ b/drivers/target/target_core_fabric_lib.c
@@ -265,9 +265,7 @@ static char *iscsi_parse_pr_out_transport_id(
 	char **port_nexus_ptr)
 {
 	char *p;
-	u32 tid_len, padding;
 	int i;
-	u16 add_len;
 	u8 format_code = (buf[0] & 0xc0);
 	/*
 	 * Check for FORMAT CODE 00b or 01b from spc4r17, section 7.5.4.6:
@@ -293,23 +291,11 @@ static char *iscsi_parse_pr_out_transport_id(
 	 */
 	if (out_tid_len) {
 		/* The shift works thanks to integer promotion rules */
-		add_len = get_unaligned_be16(&buf[2]);
-
-		tid_len = strlen(&buf[4]);
-		tid_len += 4; /* Add four bytes for iSCSI Transport ID header */
-		tid_len += 1; /* Add one byte for NULL terminator */
-		padding = ((-tid_len) & 3);
-		if (padding != 0)
-			tid_len += padding;
-
-		if ((add_len + 4) != tid_len) {
-			pr_debug("LIO-Target Extracted add_len: %hu "
-				"does not match calculated tid_len: %u,"
-				" using tid_len instead\n", add_len+4, tid_len);
-			*out_tid_len = tid_len;
-		} else
-			*out_tid_len = (add_len + 4);
+		*out_tid_len = get_unaligned_be16(&buf[2]);
+		/* Add four bytes for iSCSI Transport ID header */
+		*out_tid_len += 4;
 	}
+
 	/*
 	 * Check for ',i,0x' separator between iSCSI Name and iSCSI Initiator
 	 * Session ID as defined in Table 390 - iSCSI initiator port TransportID
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 5/7] target: fix iscsi transport id buffer setup
  2020-07-02  1:43 [PATCH 0/7] target: misc fixes for 5.9 Mike Christie
                   ` (3 preceding siblings ...)
  2020-07-02  1:43 ` [PATCH 4/7] target: fix iscsi transport id parsing Mike Christie
@ 2020-07-02  1:43 ` Mike Christie
  2020-07-02  1:43 ` [PATCH 6/7] target: fix iscsi transport id buf len calculation Mike Christie
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Mike Christie @ 2020-07-02  1:43 UTC (permalink / raw)
  To: martin.petersen, bstroesser, linux-scsi, target-devel

This fixes the following bugs with the transport id setup for iscsi:

1. Incorrectly adding NULL after initiator name for TPID format 1.

2. For TPID format 1 buffer setup we are doing off+len, off++ and
then also len+=some_value. This results in the isid going past buffer
boundaries when we then do buf[off+len]

3. The pr_reg_isid is the isid in string format which is 12 bytes, but
we are only copying 6 bytes.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
V2:
- Drop chunk that dropped format code.

 drivers/target/target_core_fabric_lib.c | 72 ++++++++++++++++-----------------
 1 file changed, 35 insertions(+), 37 deletions(-)

diff --git a/drivers/target/target_core_fabric_lib.c b/drivers/target/target_core_fabric_lib.c
index 81bc8ec..428e5a1 100644
--- a/drivers/target/target_core_fabric_lib.c
+++ b/drivers/target/target_core_fabric_lib.c
@@ -136,29 +136,22 @@ static int iscsi_get_pr_transport_id(
 
 	spin_lock_irq(&se_nacl->nacl_sess_lock);
 	/*
-	 * From spc4r17 Section 7.5.4.6: TransportID for initiator
-	 * ports using SCSI over iSCSI.
+	 * Only null terminate the last field.
 	 *
-	 * The null-terminated, null-padded (see 4.4.2) ISCSI NAME field
-	 * shall contain the iSCSI name of an iSCSI initiator node (see
-	 * RFC 3720). The first ISCSI NAME field byte containing an ASCII
-	 * null character terminates the ISCSI NAME field without regard for
-	 * the specified length of the iSCSI TransportID or the contents of
-	 * the ADDITIONAL LENGTH field.
-	 */
-	len = sprintf(&buf[off], "%s", se_nacl->initiatorname);
-	/*
-	 * Add Extra byte for NULL terminator
-	 */
-	len++;
-	/*
-	 * If there is ISID present with the registration and *format code == 1
-	 * 1, use iSCSI Initiator port TransportID format.
+	 * From spc4r37 section 7.6.4.6: TransportID for initiator ports using
+	 * SCSI over iSCSI.
 	 *
-	 * Otherwise use iSCSI Initiator device TransportID format that
-	 * does not contain the ASCII encoded iSCSI Initiator iSID value
-	 * provied by the iSCSi Initiator during the iSCSI login process.
+	 * Table 507 TPID=0 Initiator device TransportID
+	 *
+	 * The null-terminated, null-padded (see 4.3.2) ISCSI NAME field shall
+	 * contain the iSCSI name of an iSCSI initiator node (see RFC 7143).
+	 * The first ISCSI NAME field byte containing an ASCII null character
+	 * terminates the ISCSI NAME field without regard for the specified
+	 * length of the iSCSI TransportID or the contents of the ADDITIONAL
+	 * LENGTH field.
 	 */
+	len = sprintf(&buf[off], "%s", se_nacl->initiatorname);
+	off += len;
 	if ((*format_code == 1) && (pr_reg->isid_present_at_reg)) {
 		/*
 		 * Set FORMAT CODE 01b for iSCSI Initiator port TransportID
@@ -166,8 +159,12 @@ static int iscsi_get_pr_transport_id(
 		 */
 		buf[0] |= 0x40;
 		/*
-		 * From spc4r17 Section 7.5.4.6: TransportID for initiator
-		 * ports using SCSI over iSCSI.  Table 390
+		 * From spc4r37 Section 7.6.4.6
+		 *
+		 * Table 508 TPID=1 Initiator port TransportID.
+		 *
+		 * The ISCSI NAME field shall not be null-terminated
+		 * (see 4.3.2) and shall not be padded.
 		 *
 		 * The SEPARATOR field shall contain the five ASCII
 		 * characters ",i,0x".
@@ -177,23 +174,24 @@ static int iscsi_get_pr_transport_id(
 		 * (see RFC 3720) in the form of ASCII characters that are the
 		 * hexadecimal digits converted from the binary iSCSI initiator
 		 * session identifier value. The first ISCSI INITIATOR SESSION
-		 * ID field byte containing an ASCII null character
+		 * ID field byte containing an ASCII null character terminates
+		 * the ISCSI INITIATOR SESSION ID field without regard for the
+		 * specified length of the iSCSI TransportID or the contents
+		 * of the ADDITIONAL LENGTH field.
 		 */
-		buf[off+len] = 0x2c; off++; /* ASCII Character: "," */
-		buf[off+len] = 0x69; off++; /* ASCII Character: "i" */
-		buf[off+len] = 0x2c; off++; /* ASCII Character: "," */
-		buf[off+len] = 0x30; off++; /* ASCII Character: "0" */
-		buf[off+len] = 0x78; off++; /* ASCII Character: "x" */
-		len += 5;
-		buf[off+len] = pr_reg->pr_reg_isid[0]; off++;
-		buf[off+len] = pr_reg->pr_reg_isid[1]; off++;
-		buf[off+len] = pr_reg->pr_reg_isid[2]; off++;
-		buf[off+len] = pr_reg->pr_reg_isid[3]; off++;
-		buf[off+len] = pr_reg->pr_reg_isid[4]; off++;
-		buf[off+len] = pr_reg->pr_reg_isid[5]; off++;
-		buf[off+len] = '\0'; off++;
-		len += 7;
+		buf[off++] = 0x2c; /* ASCII Character: "," */
+		buf[off++] = 0x69; /* ASCII Character: "i" */
+		buf[off++] = 0x2c; /* ASCII Character: "," */
+		buf[off++] = 0x30; /* ASCII Character: "0" */
+		buf[off++] = 0x78; /* ASCII Character: "x" */
+
+		memcpy(buf + off, pr_reg->pr_reg_isid, 12);
+		off += 12;
+
+		len += 17;
 	}
+	buf[off] = '\0';
+	len += 1;
 	spin_unlock_irq(&se_nacl->nacl_sess_lock);
 	/*
 	 * The ADDITIONAL LENGTH field specifies the number of bytes that follow
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 6/7] target: fix iscsi transport id buf len calculation
  2020-07-02  1:43 [PATCH 0/7] target: misc fixes for 5.9 Mike Christie
                   ` (4 preceding siblings ...)
  2020-07-02  1:43 ` [PATCH 5/7] target: fix iscsi transport id buffer setup Mike Christie
@ 2020-07-02  1:43 ` Mike Christie
  2020-07-02  1:43 ` [PATCH 7/7] target: handle short iSIDs Mike Christie
  2020-07-08  6:06 ` [PATCH 0/7] target: misc fixes for 5.9 Martin K. Petersen
  7 siblings, 0 replies; 9+ messages in thread
From: Mike Christie @ 2020-07-02  1:43 UTC (permalink / raw)
  To: martin.petersen, bstroesser, linux-scsi, target-devel

The isid returned to the initiator is in string format which is 12
bytes. We also only add 1 terminating NULL and not one after the
initiator name and another one after the isid.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/target/target_core_fabric_lib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/target/target_core_fabric_lib.c b/drivers/target/target_core_fabric_lib.c
index 428e5a1..1d2762a 100644
--- a/drivers/target/target_core_fabric_lib.c
+++ b/drivers/target/target_core_fabric_lib.c
@@ -234,7 +234,7 @@ static int iscsi_get_pr_transport_id_len(
 	 */
 	if (pr_reg->isid_present_at_reg) {
 		len += 5; /* For ",i,0x" ASCII separator */
-		len += 7; /* For iSCSI Initiator Session ID + Null terminator */
+		len += 12; /* For iSCSI Initiator Session ID */
 		*format_code = 1;
 	} else
 		*format_code = 0;
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 7/7] target: handle short iSIDs
  2020-07-02  1:43 [PATCH 0/7] target: misc fixes for 5.9 Mike Christie
                   ` (5 preceding siblings ...)
  2020-07-02  1:43 ` [PATCH 6/7] target: fix iscsi transport id buf len calculation Mike Christie
@ 2020-07-02  1:43 ` Mike Christie
  2020-07-08  6:06 ` [PATCH 0/7] target: misc fixes for 5.9 Martin K. Petersen
  7 siblings, 0 replies; 9+ messages in thread
From: Mike Christie @ 2020-07-02  1:43 UTC (permalink / raw)
  To: martin.petersen, bstroesser, linux-scsi, target-devel

SPC4 has:

The first ISCSI INITIATOR SESSION ID field byte containing an ASCII
null character terminates the ISCSI INITIATOR SESSION ID field without
regard for the specified length of the iSCSI TransportID or the contents
of the ADDITIONAL LENGTH field.
----------------------------------------

which sounds like we can get an iSID shorter than 12 chars. SPC and
the iSCSI RFC do not say how to handle that case other than just
cutting off the iSID. This patch just makes sure that if we get an
iSID like that, we only copy/send that string.

There is no OS that does this right now, so there was no test case.
I did test with sg utils to check it works as expected and nothing
breaks.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/target/target_core_fabric_lib.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/drivers/target/target_core_fabric_lib.c b/drivers/target/target_core_fabric_lib.c
index 1d2762a..6600ae4 100644
--- a/drivers/target/target_core_fabric_lib.c
+++ b/drivers/target/target_core_fabric_lib.c
@@ -132,6 +132,7 @@ static int iscsi_get_pr_transport_id(
 	unsigned char *buf)
 {
 	u32 off = 4, padding = 0;
+	int isid_len;
 	u16 len = 0;
 
 	spin_lock_irq(&se_nacl->nacl_sess_lock);
@@ -184,11 +185,11 @@ static int iscsi_get_pr_transport_id(
 		buf[off++] = 0x2c; /* ASCII Character: "," */
 		buf[off++] = 0x30; /* ASCII Character: "0" */
 		buf[off++] = 0x78; /* ASCII Character: "x" */
+		len += 5;
 
-		memcpy(buf + off, pr_reg->pr_reg_isid, 12);
-		off += 12;
-
-		len += 17;
+		isid_len = sprintf(buf + off, "%s", pr_reg->pr_reg_isid);
+		off += isid_len;
+		len += isid_len;
 	}
 	buf[off] = '\0';
 	len += 1;
@@ -234,7 +235,7 @@ static int iscsi_get_pr_transport_id_len(
 	 */
 	if (pr_reg->isid_present_at_reg) {
 		len += 5; /* For ",i,0x" ASCII separator */
-		len += 12; /* For iSCSI Initiator Session ID */
+		len += strlen(pr_reg->pr_reg_isid);
 		*format_code = 1;
 	} else
 		*format_code = 0;
@@ -318,6 +319,16 @@ static char *iscsi_parse_pr_out_transport_id(
 		 * iscsi_target.c:lio_sess_get_initiator_sid()
 		 */
 		for (i = 0; i < 12; i++) {
+			/*
+			 * The first ISCSI INITIATOR SESSION ID field byte
+			 * containing an ASCII null character terminates the
+			 * ISCSI INITIATOR SESSION ID field without regard for
+			 * the specified length of the iSCSI TransportID or the
+			 * contents of the ADDITIONAL LENGTH field.
+			 */
+			if (*p == '\0')
+				break;
+
 			if (isdigit(*p)) {
 				p++;
 				continue;
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 0/7] target: misc fixes for 5.9
  2020-07-02  1:43 [PATCH 0/7] target: misc fixes for 5.9 Mike Christie
                   ` (6 preceding siblings ...)
  2020-07-02  1:43 ` [PATCH 7/7] target: handle short iSIDs Mike Christie
@ 2020-07-08  6:06 ` Martin K. Petersen
  7 siblings, 0 replies; 9+ messages in thread
From: Martin K. Petersen @ 2020-07-08  6:06 UTC (permalink / raw)
  To: bstroesser, target-devel, linux-scsi, Mike Christie; +Cc: Martin K . Petersen

On Wed, 1 Jul 2020 20:43:16 -0500, Mike Christie wrote:

> The following patches were made over Martin's 5.9 queue branch. They fix
> some bugs that intersect with the sysfs/configfs patchset I've been
> posting
> 
> These patches are not critical so are best for 5.9 or later. They fix
> the SPEC_I_PT handling and how we report the iscsi inititor transport id
> which seems to have always been broken and I do not think anyone uses.
> There is also a fix for a leak during target_core_mod rmmod which is
> rare.

Applied to 5.9/scsi-queue, thanks!

[1/7] scsi: target: Check enforce_pr_isids during registration
      https://git.kernel.org/mkp/scsi/c/63c9ffe473d3
[2/7] scsi: target: Fix xcopy sess release leak
      https://git.kernel.org/mkp/scsi/c/3c006c7d23aa
[3/7] scsi: target: Fix crash during SPEC_I_PT handling
      https://git.kernel.org/mkp/scsi/c/f32ba612ef0f
[4/7] scsi: target: Fix iscsi transport id parsing
      https://git.kernel.org/mkp/scsi/c/169622eee437
[5/7] scsi: target: Fix iscsi transport id buffer setup
      https://git.kernel.org/mkp/scsi/c/a6f9b6cee3f2
[6/7] scsi: target: Fix iscsi transport id buf len calculation
      https://git.kernel.org/mkp/scsi/c/bd7f65d95200
[7/7] scsi: target: Handle short iSIDs
      https://git.kernel.org/mkp/scsi/c/639341bf8836

-- 
Martin K. Petersen	Oracle Linux Engineering

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2020-07-08  6:07 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-02  1:43 [PATCH 0/7] target: misc fixes for 5.9 Mike Christie
2020-07-02  1:43 ` [PATCH 1/7] target: check enforce_pr_isids during registration Mike Christie
2020-07-02  1:43 ` [PATCH 2/7] target: fix xcopy sess release leak Mike Christie
2020-07-02  1:43 ` [PATCH 3/7] target: fix crash during SPEC_I_PT handling Mike Christie
2020-07-02  1:43 ` [PATCH 4/7] target: fix iscsi transport id parsing Mike Christie
2020-07-02  1:43 ` [PATCH 5/7] target: fix iscsi transport id buffer setup Mike Christie
2020-07-02  1:43 ` [PATCH 6/7] target: fix iscsi transport id buf len calculation Mike Christie
2020-07-02  1:43 ` [PATCH 7/7] target: handle short iSIDs Mike Christie
2020-07-08  6:06 ` [PATCH 0/7] target: misc fixes for 5.9 Martin K. Petersen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).