[PATCH v4 6/7] target: remove hardcoded T10 Vendor ID in INQUIRY response

[PATCH v4 6/7] target: remove hardcoded T10 Vendor ID in INQUIRY response

[David Disseldorp]

Use the value stored in t10_wwn.vendor, which defaults to "LIO-ORG", but
can be reconfigured via the vendor_id ConfigFS attribute.

Signed-off-by: David Disseldorp <ddiss@suse.de>
---
 drivers/target/target_core_spc.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/target/target_core_spc.c b/drivers/target/target_core_spc.c
index 8ffe712cb44d..4503f3336bc2 100644
--- a/drivers/target/target_core_spc.c
+++ b/drivers/target/target_core_spc.c
@@ -115,7 +115,8 @@ spc_emulate_inquiry_std(struct se_cmd *cmd, unsigned char *buf)
 	 */
 	memset(&buf[8], 0x20,
 	       INQUIRY_VENDOR_LEN + INQUIRY_MODEL_LEN + INQUIRY_REVISION_LEN);
-	memcpy(&buf[8], "LIO-ORG", sizeof("LIO-ORG") - 1);
+	memcpy(&buf[8], dev->t10_wwn.vendor,
+	       strnlen(dev->t10_wwn.vendor, INQUIRY_VENDOR_LEN));
 	memcpy(&buf[16], dev->t10_wwn.model,
 	       strnlen(dev->t10_wwn.model, INQUIRY_MODEL_LEN));
 	memcpy(&buf[32], dev->t10_wwn.revision,
@@ -258,8 +259,9 @@ spc_emulate_evpd_83(struct se_cmd *cmd, unsigned char *buf)
 	buf[off+1] = 0x1; /* T10 Vendor ID */
 	buf[off+2] = 0x0;
 	/* left align Vendor ID and pad with spaces */
-	memset(&buf[off+4], 0x20, 8);
-	memcpy(&buf[off+4], "LIO-ORG", sizeof("LIO-ORG") - 1);
+	memset(&buf[off+4], 0x20, INQUIRY_VENDOR_LEN);
+	memcpy(&buf[off+4], dev->t10_wwn.vendor,
+	       strnlen(dev->t10_wwn.vendor, INQUIRY_VENDOR_LEN));
 	/* Extra Byte for NULL Terminator */
 	id_len++;
 	/* Identifier Length */
-- 
2.13.7

Re: [PATCH v4 6/7] target: remove hardcoded T10 Vendor ID in INQUIRY response

[Ly, Bryant]

> On Nov 28, 2018, at 7:01 PM, David Disseldorp <ddiss@suse.de> wrote:
> 
> Use the value stored in t10_wwn.vendor, which defaults to "LIO-ORG", but
> can be reconfigured via the vendor_id ConfigFS attribute.
> 
> Signed-off-by: David Disseldorp <ddiss@suse.de>
> ---
> drivers/target/target_core_spc.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
> 

Reviewed-by: Bryant G. Ly bly@catalogicsoftware.com

Re: [PATCH v4 6/7] target: remove hardcoded T10 Vendor ID in INQUIRY response

[Lee Duncan]

On 11/28/18 5:01 PM, David Disseldorp wrote:
> Use the value stored in t10_wwn.vendor, which defaults to "LIO-ORG", but
> can be reconfigured via the vendor_id ConfigFS attribute.
> 
> Signed-off-by: David Disseldorp <ddiss@suse.de>
> ---
>  drivers/target/target_core_spc.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/target/target_core_spc.c b/drivers/target/target_core_spc.c
> index 8ffe712cb44d..4503f3336bc2 100644
> --- a/drivers/target/target_core_spc.c
> +++ b/drivers/target/target_core_spc.c
> @@ -115,7 +115,8 @@ spc_emulate_inquiry_std(struct se_cmd *cmd, unsigned char *buf)
>  	 */
>  	memset(&buf[8], 0x20,
>  	       INQUIRY_VENDOR_LEN + INQUIRY_MODEL_LEN + INQUIRY_REVISION_LEN);
> -	memcpy(&buf[8], "LIO-ORG", sizeof("LIO-ORG") - 1);
> +	memcpy(&buf[8], dev->t10_wwn.vendor,
> +	       strnlen(dev->t10_wwn.vendor, INQUIRY_VENDOR_LEN));
>  	memcpy(&buf[16], dev->t10_wwn.model,
>  	       strnlen(dev->t10_wwn.model, INQUIRY_MODEL_LEN));
>  	memcpy(&buf[32], dev->t10_wwn.revision,
> @@ -258,8 +259,9 @@ spc_emulate_evpd_83(struct se_cmd *cmd, unsigned char *buf)
>  	buf[off+1] = 0x1; /* T10 Vendor ID */
>  	buf[off+2] = 0x0;
>  	/* left align Vendor ID and pad with spaces */
> -	memset(&buf[off+4], 0x20, 8);
> -	memcpy(&buf[off+4], "LIO-ORG", sizeof("LIO-ORG") - 1);
> +	memset(&buf[off+4], 0x20, INQUIRY_VENDOR_LEN);
> +	memcpy(&buf[off+4], dev->t10_wwn.vendor,
> +	       strnlen(dev->t10_wwn.vendor, INQUIRY_VENDOR_LEN));
>  	/* Extra Byte for NULL Terminator */
>  	id_len++;
>  	/* Identifier Length */
> 


Reviewed-by: Lee Duncan <lduncan@suse.com>

Re: [PATCH v4 6/7] target: remove hardcoded T10 Vendor ID in INQUIRY response

[Bart Van Assche]

On Thu, 2018-11-29 at 02:01 +-0100, David Disseldorp wrote:
+AD4 Use the value stored in t10+AF8-wwn.vendor, which defaults to +ACI-LIO-ORG+ACI, but
+AD4 can be reconfigured via the vendor+AF8-id ConfigFS attribute.
+AD4 
+AD4 Signed-off-by: David Disseldorp +ADw-ddiss+AEA-suse.de+AD4
+AD4 ---
+AD4  drivers/target/target+AF8-core+AF8-spc.c +AHw 8 +-+-+-+-+----
+AD4  1 file changed, 5 insertions(+-), 3 deletions(-)
+AD4 
+AD4 diff --git a/drivers/target/target+AF8-core+AF8-spc.c b/drivers/target/target+AF8-core+AF8-spc.c
+AD4 index 8ffe712cb44d..4503f3336bc2 100644
+AD4 --- a/drivers/target/target+AF8-core+AF8-spc.c
+AD4 +-+-+- b/drivers/target/target+AF8-core+AF8-spc.c
+AD4 +AEAAQA -115,7 +-115,8 +AEAAQA spc+AF8-emulate+AF8-inquiry+AF8-std(struct se+AF8-cmd +ACo-cmd, unsigned char +ACo-buf)
+AD4  	 +ACo-/
+AD4  	memset(+ACY-buf+AFs-8+AF0, 0x20,
+AD4  	       INQUIRY+AF8-VENDOR+AF8-LEN +- INQUIRY+AF8-MODEL+AF8-LEN +- INQUIRY+AF8-REVISION+AF8-LEN)+ADs
+AD4 -	memcpy(+ACY-buf+AFs-8+AF0, +ACI-LIO-ORG+ACI, sizeof(+ACI-LIO-ORG+ACI) - 1)+ADs
+AD4 +-	memcpy(+ACY-buf+AFs-8+AF0, dev-+AD4-t10+AF8-wwn.vendor,
+AD4 +-	       strnlen(dev-+AD4-t10+AF8-wwn.vendor, INQUIRY+AF8-VENDOR+AF8-LEN))+ADs
+AD4  	memcpy(+ACY-buf+AFs-16+AF0, dev-+AD4-t10+AF8-wwn.model,
+AD4  	       strnlen(dev-+AD4-t10+AF8-wwn.model, INQUIRY+AF8-MODEL+AF8-LEN))+ADs
+AD4  	memcpy(+ACY-buf+AFs-32+AF0, dev-+AD4-t10+AF8-wwn.revision,
+AD4 +AEAAQA -258,8 +-259,9 +AEAAQA spc+AF8-emulate+AF8-evpd+AF8-83(struct se+AF8-cmd +ACo-cmd, unsigned char +ACo-buf)
+AD4  	buf+AFs-off+-1+AF0 +AD0 0x1+ADs /+ACo T10 Vendor ID +ACo-/
+AD4  	buf+AFs-off+-2+AF0 +AD0 0x0+ADs
+AD4  	/+ACo left align Vendor ID and pad with spaces +ACo-/
+AD4 -	memset(+ACY-buf+AFs-off+-4+AF0, 0x20, 8)+ADs
+AD4 -	memcpy(+ACY-buf+AFs-off+-4+AF0, +ACI-LIO-ORG+ACI, sizeof(+ACI-LIO-ORG+ACI) - 1)+ADs
+AD4 +-	memset(+ACY-buf+AFs-off+-4+AF0, 0x20, INQUIRY+AF8-VENDOR+AF8-LEN)+ADs
+AD4 +-	memcpy(+ACY-buf+AFs-off+-4+AF0, dev-+AD4-t10+AF8-wwn.vendor,
+AD4 +-	       strnlen(dev-+AD4-t10+AF8-wwn.vendor, INQUIRY+AF8-VENDOR+AF8-LEN))+ADs
+AD4  	/+ACo Extra Byte for NULL Terminator +ACo-/
+AD4  	id+AF8-len+-+-+ADs
+AD4  	/+ACo Identifier Length +ACo-/

Where is the code that initializes dev-+AD4-t10+AF8-wwn.vendor to +ACI-LIO-ORG+ACI? Did I
perhaps overlook something? Additionally, why are you using strnlen() for
a string of which it is guaranteed that its length is less than or equal to
the second strnlen() argument?

Thanks,

Bart.

Re: [PATCH v4 6/7] target: remove hardcoded T10 Vendor ID in INQUIRY response

[David Disseldorp]

On Thu, 29 Nov 2018 08:35:26 -0800, Bart Van Assche wrote:

> Where is the code that initializes dev->t10_wwn.vendor to "LIO-ORG"? Did I
> perhaps overlook something?

This is done in target_configure_device() .

> Additionally, why are you using strnlen() for
> a string of which it is guaranteed that its length is less than or equal to
> the second strnlen() argument?

Belt and braces :) Actually, I think it's a little more readable this
way.

Cheers, David

Re: [PATCH v4 6/7] target: remove hardcoded T10 Vendor ID in INQUIRY response

[David Disseldorp]

On Fri, 30 Nov 2018 14:17:49 +0100, David Disseldorp wrote:

> > Where is the code that initializes dev->t10_wwn.vendor to "LIO-ORG"? Did I
> > perhaps overlook something?  
> 
> This is done in target_configure_device() .

Hmm, continuing to do it there may cause a bit of confusion if vendor_id
is set before the backstore is enabled, which would cause it to be
overwritten. That said, we already have similarly strange behaviour for
emulate_model_alias / product. E.g.:

rapido1:/# cd /sys/kernel/config/target/
rapido1:target# mkdir -p core/fileio_1/testing
rapido1:target# echo -n "fd_dev_name=/asdf,fd_dev_size@96" > core/fileio_1/testing/control
rapido1:target# echo -n "testing1" > core/fileio_1/testing/wwn/vendor_id
rapido1:target# cat core/fileio_1/testing/wwn/vendor_id
testing1
rapido1:target# echo 1 > ./core/fileio_1/testing/attrib/emulate_model_alias
rapido1:target# cat ./core/fileio_1/testing/statistics/scsi_lu/prod
testing
rapido1:target# echo 1 > core/fileio_1/testing/enable
rapido1:target# cat core/fileio_1/testing/wwn/vendor_id
LIO-ORG
rapido1:target# cat ./core/fileio_1/testing/statistics/scsi_lu/prod
FILEIO
rapido1:target# cat ./core/fileio_1/testing/attrib/emulate_model_alias
1

Not sure how best to handle this...
1. move vendor/model/rev initialization into target_alloc_device()
2. move vendor (only) initialization into target_alloc_device()
3. fail attempts to set emulate_model_alias or vendor_id before the
   backstore has been enabled
4. leave as-is

(1) would IMO be the most straightforward, but it's a slight change to
the existing (IMO broken) emulate_model_alias user interface.

Cheers, David

Re: [PATCH v4 6/7] target: remove hardcoded T10 Vendor ID in INQUIRY response

[Bart Van Assche]

On Fri, 2018-11-30 at 15:41 +-0100, David Disseldorp wrote:
+AD4 On Fri, 30 Nov 2018 14:17:49 +-0100, David Disseldorp wrote:
+AD4 
+AD4 +AD4 +AD4 Where is the code that initializes dev-+AD4-t10+AF8-wwn.vendor to +ACI-LIO-ORG+ACI? Did I
+AD4 +AD4 +AD4 perhaps overlook something?  
+AD4 +AD4 
+AD4 +AD4 This is done in target+AF8-configure+AF8-device() .
+AD4 
+AD4 Hmm, continuing to do it there may cause a bit of confusion if vendor+AF8-id
+AD4 is set before the backstore is enabled, which would cause it to be
+AD4 overwritten. That said, we already have similarly strange behaviour for
+AD4 emulate+AF8-model+AF8-alias / product. E.g.:
+AD4 
+AD4 rapido1:/+ACM cd /sys/kernel/config/target/
+AD4 rapido1:target+ACM mkdir -p core/fileio+AF8-1/testing
+AD4 rapido1:target+ACM echo -n +ACI-fd+AF8-dev+AF8-name+AD0-/asdf,fd+AF8-dev+AF8-size+AD0-4096+ACI +AD4 core/fileio+AF8-1/testing/control
+AD4 rapido1:target+ACM echo -n +ACI-testing1+ACI +AD4 core/fileio+AF8-1/testing/wwn/vendor+AF8-id
+AD4 rapido1:target+ACM cat core/fileio+AF8-1/testing/wwn/vendor+AF8-id
+AD4 testing1
+AD4 rapido1:target+ACM echo 1 +AD4 ./core/fileio+AF8-1/testing/attrib/emulate+AF8-model+AF8-alias
+AD4 rapido1:target+ACM cat ./core/fileio+AF8-1/testing/statistics/scsi+AF8-lu/prod
+AD4 testing
+AD4 rapido1:target+ACM echo 1 +AD4 core/fileio+AF8-1/testing/enable
+AD4 rapido1:target+ACM cat core/fileio+AF8-1/testing/wwn/vendor+AF8-id
+AD4 LIO-ORG
+AD4 rapido1:target+ACM cat ./core/fileio+AF8-1/testing/statistics/scsi+AF8-lu/prod
+AD4 FILEIO
+AD4 rapido1:target+ACM cat ./core/fileio+AF8-1/testing/attrib/emulate+AF8-model+AF8-alias
+AD4 1
+AD4 
+AD4 Not sure how best to handle this...
+AD4 1. move vendor/model/rev initialization into target+AF8-alloc+AF8-device()
+AD4 2. move vendor (only) initialization into target+AF8-alloc+AF8-device()
+AD4 3. fail attempts to set emulate+AF8-model+AF8-alias or vendor+AF8-id before the
+AD4    backstore has been enabled
+AD4 4. leave as-is
+AD4 
+AD4 (1) would IMO be the most straightforward, but it's a slight change to
+AD4 the existing (IMO broken) emulate+AF8-model+AF8-alias user interface.

I'm in favor of moving some of the target+AF8-configure+AF8-device() code into the
target+AF8-alloc+AF8-device() function. Today target+AF8-configure+AF8-device() overwrites
the vendor, model and revision string and is called when +ACI-1+ACI is written
into the +ACI-enable+ACI attribute. Overwriting these attributes when enabling a
backstore seems wrong to me.

Thanks,

Bart.