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

2018-12-05 Thread David Disseldorp
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(-)

diff --git a/drivers/target/target_core_configfs.c 
b/drivers/target/target_core_configfs.c
index f6b1549f4142..8277bcf81d6e 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->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,
+   "INQUIRY_MODEL, truncating to 15 characters\n", dev,
configname);
}
-   snprintf(&dev->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);
 }
 
 static ssize_t emulate_model_alias_store(struct config_item *item,
@@ -640,11 +645,12 @@ static ssize_t emulate_model_alias_store(struct 
config_item *item,
if (ret < 0)
return ret;
 
+   BUILD_BUG_ON(sizeof(dev->t10_wwn.model) != INQUIRY_MODEL_LEN + 1);
if (flag) {
dev_set_t10_wwn_model_alias(dev);
} else {
-   strncpy(&dev->t10_wwn.model[0],
-   dev->transport->inquiry_prod, 16);
+   strlcpy(dev->t10_wwn.model, dev->transport->inquiry_prod,
+   sizeof(dev->t10_wwn.model));
}
da->emulate_model_alias = flag;
return count;
diff --git a/drivers/target/target_core_device.c 
b/drivers/target/target_core_device.c
index 47b5ef153135..ebd787bb29a8 100644
--- a/drivers/target/target_core_device.c
+++ b/drivers/target/target_core_device.c
@@ -720,36 +720,17 @@ void core_dev_free_initiator_node_lun_acl(
 static void scsi_dump_inquiry(struct se_device *dev)
 {
struct t10_wwn *wwn = &dev->t10_wwn;
-   char buf[17];
-   int i, device_type;
+   int device_type = dev->transport->get_device_type(dev);
+
/*
 * Print Linux/SCSI style INQUIRY formatting to the kernel ring buffer
 */
-   for (i = 0; i < 8; i++)
-   if (wwn->vendor[i] >= 0x20)
-   buf[i] = wwn->vendor[i];
-   else
-   buf[i] = ' ';
-   buf[i] = '\0';
-   pr_debug("  Vendor: %s\n", buf);
-
-   for (i = 0; i < 16; i++)
-   if (wwn->model[i] >= 0x20)
-   buf[i] = wwn->model[i];
-   else
-   buf[i] = ' ';
-   buf[i] = '\0';
-   pr_debug("  Model: %s\n", buf);
-
-   for (i = 0; i < 4; i++)
-   if (wwn->revision[i] >= 0x20)
-   buf[i] = wwn->revision[i];
-   else
-   buf[i] = ' ';
-   buf[i] = '\0';
-   pr_debug("  Revision: %s\n", buf);
-
-   device_type = dev->transport->get_device_type(dev);
+   pr_debug("  Vendor: %-" __stringify(INQUIRY_VENDOR_LEN) "s\n",
+   wwn->vendor);
+   pr_debug("  Model: %-" __stringify(INQUIRY_MODEL_LEN) "s\n",
+   wwn->model);
+   pr_debug("  Revision: %-" __stringify(INQUIRY_REVISION_LEN) "s\n",
+   wwn->revision);
pr_debug("  Type:   %s ", scsi_device_type(device_type));
 }
 
@@ -1009,11 +990,12 @@ int target_configure_device(struct se_device *dev)
 * passthrough because this is being provided by the backend LLD.
 */
if (!(dev->transport->transport_flags & TRANSPORT_FLAG_PASSTHROUGH)) {
-   strncpy(&dev->t10_wwn.vendor[0], "LIO

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