[PATCH 02/15] target: fix isid copying and comparision

[PATCH 02/15] target: fix isid copying and comparision

[Mike Christie]

The isid is 48 bits, and in hex string format it's 12 bytes.
We are currently copying the 12 byte hex string to a u64
so we can easily compare it, but this has the problem that
only 8 bytes of the 12 bytes are copied.

The next patches will want to print se_session sess_bin_isid
so this has us use hex2bin to when converting from the hex
sting to the bin value.

Signed-off-by: Mike Christie <mchristi@redhat.com>
---
 drivers/target/target_core_pr.c        | 20 ++++++++++++++++----
 drivers/target/target_core_transport.c |  3 ++-
 2 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
index 01ac306..65e5253 100644
--- a/drivers/target/target_core_pr.c
+++ b/drivers/target/target_core_pr.c
@@ -662,8 +662,7 @@ static struct t10_pr_registration *__core_scsi3_do_alloc_registration(
 			rcu_read_unlock();
 			pr_err("Unable to locate PR deve %s mapped_lun: %llu\n",
 				nacl->initiatorname, mapped_lun);
-			kmem_cache_free(t10_pr_reg_cache, pr_reg);
-			return NULL;
+			goto free_reg;
 		}
 		kref_get(&pr_reg->pr_reg_deve->pr_kref);
 		rcu_read_unlock();
@@ -679,12 +678,20 @@ static struct t10_pr_registration *__core_scsi3_do_alloc_registration(
 	 * save it to the registration now.
 	 */
 	if (isid != NULL) {
-		pr_reg->pr_reg_bin_isid = get_unaligned_be64(isid);
+		if (hex2bin((u8 *)&pr_reg->pr_reg_bin_isid, isid, 6)) {
+			pr_err("Invalid isid %s\n", isid);
+			goto free_reg;
+		}
+
 		snprintf(pr_reg->pr_reg_isid, PR_REG_ISID_LEN, "%s", isid);
 		pr_reg->isid_present_at_reg = 1;
 	}
 
 	return pr_reg;
+
+free_reg:
+	kmem_cache_free(t10_pr_reg_cache, pr_reg);
+	return NULL;
 }
 
 static int core_scsi3_lunacl_depend_item(struct se_dev_entry *);
@@ -873,7 +880,12 @@ int core_scsi3_alloc_aptpl_registration(
 	 * SCSI Initiator Port, restore it now.
 	 */
 	if (isid != NULL) {
-		pr_reg->pr_reg_bin_isid = get_unaligned_be64(isid);
+		if (hex2bin((u8 *)&pr_reg->pr_reg_bin_isid, isid, 6)) {
+			pr_err("Invalid isid %s\n", isid);
+			kmem_cache_free(t10_pr_reg_cache, pr_reg);
+			return -EINVAL;
+		}
+
 		snprintf(pr_reg->pr_reg_isid, PR_REG_ISID_LEN, "%s", isid);
 		pr_reg->isid_present_at_reg = 1;
 	}
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 7261561..6324743 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -380,7 +380,8 @@ void __transport_register_session(
 			memset(&buf[0], 0, PR_REG_ISID_LEN);
 			se_tpg->se_tpg_tfo->sess_get_initiator_sid(se_sess,
 					&buf[0], PR_REG_ISID_LEN);
-			se_sess->sess_bin_isid = get_unaligned_be64(&buf[0]);
+
+			WARN_ON(hex2bin((u8 *)&se_sess->sess_bin_isid, buf, 6));
 		}
 
 		spin_lock_irq(&se_nacl->nacl_sess_lock);
-- 
2.7.2

Re: [PATCH 02/15] target: fix isid copying and comparision

[Bart Van Assche]

On Sun, 2018-07-15 at 18:16 -0500, Mike Christie wrote:
+AD4- The isid is 48 bits, and in hex string format it's 12 bytes.
+AD4- We are currently copying the 12 byte hex string to a u64
+AD4- so we can easily compare it, but this has the problem that
+AD4- only 8 bytes of the 12 bytes are copied.
+AD4- 
+AD4- The next patches will want to print se+AF8-session sess+AF8-bin+AF8-isid
+AD4- so this has us use hex2bin to when converting from the hex
+AD4- sting to the bin value.
  +AF4AXgBeAF4AXg-
  string?

Which spec defines that an ISID is 48 bits? All I know about SCSI registrations
is that these involve a transport ID and that that transport ID can be up to 228
bytes long for iSCSI.

+AD4-  	if (isid +ACEAPQ- NULL) +AHs-
+AD4- -		pr+AF8-reg-+AD4-pr+AF8-reg+AF8-bin+AF8-isid +AD0- get+AF8-unaligned+AF8-be64(isid)+ADs-
+AD4- +-		if (hex2bin((u8 +ACo-)+ACY-pr+AF8-reg-+AD4-pr+AF8-reg+AF8-bin+AF8-isid, isid, 6)) +AHs-
+AD4- +-			pr+AF8-err(+ACI-Invalid isid +ACU-s+AFw-n+ACI-, isid)+ADs-
+AD4- +-			goto free+AF8-reg+ADs-
+AD4- +-		+AH0-

Why is it necessary to convert the ISID from hex to binary format? If this
conversion is necessary, isn't that something that should be handled by the
iSCSI target driver instead of the target core?

Thanks,

Bart.

Re: [PATCH 02/15] target: fix isid copying and comparision

[Mike Christie]

On 07/18/2018 05:09 PM, Bart Van Assche wrote:
> On Sun, 2018-07-15 at 18:16 -0500, Mike Christie wrote:
>> The isid is 48 bits, and in hex string format it's 12 bytes.
>> We are currently copying the 12 byte hex string to a u64
>> so we can easily compare it, but this has the problem that
>> only 8 bytes of the 12 bytes are copied.
>>
>> The next patches will want to print se_session sess_bin_isid
>> so this has us use hex2bin to when converting from the hex
>> sting to the bin value.
>   ^^^^^
>   string?
> 
> Which spec defines that an ISID is 48 bits? All I know about SCSI registrations

The iscsi spec defines it as 48 bits.

> is that these involve a transport ID and that that transport ID can be up to 228
> bytes long for iSCSI.

I am talking about the Initiator Session ID above. That along with the
iscsi name make up the Initiator Port Transport ID. In spc4r37 checkout
table 508 or in SAM 5r21 checkout table A.4.

So in the SCSI specs as part of the Initiator Port Transport ID we have
this from that SAM table:

The Initiator Session Identifier (ISID) portion of the string is a UTF-8
encoded hexadecimal representation of a six byte binary value.

---

In the PR parts of SPC it sometimes mentions only "Transport ID" but
then clarifies the initiator port so I am assuming in those cases it
means "Initiator Port Transport ID" so it is both the name and isid for
iscsi.


> 
>>  	if (isid != NULL) {
>> -		pr_reg->pr_reg_bin_isid = get_unaligned_be64(isid);
>> +		if (hex2bin((u8 *)&pr_reg->pr_reg_bin_isid, isid, 6)) {
>> +			pr_err("Invalid isid %s\n", isid);
>> +			goto free_reg;
>> +		}
> 
> Why is it necessary to convert the ISID from hex to binary format? If this
> conversion is necessary, isn't that something that should be handled by the
> iSCSI target driver instead of the target core?
> 

The target drivers get the value as a 48 bit binary value.

The PR code gets it in the string format as describe above.

I had thought the code preferred to store it in the binary format
because it was easier to test than comparing strings or maybe for speed,
so I just fixed that code.

I think we can just keep it in string format in
__transport_register_session then just compare strings in target_core_pr.c.

Re: [PATCH 02/15] target: fix isid copying and comparision

[Mike Christie]

On 07/18/2018 07:03 PM, Mike Christie wrote:
> On 07/18/2018 05:09 PM, Bart Van Assche wrote:
>> On Sun, 2018-07-15 at 18:16 -0500, Mike Christie wrote:
>>> The isid is 48 bits, and in hex string format it's 12 bytes.
>>> We are currently copying the 12 byte hex string to a u64
>>> so we can easily compare it, but this has the problem that
>>> only 8 bytes of the 12 bytes are copied.
>>>
>>> The next patches will want to print se_session sess_bin_isid
>>> so this has us use hex2bin to when converting from the hex
>>> sting to the bin value.
>>   ^^^^^
>>   string?
>>
>> Which spec defines that an ISID is 48 bits? All I know about SCSI registrations
> 
> The iscsi spec defines it as 48 bits.
> 
>> is that these involve a transport ID and that that transport ID can be up to 228
>> bytes long for iSCSI.
> 
> I am talking about the Initiator Session ID above. That along with the
> iscsi name make up the Initiator Port Transport ID. In spc4r37 checkout
> table 508 or in SAM 5r21 checkout table A.4.
> 
> So in the SCSI specs as part of the Initiator Port Transport ID we have
> this from that SAM table:
> 
> The Initiator Session Identifier (ISID) portion of the string is a UTF-8
> encoded hexadecimal representation of a six byte binary value.
> 
> ---
> 
> In the PR parts of SPC it sometimes mentions only "Transport ID" but
> then clarifies the initiator port so I am assuming in those cases it
> means "Initiator Port Transport ID" so it is both the name and isid for
> iscsi.

It looks like we are supposed to go by what the initiator specifies in
the TPID field, so it can be either the Transport ID or Initiator Port
Transport ID.

Re: [PATCH 02/15] target: fix isid copying and comparision

[Bart Van Assche]

On Wed, 2018-07-18 at 20:02 -0500, Mike Christie wrote:
+AD4- On 07/18/2018 07:03 PM, Mike Christie wrote:
+AD4- +AD4- On 07/18/2018 05:09 PM, Bart Van Assche wrote:
+AD4- +AD4- +AD4- +AFs- ... +AF0-
+AD4- +AD4- +AD4- is that these involve a transport ID and that that transport ID can be up to 228
+AD4- +AD4- +AD4- bytes long for iSCSI.
+AD4- +AD4- 
+AD4- +AD4- I am talking about the Initiator Session ID above. That along with the
+AD4- +AD4- iscsi name make up the Initiator Port Transport ID. In spc4r37 checkout
+AD4- +AD4- table 508 or in SAM 5r21 checkout table A.4.
+AD4- +AD4- 
+AD4- +AD4- So in the SCSI specs as part of the Initiator Port Transport ID we have
+AD4- +AD4- this from that SAM table:
+AD4- +AD4- 
+AD4- +AD4- The Initiator Session Identifier (ISID) portion of the string is a UTF-8
+AD4- +AD4- encoded hexadecimal representation of a six byte binary value.
+AD4- +AD4- 
+AD4- +AD4- ---
+AD4- +AD4- 
+AD4- +AD4- In the PR parts of SPC it sometimes mentions only +ACI-Transport ID+ACI- but
+AD4- +AD4- then clarifies the initiator port so I am assuming in those cases it
+AD4- +AD4- means +ACI-Initiator Port Transport ID+ACI- so it is both the name and isid for
+AD4- +AD4- iscsi.
+AD4- 
+AD4- It looks like we are supposed to go by what the initiator specifies in
+AD4- the TPID field, so it can be either the Transport ID or Initiator Port
+AD4- Transport ID.

Hello Mike,

Since the ISID is iSCSI-specific I think that all code that knows about the
ISID and its encoding should be in the iSCSI target driver instead of the
target core. Do you think an approach similar to that of the SCST function
iscsi+AF8-get+AF8-initiator+AF8-port+AF8-transport+AF8-id() can be implemented in LIO? The caller
of that function is in source file scst/src/scst+AF8-targ.c:

		res +AD0- sess-+AD4-tgt-+AD4-tgtt-+AD4-get+AF8-initiator+AF8-port+AF8-transport+AF8-id(
					sess-+AD4-tgt, sess, +ACY-sess-+AD4-transport+AF8-id)+ADs-

Thanks,

Bart.

Re: [PATCH 02/15] target: fix isid copying and comparision

[Mike Christie]

On 07/19/2018 10:15 AM, Bart Van Assche wrote:
> On Wed, 2018-07-18 at 20:02 -0500, Mike Christie wrote:
>> On 07/18/2018 07:03 PM, Mike Christie wrote:
>>> On 07/18/2018 05:09 PM, Bart Van Assche wrote:
>>>> [ ... ]
>>>> is that these involve a transport ID and that that transport ID can be up to 228
>>>> bytes long for iSCSI.
>>>
>>> I am talking about the Initiator Session ID above. That along with the
>>> iscsi name make up the Initiator Port Transport ID. In spc4r37 checkout
>>> table 508 or in SAM 5r21 checkout table A.4.
>>>
>>> So in the SCSI specs as part of the Initiator Port Transport ID we have
>>> this from that SAM table:
>>>
>>> The Initiator Session Identifier (ISID) portion of the string is a UTF-8
>>> encoded hexadecimal representation of a six byte binary value.
>>>
>>> ---
>>>
>>> In the PR parts of SPC it sometimes mentions only "Transport ID" but
>>> then clarifies the initiator port so I am assuming in those cases it
>>> means "Initiator Port Transport ID" so it is both the name and isid for
>>> iscsi.
>>
>> It looks like we are supposed to go by what the initiator specifies in
>> the TPID field, so it can be either the Transport ID or Initiator Port
>> Transport ID.
> 
> Hello Mike,
> 
> Since the ISID is iSCSI-specific I think that all code that knows about the
> ISID and its encoding should be in the iSCSI target driver instead of the
> target core. Do you think an approach similar to that of the SCST function
> iscsi_get_initiator_port_transport_id() can be implemented in LIO? The caller

Yeah, I can make that work.

> of that function is in source file scst/src/scst_targ.c:
> 
> 		res = sess->tgt->tgtt->get_initiator_port_transport_id(
> 					sess->tgt, sess, &sess->transport_id);
> 
> Thanks,
> 
> Bart.--
> To unsubscribe from this list: send the line "unsubscribe target-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 02/15] target: fix isid copying and comparision

[Mike Christie]

On 07/19/2018 11:13 AM, Mike Christie wrote:
> On 07/19/2018 10:15 AM, Bart Van Assche wrote:
>> On Wed, 2018-07-18 at 20:02 -0500, Mike Christie wrote:
>>> On 07/18/2018 07:03 PM, Mike Christie wrote:
>>>> On 07/18/2018 05:09 PM, Bart Van Assche wrote:
>>>>> [ ... ]
>>>>> is that these involve a transport ID and that that transport ID can be up to 228
>>>>> bytes long for iSCSI.
>>>>
>>>> I am talking about the Initiator Session ID above. That along with the
>>>> iscsi name make up the Initiator Port Transport ID. In spc4r37 checkout
>>>> table 508 or in SAM 5r21 checkout table A.4.
>>>>
>>>> So in the SCSI specs as part of the Initiator Port Transport ID we have
>>>> this from that SAM table:
>>>>
>>>> The Initiator Session Identifier (ISID) portion of the string is a UTF-8
>>>> encoded hexadecimal representation of a six byte binary value.
>>>>
>>>> ---
>>>>
>>>> In the PR parts of SPC it sometimes mentions only "Transport ID" but
>>>> then clarifies the initiator port so I am assuming in those cases it
>>>> means "Initiator Port Transport ID" so it is both the name and isid for
>>>> iscsi.
>>>
>>> It looks like we are supposed to go by what the initiator specifies in
>>> the TPID field, so it can be either the Transport ID or Initiator Port
>>> Transport ID.
>>
>> Hello Mike,
>>
>> Since the ISID is iSCSI-specific I think that all code that knows about the
>> ISID and its encoding should be in the iSCSI target driver instead of the
>> target core. Do you think an approach similar to that of the SCST function
>> iscsi_get_initiator_port_transport_id() can be implemented in LIO? The caller
> 
> Yeah, I can make that work.

Hey Bart and Christoph,

Bart, I noticed we basically had what you are requesting but Christoph
had moved the id code from the fabric drivers to lio core in this commit:

commit 2650d71e244fb3637b5f58a0080682a8bf9c7091
Author: Christoph Hellwig <hch@lst.de>
Date: Fri May 1 17:47:58 2015 +0200

target: move transport ID handling to the core


So what do you guys want to do here? Revert Christoph's patch and go
back to that style or have lio core do the transport ID processing?


> 
>> of that function is in source file scst/src/scst_targ.c:
>>
>> 		res = sess->tgt->tgtt->get_initiator_port_transport_id(
>> 					sess->tgt, sess, &sess->transport_id);
>>
>> Thanks,
>>
>> Bart.--
>> To unsubscribe from this list: send the line "unsubscribe target-devel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe target-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 02/15] target: fix isid copying and comparision

[Mike Christie]

On 07/20/2018 04:08 PM, Mike Christie wrote:
> On 07/19/2018 11:13 AM, Mike Christie wrote:
>> > On 07/19/2018 10:15 AM, Bart Van Assche wrote:
>>> >> On Wed, 2018-07-18 at 20:02 -0500, Mike Christie wrote:
>>>> >>> On 07/18/2018 07:03 PM, Mike Christie wrote:
>>>>> >>>> On 07/18/2018 05:09 PM, Bart Van Assche wrote:
>>>>>> >>>>> [ ... ]
>>>>>> >>>>> is that these involve a transport ID and that that transport ID can be up to 228
>>>>>> >>>>> bytes long for iSCSI.
>>>>> >>>>
>>>>> >>>> I am talking about the Initiator Session ID above. That along with the
>>>>> >>>> iscsi name make up the Initiator Port Transport ID. In spc4r37 checkout
>>>>> >>>> table 508 or in SAM 5r21 checkout table A.4.
>>>>> >>>>
>>>>> >>>> So in the SCSI specs as part of the Initiator Port Transport ID we have
>>>>> >>>> this from that SAM table:
>>>>> >>>>
>>>>> >>>> The Initiator Session Identifier (ISID) portion of the string is a UTF-8
>>>>> >>>> encoded hexadecimal representation of a six byte binary value.
>>>>> >>>>
>>>>> >>>> ---
>>>>> >>>>
>>>>> >>>> In the PR parts of SPC it sometimes mentions only "Transport ID" but
>>>>> >>>> then clarifies the initiator port so I am assuming in those cases it
>>>>> >>>> means "Initiator Port Transport ID" so it is both the name and isid for
>>>>> >>>> iscsi.
>>>> >>>
>>>> >>> It looks like we are supposed to go by what the initiator specifies in
>>>> >>> the TPID field, so it can be either the Transport ID or Initiator Port
>>>> >>> Transport ID.
>>> >>
>>> >> Hello Mike,
>>> >>
>>> >> Since the ISID is iSCSI-specific I think that all code that knows about the
>>> >> ISID and its encoding should be in the iSCSI target driver instead of the
>>> >> target core. Do you think an approach similar to that of the SCST function
>>> >> iscsi_get_initiator_port_transport_id() can be implemented in LIO? The caller
>> > 
>> > Yeah, I can make that work.
> Hey Bart and Christoph,
> 
> Bart, I noticed we basically had what you are requesting but Christoph
> had moved the id code from the fabric drivers to lio core in this commit:
> 
> commit 2650d71e244fb3637b5f58a0080682a8bf9c7091
> Author: Christoph Hellwig <hch@lst.de>
> Date: Fri May 1 17:47:58 2015 +0200
> 
> target: move transport ID handling to the core
> 
> 
> So what do you guys want to do here? Revert Christoph's patch and go
> back to that style or have lio core do the transport ID processing?

Oh yeah for the latter, we can make it so at least target_core_pr.c does
not have to do any isid conversions. We could just rename sess_bin_isid
to sess_str_isid, store the isid as a string, and then never convert it
between the binary and string format. All the isid tests in
target_core_pr.c would be strcmps.

Re: [PATCH 02/15] target: fix isid copying and comparision

[Bart Van Assche]

On Fri, 2018-07-20 at 16:08 -0500, Mike Christie wrote:
+AD4- Hey Bart and Christoph,
+AD4- 
+AD4- Bart, I noticed we basically had what you are requesting but Christoph
+AD4- had moved the id code from the fabric drivers to lio core in this commit:
+AD4- 
+AD4- commit 2650d71e244fb3637b5f58a0080682a8bf9c7091
+AD4- Author: Christoph Hellwig +ADw-hch+AEA-lst.de+AD4-
+AD4- Date: Fri May 1 17:47:58 2015 +-0200
+AD4- 
+AD4- target: move transport ID handling to the core

Hello Mike,

I'm not in favor of reverting Christoph's patch because that patch simplified
the target code significantly. On the other hand, it's inconvenient that with
the current approach there is some code and knowledge in the target core that
should be in target drivers. I think that the code for parsing the initiator
name in srp+AF8-get+AF8-pr+AF8-transport+AF8-id() should be in the SRP target driver instead
of the core. When I added support for a new initiator name format in the SRP
target driver I overlooked that I had to update srp+AF8-get+AF8-pr+AF8-transport+AF8-id()
because that function is in the core instead of ib+AF8-srpt.c. See also commit
2dc98f09f9e6 (+ACI-IB/srpt: Use the source GUID as session name+ACI-). Christoph, do
you want me to add support for the new ib+AF8-srpt initiator name format in 
drivers/target/target+AF8-core+AF8-fabric+AF8-lib.c or should I find a way to move the
code for parsing the initiator name into ib+AF8-srpt.c?

Thanks,

Bart.