Re: [PATCH v7 2/5] target: consistently null-terminate t10_wwn strings

2018-12-06 Thread Roman Bolshakov 
On Wed, Dec 05, 2018 at 01:18:35PM +0100, David Disseldorp wrote:
> In preparation for supporting user provided vendor strings, add an extra
> byte to the vendor, model and revision arrays in struct t10_wwn. This
> ensures that the full INQUIRY data can be carried in the arrays along
> with a null-terminator.
> 
> Change a number of array readers and writers so that they account for
> explicit null-termination:
> - The pscsi_set_inquiry_info() and emulate_model_alias_store() codepaths
>   don't currently explicitly null-terminate; fix this.
> - Existing t10_wwn field dumps use for-loops which step over
>   null-terminators for right-padding.
>   + Use printf with width specifiers instead.
> 
> Signed-off-by: David Disseldorp 
> ---
>  drivers/target/target_core_configfs.c | 16 +++
>  drivers/target/target_core_device.c   | 46 ++--
>  drivers/target/target_core_pscsi.c| 50 
> +++
>  drivers/target/target_core_spc.c  |  7 ++---
>  drivers/target/target_core_stat.c | 32 +-
>  include/target/target_core_base.h | 14 +++---
>  6 files changed, 63 insertions(+), 102 deletions(-)
> 

Reviewed-by: Roman Bolshakov 

Thank you,
Roman Bolshakov

Re: [PATCH v7 3/5] target: add device vendor_id configfs attribute

2018-12-06 Thread Roman Bolshakov 
On Wed, Dec 05, 2018 at 01:18:36PM +0100, David Disseldorp wrote:
> The vendor_id attribute will allow for the modification of the T10
> Vendor Identification string returned in inquiry responses. Its value
> can be viewed and modified via the ConfigFS path at:
> target/core/$backstore/$name/wwn/vendor_id
> 
> "LIO-ORG" remains the default value, which is set when the backstore
> device is enabled.
> 
> Signed-off-by: David Disseldorp 
> ---
>  drivers/target/target_core_configfs.c | 70 
> +++
>  1 file changed, 70 insertions(+)
> 
> diff --git a/drivers/target/target_core_configfs.c 
> b/drivers/target/target_core_configfs.c
> index 8277bcf81d6e..f099c2ae451f 100644
> --- a/drivers/target/target_core_configfs.c
> +++ b/drivers/target/target_core_configfs.c
> @@ -1217,6 +1217,74 @@ static struct t10_wwn *to_t10_wwn(struct config_item 
> *item)
>  }
>  
>  /*
> + * STANDARD and VPD page 0x80 T10 Vendor Identification

Perhaps you meant 0x83 (Device Identification VPD page, T10 vendor ID
based designator). INQUIRY page 0x80 is Unit Serial Number.

Besides that,
Reviewed-by: Roman Bolshakov 

Thank you,
Roman

Re: [PATCH v6 3/5] target: add device vendor_id configfs attribute

2018-12-04 Thread Roman Bolshakov 
On Tue, Dec 04, 2018 at 03:13:51PM +0300, Roman Bolshakov wrote:
> On Tue, Dec 04, 2018 at 11:12:36AM +0100, David Disseldorp wrote:
> > The vendor_id attribute will allow for the modification of the T10
> > Vendor Identification string returned in inquiry responses. Its value
> > can be viewed and modified via the ConfigFS path at:
> > target/core/$backstore/$name/wwn/vendor_id
> > 
> > "LIO-ORG" remains the default value, which is set when the backstore
> > device is enabled.
> > 
> > Signed-off-by: David Disseldorp 
> > Reviewed-by: Bryant G. Ly 
> > Reviewed-by: Lee Duncan 
> > Reviewed-by: Hannes Reinecke 
> > ---
> >  drivers/target/target_core_configfs.c | 48 
> > +++
> >  1 file changed, 48 insertions(+)
> > 
> > diff --git a/drivers/target/target_core_configfs.c 
> > b/drivers/target/target_core_configfs.c
> > index 34872f24e8bf..67303c3f9cb4 100644
> > --- a/drivers/target/target_core_configfs.c
> > +++ b/drivers/target/target_core_configfs.c
> > +
> > +   /* Assume ASCII encoding. Strip any newline added from userspace. */
> > +   BUILD_BUG_ON(sizeof(dev->t10_wwn.vendor) != INQUIRY_VENDOR_LEN + 1);
> > +   strlcpy(dev->t10_wwn.vendor, strstrip(buf),
> > +   sizeof(dev->t10_wwn.vendor));
> > +
> 
> Should we allow non-ASCII characters? It's okay to strip final newline
> for convenience. A simple loop would ensure the rest is conformant to
> SPC. EINVAL can be returned otherwise.
> 
> And for fuzzy purposes there could be a special backstore that does all
> sorts of nasty things.
> 
> According to SPC 4.3.1:
> ASCII data fields shall contain only ASCII printable characters (i.e.,
> code values 20h to 7Eh) and may be terminated with one or more ASCII
> null (00h) characters.
> 
> 3.3.10 shall
> keyword indicating a mandatory requirement
> Note 1 to entry: Designers are required to implement all such
> interoperability with other products that conform to this standard.
> 
> Thank you,
> Roman

wrt PATCH 5 in the series. Should we allow to set vendor_id for for
pscsi? target_transport_configure sets t10_wwn fields for pscsi, but but
an attempt to set vendor_id will overwrite the value recieved from
scsi_device.

And (please correct me if I'm wrong) it's not used anywhere except
statistics config_group. That means in the actual INQUIRY commands it
will still return vendor_id of the underlying storage. If that's that's
true, this means an attempt to set vendor_id will be succesful but it
won't do what's intended for.

Perhaps -ENOSYS or an error code of your preference could be returned if
device has TRANSPORT_FLAG_PASSTHROUGH.

Roman

Re: [PATCH v6 5/5] target: perform t10_wwn ID initialisation in target_alloc_device()

2018-12-04 Thread Roman Bolshakov 
On Tue, Dec 04, 2018 at 11:12:38AM +0100, David Disseldorp wrote:
> Initialise the t10_wwn vendor, model and revision defaults when a
> device is allocated instead of when it's enabled. This ensures that
> custom vendor or model strings set prior to enablement are not later
> overwritten with default values.
> 
> The TRANSPORT_FLAG_PASSTHROUGH conditional can be dropped for the
> following reasons:
> - target_core_pscsi overwrites the defaults in the
>   pscsi_configure_device() callback.
>   + the contents is then only used for ConfigFS via
> $pscsi_dev/info, $pscsi_dev/statistics/scsi_lu/vend, etc.
> - target_core_user doesn't touch the defaults, nor are they used for
>   anything outside of ConfigFS.
> 
> Signed-off-by: David Disseldorp 
> ---
>  drivers/target/target_core_device.c | 27 ++-
>  1 file changed, 10 insertions(+), 17 deletions(-)
> 

Reviewed-by: Roman Bolshakov 

Thanks,
Roman

Re: [PATCH v6 4/5] target: remove hardcoded T10 Vendor ID in INQUIRY response

2018-12-04 Thread Roman Bolshakov 
On Tue, Dec 04, 2018 at 11:12:37AM +0100, 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 
> Reviewed-by: Bryant G. Ly 
> Reviewed-by: Lee Duncan 
> Reviewed-by: Hannes Reinecke 
> ---
>  drivers/target/target_core_spc.c | 8 +---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 

Reviewed-by: Roman Bolshakov 

Thank you,
Roman

Re: [PATCH v6 3/5] target: add device vendor_id configfs attribute

2018-12-04 Thread Roman Bolshakov 
On Tue, Dec 04, 2018 at 11:12:36AM +0100, David Disseldorp wrote:
> The vendor_id attribute will allow for the modification of the T10
> Vendor Identification string returned in inquiry responses. Its value
> can be viewed and modified via the ConfigFS path at:
> target/core/$backstore/$name/wwn/vendor_id
> 
> "LIO-ORG" remains the default value, which is set when the backstore
> device is enabled.
> 
> Signed-off-by: David Disseldorp 
> Reviewed-by: Bryant G. Ly 
> Reviewed-by: Lee Duncan 
> Reviewed-by: Hannes Reinecke 
> ---
>  drivers/target/target_core_configfs.c | 48 
> +++
>  1 file changed, 48 insertions(+)
> 
> diff --git a/drivers/target/target_core_configfs.c 
> b/drivers/target/target_core_configfs.c
> index 34872f24e8bf..67303c3f9cb4 100644
> --- a/drivers/target/target_core_configfs.c
> +++ b/drivers/target/target_core_configfs.c
> +
> + /* Assume ASCII encoding. Strip any newline added from userspace. */
> + BUILD_BUG_ON(sizeof(dev->t10_wwn.vendor) != INQUIRY_VENDOR_LEN + 1);
> + strlcpy(dev->t10_wwn.vendor, strstrip(buf),
> + sizeof(dev->t10_wwn.vendor));
> +

Should we allow non-ASCII characters? It's okay to strip final newline
for convenience. A simple loop would ensure the rest is conformant to
SPC. EINVAL can be returned otherwise.

And for fuzzy purposes there could be a special backstore that does all
sorts of nasty things.

According to SPC 4.3.1:
ASCII data fields shall contain only ASCII printable characters (i.e.,
code values 20h to 7Eh) and may be terminated with one or more ASCII
null (00h) characters.

3.3.10 shall
keyword indicating a mandatory requirement
Note 1 to entry: Designers are required to implement all such
interoperability with other products that conform to this standard.

Thank you,
Roman

Re: [PATCH v5 2/5] target: consistently null-terminate t10_wwn strings

2018-12-04 Thread Roman Bolshakov 
On Sat, Dec 01, 2018 at 12:34:20AM +0100, David Disseldorp wrote:
> In preparation for supporting user provided vendor strings, add an extra
> byte to the vendor, model and revision arrays in struct t10_wwn. This
> ensures that the full INQUIRY data can be carried in the arrays along
> with a null-terminator.
> 
> Change a number of array readers and writers so that they account for
> explicit null-termination:
> - The pscsi_set_inquiry_info() and emulate_model_alias_store() codepaths
>   don't currently explicitly null-terminate; fix this.
> - Existing t10_wwn field dumps use for-loops which step over
>   null-terminators for right-padding.
>   + Use printf with width specifiers instead.
> 
> Signed-off-by: David Disseldorp 
> ---
>  drivers/target/target_core_configfs.c | 14 +++---
>  drivers/target/target_core_device.c   | 49 
> ---
>  drivers/target/target_core_pscsi.c| 18 -
>  drivers/target/target_core_spc.c  |  7 ++---
>  drivers/target/target_core_stat.c | 32 +--
>  include/target/target_core_base.h | 14 +++---
>  6 files changed, 61 insertions(+), 73 deletions(-)
> 
> diff --git a/drivers/target/target_core_configfs.c 
> b/drivers/target/target_core_configfs.c
> index f6b1549f4142..34872f24e8bf 100644
> --- a/drivers/target/target_core_configfs.c
> +++ b/drivers/target/target_core_configfs.c
> @@ -613,12 +613,17 @@ static void dev_set_t10_wwn_model_alias(struct 
> se_device *dev)
>   const char *configname;
>  
>   configname = config_item_name(>dev_group.cg_item);
> - if (strlen(configname) >= 16) {
> + if (strlen(configname) >= INQUIRY_MODEL_LEN) {
>   pr_warn("dev[%p]: Backstore name '%s' is too long for "
>   "INQUIRY_MODEL, truncating to 16 bytes\n", dev,

The warning (which I understand predates your patch) is misleading, it
should mention truncation to 15 instead of 16 bytes and your comment
just below explains this.

>   configname);
>   }
> - snprintf(>t10_wwn.model[0], 16, "%s", configname);
> + /*
> +  * XXX We can't use sizeof(dev->t10_wwn.model) (INQUIRY_MODEL_LEN + 1)
> +  * here without potentially breaking existing setups, so continue to
> +  * truncate one byte shorter than what can be carried in INQUIRY.
> +  */
> + strlcpy(dev->t10_wwn.model, configname, INQUIRY_MODEL_LEN);
>  }
>  

> diff --git a/drivers/target/target_core_device.c 
> b/drivers/target/target_core_device.c
> index 47b5ef153135..5512871f50e4 100644
> --- a/drivers/target/target_core_device.c
> +++ b/drivers/target/target_core_device.c
> @@ -1008,12 +989,16 @@ int target_configure_device(struct se_device *dev)
>* anything virtual (IBLOCK, FILEIO, RAMDISK), but not for TCM/pSCSI
>* passthrough because this is being provided by the backend LLD.
>*/
> + BUILD_BUG_ON(sizeof(dev->t10_wwn.vendor) != INQUIRY_VENDOR_LEN + 1);
> + BUILD_BUG_ON(sizeof(dev->t10_wwn.model) != INQUIRY_MODEL_LEN + 1);
> + BUILD_BUG_ON(sizeof(dev->t10_wwn.revision) != INQUIRY_REVISION_LEN + 1);


I'm sorry I'm missing something. Why BUILD_BUG_ON is added in many
places?

> diff --git a/drivers/target/target_core_pscsi.c 
> b/drivers/target/target_core_pscsi.c
> index 47d76c862014..1002829f2038 100644
> --- a/drivers/target/target_core_pscsi.c
> +++ b/drivers/target/target_core_pscsi.c
> @@ -190,9 +190,15 @@ pscsi_set_inquiry_info(struct scsi_device *sdev, struct 
> t10_wwn *wwn)
>   /*
>* Use sdev->inquiry from drivers/scsi/scsi_scan.c:scsi_alloc_sdev()
>*/
> - memcpy(>vendor[0], [8], sizeof(wwn->vendor));
> - memcpy(>model[0], [16], sizeof(wwn->model));
> - memcpy(>revision[0], [32], sizeof(wwn->revision));
> + BUILD_BUG_ON(sizeof(wwn->vendor) != INQUIRY_VENDOR_LEN + 1);
> + snprintf(wwn->vendor, sizeof(wwn->vendor),
> +  "%." __stringify(INQUIRY_VENDOR_LEN) "s", [8]);
> + BUILD_BUG_ON(sizeof(wwn->model) != INQUIRY_MODEL_LEN + 1);
> + snprintf(wwn->model, sizeof(wwn->model),
> +  "%." __stringify(INQUIRY_MODEL_LEN) "s", [16]);
> + BUILD_BUG_ON(sizeof(wwn->revision) != INQUIRY_REVISION_LEN + 1);
> + snprintf(wwn->revision, sizeof(wwn->revision),
> +  "%." __stringify(INQUIRY_REVISION_LEN) "s", [32]);
>  }
>  

The parts of the sdev->inquiry have been already right-padded with
spaces by scsi_sanitize_inquiry_string in scsi_probe_lun. Thus, it's
enough to replace sizeof with the new length definitions. Also, it's
possible to use sdev->model,vendor,rev pointers like in
pscsi_show_configfs_dev_params instead of explicit offsets [8],
[16], [32].

>  static int
> @@ -826,21 +832,21 @@ static ssize_t pscsi_show_configfs_dev_params(struct 
> se_device *dev, char *b)
>   if (sd) {
>   bl += sprintf(b + bl, "");
>   bl += sprintf(b + bl, "Vendor: ");
> - for (i = 0; i < 8; i++) {
> +

Re: [PATCH v5 1/5] target: use consistent left-aligned ASCII INQUIRY data

2018-12-04 Thread Roman Bolshakov 
On Sat, Dec 01, 2018 at 12:34:19AM +0100, David Disseldorp wrote:
> spc5r17.pdf specifies:
>   4.3.1 ASCII data field requirements
>   ASCII data fields shall contain only ASCII printable characters (i.e.,
>   code values 20h to 7Eh) and may be terminated with one or more ASCII
>   null (00h) characters.
>   ASCII data fields described as being left-aligned shall have any
>   unused bytes at the end of the field (i.e., highest offset) and the
>   unused bytes shall be filled with ASCII space characters (20h).
> 
> LIO currently space-pads the T10 VENDOR IDENTIFICATION and PRODUCT
> IDENTIFICATION fields in the standard INQUIRY data. However, the
> PRODUCT REVISION LEVEL field in the standard INQUIRY data as well as the
> T10 VENDOR IDENTIFICATION field in the INQUIRY Device Identification VPD
> Page are zero-terminated/zero-padded.
> 
> Fix this inconsistency by using space-padding for all of the above
> fields.
> 
> Signed-off-by: David Disseldorp 
> Reviewed-by: Christoph Hellwig 
> Reviewed-by: Bryant G. Ly 
> Reviewed-by: Lee Duncan 
> ---
>  drivers/target/target_core_spc.c | 17 -----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 

Reviewed-by: Roman Bolshakov 

Best regards,
Roman