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

[David Disseldorp]

On Thu, 6 Dec 2018 15:25:42 +0300, Roman Bolshakov wrote:

> >  /*
> > + * 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.

Indeed, the comment should refer to page 0x83.
@Martin: all patches in this series have now been reviewed+acked. Can
 you fix the above comment (s/0x80/0x83) if/when you merge, or
 should I resend the series with this fixed?

Cheers, David

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

[David Disseldorp]

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
+ */
+static ssize_t target_wwn_vendor_id_show(struct config_item *item,
+   char *page)
+{
+   return sprintf(page, "%s\n", _t10_wwn(item)->vendor[0]);
+}
+
+static ssize_t target_wwn_vendor_id_store(struct config_item *item,
+   const char *page, size_t count)
+{
+   struct t10_wwn *t10_wwn = to_t10_wwn(item);
+   struct se_device *dev = t10_wwn->t10_dev;
+   /* +2 to allow for a trailing (stripped) '\n' and null-terminator */
+   unsigned char buf[INQUIRY_VENDOR_LEN + 2];
+   char *stripped = NULL;
+   size_t len;
+   int i;
+
+   len = strlcpy(buf, page, sizeof(buf));
+   if (len < sizeof(buf)) {
+   /* Strip any newline added from userspace. */
+   stripped = strstrip(buf);
+   len = strlen(stripped);
+   }
+   if (len > INQUIRY_VENDOR_LEN) {
+   pr_err("Emulated T10 Vendor Identification exceeds"
+   " INQUIRY_VENDOR_LEN: " __stringify(INQUIRY_VENDOR_LEN)
+   "\n");
+   return -EOVERFLOW;
+   }
+
+   /*
+* 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.
+*/
+   for (i = 0; i < len; i++) {
+   if ((stripped[i] < 0x20) || (stripped[i] > 0x7E)) {
+   pr_err("Emulated T10 Vendor Identification contains"
+   " non-ASCII-printable characters\n");
+   return -EINVAL;
+   }
+   }
+
+   /*
+* Check to see if any active exports exist.  If they do exist, fail
+* here as changing this information on the fly (underneath the
+* initiator side OS dependent multipath code) could cause negative
+* effects.
+*/
+   if (dev->export_count) {
+   pr_err("Unable to set T10 Vendor Identification while"
+   " active %d exports exist\n", dev->export_count);
+   return -EINVAL;
+   }
+
+   BUILD_BUG_ON(sizeof(dev->t10_wwn.vendor) != INQUIRY_VENDOR_LEN + 1);
+   strlcpy(dev->t10_wwn.vendor, stripped, sizeof(dev->t10_wwn.vendor));
+
+   pr_debug("Target_Core_ConfigFS: Set emulated T10 Vendor Identification:"
+" %s\n", dev->t10_wwn.vendor);
+
+   return count;
+}
+
+/*
  * VPD page 0x80 Unit serial
  */
 static ssize_t target_wwn_vpd_unit_serial_show(struct config_item *item,
@@ -1362,6 +1430,7 @@ DEF_DEV_WWN_ASSOC_SHOW(vpd_assoc_target_port, 0x10);
 /* VPD page 0x83 Association: SCSI Target Device */
 DEF_DEV_WWN_ASSOC_SHOW(vpd_assoc_scsi_target_device, 0x20);
 
+CONFIGFS_ATTR(target_wwn_, vendor_id);
 CONFIGFS_ATTR(target_wwn_, vpd_unit_serial);
 CONFIGFS_ATTR_RO(target_wwn_, vpd_protocol_identifier);
 CONFIGFS_ATTR_RO(target_wwn_, vpd_assoc_logical_unit);
@@ -1369,6 +1438,7 @@ CONFIGFS_ATTR_RO(target_wwn_, vpd_assoc_target_port);
 CONFIGFS_ATTR_RO(target_wwn_, vpd_assoc_scsi_target_device);
 
 static struct configfs_attribute *target_core_dev_wwn_attrs[] = {
+   _wwn_attr_vendor_id,
_wwn_attr_vpd_unit_serial,
_wwn_attr_vpd_protocol_identifier,
_wwn_attr_vpd_assoc_logical_unit,
-- 
2.13.7

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

[David Disseldorp]

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/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 
Reviewed-by: Roman Bolshakov 
---
 drivers/target/target_core_device.c | 21 +++--
 1 file changed, 7 insertions(+), 14 deletions(-)

diff --git a/drivers/target/target_core_device.c 
b/drivers/target/target_core_device.c
index ebd787bb29a8..5ead7eae30b5 100644
--- a/drivers/target/target_core_device.c
+++ b/drivers/target/target_core_device.c
@@ -810,6 +810,13 @@ struct se_device *target_alloc_device(struct se_hba *hba, 
const char *name)
mutex_init(_lun->lun_tg_pt_md_mutex);
xcopy_lun->lun_tpg = _pt_tpg;
 
+   /* Preload the default INQUIRY const values */
+   strlcpy(dev->t10_wwn.vendor, "LIO-ORG", sizeof(dev->t10_wwn.vendor));
+   strlcpy(dev->t10_wwn.model, dev->transport->inquiry_prod,
+   sizeof(dev->t10_wwn.model));
+   strlcpy(dev->t10_wwn.revision, dev->transport->inquiry_rev,
+   sizeof(dev->t10_wwn.revision));
+
return dev;
 }
 
@@ -984,20 +991,6 @@ int target_configure_device(struct se_device *dev)
 */
INIT_WORK(>qf_work_queue, target_qf_do_work);
 
-   /*
-* Preload the initial INQUIRY const values if we are doing
-* anything virtual (IBLOCK, FILEIO, RAMDISK), but not for TCM/pSCSI
-* passthrough because this is being provided by the backend LLD.
-*/
-   if (!(dev->transport->transport_flags & TRANSPORT_FLAG_PASSTHROUGH)) {
-   strlcpy(dev->t10_wwn.vendor, "LIO-ORG",
-   sizeof(dev->t10_wwn.vendor));
-   strlcpy(dev->t10_wwn.model, dev->transport->inquiry_prod,
-   sizeof(dev->t10_wwn.model));
-   strlcpy(dev->t10_wwn.revision, dev->transport->inquiry_rev,
-   sizeof(dev->t10_wwn.revision));
-   }
-
scsi_dump_inquiry(dev);
 
spin_lock(>device_lock);
-- 
2.13.7

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

[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_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(>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(>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 = >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_dev

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

[David Disseldorp]

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 
Reviewed-by: Hannes Reinecke 
Reviewed-by: Roman Bolshakov 
---
 drivers/target/target_core_spc.c | 17 -
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/target/target_core_spc.c b/drivers/target/target_core_spc.c
index f459118bc11b..c37dd36ec77d 100644
--- a/drivers/target/target_core_spc.c
+++ b/drivers/target/target_core_spc.c
@@ -108,12 +108,17 @@ spc_emulate_inquiry_std(struct se_cmd *cmd, unsigned char 
*buf)
 
buf[7] = 0x2; /* CmdQue=1 */
 
-   memcpy([8], "LIO-ORG ", 8);
-   memset([16], 0x20, 16);
+   /*
+* 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).
+*/
+   memset([8], 0x20, 8 + 16 + 4);
+   memcpy([8], "LIO-ORG", sizeof("LIO-ORG") - 1);
memcpy([16], dev->t10_wwn.model,
-  min_t(size_t, strlen(dev->t10_wwn.model), 16));
+  strnlen(dev->t10_wwn.model, 16));
memcpy([32], dev->t10_wwn.revision,
-  min_t(size_t, strlen(dev->t10_wwn.revision), 4));
+  strnlen(dev->t10_wwn.revision, 4));
buf[4] = 31; /* Set additional length to 31 */
 
return 0;
@@ -251,7 +256,9 @@ spc_emulate_evpd_83(struct se_cmd *cmd, unsigned char *buf)
buf[off] = 0x2; /* ASCII */
buf[off+1] = 0x1; /* T10 Vendor ID */
buf[off+2] = 0x0;
-   memcpy([off+4], "LIO-ORG", 8);
+   /* left align Vendor ID and pad with spaces */
+   memset([off+4], 0x20, 8);
+   memcpy([off+4], "LIO-ORG", sizeof("LIO-ORG") - 1);
/* Extra Byte for NULL Terminator */
id_len++;
/* Identifier Length */
-- 
2.13.7

[PATCH v7 0/5] target: user configurable T10 Vendor ID

[David Disseldorp]

This patch-set allows for the modification of the T10 Vendor
Identification string returned in the SCSI INQUIRY response, via the
target/core/$backstore/$name/wwn/vendor_id ConfigFS path.

Changes since v6:
- PATCH 2/5
  + fill pscsi inquiry data using proper sd->inquiry pointer names
  + dump pscsi sd->inquiry data using sprintf
  + tweak emulate_model_alias truncation warning
  + drop a few duplicate BUILD_BUG_ON()s
  + drop Hannes' sign-off
- PATCH 3/5
  + check user vendor string for non-ascii-printable chars
  + strip newline before INQUIRY_VENDOR_LEN length check
  + drop sign-offs from Bryant, Lee and Hannes

Changes since v5:
- remove unnecessary TRANSPORT_FLAG_PASSTHROUGH conditional from t10_wwn
  ID defaults initialisation.

Changes since v4:
- merge null-termination changes into a single patch
- add patch to initialise t10_wwn ID defaults earlier
- use strlcpy() instead of strncpy() in some places

Changes since v3:
- perform explicit null termination of t10_wwn vendor, model and
  revision fields.
- replace field dump for-loops

Changes since v2:
- https://www.spinics.net/lists/target-devel/msg10720.html
- Support eight byte vendor ID strings
- Split out consistent INQUIRY data padding as a separate patch
- Drop t10_wwn.model buffer print fix, already upstream

Changes since v1:
- https://www.spinics.net/lists/target-devel/msg10545.html
- Rebase against nab's for-next branch, which includes Christoph's
  configfs API changes.

David Disseldorp (5):
  target: use consistent left-aligned ASCII INQUIRY data
  target: consistently null-terminate t10_wwn strings
  target: add device vendor_id configfs attribute
  target: remove hardcoded T10 Vendor ID in INQUIRY response
  target: perform t10_wwn ID initialisation in target_alloc_device()

 drivers/target/target_core_configfs.c | 86 +--
 drivers/target/target_core_device.c   | 55 +
 drivers/target/target_core_pscsi.c| 50 +---
 drivers/target/target_core_spc.c  | 20 +--
 drivers/target/target_core_stat.c | 32 +++---
 include/target/target_core_base.h | 14 -
 6 files changed, 145 insertions(+), 112 deletions(-)

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

[David Disseldorp]

On Tue, 4 Dec 2018 15:13:51 +0300, Roman Bolshakov wrote:

> > +   /* 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?

No :)

> 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.

I'll add an isascii() loop in the next round.

Cheers, David

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

[David Disseldorp]

On Tue, 4 Dec 2018 16:26:59 +0300, Roman Bolshakov wrote:

> 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.

I considered adding an if (PASSTHROUGH){return -EINVAL}, but we already
allow the model/product string to be set for pscsi+tcmu backends via
emulate_model_alias, so decided against it.

> 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.

That's correct.

Cheers, David

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

[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 
Reviewed-by: Hannes Reinecke 
---
 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,
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);
 }
 
 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(>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..5512871f50e4 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 = >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));
 }
 
@@ -1008,12 +989,16 @@ int target_configure_device(struct se_device *dev)
 * anything virtual (IBLOCK, FILEIO, RAM

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

[David Disseldorp]

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
@@ -1217,6 +1217,52 @@ static struct t10_wwn *to_t10_wwn(struct config_item 
*item)
 }
 
 /*
+ * STANDARD and VPD page 0x80 T10 Vendor Identification
+ */
+static ssize_t target_wwn_vendor_id_show(struct config_item *item,
+   char *page)
+{
+   return sprintf(page, "%s\n", _t10_wwn(item)->vendor[0]);
+}
+
+static ssize_t target_wwn_vendor_id_store(struct config_item *item,
+   const char *page, size_t count)
+{
+   struct t10_wwn *t10_wwn = to_t10_wwn(item);
+   struct se_device *dev = t10_wwn->t10_dev;
+   unsigned char buf[INQUIRY_VENDOR_LEN + 1];
+
+   if (strlen(page) > INQUIRY_VENDOR_LEN) {
+   pr_err("Emulated T10 Vendor Identification exceeds"
+   " INQUIRY_VENDOR_LEN: " __stringify(INQUIRY_VENDOR_LEN)
+   "\n");
+   return -EOVERFLOW;
+   }
+   strlcpy(buf, page, sizeof(buf));
+   /*
+* Check to see if any active exports exist.  If they do exist, fail
+* here as changing this information on the fly (underneath the
+* initiator side OS dependent multipath code) could cause negative
+* effects.
+*/
+   if (dev->export_count) {
+   pr_err("Unable to set T10 Vendor Identification while"
+   " active %d exports exist\n", dev->export_count);
+   return -EINVAL;
+   }
+
+   /* 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));
+
+   pr_debug("Target_Core_ConfigFS: Set emulated T10 Vendor Identification:"
+" %s\n", dev->t10_wwn.vendor);
+
+   return count;
+}
+
+/*
  * VPD page 0x80 Unit serial
  */
 static ssize_t target_wwn_vpd_unit_serial_show(struct config_item *item,
@@ -1362,6 +1408,7 @@ DEF_DEV_WWN_ASSOC_SHOW(vpd_assoc_target_port, 0x10);
 /* VPD page 0x83 Association: SCSI Target Device */
 DEF_DEV_WWN_ASSOC_SHOW(vpd_assoc_scsi_target_device, 0x20);
 
+CONFIGFS_ATTR(target_wwn_, vendor_id);
 CONFIGFS_ATTR(target_wwn_, vpd_unit_serial);
 CONFIGFS_ATTR_RO(target_wwn_, vpd_protocol_identifier);
 CONFIGFS_ATTR_RO(target_wwn_, vpd_assoc_logical_unit);
@@ -1369,6 +1416,7 @@ CONFIGFS_ATTR_RO(target_wwn_, vpd_assoc_target_port);
 CONFIGFS_ATTR_RO(target_wwn_, vpd_assoc_scsi_target_device);
 
 static struct configfs_attribute *target_core_dev_wwn_attrs[] = {
+   _wwn_attr_vendor_id,
_wwn_attr_vpd_unit_serial,
_wwn_attr_vpd_protocol_identifier,
_wwn_attr_vpd_assoc_logical_unit,
-- 
2.13.7

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

[David Disseldorp]

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 
Reviewed-by: Hannes Reinecke 
Reviewed-by: Roman Bolshakov 
---
 drivers/target/target_core_spc.c | 17 -
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/target/target_core_spc.c b/drivers/target/target_core_spc.c
index f459118bc11b..c37dd36ec77d 100644
--- a/drivers/target/target_core_spc.c
+++ b/drivers/target/target_core_spc.c
@@ -108,12 +108,17 @@ spc_emulate_inquiry_std(struct se_cmd *cmd, unsigned char 
*buf)
 
buf[7] = 0x2; /* CmdQue=1 */
 
-   memcpy([8], "LIO-ORG ", 8);
-   memset([16], 0x20, 16);
+   /*
+* 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).
+*/
+   memset([8], 0x20, 8 + 16 + 4);
+   memcpy([8], "LIO-ORG", sizeof("LIO-ORG") - 1);
memcpy([16], dev->t10_wwn.model,
-  min_t(size_t, strlen(dev->t10_wwn.model), 16));
+  strnlen(dev->t10_wwn.model, 16));
memcpy([32], dev->t10_wwn.revision,
-  min_t(size_t, strlen(dev->t10_wwn.revision), 4));
+  strnlen(dev->t10_wwn.revision, 4));
buf[4] = 31; /* Set additional length to 31 */
 
return 0;
@@ -251,7 +256,9 @@ spc_emulate_evpd_83(struct se_cmd *cmd, unsigned char *buf)
buf[off] = 0x2; /* ASCII */
buf[off+1] = 0x1; /* T10 Vendor ID */
buf[off+2] = 0x0;
-   memcpy([off+4], "LIO-ORG", 8);
+   /* left align Vendor ID and pad with spaces */
+   memset([off+4], 0x20, 8);
+   memcpy([off+4], "LIO-ORG", sizeof("LIO-ORG") - 1);
/* Extra Byte for NULL Terminator */
id_len++;
/* Identifier Length */
-- 
2.13.7

[PATCH v6 4/5] 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 
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(-)

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([8], 0x20,
   INQUIRY_VENDOR_LEN + INQUIRY_MODEL_LEN + INQUIRY_REVISION_LEN);
-   memcpy([8], "LIO-ORG", sizeof("LIO-ORG") - 1);
+   memcpy([8], dev->t10_wwn.vendor,
+  strnlen(dev->t10_wwn.vendor, INQUIRY_VENDOR_LEN));
memcpy([16], dev->t10_wwn.model,
   strnlen(dev->t10_wwn.model, INQUIRY_MODEL_LEN));
memcpy([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([off+4], 0x20, 8);
-   memcpy([off+4], "LIO-ORG", sizeof("LIO-ORG") - 1);
+   memset([off+4], 0x20, INQUIRY_VENDOR_LEN);
+   memcpy([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

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

[David Disseldorp]

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(-)

diff --git a/drivers/target/target_core_device.c 
b/drivers/target/target_core_device.c
index 5512871f50e4..bfc1b8b49940 100644
--- a/drivers/target/target_core_device.c
+++ b/drivers/target/target_core_device.c
@@ -810,6 +810,16 @@ struct se_device *target_alloc_device(struct se_hba *hba, 
const char *name)
mutex_init(_lun->lun_tg_pt_md_mutex);
xcopy_lun->lun_tpg = _pt_tpg;
 
+   /* Preload the default INQUIRY const values */
+   BUILD_BUG_ON(sizeof(dev->t10_wwn.vendor) != INQUIRY_VENDOR_LEN + 1);
+   strlcpy(dev->t10_wwn.vendor, "LIO-ORG", sizeof(dev->t10_wwn.vendor));
+   BUILD_BUG_ON(sizeof(dev->t10_wwn.model) != INQUIRY_MODEL_LEN + 1);
+   strlcpy(dev->t10_wwn.model, dev->transport->inquiry_prod,
+   sizeof(dev->t10_wwn.model));
+   BUILD_BUG_ON(sizeof(dev->t10_wwn.revision) != INQUIRY_REVISION_LEN + 1);
+   strlcpy(dev->t10_wwn.revision, dev->transport->inquiry_rev,
+   sizeof(dev->t10_wwn.revision));
+
return dev;
 }
 
@@ -984,23 +994,6 @@ int target_configure_device(struct se_device *dev)
 */
INIT_WORK(>qf_work_queue, target_qf_do_work);
 
-   /*
-* Preload the initial INQUIRY const values if we are doing
-* 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);
-   if (!(dev->transport->transport_flags & TRANSPORT_FLAG_PASSTHROUGH)) {
-   strlcpy(dev->t10_wwn.vendor, "LIO-ORG",
-   sizeof(dev->t10_wwn.vendor));
-   strlcpy(dev->t10_wwn.model, dev->transport->inquiry_prod,
-   sizeof(dev->t10_wwn.model));
-   strlcpy(dev->t10_wwn.revision, dev->transport->inquiry_rev,
-   sizeof(dev->t10_wwn.revision));
-   }
-
scsi_dump_inquiry(dev);
 
spin_lock(>device_lock);
-- 
2.13.7

[PATCH v6 0/5] target: user configurable T10 Vendor ID

[David Disseldorp]

This patch-set allows for the modification of the T10 Vendor
Identification string returned in the SCSI INQUIRY response, via the
target/core/$backstore/$name/wwn/vendor_id ConfigFS path.

Changes since v5:
- remove unnecessary TRANSPORT_FLAG_PASSTHROUGH conditional from t10_wwn
  ID defaults initialisation.

Changes since v4:
- merge null-termination changes into a single patch
- add patch to initialise t10_wwn ID defaults earlier
- use strlcpy() instead of strncpy() in some places

Changes since v3:
- perform explicit null termination of t10_wwn vendor, model and
  revision fields.
- replace field dump for-loops

Changes since v2:
- https://www.spinics.net/lists/target-devel/msg10720.html
- Support eight byte vendor ID strings
- Split out consistent INQUIRY data padding as a separate patch
- Drop t10_wwn.model buffer print fix, already upstream

Changes since v1:
- https://www.spinics.net/lists/target-devel/msg10545.html
- Rebase against nab's for-next branch, which includes Christoph's
  configfs API changes.

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

[David Disseldorp]

On Sun, 2 Dec 2018 23:22:23 +0100, David Disseldorp wrote:

> > > + if (!(dev->transport->transport_flags & TRANSPORT_FLAG_PASSTHROUGH)) {
> > > + strlcpy(dev->t10_wwn.vendor, "LIO-ORG",
> > > + sizeof(dev->t10_wwn.vendor));
> > > + strlcpy(dev->t10_wwn.model, dev->transport->inquiry_prod,
> > > + sizeof(dev->t10_wwn.model));
> > > + strlcpy(dev->t10_wwn.revision, dev->transport->inquiry_rev,
> > > + sizeof(dev->t10_wwn.revision));
> > > + }
> > > +
> > >   return dev;
> > >   }
> > >   
> > This is odd. I'd rather have it consistent across backends, ie either 
> > move the initialisation into the backends, or provide a means to check 
> > if the inquiry data has already been pre-filled.
> > But this check really is awkward.  
> 
> Not quite sure I follow here. I could the default setting to the
> target_backend_ops.alloc_device() code paths, but I don't think the
> duplication would make this much cleaner, if at all.
> I can look into this further if you like (target_backend_ops.inquiry_rev
> could be dropped that way),

Looking a little closer, I think we can drop the conditional completely
and set the vendor/model/rev defaults for all cases here:
- 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.

> but my preference would be to do so as a
> follow-up patch-set.

This is still my preference.

Cheers, David

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

[David Disseldorp]

Hi Hannes,

Thanks for the feedback...

On Sat, 1 Dec 2018 15:59:25 +0100, Hannes Reinecke wrote:

> On 12/1/18 12:34 AM, David Disseldorp wrote:
...
> > @@ -810,6 +810,23 @@ struct se_device *target_alloc_device(struct se_hba 
> > *hba, const char *name)
> > mutex_init(_lun->lun_tg_pt_md_mutex);
> > xcopy_lun->lun_tpg = _pt_tpg;
> >   
> > +   /*
> > +* Preload the initial INQUIRY const values if we are doing
> > +* 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);
> > +   if (!(dev->transport->transport_flags & TRANSPORT_FLAG_PASSTHROUGH)) {
> > +   strlcpy(dev->t10_wwn.vendor, "LIO-ORG",
> > +   sizeof(dev->t10_wwn.vendor));
> > +   strlcpy(dev->t10_wwn.model, dev->transport->inquiry_prod,
> > +   sizeof(dev->t10_wwn.model));
> > +   strlcpy(dev->t10_wwn.revision, dev->transport->inquiry_rev,
> > +   sizeof(dev->t10_wwn.revision));
> > +   }
> > +
> > return dev;
> >   }
> > 
> This is odd. I'd rather have it consistent across backends, ie either 
> move the initialisation into the backends, or provide a means to check 
> if the inquiry data has already been pre-filled.
> But this check really is awkward.

Not quite sure I follow here. I could the default setting to the
target_backend_ops.alloc_device() code paths, but I don't think the
duplication would make this much cleaner, if at all.
I can look into this further if you like (target_backend_ops.inquiry_rev
could be dropped that way), but my preference would be to do so as a
follow-up patch-set.

Cheers, David

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

[David Disseldorp]

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.

Signed-off-by: David Disseldorp 
---
 drivers/target/target_core_device.c | 34 +-
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/drivers/target/target_core_device.c 
b/drivers/target/target_core_device.c
index 5512871f50e4..6318d59a1564 100644
--- a/drivers/target/target_core_device.c
+++ b/drivers/target/target_core_device.c
@@ -810,6 +810,23 @@ struct se_device *target_alloc_device(struct se_hba *hba, 
const char *name)
mutex_init(_lun->lun_tg_pt_md_mutex);
xcopy_lun->lun_tpg = _pt_tpg;
 
+   /*
+* Preload the initial INQUIRY const values if we are doing
+* 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);
+   if (!(dev->transport->transport_flags & TRANSPORT_FLAG_PASSTHROUGH)) {
+   strlcpy(dev->t10_wwn.vendor, "LIO-ORG",
+   sizeof(dev->t10_wwn.vendor));
+   strlcpy(dev->t10_wwn.model, dev->transport->inquiry_prod,
+   sizeof(dev->t10_wwn.model));
+   strlcpy(dev->t10_wwn.revision, dev->transport->inquiry_rev,
+   sizeof(dev->t10_wwn.revision));
+   }
+
return dev;
 }
 
@@ -984,23 +1001,6 @@ int target_configure_device(struct se_device *dev)
 */
INIT_WORK(>qf_work_queue, target_qf_do_work);
 
-   /*
-* Preload the initial INQUIRY const values if we are doing
-* 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);
-   if (!(dev->transport->transport_flags & TRANSPORT_FLAG_PASSTHROUGH)) {
-   strlcpy(dev->t10_wwn.vendor, "LIO-ORG",
-   sizeof(dev->t10_wwn.vendor));
-   strlcpy(dev->t10_wwn.model, dev->transport->inquiry_prod,
-   sizeof(dev->t10_wwn.model));
-   strlcpy(dev->t10_wwn.revision, dev->transport->inquiry_rev,
-   sizeof(dev->t10_wwn.revision));
-   }
-
scsi_dump_inquiry(dev);
 
spin_lock(>device_lock);
-- 
2.13.7

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

[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 | 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,
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);
 }
 
 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(>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..5512871f50e4 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 = >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));
 }
 
@@ -1008,12 +989,16 @@ int target_configure_device(struct se_device *dev)
 * anything virtual (IBLOCK, FILEIO, RAMDISK), but not for TCM/pSC

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

[David Disseldorp]

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 
---
 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
@@ -1217,6 +1217,52 @@ static struct t10_wwn *to_t10_wwn(struct config_item 
*item)
 }
 
 /*
+ * STANDARD and VPD page 0x80 T10 Vendor Identification
+ */
+static ssize_t target_wwn_vendor_id_show(struct config_item *item,
+   char *page)
+{
+   return sprintf(page, "%s\n", _t10_wwn(item)->vendor[0]);
+}
+
+static ssize_t target_wwn_vendor_id_store(struct config_item *item,
+   const char *page, size_t count)
+{
+   struct t10_wwn *t10_wwn = to_t10_wwn(item);
+   struct se_device *dev = t10_wwn->t10_dev;
+   unsigned char buf[INQUIRY_VENDOR_LEN + 1];
+
+   if (strlen(page) > INQUIRY_VENDOR_LEN) {
+   pr_err("Emulated T10 Vendor Identification exceeds"
+   " INQUIRY_VENDOR_LEN: " __stringify(INQUIRY_VENDOR_LEN)
+   "\n");
+   return -EOVERFLOW;
+   }
+   strlcpy(buf, page, sizeof(buf));
+   /*
+* Check to see if any active exports exist.  If they do exist, fail
+* here as changing this information on the fly (underneath the
+* initiator side OS dependent multipath code) could cause negative
+* effects.
+*/
+   if (dev->export_count) {
+   pr_err("Unable to set T10 Vendor Identification while"
+   " active %d exports exist\n", dev->export_count);
+   return -EINVAL;
+   }
+
+   /* 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));
+
+   pr_debug("Target_Core_ConfigFS: Set emulated T10 Vendor Identification:"
+" %s\n", dev->t10_wwn.vendor);
+
+   return count;
+}
+
+/*
  * VPD page 0x80 Unit serial
  */
 static ssize_t target_wwn_vpd_unit_serial_show(struct config_item *item,
@@ -1362,6 +1408,7 @@ DEF_DEV_WWN_ASSOC_SHOW(vpd_assoc_target_port, 0x10);
 /* VPD page 0x83 Association: SCSI Target Device */
 DEF_DEV_WWN_ASSOC_SHOW(vpd_assoc_scsi_target_device, 0x20);
 
+CONFIGFS_ATTR(target_wwn_, vendor_id);
 CONFIGFS_ATTR(target_wwn_, vpd_unit_serial);
 CONFIGFS_ATTR_RO(target_wwn_, vpd_protocol_identifier);
 CONFIGFS_ATTR_RO(target_wwn_, vpd_assoc_logical_unit);
@@ -1369,6 +1416,7 @@ CONFIGFS_ATTR_RO(target_wwn_, vpd_assoc_target_port);
 CONFIGFS_ATTR_RO(target_wwn_, vpd_assoc_scsi_target_device);
 
 static struct configfs_attribute *target_core_dev_wwn_attrs[] = {
+   _wwn_attr_vendor_id,
_wwn_attr_vpd_unit_serial,
_wwn_attr_vpd_protocol_identifier,
_wwn_attr_vpd_assoc_logical_unit,
-- 
2.13.7

[PATCH v5 4/5] 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 
Reviewed-by: Bryant G. Ly 
Reviewed-by: Lee Duncan 
---
 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([8], 0x20,
   INQUIRY_VENDOR_LEN + INQUIRY_MODEL_LEN + INQUIRY_REVISION_LEN);
-   memcpy([8], "LIO-ORG", sizeof("LIO-ORG") - 1);
+   memcpy([8], dev->t10_wwn.vendor,
+  strnlen(dev->t10_wwn.vendor, INQUIRY_VENDOR_LEN));
memcpy([16], dev->t10_wwn.model,
   strnlen(dev->t10_wwn.model, INQUIRY_MODEL_LEN));
memcpy([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([off+4], 0x20, 8);
-   memcpy([off+4], "LIO-ORG", sizeof("LIO-ORG") - 1);
+   memset([off+4], 0x20, INQUIRY_VENDOR_LEN);
+   memcpy([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

[PATCH v5 0/5] target: user configurable T10 Vendor ID

[David Disseldorp]

This patch-set allows for the modification of the T10 Vendor
Identification string returned in the SCSI INQUIRY response, via the
target/core/$backstore/$name/wwn/vendor_id ConfigFS path.

Changes since v4:
- merge null-termination changes into a single patch
- add patch to initialise t10_wwn ID defaults earlier
- use strlcpy() instead of strncpy() in some places

Changes since v3:
- perform explicit null termination of t10_wwn vendor, model and
  revision fields.
- replace field dump for-loops

Changes since v2:
- https://www.spinics.net/lists/target-devel/msg10720.html
- Support eight byte vendor ID strings
- Split out consistent INQUIRY data padding as a separate patch
- Drop t10_wwn.model buffer print fix, already upstream

Changes since v1:
- https://www.spinics.net/lists/target-devel/msg10545.html
- Rebase against nab's for-next branch, which includes Christoph's
  configfs API changes.

David Disseldorp (5):
  target: use consistent left-aligned ASCII INQUIRY data
  target: consistently null-terminate t10_wwn strings
  target: add device vendor_id configfs attribute
  target: remove hardcoded T10 Vendor ID in INQUIRY response
  target: perform t10_wwn ID initialisation in target_alloc_device()

 drivers/target/target_core_configfs.c | 62 +++--
 drivers/target/target_core_device.c   | 65 +++
 drivers/target/target_core_pscsi.c| 18 +---
 drivers/target/target_core_spc.c  | 20 ++---
 drivers/target/target_core_stat.c | 32 +++--
 include/target/target_core_base.h | 14 --
 6 files changed, 128 insertions(+), 83 deletions(-)

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

[David Disseldorp]

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(-)

diff --git a/drivers/target/target_core_spc.c b/drivers/target/target_core_spc.c
index f459118bc11b..c37dd36ec77d 100644
--- a/drivers/target/target_core_spc.c
+++ b/drivers/target/target_core_spc.c
@@ -108,12 +108,17 @@ spc_emulate_inquiry_std(struct se_cmd *cmd, unsigned char 
*buf)
 
buf[7] = 0x2; /* CmdQue=1 */
 
-   memcpy([8], "LIO-ORG ", 8);
-   memset([16], 0x20, 16);
+   /*
+* 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).
+*/
+   memset([8], 0x20, 8 + 16 + 4);
+   memcpy([8], "LIO-ORG", sizeof("LIO-ORG") - 1);
memcpy([16], dev->t10_wwn.model,
-  min_t(size_t, strlen(dev->t10_wwn.model), 16));
+  strnlen(dev->t10_wwn.model, 16));
memcpy([32], dev->t10_wwn.revision,
-  min_t(size_t, strlen(dev->t10_wwn.revision), 4));
+  strnlen(dev->t10_wwn.revision, 4));
buf[4] = 31; /* Set additional length to 31 */
 
return 0;
@@ -251,7 +256,9 @@ spc_emulate_evpd_83(struct se_cmd *cmd, unsigned char *buf)
buf[off] = 0x2; /* ASCII */
buf[off+1] = 0x1; /* T10 Vendor ID */
buf[off+2] = 0x0;
-   memcpy([off+4], "LIO-ORG", 8);
+   /* left align Vendor ID and pad with spaces */
+   memset([off+4], 0x20, 8);
+   memcpy([off+4], "LIO-ORG", sizeof("LIO-ORG") - 1);
/* 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

[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=4096" > 
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

[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 5/7] target: add device vendor_id configfs attribute

[David Disseldorp]

On Thu, 29 Nov 2018 08:32:47 -0800, Bart Van Assche wrote:

> > +   unsigned char buf[INQUIRY_VENDOR_LEN + 1];
> > +
> > +   if (strlen(page) > INQUIRY_VENDOR_LEN) {
> > +   pr_err("Emulated T10 Vendor Identification exceeds"
> > +   " INQUIRY_VENDOR_LEN: " __stringify(INQUIRY_VENDOR_LEN)
> > +   "\n");
> > +   return -EOVERFLOW;
> > +   }  
> 
> Does this force users to use "echo -n" to configure vendor IDs that are
> INQUIRY_VENDOR_LEN bytes long?

It does. As mentioned in v3, this follows the convention used in
target_wwn_vpd_unit_serial_store(). I don't feel too strongly about it,
but it does make buf allocation a little less error prone IMO.

Cheers, David

Re: [PATCH v4 3/7] target: consistently null-terminate t10_wwn.model

[David Disseldorp]

On Thu, 29 Nov 2018 08:24:38 -0800, Bart Van Assche wrote:

> On Thu, 2018-11-29 at 02:01 +0100, David Disseldorp wrote:
> > diff --git a/drivers/target/target_core_configfs.c 
> > b/drivers/target/target_core_configfs.c
> > index f6b1549f4142..9f49b1afd685 100644
> > --- a/drivers/target/target_core_configfs.c
> > +++ b/drivers/target/target_core_configfs.c
> > @@ -613,12 +613,12 @@ 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,
> > configname);
> > }
> > -   snprintf(>t10_wwn.model[0], 16, "%s", configname);
> > +   snprintf(>t10_wwn.model[0], INQUIRY_MODEL_LEN, "%s", configname);  
> 
> Both the old and the new statement truncate inquiry strings that are 16 bytes
> long, which is a bug.

As mentioned in the changelog, I don't think we can fix this without
potentially breaking existing deployments - e.g. a "fourfourfourfour"
backstore name with emulate_model_alias=1 would change inquiry product
ID from "fourfourfourfou" to "fourfourfourfour" following kernel
upgrade.

> Additionally, have you considered to use strlcpy()
> instead of snprintf()?

Happy to change the logic below over if you find it easier to follow.

Cheers, David

[PATCH v4 3/7] target: consistently null-terminate t10_wwn.model

[David Disseldorp]

The pscsi_set_inquiry_info() and emulate_model_alias_store() codepaths
don't currently explicitly null-terminate t10_wwn.model.
Add an extra byte to the t10_wwn.model buffer and perform null string
termination in all cases.

dev_set_t10_wwn_model_alias() continues to truncate at the same length
to avoid changing the model string for existing deployments.

Signed-off-by: David Disseldorp 
---
 drivers/target/target_core_configfs.c | 8 +---
 drivers/target/target_core_device.c   | 8 +---
 drivers/target/target_core_pscsi.c| 6 --
 drivers/target/target_core_spc.c  | 2 +-
 drivers/target/target_core_stat.c | 4 ++--
 include/target/target_core_base.h | 3 ++-
 6 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/drivers/target/target_core_configfs.c 
b/drivers/target/target_core_configfs.c
index f6b1549f4142..9f49b1afd685 100644
--- a/drivers/target/target_core_configfs.c
+++ b/drivers/target/target_core_configfs.c
@@ -613,12 +613,12 @@ 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,
configname);
}
-   snprintf(>t10_wwn.model[0], 16, "%s", configname);
+   snprintf(>t10_wwn.model[0], INQUIRY_MODEL_LEN, "%s", configname);
 }
 
 static ssize_t emulate_model_alias_store(struct config_item *item,
@@ -640,11 +640,13 @@ 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(>t10_wwn.model[0],
-   dev->transport->inquiry_prod, 16);
+   dev->transport->inquiry_prod, INQUIRY_MODEL_LEN);
+   dev->t10_wwn.model[INQUIRY_MODEL_LEN] = '\0';
}
da->emulate_model_alias = flag;
return count;
diff --git a/drivers/target/target_core_device.c 
b/drivers/target/target_core_device.c
index fe4c4db51137..0d7382efb2d4 100644
--- a/drivers/target/target_core_device.c
+++ b/drivers/target/target_core_device.c
@@ -720,7 +720,7 @@ void core_dev_free_initiator_node_lun_acl(
 static void scsi_dump_inquiry(struct se_device *dev)
 {
struct t10_wwn *wwn = >t10_wwn;
-   char buf[17];
+   char buf[INQUIRY_MODEL_LEN + 1];
int i, device_type;
/*
 * Print Linux/SCSI style INQUIRY formatting to the kernel ring buffer
@@ -733,7 +733,7 @@ static void scsi_dump_inquiry(struct se_device *dev)
buf[i] = '\0';
pr_debug("  Vendor: %s\n", buf);
 
-   for (i = 0; i < 16; i++)
+   for (i = 0; i < INQUIRY_MODEL_LEN; i++)
if (wwn->model[i] >= 0x20)
buf[i] = wwn->model[i];
else
@@ -1009,11 +1009,13 @@ int target_configure_device(struct se_device *dev)
 * 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);
if (!(dev->transport->transport_flags & TRANSPORT_FLAG_PASSTHROUGH)) {
strncpy(>t10_wwn.vendor[0], "LIO-ORG", INQUIRY_VENDOR_LEN);
dev->t10_wwn.vendor[INQUIRY_VENDOR_LEN] = '\0';
strncpy(>t10_wwn.model[0],
-   dev->transport->inquiry_prod, 16);
+   dev->transport->inquiry_prod, INQUIRY_MODEL_LEN);
+   dev->t10_wwn.model[INQUIRY_MODEL_LEN] = '\0';
strncpy(>t10_wwn.revision[0],
dev->transport->inquiry_rev, 4);
}
diff --git a/drivers/target/target_core_pscsi.c 
b/drivers/target/target_core_pscsi.c
index ee65b5bb674c..1633babc2d4e 100644
--- a/drivers/target/target_core_pscsi.c
+++ b/drivers/target/target_core_pscsi.c
@@ -193,7 +193,9 @@ pscsi_set_inquiry_info(struct scsi_device *sdev, struct 
t10_wwn *wwn)
BUILD_BUG_ON(sizeof(wwn->vendor) != INQUIRY_VENDOR_LEN + 1);
memcpy(>vendor[0], [8], INQUIRY_VENDOR_LEN);
wwn->vendor[INQUIRY_VENDOR_LEN] = '\0';
-   memcpy(>model[0], [16], sizeof(wwn->model));
+   BUILD_BUG_ON(sizeof(wwn->model) != INQUIRY_MODEL_LEN + 1);
+   memcpy(>model[0], [16], INQUIRY_MODEL_LEN);
+   wwn->model[INQUIRY_MODEL_LEN] = '\0';
memcpy(>revision[0], [32], sizeof(wwn->revision));
 }
 
@@ -835,7 +837,7 @@ sta

[PATCH v4 7/7] target: use printf width specifier for t10_wwn field dumps

[David Disseldorp]

From: Bart Van Assche 

The existing for loops step over null-terminators for right-padding.
Padding can be achieved in a much simpler way using printf width
specifiers.

Signed-off-by: David Disseldorp 
---
 drivers/target/target_core_device.c | 35 ---
 drivers/target/target_core_stat.c   | 32 +++-
 2 files changed, 15 insertions(+), 52 deletions(-)

diff --git a/drivers/target/target_core_device.c 
b/drivers/target/target_core_device.c
index b3d0bd1ab09f..7f2d307e411b 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 = >t10_wwn;
-   char buf[INQUIRY_MODEL_LEN + 1];
-   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 < INQUIRY_VENDOR_LEN; 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 < INQUIRY_MODEL_LEN; 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 < INQUIRY_REVISION_LEN; 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));
 }
 
diff --git a/drivers/target/target_core_stat.c 
b/drivers/target/target_core_stat.c
index e437ba494865..87fd2b11fe3b 100644
--- a/drivers/target/target_core_stat.c
+++ b/drivers/target/target_core_stat.c
@@ -246,43 +246,25 @@ static ssize_t target_stat_lu_lu_name_show(struct 
config_item *item, char *page)
 static ssize_t target_stat_lu_vend_show(struct config_item *item, char *page)
 {
struct se_device *dev = to_stat_lu_dev(item);
-   int i;
-   char str[INQUIRY_VENDOR_LEN+1];
 
-   /* scsiLuVendorId */
-   for (i = 0; i < INQUIRY_VENDOR_LEN; i++)
-   str[i] = ISPRINT(dev->t10_wwn.vendor[i]) ?
-   dev->t10_wwn.vendor[i] : ' ';
-   str[i] = '\0';
-   return snprintf(page, PAGE_SIZE, "%s\n", str);
+   return snprintf(page, PAGE_SIZE, "%-" __stringify(INQUIRY_VENDOR_LEN)
+   "s\n", dev->t10_wwn.vendor);
 }
 
 static ssize_t target_stat_lu_prod_show(struct config_item *item, char *page)
 {
struct se_device *dev = to_stat_lu_dev(item);
-   int i;
-   char str[INQUIRY_MODEL_LEN+1];
 
-   /* scsiLuProductId */
-   for (i = 0; i < INQUIRY_MODEL_LEN; i++)
-   str[i] = ISPRINT(dev->t10_wwn.model[i]) ?
-   dev->t10_wwn.model[i] : ' ';
-   str[i] = '\0';
-   return snprintf(page, PAGE_SIZE, "%s\n", str);
+   return snprintf(page, PAGE_SIZE, "%-" __stringify(INQUIRY_MODEL_LEN)
+   "s\n", dev->t10_wwn.model);
 }
 
 static ssize_t target_stat_lu_rev_show(struct config_item *item, char *page)
 {
struct se_device *dev = to_stat_lu_dev(item);
-   int i;
-   char str[INQUIRY_REVISION_LEN+1];
-
-   /* scsiLuRevisionId */
-   for (i = 0; i < INQUIRY_REVISION_LEN; i++)
-   str[i] = ISPRINT(dev->t10_wwn.revision[i]) ?
-   dev->t10_wwn.revision[i] : ' ';
-   str[i] = '\0';
-   return snprintf(page, PAGE_SIZE, "%s\n", str);
+
+   return snprintf(page, PAGE_SIZE, "%-" __stringify(INQUIRY_REVISION_LEN)
+   "s\n", dev->t10_wwn.revision);
 }
 
 static ssize_t target_stat_lu_dev_type_show(struct config_item *item, char 
*page)
-- 
2.13.7

[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 
---
 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([8], 0x20,
   INQUIRY_VENDOR_LEN + INQUIRY_MODEL_LEN + INQUIRY_REVISION_LEN);
-   memcpy([8], "LIO-ORG", sizeof("LIO-ORG") - 1);
+   memcpy([8], dev->t10_wwn.vendor,
+  strnlen(dev->t10_wwn.vendor, INQUIRY_VENDOR_LEN));
memcpy([16], dev->t10_wwn.model,
   strnlen(dev->t10_wwn.model, INQUIRY_MODEL_LEN));
memcpy([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([off+4], 0x20, 8);
-   memcpy([off+4], "LIO-ORG", sizeof("LIO-ORG") - 1);
+   memset([off+4], 0x20, INQUIRY_VENDOR_LEN);
+   memcpy([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

[PATCH v4 4/7] target: consistently null-terminate t10_wwn.revision

[David Disseldorp]

The pscsi_set_inquiry_info() codepath doesn't currently explicitly
null-terminate t10_wwn.revision.
Add an extra byte to the t10_wwn.model buffer and perform null string
termination in all cases.

Signed-off-by: David Disseldorp 
---
 drivers/target/target_core_device.c | 6 --
 drivers/target/target_core_pscsi.c  | 4 +++-
 drivers/target/target_core_spc.c| 5 +++--
 drivers/target/target_core_stat.c   | 4 ++--
 include/target/target_core_base.h   | 3 ++-
 5 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/target/target_core_device.c 
b/drivers/target/target_core_device.c
index 0d7382efb2d4..b3d0bd1ab09f 100644
--- a/drivers/target/target_core_device.c
+++ b/drivers/target/target_core_device.c
@@ -741,7 +741,7 @@ static void scsi_dump_inquiry(struct se_device *dev)
buf[i] = '\0';
pr_debug("  Model: %s\n", buf);
 
-   for (i = 0; i < 4; i++)
+   for (i = 0; i < INQUIRY_REVISION_LEN; i++)
if (wwn->revision[i] >= 0x20)
buf[i] = wwn->revision[i];
else
@@ -1010,6 +1010,7 @@ int target_configure_device(struct se_device *dev)
 */
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);
if (!(dev->transport->transport_flags & TRANSPORT_FLAG_PASSTHROUGH)) {
strncpy(>t10_wwn.vendor[0], "LIO-ORG", INQUIRY_VENDOR_LEN);
dev->t10_wwn.vendor[INQUIRY_VENDOR_LEN] = '\0';
@@ -1017,7 +1018,8 @@ int target_configure_device(struct se_device *dev)
dev->transport->inquiry_prod, INQUIRY_MODEL_LEN);
dev->t10_wwn.model[INQUIRY_MODEL_LEN] = '\0';
strncpy(>t10_wwn.revision[0],
-   dev->transport->inquiry_rev, 4);
+   dev->transport->inquiry_rev, INQUIRY_REVISION_LEN);
+   dev->t10_wwn.revision[INQUIRY_REVISION_LEN] = '\0';
}
 
scsi_dump_inquiry(dev);
diff --git a/drivers/target/target_core_pscsi.c 
b/drivers/target/target_core_pscsi.c
index 1633babc2d4e..5493f620b7f4 100644
--- a/drivers/target/target_core_pscsi.c
+++ b/drivers/target/target_core_pscsi.c
@@ -196,7 +196,9 @@ pscsi_set_inquiry_info(struct scsi_device *sdev, struct 
t10_wwn *wwn)
BUILD_BUG_ON(sizeof(wwn->model) != INQUIRY_MODEL_LEN + 1);
memcpy(>model[0], [16], INQUIRY_MODEL_LEN);
wwn->model[INQUIRY_MODEL_LEN] = '\0';
-   memcpy(>revision[0], [32], sizeof(wwn->revision));
+   BUILD_BUG_ON(sizeof(wwn->revision) != INQUIRY_REVISION_LEN + 1);
+   memcpy(>revision[0], [32], INQUIRY_REVISION_LEN);
+   wwn->revision[INQUIRY_REVISION_LEN] = '\0';
 }
 
 static int
diff --git a/drivers/target/target_core_spc.c b/drivers/target/target_core_spc.c
index 78eddee4b6e6..8ffe712cb44d 100644
--- a/drivers/target/target_core_spc.c
+++ b/drivers/target/target_core_spc.c
@@ -113,12 +113,13 @@ spc_emulate_inquiry_std(struct se_cmd *cmd, unsigned char 
*buf)
 * unused bytes at the end of the field (i.e., highest offset) and the
 * unused bytes shall be filled with ASCII space characters (20h).
 */
-   memset([8], 0x20, 8 + 16 + 4);
+   memset([8], 0x20,
+  INQUIRY_VENDOR_LEN + INQUIRY_MODEL_LEN + INQUIRY_REVISION_LEN);
memcpy([8], "LIO-ORG", sizeof("LIO-ORG") - 1);
memcpy([16], dev->t10_wwn.model,
   strnlen(dev->t10_wwn.model, INQUIRY_MODEL_LEN));
memcpy([32], dev->t10_wwn.revision,
-  strnlen(dev->t10_wwn.revision, 4));
+  strnlen(dev->t10_wwn.revision, INQUIRY_REVISION_LEN));
buf[4] = 31; /* Set additional length to 31 */
 
return 0;
diff --git a/drivers/target/target_core_stat.c 
b/drivers/target/target_core_stat.c
index 9123c5137da5..e437ba494865 100644
--- a/drivers/target/target_core_stat.c
+++ b/drivers/target/target_core_stat.c
@@ -275,10 +275,10 @@ static ssize_t target_stat_lu_rev_show(struct config_item 
*item, char *page)
 {
struct se_device *dev = to_stat_lu_dev(item);
int i;
-   char str[sizeof(dev->t10_wwn.revision)+1];
+   char str[INQUIRY_REVISION_LEN+1];
 
/* scsiLuRevisionId */
-   for (i = 0; i < sizeof(dev->t10_wwn.revision); i++)
+   for (i = 0; i < INQUIRY_REVISION_LEN; i++)
str[i] = ISPRINT(dev->t10_wwn.revision[i]) ?
dev->t10_wwn.revision[i] : ' ';
str[i] = '\0';
diff --git a/include/target/target_core_base.h 
b/include/target/target_core_base.h
index cfc279686cf4..497853a09fee 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -48,6 +48,7 @@
 
 #define INQUIRY_VENDOR_LE

[PATCH v4 5/7] target: add device vendor_id configfs attribute

[David Disseldorp]

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 | 48 +++
 1 file changed, 48 insertions(+)

diff --git a/drivers/target/target_core_configfs.c 
b/drivers/target/target_core_configfs.c
index 9f49b1afd685..fc60c4db718d 100644
--- a/drivers/target/target_core_configfs.c
+++ b/drivers/target/target_core_configfs.c
@@ -1213,6 +1213,52 @@ static struct t10_wwn *to_t10_wwn(struct config_item 
*item)
 }
 
 /*
+ * STANDARD and VPD page 0x80 T10 Vendor Identification
+ */
+static ssize_t target_wwn_vendor_id_show(struct config_item *item,
+   char *page)
+{
+   return sprintf(page, "%s\n", _t10_wwn(item)->vendor[0]);
+}
+
+static ssize_t target_wwn_vendor_id_store(struct config_item *item,
+   const char *page, size_t count)
+{
+   struct t10_wwn *t10_wwn = to_t10_wwn(item);
+   struct se_device *dev = t10_wwn->t10_dev;
+   unsigned char buf[INQUIRY_VENDOR_LEN + 1];
+
+   if (strlen(page) > INQUIRY_VENDOR_LEN) {
+   pr_err("Emulated T10 Vendor Identification exceeds"
+   " INQUIRY_VENDOR_LEN: " __stringify(INQUIRY_VENDOR_LEN)
+   "\n");
+   return -EOVERFLOW;
+   }
+   strlcpy(buf, page, sizeof(buf));
+   /*
+* Check to see if any active exports exist.  If they do exist, fail
+* here as changing this information on the fly (underneath the
+* initiator side OS dependent multipath code) could cause negative
+* effects.
+*/
+   if (dev->export_count) {
+   pr_err("Unable to set T10 Vendor Identification while"
+   " active %d exports exist\n", dev->export_count);
+   return -EINVAL;
+   }
+
+   /* 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));
+
+   pr_debug("Target_Core_ConfigFS: Set emulated T10 Vendor Identification:"
+" %s\n", dev->t10_wwn.vendor);
+
+   return count;
+}
+
+/*
  * VPD page 0x80 Unit serial
  */
 static ssize_t target_wwn_vpd_unit_serial_show(struct config_item *item,
@@ -1358,6 +1404,7 @@ DEF_DEV_WWN_ASSOC_SHOW(vpd_assoc_target_port, 0x10);
 /* VPD page 0x83 Association: SCSI Target Device */
 DEF_DEV_WWN_ASSOC_SHOW(vpd_assoc_scsi_target_device, 0x20);
 
+CONFIGFS_ATTR(target_wwn_, vendor_id);
 CONFIGFS_ATTR(target_wwn_, vpd_unit_serial);
 CONFIGFS_ATTR_RO(target_wwn_, vpd_protocol_identifier);
 CONFIGFS_ATTR_RO(target_wwn_, vpd_assoc_logical_unit);
@@ -1365,6 +1412,7 @@ CONFIGFS_ATTR_RO(target_wwn_, vpd_assoc_target_port);
 CONFIGFS_ATTR_RO(target_wwn_, vpd_assoc_scsi_target_device);
 
 static struct configfs_attribute *target_core_dev_wwn_attrs[] = {
+   _wwn_attr_vendor_id,
_wwn_attr_vpd_unit_serial,
_wwn_attr_vpd_protocol_identifier,
_wwn_attr_vpd_assoc_logical_unit,
-- 
2.13.7

[PATCH v4 2/7] target: consistently null-terminate t10_wwn.vendor

[David Disseldorp]

In preparation for supporting user provided vendor strings, add an extra
byte to t10_wwn.vendor which will ensure null string termination when an
eight byte vendor string is provided by the user.

Signed-off-by: David Disseldorp 
---
 drivers/target/target_core_device.c | 6 --
 drivers/target/target_core_pscsi.c  | 6 --
 drivers/target/target_core_stat.c   | 4 ++--
 include/target/target_core_base.h   | 8 +++-
 4 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/drivers/target/target_core_device.c 
b/drivers/target/target_core_device.c
index 47b5ef153135..fe4c4db51137 100644
--- a/drivers/target/target_core_device.c
+++ b/drivers/target/target_core_device.c
@@ -725,7 +725,7 @@ static void scsi_dump_inquiry(struct se_device *dev)
/*
 * Print Linux/SCSI style INQUIRY formatting to the kernel ring buffer
 */
-   for (i = 0; i < 8; i++)
+   for (i = 0; i < INQUIRY_VENDOR_LEN; i++)
if (wwn->vendor[i] >= 0x20)
buf[i] = wwn->vendor[i];
else
@@ -1008,8 +1008,10 @@ 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);
if (!(dev->transport->transport_flags & TRANSPORT_FLAG_PASSTHROUGH)) {
-   strncpy(>t10_wwn.vendor[0], "LIO-ORG", 8);
+   strncpy(>t10_wwn.vendor[0], "LIO-ORG", INQUIRY_VENDOR_LEN);
+   dev->t10_wwn.vendor[INQUIRY_VENDOR_LEN] = '\0';
strncpy(>t10_wwn.model[0],
dev->transport->inquiry_prod, 16);
strncpy(>t10_wwn.revision[0],
diff --git a/drivers/target/target_core_pscsi.c 
b/drivers/target/target_core_pscsi.c
index 47d76c862014..ee65b5bb674c 100644
--- a/drivers/target/target_core_pscsi.c
+++ b/drivers/target/target_core_pscsi.c
@@ -190,7 +190,9 @@ 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));
+   BUILD_BUG_ON(sizeof(wwn->vendor) != INQUIRY_VENDOR_LEN + 1);
+   memcpy(>vendor[0], [8], INQUIRY_VENDOR_LEN);
+   wwn->vendor[INQUIRY_VENDOR_LEN] = '\0';
memcpy(>model[0], [16], sizeof(wwn->model));
memcpy(>revision[0], [32], sizeof(wwn->revision));
 }
@@ -826,7 +828,7 @@ 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++) {
+   for (i = 0; i < INQUIRY_VENDOR_LEN; i++) {
if (ISPRINT(sd->vendor[i]))   /* printable character? */
bl += sprintf(b + bl, "%c", sd->vendor[i]);
else
diff --git a/drivers/target/target_core_stat.c 
b/drivers/target/target_core_stat.c
index f0db91ebd735..4210cf625d84 100644
--- a/drivers/target/target_core_stat.c
+++ b/drivers/target/target_core_stat.c
@@ -247,10 +247,10 @@ static ssize_t target_stat_lu_vend_show(struct 
config_item *item, char *page)
 {
struct se_device *dev = to_stat_lu_dev(item);
int i;
-   char str[sizeof(dev->t10_wwn.vendor)+1];
+   char str[INQUIRY_VENDOR_LEN+1];
 
/* scsiLuVendorId */
-   for (i = 0; i < sizeof(dev->t10_wwn.vendor); i++)
+   for (i = 0; i < INQUIRY_VENDOR_LEN; i++)
str[i] = ISPRINT(dev->t10_wwn.vendor[i]) ?
dev->t10_wwn.vendor[i] : ' ';
str[i] = '\0';
diff --git a/include/target/target_core_base.h 
b/include/target/target_core_base.h
index e3bdb0550a59..cb1f3f574e2a 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -46,6 +46,8 @@
 /* Used by transport_get_inquiry_vpd_device_ident() */
 #define INQUIRY_VPD_DEVICE_IDENTIFIER_LEN  254
 
+#define INQUIRY_VENDOR_LEN 8
+
 /* Attempts before moving from SHORT to LONG */
 #define PYX_TRANSPORT_WINDOW_CLOSED_THRESHOLD  3
 #define PYX_TRANSPORT_WINDOW_CLOSED_WAIT_SHORT 3  /* In milliseconds */
@@ -314,7 +316,11 @@ struct t10_vpd {
 };
 
 struct t10_wwn {
-   char vendor[8];
+   /*
+* SCSI left aligned strings may not be null terminated. +1 to ensure a
+* null terminator is always present.
+*/
+   char vendor[INQUIRY_VENDOR_LEN + 1];
char model[16];
char revision[4];
char unit_serial[INQUIRY_VPD_SERIAL_LEN];
-- 
2.13.7

[PATCH v4 0/7] target: user configurable T10 Vendor ID

[David Disseldorp]

This patchset allows for the modification of the T10 Vendor
Identification string returned in the SCSI INQUIRY response, via the
target/core/$backstore/$name/wwn/vendor_id ConfigFS path.

Changes since v3:
- perform explicit null termination of t10_wwn vendor, model and
  revision fields.
- replace field dump for-loops

Changes since v2:
- https://www.spinics.net/lists/target-devel/msg10720.html
- Support eight byte vendor ID strings
- Split out consistent INQUIRY data padding as a separate patch
- Drop t10_wwn.model buffer print fix, already upstream

Changes since v1:
- https://www.spinics.net/lists/target-devel/msg10545.html
- Rebase against nab's for-next branch, which includes Christoph's
  configfs API changes.

Bart Van Assche (1):
  target: use printf width specifier for t10_wwn field dumps

David Disseldorp (6):
  target: use consistent left-aligned ASCII INQUIRY data
  target: consistently null-terminate t10_wwn.vendor
  target: consistently null-terminate t10_wwn.model
  target: consistently null-terminate t10_wwn.revision
  target: add device vendor_id configfs attribute
  target: remove hardcoded T10 Vendor ID in INQUIRY response

 drivers/target/target_core_configfs.c | 56 +--
 drivers/target/target_core_device.c   | 47 --
 drivers/target/target_core_pscsi.c| 16 +---
 drivers/target/target_core_spc.c  | 20 +++---
 drivers/target/target_core_stat.c | 32 ---
 include/target/target_core_base.h | 14 +--
 6 files changed, 114 insertions(+), 71 deletions(-)

[PATCH v4 1/7] target: use consistent left-aligned ASCII INQUIRY data

[David Disseldorp]

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 
---
 drivers/target/target_core_spc.c | 17 -
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/target/target_core_spc.c b/drivers/target/target_core_spc.c
index f459118bc11b..c37dd36ec77d 100644
--- a/drivers/target/target_core_spc.c
+++ b/drivers/target/target_core_spc.c
@@ -108,12 +108,17 @@ spc_emulate_inquiry_std(struct se_cmd *cmd, unsigned char 
*buf)
 
buf[7] = 0x2; /* CmdQue=1 */
 
-   memcpy([8], "LIO-ORG ", 8);
-   memset([16], 0x20, 16);
+   /*
+* 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).
+*/
+   memset([8], 0x20, 8 + 16 + 4);
+   memcpy([8], "LIO-ORG", sizeof("LIO-ORG") - 1);
memcpy([16], dev->t10_wwn.model,
-  min_t(size_t, strlen(dev->t10_wwn.model), 16));
+  strnlen(dev->t10_wwn.model, 16));
memcpy([32], dev->t10_wwn.revision,
-  min_t(size_t, strlen(dev->t10_wwn.revision), 4));
+  strnlen(dev->t10_wwn.revision, 4));
buf[4] = 31; /* Set additional length to 31 */
 
return 0;
@@ -251,7 +256,9 @@ spc_emulate_evpd_83(struct se_cmd *cmd, unsigned char *buf)
buf[off] = 0x2; /* ASCII */
buf[off+1] = 0x1; /* T10 Vendor ID */
buf[off+2] = 0x0;
-   memcpy([off+4], "LIO-ORG", 8);
+   /* left align Vendor ID and pad with spaces */
+   memset([off+4], 0x20, 8);
+   memcpy([off+4], "LIO-ORG", sizeof("LIO-ORG") - 1);
/* Extra Byte for NULL Terminator */
id_len++;
/* Identifier Length */
-- 
2.13.7

Re: [PATCH v3 3/4] target: add device vendor_id configfs attribute

[David Disseldorp]

Hi Bart,

On Wed, 28 Nov 2018 08:36:19 -0800, Bart Van Assche wrote:

> Maybe I'm missing something, but why is zeroing of unused bytes in these 
> functions
> necessary? Would the following be correct if all strings in struct t10_wwn 
> would be
> '\0'-terminated?

Your patch looks good to me. Mind if I tack it on to the end of my
t10_wwn.vendor/model/revision[size+1] patchset, with your authorship?

Cheers, David

Re: [PATCH v3 3/4] target: add device vendor_id configfs attribute

[David Disseldorp]

On Wed, 28 Nov 2018 08:08:30 -0800, Bart Van Assche wrote:

> > Just a follow up here. I think it's safer to retain strncpy() in this
> > function for the purpose of zero filling. scsi_dump_inquiry() and
> > target_stat_lu_vend_show() walk the entire length of the t10_wwn.vendor
> > buffer.  
> 
> How about adding a comment about above struct t10_wwn that vendor[], model[]
> and revision[] in that structure may but do not have to be terminated with a
> trailing '\0' and also that unit_serial[] is '\0'-terminated?
> 
> It would make me happy if the size of the character arrays in that structure
> would be increased by one and if the target code would be modified such that
> these strings are always '\0'-terminated.

I'm working on the +1 length increase for those t10_wwn members ATM, but
just thought I'd mention that the zeroing of unused bytes is still
required due to the scsi_dump_inquiry() + target_stat_lu_vend_show()
behaviour.

Cheers, David

Re: [PATCH v3 3/4] target: add device vendor_id configfs attribute

[David Disseldorp]

On Tue, 20 Nov 2018 19:00:57 +0100, David Disseldorp wrote:

> > > + strncpy(buf, page, sizeof(buf));
> > 
> > Isn't strncpy() deprecated? How about using strlcpy() instead?  
> 
> Will change to use strlcpy in the next round.

Just a follow up here. I think it's safer to retain strncpy() in this
function for the purpose of zero filling. scsi_dump_inquiry() and
target_stat_lu_vend_show() walk the entire length of the t10_wwn.vendor
buffer.

Cheers, David

Re: [PATCH] target: drop unnecessary get_fabric_name() accessor from fabric_ops

[David Disseldorp]

On Fri, 23 Nov 2018 11:22:08 +0100, David Disseldorp wrote:

> > > Both fabric_ops.get_fabric_name() and fabric_ops.name are user facing,
> > > with the former being used for PR/ALUA state and the latter for configFS
> > > (config/target/$name), so we unfortunately need to keep both strings
> > > around for now.
> > 
> > Would it make sense to just use .name unless .fabric_name is set
> > to mostly avoid the duplication?  
> 
> Yeah, was thinking more along the lines of renaming .name to
> .fabric_alias and only setting it for the "iscsi" configfs case.
> What's your preference?

I've sent a follow-up patchset which includes this change as:
[PATCH 3/3] target: replace fabric_ops.name with fabric_alias

Feedback appreciated.

Cheers, David

[PATCH 2/3] target: drop unnecessary get_fabric_name() accessor from fabric_ops

[David Disseldorp]

All fabrics return a const string. In all cases *except* iSCSI the
get_fabric_name() string matches fabric_ops.name.

Both fabric_ops.get_fabric_name() and fabric_ops.name are user facing,
with the former being used for PR/ALUA state and the latter for configFS
(config/target/$name), so we unfortunately need to keep both strings
around for now.
Replace the useless .get_fabric_name() accessor function with a const
string fabric_name member variable.

Signed-off-by: David Disseldorp 
---
 drivers/infiniband/ulp/srpt/ib_srpt.c|  7 +--
 drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c |  7 +--
 drivers/scsi/qla2xxx/tcm_qla2xxx.c   | 14 +
 drivers/target/iscsi/iscsi_target_configfs.c |  7 +--
 drivers/target/loopback/tcm_loop.c   |  7 +--
 drivers/target/sbp/sbp_target.c  |  7 +--
 drivers/target/target_core_alua.c|  6 +-
 drivers/target/target_core_configfs.c| 18 +++---
 drivers/target/target_core_device.c  | 26 
 drivers/target/target_core_fabric_configfs.c |  2 +-
 drivers/target/target_core_pr.c  | 88 ++--
 drivers/target/target_core_stat.c|  4 +-
 drivers/target/target_core_tmr.c |  4 +-
 drivers/target/target_core_tpg.c | 22 +++
 drivers/target/target_core_transport.c   | 10 ++--
 drivers/target/target_core_ua.c  |  4 +-
 drivers/target/target_core_xcopy.c   |  7 +--
 drivers/target/tcm_fc/tfc_conf.c |  7 +--
 drivers/usb/gadget/function/f_tcm.c  |  7 +--
 drivers/vhost/scsi.c |  7 +--
 drivers/xen/xen-scsiback.c   |  7 +--
 include/target/target_core_fabric.h  |  6 +-
 22 files changed, 109 insertions(+), 165 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c 
b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 2357aa727dcf..657d728da40c 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -3147,11 +3147,6 @@ static int srpt_check_false(struct se_portal_group 
*se_tpg)
return 0;
 }
 
-static char *srpt_get_fabric_name(void)
-{
-   return "srpt";
-}
-
 static struct srpt_port *srpt_tpg_to_sport(struct se_portal_group *tpg)
 {
return tpg->se_tpg_wwn->priv;
@@ -3679,7 +3674,7 @@ static struct configfs_attribute *srpt_wwn_attrs[] = {
 static const struct target_core_fabric_ops srpt_template = {
.module = THIS_MODULE,
.name   = "srpt",
-   .get_fabric_name= srpt_get_fabric_name,
+   .fabric_name= "srpt",
.tpg_get_wwn= srpt_get_fabric_wwn,
.tpg_get_tag= srpt_get_tag,
.tpg_check_demo_mode= srpt_check_false,
diff --git a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c 
b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
index e63aadd10dfd..6e1c3e65f37b 100644
--- a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
+++ b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
@@ -3695,11 +3695,6 @@ static int ibmvscsis_get_system_info(void)
return 0;
 }
 
-static char *ibmvscsis_get_fabric_name(void)
-{
-   return "ibmvscsis";
-}
-
 static char *ibmvscsis_get_fabric_wwn(struct se_portal_group *se_tpg)
 {
struct ibmvscsis_tport *tport =
@@ -4045,8 +4040,8 @@ static struct configfs_attribute *ibmvscsis_tpg_attrs[] = 
{
 static const struct target_core_fabric_ops ibmvscsis_ops = {
.module = THIS_MODULE,
.name   = "ibmvscsis",
+   .fabric_name= "ibmvscsis",
.max_data_sg_nents  = MAX_TXU / PAGE_SIZE,
-   .get_fabric_name= ibmvscsis_get_fabric_name,
.tpg_get_wwn= ibmvscsis_get_fabric_wwn,
.tpg_get_tag= ibmvscsis_get_tag,
.tpg_get_default_depth  = ibmvscsis_get_default_depth,
diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c 
b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
index 65053c066680..ff8735effe28 100644
--- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
+++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
@@ -108,11 +108,6 @@ static ssize_t tcm_qla2xxx_format_wwn(char *buf, size_t 
len, u64 wwn)
b[0], b[1], b[2], b[3], b[4], b[5], b[6], b[7]);
 }
 
-static char *tcm_qla2xxx_get_fabric_name(void)
-{
-   return "qla2xxx";
-}
-
 /*
  * From drivers/scsi/scsi_transport_fc.c:fc_parse_wwn
  */
@@ -178,11 +173,6 @@ static int tcm_qla2xxx_npiv_parse_wwn(
return 0;
 }
 
-static char *tcm_qla2xxx_npiv_get_fabric_name(void)
-{
-   return "qla2xxx_npiv";
-}
-
 static char *tcm_qla2xxx_get_fabric_wwn(struct se_portal_group *se_tpg)
 {
struct tcm_qla2xxx_tpg *tpg = container_of(se_tpg,
@@ -1921,13 +1911,13 @@ static struct configfs_attribute 
*tcm_qla2xxx_wwn_attrs

[PATCH 0/3] target: drop unneeded pi_prot_format and get_fabric_name()

[David Disseldorp]

This patchset removes unneeded se_dev_attrib.pi_prot_format and
fabric_ops.get_fabric_name() struct members. Removal of the latter
allowed for further cleanup due to the fact that all fabric modules
except iscsi_target_mod provide matching strings for fabric_ops.name
and fabric_ops.get_fabric_name() - use a new fabric_ops.fabric_alias
member to handle the iscsi_target_mod special case.

Cheers, David

David Disseldorp (3):
  target: drop unused pi_prot_format attribute storage
  target: drop unnecessary get_fabric_name() accessor from fabric_ops
  target: replace fabric_ops.name with fabric_alias

 drivers/infiniband/ulp/srpt/ib_srpt.c|  8 +-
 drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c |  8 +-
 drivers/scsi/qla2xxx/tcm_qla2xxx.c   | 16 +---
 drivers/target/iscsi/iscsi_target_configfs.c |  9 +-
 drivers/target/loopback/tcm_loop.c   |  8 +-
 drivers/target/sbp/sbp_target.c  |  8 +-
 drivers/target/target_core_alua.c|  6 +-
 drivers/target/target_core_configfs.c| 47 ++-
 drivers/target/target_core_device.c  | 26 +++---
 drivers/target/target_core_fabric_configfs.c |  2 +-
 drivers/target/target_core_pr.c  | 88 ++--
 drivers/target/target_core_stat.c|  4 +-
 drivers/target/target_core_tmr.c |  4 +-
 drivers/target/target_core_tpg.c | 22 ++---
 drivers/target/target_core_transport.c   | 10 +--
 drivers/target/target_core_ua.c  |  4 +-
 drivers/target/target_core_xcopy.c   |  7 +-
 drivers/target/tcm_fc/tfc_conf.c |  8 +-
 drivers/usb/gadget/function/f_tcm.c  |  8 +-
 drivers/vhost/scsi.c |  8 +-
 drivers/xen/xen-scsiback.c   |  8 +-
 include/target/target_core_base.h|  1 -
 include/target/target_core_fabric.h  | 14 +++-
 23 files changed, 134 insertions(+), 190 deletions(-)

[PATCH 3/3] target: replace fabric_ops.name with fabric_alias

[David Disseldorp]

iscsi_target_mod is the only LIO fabric where fabric_ops.name differs
from the fabric_ops.fabric_name string.
fabric_ops.name is used when matching target/$fabric ConfigFS create
paths, so rename it .fabric_alias and fallback to target/$fabric vs
.fabric_name comparison if .fabric_alias isn't initialised.
iscsi_target_mod is the only fabric module to set .fabric_alias . All
other fabric modules rely on .fabric_name matching and can drop the
duplicate string.

Signed-off-by: David Disseldorp 
---
 drivers/infiniband/ulp/srpt/ib_srpt.c|  1 -
 drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c |  1 -
 drivers/scsi/qla2xxx/tcm_qla2xxx.c   |  2 --
 drivers/target/iscsi/iscsi_target_configfs.c |  2 +-
 drivers/target/loopback/tcm_loop.c   |  1 -
 drivers/target/sbp/sbp_target.c  |  1 -
 drivers/target/target_core_configfs.c| 30 +---
 drivers/target/tcm_fc/tfc_conf.c |  1 -
 drivers/usb/gadget/function/f_tcm.c  |  1 -
 drivers/vhost/scsi.c |  1 -
 drivers/xen/xen-scsiback.c   |  1 -
 include/target/target_core_fabric.h  | 12 ---
 12 files changed, 28 insertions(+), 26 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c 
b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 657d728da40c..41ee1f263bd6 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -3673,7 +3673,6 @@ static struct configfs_attribute *srpt_wwn_attrs[] = {
 
 static const struct target_core_fabric_ops srpt_template = {
.module = THIS_MODULE,
-   .name   = "srpt",
.fabric_name= "srpt",
.tpg_get_wwn= srpt_get_fabric_wwn,
.tpg_get_tag= srpt_get_tag,
diff --git a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c 
b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
index 6e1c3e65f37b..cc9cae469c4b 100644
--- a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
+++ b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
@@ -4039,7 +4039,6 @@ static struct configfs_attribute *ibmvscsis_tpg_attrs[] = 
{
 
 static const struct target_core_fabric_ops ibmvscsis_ops = {
.module = THIS_MODULE,
-   .name   = "ibmvscsis",
.fabric_name= "ibmvscsis",
.max_data_sg_nents  = MAX_TXU / PAGE_SIZE,
.tpg_get_wwn= ibmvscsis_get_fabric_wwn,
diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c 
b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
index ff8735effe28..fc312a5eab75 100644
--- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
+++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
@@ -1910,7 +1910,6 @@ static struct configfs_attribute *tcm_qla2xxx_wwn_attrs[] 
= {
 
 static const struct target_core_fabric_ops tcm_qla2xxx_ops = {
.module = THIS_MODULE,
-   .name   = "qla2xxx",
.fabric_name= "qla2xxx",
.node_acl_size  = sizeof(struct tcm_qla2xxx_nacl),
/*
@@ -1959,7 +1958,6 @@ static const struct target_core_fabric_ops 
tcm_qla2xxx_ops = {
 
 static const struct target_core_fabric_ops tcm_qla2xxx_npiv_ops = {
.module = THIS_MODULE,
-   .name   = "qla2xxx_npiv",
.fabric_name= "qla2xxx_npiv",
.node_acl_size  = sizeof(struct tcm_qla2xxx_nacl),
.tpg_get_wwn= tcm_qla2xxx_get_fabric_wwn,
diff --git a/drivers/target/iscsi/iscsi_target_configfs.c 
b/drivers/target/iscsi/iscsi_target_configfs.c
index 5c9e98ee42de..39a700a41f6e 100644
--- a/drivers/target/iscsi/iscsi_target_configfs.c
+++ b/drivers/target/iscsi/iscsi_target_configfs.c
@@ -1544,7 +1544,7 @@ static void lio_release_cmd(struct se_cmd *se_cmd)
 
 const struct target_core_fabric_ops iscsi_ops = {
.module = THIS_MODULE,
-   .name   = "iscsi",
+   .fabric_alias   = "iscsi",
.fabric_name= "iSCSI",
.node_acl_size  = sizeof(struct iscsi_node_acl),
.tpg_get_wwn= lio_tpg_get_endpoint_wwn,
diff --git a/drivers/target/loopback/tcm_loop.c 
b/drivers/target/loopback/tcm_loop.c
index 962845224c19..b0991e86587f 100644
--- a/drivers/target/loopback/tcm_loop.c
+++ b/drivers/target/loopback/tcm_loop.c
@@ -1144,7 +1144,6 @@ static struct configfs_attribute *tcm_loop_wwn_attrs[] = {
 
 static const struct target_core_fabric_ops loop_ops = {
.module = THIS_MODULE,
-   .name   = "loopback",
.fabric_name= "loopback",
.

[PATCH 1/3] target: drop unused pi_prot_format attribute storage

[David Disseldorp]

On write, the pi_prot_format configfs attribute invokes the device
format_prot() callback if present. Read dumps the contents of
se_dev_attrib.pi_prot_format , which is always zero.
Make the configfs attribute write-only, and drop the always zero
se_dev_attrib.pi_prot_format storage.

Signed-off-by: David Disseldorp 
---
 drivers/target/target_core_configfs.c | 3 +--
 include/target/target_core_base.h | 1 -
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/target/target_core_configfs.c 
b/drivers/target/target_core_configfs.c
index 70b9f6755c36..62427acdf503 100644
--- a/drivers/target/target_core_configfs.c
+++ b/drivers/target/target_core_configfs.c
@@ -535,7 +535,6 @@ DEF_CONFIGFS_ATTRIB_SHOW(emulate_3pc);
 DEF_CONFIGFS_ATTRIB_SHOW(emulate_pr);
 DEF_CONFIGFS_ATTRIB_SHOW(pi_prot_type);
 DEF_CONFIGFS_ATTRIB_SHOW(hw_pi_prot_type);
-DEF_CONFIGFS_ATTRIB_SHOW(pi_prot_format);
 DEF_CONFIGFS_ATTRIB_SHOW(pi_prot_verify);
 DEF_CONFIGFS_ATTRIB_SHOW(enforce_pr_isids);
 DEF_CONFIGFS_ATTRIB_SHOW(is_nonrot);
@@ -1121,7 +1120,7 @@ CONFIGFS_ATTR(, emulate_3pc);
 CONFIGFS_ATTR(, emulate_pr);
 CONFIGFS_ATTR(, pi_prot_type);
 CONFIGFS_ATTR_RO(, hw_pi_prot_type);
-CONFIGFS_ATTR(, pi_prot_format);
+CONFIGFS_ATTR_WO(, pi_prot_format);
 CONFIGFS_ATTR(, pi_prot_verify);
 CONFIGFS_ATTR(, enforce_pr_isids);
 CONFIGFS_ATTR(, is_nonrot);
diff --git a/include/target/target_core_base.h 
b/include/target/target_core_base.h
index c15054116b86..53b90cc18902 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -667,7 +667,6 @@ struct se_dev_attrib {
int emulate_caw;
int emulate_3pc;
int emulate_pr;
-   int pi_prot_format;
enum target_prot_type pi_prot_type;
enum target_prot_type hw_pi_prot_type;
int pi_prot_verify;
-- 
2.13.7

Re: [PATCH] target: drop unnecessary get_fabric_name() accessor from fabric_ops

[David Disseldorp]

On Thu, 22 Nov 2018 23:19:10 -0800, Christoph Hellwig wrote:

> On Thu, Nov 22, 2018 at 03:16:23PM +0100, David Disseldorp wrote:
> > All fabrics return a const string. In all cases *except* iSCSI the
> > get_fabric_name() string matches fabric_ops.name.
> >
> > Both fabric_ops.get_fabric_name() and fabric_ops.name are user facing,
> > with the former being used for PR/ALUA state and the latter for configFS
> > (config/target/$name), so we unfortunately need to keep both strings
> > around for now.  
> 
> Would it make sense to just use .name unless .fabric_name is set
> to mostly avoid the duplication?

Yeah, was thinking more along the lines of renaming .name to
.fabric_alias and only setting it for the "iscsi" configfs case.
What's your preference?

Cheers, David

[PATCH] target: drop unnecessary get_fabric_name() accessor from fabric_ops

[David Disseldorp]

All fabrics return a const string. In all cases *except* iSCSI the
get_fabric_name() string matches fabric_ops.name.

Both fabric_ops.get_fabric_name() and fabric_ops.name are user facing,
with the former being used for PR/ALUA state and the latter for configFS
(config/target/$name), so we unfortunately need to keep both strings
around for now.
Replace the useless .get_fabric_name() accessor function with a const
string fabric_name member variable.

Signed-off-by: David Disseldorp 
---
Note: This conflicts with:
[RFC PATCH] target: sanitize ALUA and PR state file paths before use
I'll resolve this once we decide whether or not the RFC change should
go in as-is.

 drivers/infiniband/ulp/srpt/ib_srpt.c|  7 +--
 drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c |  7 +--
 drivers/scsi/qla2xxx/tcm_qla2xxx.c   | 14 +
 drivers/target/iscsi/iscsi_target_configfs.c |  7 +--
 drivers/target/loopback/tcm_loop.c   |  7 +--
 drivers/target/sbp/sbp_target.c  |  7 +--
 drivers/target/target_core_alua.c|  6 +-
 drivers/target/target_core_configfs.c| 18 +++---
 drivers/target/target_core_device.c  | 26 
 drivers/target/target_core_fabric_configfs.c |  2 +-
 drivers/target/target_core_pr.c  | 88 ++--
 drivers/target/target_core_stat.c|  4 +-
 drivers/target/target_core_tmr.c |  4 +-
 drivers/target/target_core_tpg.c | 22 +++
 drivers/target/target_core_transport.c   | 10 ++--
 drivers/target/target_core_ua.c  |  4 +-
 drivers/target/target_core_xcopy.c   |  7 +--
 drivers/target/tcm_fc/tfc_conf.c |  7 +--
 drivers/usb/gadget/function/f_tcm.c  |  7 +--
 drivers/vhost/scsi.c |  7 +--
 drivers/xen/xen-scsiback.c   |  7 +--
 include/target/target_core_fabric.h  |  6 +-
 22 files changed, 109 insertions(+), 165 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c 
b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 2357aa727dcf..657d728da40c 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -3147,11 +3147,6 @@ static int srpt_check_false(struct se_portal_group 
*se_tpg)
return 0;
 }
 
-static char *srpt_get_fabric_name(void)
-{
-   return "srpt";
-}
-
 static struct srpt_port *srpt_tpg_to_sport(struct se_portal_group *tpg)
 {
return tpg->se_tpg_wwn->priv;
@@ -3679,7 +3674,7 @@ static struct configfs_attribute *srpt_wwn_attrs[] = {
 static const struct target_core_fabric_ops srpt_template = {
.module = THIS_MODULE,
.name   = "srpt",
-   .get_fabric_name= srpt_get_fabric_name,
+   .fabric_name= "srpt",
.tpg_get_wwn= srpt_get_fabric_wwn,
.tpg_get_tag= srpt_get_tag,
.tpg_check_demo_mode= srpt_check_false,
diff --git a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c 
b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
index e63aadd10dfd..6e1c3e65f37b 100644
--- a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
+++ b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
@@ -3695,11 +3695,6 @@ static int ibmvscsis_get_system_info(void)
return 0;
 }
 
-static char *ibmvscsis_get_fabric_name(void)
-{
-   return "ibmvscsis";
-}
-
 static char *ibmvscsis_get_fabric_wwn(struct se_portal_group *se_tpg)
 {
struct ibmvscsis_tport *tport =
@@ -4045,8 +4040,8 @@ static struct configfs_attribute *ibmvscsis_tpg_attrs[] = 
{
 static const struct target_core_fabric_ops ibmvscsis_ops = {
.module = THIS_MODULE,
.name   = "ibmvscsis",
+   .fabric_name= "ibmvscsis",
.max_data_sg_nents  = MAX_TXU / PAGE_SIZE,
-   .get_fabric_name= ibmvscsis_get_fabric_name,
.tpg_get_wwn= ibmvscsis_get_fabric_wwn,
.tpg_get_tag= ibmvscsis_get_tag,
.tpg_get_default_depth  = ibmvscsis_get_default_depth,
diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c 
b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
index 65053c066680..ff8735effe28 100644
--- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
+++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
@@ -108,11 +108,6 @@ static ssize_t tcm_qla2xxx_format_wwn(char *buf, size_t 
len, u64 wwn)
b[0], b[1], b[2], b[3], b[4], b[5], b[6], b[7]);
 }
 
-static char *tcm_qla2xxx_get_fabric_name(void)
-{
-   return "qla2xxx";
-}
-
 /*
  * From drivers/scsi/scsi_transport_fc.c:fc_parse_wwn
  */
@@ -178,11 +173,6 @@ static int tcm_qla2xxx_npiv_parse_wwn(
return 0;
 }
 
-static char *tcm_qla2xxx_npiv_get_fabric_name(void)
-{
-   return "qla2xxx_npiv";
-}
-
 static char *tcm_qla2xxx_get_fabric_wwn

[RFC PATCH] target: sanitize ALUA and PR state file paths before use

[David Disseldorp]

Block ALUA and PR state storage if any of the dynamic subdirectory
components include a path separator.

Fixes: c66ac9db8d4a ("[SCSI] target: Add LIO target core v4.0.0-rc6")
Signed-off-by: David Disseldorp 
Signed-off-by: Lee Duncan 
---
Note:
Submitted as an RFC, as I've not properly tested the alua code path.
I'm also not sure whether it's reasonable to break existing setups
with a '/' in the configured unit_serial. Where "break" means fail
APTPL PR requests; ALUA state-save failures are ignored internally.

 drivers/target/target_core_alua.c | 27 ---
 drivers/target/target_core_pr.c   |  5 +
 2 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/drivers/target/target_core_alua.c 
b/drivers/target/target_core_alua.c
index 4f134b0c3e29..517945f881e0 100644
--- a/drivers/target/target_core_alua.c
+++ b/drivers/target/target_core_alua.c
@@ -918,9 +918,16 @@ static int core_alua_update_tpg_primary_metadata(
 {
unsigned char *md_buf;
struct t10_wwn *wwn = _pt_gp->tg_pt_gp_dev->t10_wwn;
+   const char *tpgs_name;
char *path;
int len, rc;
 
+   tpgs_name = config_item_name(_pt_gp->tg_pt_gp_group.cg_item);
+   if (strchr(wwn->unit_serial, '/') || strchr(tpgs_name, '/')) {
+   pr_err("Unable to construct valid ALUA metadata path\n");
+   return -EINVAL;
+   }
+
md_buf = kzalloc(ALUA_MD_BUF_LEN, GFP_KERNEL);
if (!md_buf) {
pr_err("Unable to allocate buf for ALUA metadata\n");
@@ -937,8 +944,7 @@ static int core_alua_update_tpg_primary_metadata(
 
rc = -ENOMEM;
path = kasprintf(GFP_KERNEL, "%s/alua/tpgs_%s/%s", db_root,
-   >unit_serial[0],
-   config_item_name(_pt_gp->tg_pt_gp_group.cg_item));
+   >unit_serial[0], tpgs_name);
if (path) {
rc = core_alua_write_tpg_metadata(path, md_buf, len);
kfree(path);
@@ -1210,6 +1216,8 @@ static int core_alua_update_tpg_secondary_metadata(struct 
se_lun *lun)
 {
struct se_portal_group *se_tpg = lun->lun_tpg;
unsigned char *md_buf;
+   const char *fabric_name;
+   const char *wwn;
char *path;
int len, rc;
 
@@ -1227,17 +1235,22 @@ static int 
core_alua_update_tpg_secondary_metadata(struct se_lun *lun)
atomic_read(>lun_tg_pt_secondary_offline),
lun->lun_tg_pt_secondary_stat);
 
+   fabric_name = se_tpg->se_tpg_tfo->get_fabric_name();
+   wwn = se_tpg->se_tpg_tfo->tpg_get_wwn(se_tpg);
+   if (strchr(fabric_name, '/') || strchr(wwn, '/')) {
+   pr_err("Unable to construct valid ALUA metadata path\n");
+   rc = -EINVAL;
+   goto out_free;
+   }
+
if (se_tpg->se_tpg_tfo->tpg_get_tag != NULL) {
path = kasprintf(GFP_KERNEL, "%s/alua/%s/%s+%hu/lun_%llu",
-   db_root, se_tpg->se_tpg_tfo->get_fabric_name(),
-   se_tpg->se_tpg_tfo->tpg_get_wwn(se_tpg),
+   db_root, fabric_name, wwn,
se_tpg->se_tpg_tfo->tpg_get_tag(se_tpg),
lun->unpacked_lun);
} else {
path = kasprintf(GFP_KERNEL, "%s/alua/%s/%s/lun_%llu",
-   db_root, se_tpg->se_tpg_tfo->get_fabric_name(),
-   se_tpg->se_tpg_tfo->tpg_get_wwn(se_tpg),
-   lun->unpacked_lun);
+   db_root, fabric_name, wwn, lun->unpacked_lun);
}
if (!path) {
rc = -ENOMEM;
diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
index 10db5656fd5d..48397cf919d4 100644
--- a/drivers/target/target_core_pr.c
+++ b/drivers/target/target_core_pr.c
@@ -1980,6 +1980,11 @@ static int __core_scsi3_write_aptpl_to_file(
int ret;
loff_t pos = 0;
 
+   if (strchr(wwn->unit_serial, '/')) {
+   pr_err("Unable to construct valid APTPL metadata path\n");
+   return -EINVAL;
+   }
+
path = kasprintf(GFP_KERNEL, "%s/pr/aptpl_%s", db_root,
>unit_serial[0]);
if (!path)
-- 
2.13.7

Re: [PATCH v3 3/4] target: add device vendor_id configfs attribute

[David Disseldorp]

On Tue, 20 Nov 2018 09:24:39 -0800, Bart Van Assche wrote:

> On Mon, 2018-11-19 at 22:06 +0100, David Disseldorp wrote:
> >  /*
> > + * STANDARD and VPD page 0x80 T10 Vendor Identification
> > + */
> > +static ssize_t target_wwn_vendor_id_show(struct config_item *item,
> > +   char *page)
> > +{
> > +   return sprintf(page, "T10 Vendor Identification: %."
> > +  __stringify(INQUIRY_VENDOR_IDENTIFIER_LEN) "s\n",
> > +  _t10_wwn(item)->vendor[0]);
> > +}  
> 
> This doesn't follow the convention used by other configfs attributes,
> namely that only the value should be reported and no prefix. Please leave
> out the "T10 Vendor Identification: " prefix.

I based this on the convention used with
target_wwn_vpd_unit_serial_show(). I'm happy to drop the prefix if you
prefer.

> > +static ssize_t target_wwn_vendor_id_store(struct config_item *item,
> > +   const char *page, size_t count)
> > +{
> > +   struct t10_wwn *t10_wwn = to_t10_wwn(item);
> > +   struct se_device *dev = t10_wwn->t10_dev;
> > +   /* +1 to ensure buf is zero terminated for stripping */
> > +   unsigned char buf[INQUIRY_VENDOR_IDENTIFIER_LEN + 1];
> > +
> > +   if (strlen(page) > INQUIRY_VENDOR_IDENTIFIER_LEN) {
> > +   pr_err("Emulated T10 Vendor Identification exceeds"
> > +   " INQUIRY_VENDOR_IDENTIFIER_LEN: %d\n",
> > +   INQUIRY_VENDOR_IDENTIFIER_LEN);
> > +   return -EOVERFLOW;
> > +   }  
> 
> Trailing newline(s) should be stripped before the length check is performed. I
> don't think that you want to force users to use "echo -n" instead of "echo" 
> when
> setting this attribute.

This is also a target_wwn_vpd_unit_serial_store() carryover, which
checks the length prior to the strip. Doing so makes buffer length
a little easier to determine.

> > +   strncpy(buf, page, sizeof(buf));  
> 
> Isn't strncpy() deprecated? How about using strlcpy() instead?

Will change to use strlcpy in the next round.

> > +   /*
> > +* Check to see if any active $FABRIC_MOD exports exist.  If they
> > +* do exist, fail here as changing this information on the fly
> > +* (underneath the initiator side OS dependent multipath code)
> > +* could cause negative effects.
> > +*/
> > +   if (dev->export_count) {
> > +   pr_err("Unable to set T10 Vendor Identification while"
> > +   " active %d $FABRIC_MOD exports exist\n",
> > +   dev->export_count);
> > +   return -EINVAL;
> > +   }  
> 
> Are there any users who understand what "$FABRIC_MOD" means? Please leave out 
> that
> string or change it into the name of the fabric driver followed by the name 
> of the
> target port associated with 'item'.

Another target_wwn_vpd_unit_serial_store() carryover. Will drop the
string from the next round.

> > +
> > +   /*
> > +* Assume ASCII encoding. Strip any newline added from userspace.
> > +* The result may *not* be null terminated.
> > +*/
> > +   strncpy(dev->t10_wwn.vendor, strstrip(buf),
> > +   INQUIRY_VENDOR_IDENTIFIER_LEN);  
> 
> Keeping strings around that are not '\0'-terminated is a booby trap. It is 
> very
> easy for anyone who modifies or reviews code that uses such strings to 
> overlook
> that the string is not '\0'-terminated. Please increase the size of the 
> vendor[]
> array by one and make sure that that string is '\0'-terminated.

I tend to agree that it's dangerous, but chose to stay somewhat
consistent with the other t10_wwn strings that are treated as though
they may not be NULL terminated.

If you're in favour adding an extra terminator byte here, then I think
it'd make sense to do the same for model[], revision[] and unit_serial[]
too. Are you okay with that approach?

Cheers, David

Re: [PATCH v3 2/4] target: don't assume t10_wwn.vendor is null terminated

[David Disseldorp]

On Tue, 20 Nov 2018 08:49:24 -0800, Christoph Hellwig wrote:

> This could use a little more explanation, the code doesn't just
> add a little if but also changes the existing case.  Also where
> can't it be null currently?

I'll add an explanation in the next round. This patch shouldn't cause
any change in behaviour. The vendor string is currently always NULL
terminated, but won't be once patch 3/4 is applied, hence the need to
add the maximum string width specifier.

Thanks for the feedback.

[PATCH v3 1/4] target: use consistent left-aligned ASCII INQUIRY data

[David Disseldorp]

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 
---
 drivers/target/target_core_spc.c | 17 -
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/target/target_core_spc.c b/drivers/target/target_core_spc.c
index f459118bc11b..c37dd36ec77d 100644
--- a/drivers/target/target_core_spc.c
+++ b/drivers/target/target_core_spc.c
@@ -108,12 +108,17 @@ spc_emulate_inquiry_std(struct se_cmd *cmd, unsigned char 
*buf)
 
buf[7] = 0x2; /* CmdQue=1 */
 
-   memcpy([8], "LIO-ORG ", 8);
-   memset([16], 0x20, 16);
+   /*
+* 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).
+*/
+   memset([8], 0x20, 8 + 16 + 4);
+   memcpy([8], "LIO-ORG", sizeof("LIO-ORG") - 1);
memcpy([16], dev->t10_wwn.model,
-  min_t(size_t, strlen(dev->t10_wwn.model), 16));
+  strnlen(dev->t10_wwn.model, 16));
memcpy([32], dev->t10_wwn.revision,
-  min_t(size_t, strlen(dev->t10_wwn.revision), 4));
+  strnlen(dev->t10_wwn.revision, 4));
buf[4] = 31; /* Set additional length to 31 */
 
return 0;
@@ -251,7 +256,9 @@ spc_emulate_evpd_83(struct se_cmd *cmd, unsigned char *buf)
buf[off] = 0x2; /* ASCII */
buf[off+1] = 0x1; /* T10 Vendor ID */
buf[off+2] = 0x0;
-   memcpy([off+4], "LIO-ORG", 8);
+   /* left align Vendor ID and pad with spaces */
+   memset([off+4], 0x20, 8);
+   memcpy([off+4], "LIO-ORG", sizeof("LIO-ORG") - 1);
/* Extra Byte for NULL Terminator */
id_len++;
/* Identifier Length */
-- 
2.13.7

[PATCH v3 2/4] target: don't assume t10_wwn.vendor is null terminated

[David Disseldorp]

Signed-off-by: David Disseldorp 
---
 drivers/target/target_core_stat.c | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/target/target_core_stat.c 
b/drivers/target/target_core_stat.c
index f0db91ebd735..89f35167f036 100644
--- a/drivers/target/target_core_stat.c
+++ b/drivers/target/target_core_stat.c
@@ -804,10 +804,17 @@ static ssize_t target_stat_transport_dev_name_show(struct 
config_item *item,
if (dev) {
wwn = >t10_wwn;
/* scsiTransportDevName */
-   ret = snprintf(page, PAGE_SIZE, "%s+%s\n",
+   if (strlen(wwn->unit_serial)) {
+   ret = snprintf(page, PAGE_SIZE, "%s+%s\n",
+   tpg->se_tpg_tfo->tpg_get_wwn(tpg),
+   wwn->unit_serial);
+   } else {
+   ret = snprintf(page, PAGE_SIZE, "%s+%."
+   __stringify(INQUIRY_VENDOR_IDENTIFIER_LEN)
+   "s\n",
tpg->se_tpg_tfo->tpg_get_wwn(tpg),
-   (strlen(wwn->unit_serial)) ? wwn->unit_serial :
wwn->vendor);
+   }
}
rcu_read_unlock();
return ret;
-- 
2.13.7

[PATCH v3 4/4] 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 
---
 drivers/target/target_core_spc.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/target/target_core_spc.c b/drivers/target/target_core_spc.c
index c37dd36ec77d..7e8582d7f4a4 100644
--- a/drivers/target/target_core_spc.c
+++ b/drivers/target/target_core_spc.c
@@ -114,7 +114,8 @@ spc_emulate_inquiry_std(struct se_cmd *cmd, unsigned char 
*buf)
 * unused bytes shall be filled with ASCII space characters (20h).
 */
memset([8], 0x20, 8 + 16 + 4);
-   memcpy([8], "LIO-ORG", sizeof("LIO-ORG") - 1);
+   memcpy([8], dev->t10_wwn.vendor,
+  strnlen(dev->t10_wwn.vendor, 8));
memcpy([16], dev->t10_wwn.model,
   strnlen(dev->t10_wwn.model, 16));
memcpy([32], dev->t10_wwn.revision,
@@ -258,7 +259,8 @@ spc_emulate_evpd_83(struct se_cmd *cmd, unsigned char *buf)
buf[off+2] = 0x0;
/* left align Vendor ID and pad with spaces */
memset([off+4], 0x20, 8);
-   memcpy([off+4], "LIO-ORG", sizeof("LIO-ORG") - 1);
+   memcpy([off+4], dev->t10_wwn.vendor,
+  strnlen(dev->t10_wwn.vendor, 8));
/* Extra Byte for NULL Terminator */
id_len++;
/* Identifier Length */
-- 
2.13.7

[PATCH v3 0/4] target: user configurable T10 Vendor ID

[David Disseldorp]

This patchset allows for the modification of the T10 Vendor
Identification string returned in the SCSI INQUIRY response, via the
target/core/$backstore/$name/wwn/vendor_id ConfigFS path.

Changes since v2:
- https://www.spinics.net/lists/target-devel/msg10720.html
- Support eight byte vendor ID strings
- Split out consistent INQUIRY data padding as a separate patch
- Drop t10_wwn.model buffer print fix, already upstream

Changes since v1:
- https://www.spinics.net/lists/target-devel/msg10545.html
- Rebase against nab's for-next branch, which includes Christoph's
  configfs API changes.

David Disseldorp (4):
  target: use consistent left-aligned ASCII INQUIRY data
  target: don't assume t10_wwn.vendor is null terminated
  target: add device vendor_id configfs attribute
  target: remove hardcoded T10 Vendor ID in INQUIRY response

 drivers/target/target_core_configfs.c | 55 +++
 drivers/target/target_core_spc.c  | 19 ++---
 drivers/target/target_core_stat.c | 11 +-
 include/target/target_core_base.h |  3 +-
 4 files changed, 80 insertions(+), 8 deletions(-)

[PATCH v3 3/4] target: add device vendor_id configfs attribute

[David Disseldorp]

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 | 55 +++
 include/target/target_core_base.h |  3 +-
 2 files changed, 57 insertions(+), 1 deletion(-)

diff --git a/drivers/target/target_core_configfs.c 
b/drivers/target/target_core_configfs.c
index f6b1549f4142..64e95376d998 100644
--- a/drivers/target/target_core_configfs.c
+++ b/drivers/target/target_core_configfs.c
@@ -1211,6 +1211,59 @@ static struct t10_wwn *to_t10_wwn(struct config_item 
*item)
 }
 
 /*
+ * STANDARD and VPD page 0x80 T10 Vendor Identification
+ */
+static ssize_t target_wwn_vendor_id_show(struct config_item *item,
+   char *page)
+{
+   return sprintf(page, "T10 Vendor Identification: %."
+  __stringify(INQUIRY_VENDOR_IDENTIFIER_LEN) "s\n",
+  _t10_wwn(item)->vendor[0]);
+}
+
+static ssize_t target_wwn_vendor_id_store(struct config_item *item,
+   const char *page, size_t count)
+{
+   struct t10_wwn *t10_wwn = to_t10_wwn(item);
+   struct se_device *dev = t10_wwn->t10_dev;
+   /* +1 to ensure buf is zero terminated for stripping */
+   unsigned char buf[INQUIRY_VENDOR_IDENTIFIER_LEN + 1];
+
+   if (strlen(page) > INQUIRY_VENDOR_IDENTIFIER_LEN) {
+   pr_err("Emulated T10 Vendor Identification exceeds"
+   " INQUIRY_VENDOR_IDENTIFIER_LEN: %d\n",
+   INQUIRY_VENDOR_IDENTIFIER_LEN);
+   return -EOVERFLOW;
+   }
+   strncpy(buf, page, sizeof(buf));
+   /*
+* Check to see if any active $FABRIC_MOD exports exist.  If they
+* do exist, fail here as changing this information on the fly
+* (underneath the initiator side OS dependent multipath code)
+* could cause negative effects.
+*/
+   if (dev->export_count) {
+   pr_err("Unable to set T10 Vendor Identification while"
+   " active %d $FABRIC_MOD exports exist\n",
+   dev->export_count);
+   return -EINVAL;
+   }
+
+   /*
+* Assume ASCII encoding. Strip any newline added from userspace.
+* The result may *not* be null terminated.
+*/
+   strncpy(dev->t10_wwn.vendor, strstrip(buf),
+   INQUIRY_VENDOR_IDENTIFIER_LEN);
+
+   pr_debug("Target_Core_ConfigFS: Set emulated T10 Vendor Identification:"
+" %." __stringify(INQUIRY_VENDOR_IDENTIFIER_LEN) "s\n",
+dev->t10_wwn.vendor);
+
+   return count;
+}
+
+/*
  * VPD page 0x80 Unit serial
  */
 static ssize_t target_wwn_vpd_unit_serial_show(struct config_item *item,
@@ -1356,6 +1409,7 @@ DEF_DEV_WWN_ASSOC_SHOW(vpd_assoc_target_port, 0x10);
 /* VPD page 0x83 Association: SCSI Target Device */
 DEF_DEV_WWN_ASSOC_SHOW(vpd_assoc_scsi_target_device, 0x20);
 
+CONFIGFS_ATTR(target_wwn_, vendor_id);
 CONFIGFS_ATTR(target_wwn_, vpd_unit_serial);
 CONFIGFS_ATTR_RO(target_wwn_, vpd_protocol_identifier);
 CONFIGFS_ATTR_RO(target_wwn_, vpd_assoc_logical_unit);
@@ -1363,6 +1417,7 @@ CONFIGFS_ATTR_RO(target_wwn_, vpd_assoc_target_port);
 CONFIGFS_ATTR_RO(target_wwn_, vpd_assoc_scsi_target_device);
 
 static struct configfs_attribute *target_core_dev_wwn_attrs[] = {
+   _wwn_attr_vendor_id,
_wwn_attr_vpd_unit_serial,
_wwn_attr_vpd_protocol_identifier,
_wwn_attr_vpd_assoc_logical_unit,
diff --git a/include/target/target_core_base.h 
b/include/target/target_core_base.h
index e3bdb0550a59..45be5427326d 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -45,6 +45,7 @@
 #define INQUIRY_VPD_SERIAL_LEN 254
 /* Used by transport_get_inquiry_vpd_device_ident() */
 #define INQUIRY_VPD_DEVICE_IDENTIFIER_LEN  254
+#define INQUIRY_VENDOR_IDENTIFIER_LEN  8
 
 /* Attempts before moving from SHORT to LONG */
 #define PYX_TRANSPORT_WINDOW_CLOSED_THRESHOLD  3
@@ -314,7 +315,7 @@ struct t10_vpd {
 };
 
 struct t10_wwn {
-   char vendor[8];
+   char vendor[INQUIRY_VENDOR_IDENTIFIER_LEN];
char model[16];
char revision[4];
char unit_serial[INQUIRY_VPD_SERIAL_LEN];
-- 
2.13.7

Re: [PATCH v5] target: add emulate_pr backstore attr to toggle PR support

[David Disseldorp]

Ping, did anyone get a chance to look at this one?

Cheers, David

Re: [RFC PATCH 0/3] target: remove some unused stats

[David Disseldorp]

Hi Mike

On Tue, 30 Oct 2018 10:54:07 -0500, Mike Christie wrote:

> >> This patchset removes a couple of unused error stat counters and a
> >> redundant cumulative counter.
> >> I've tagged this patchset RFC, as it may be considered a kernel<->user
> >> (configfs) API change.  
> > 
> > Ping, any thoughts on this patchset?
> >   
> 
> I think these stats were supposed to match the iSCSI MIB definitions. It
> was not clear why we didn't just fix them so they report the correct values?

I finally got a chance to look through rfc#4544. It does indeed look
like these counters correspond to:
IscsiInstanceSsnErrorStatsEntry ::= SEQUENCE {
iscsiInstSsnDigestErrors   Counter32,
iscsiInstSsnCxnTimeoutErrors   Counter32,
iscsiInstSsnFormatErrors   Counter32
}

There doesn't appear to be anything stopping us from plumbing them into
the corresponding error paths, so I'll withdraw this patchset.

Cheers, David

[PATCH v5] target: add emulate_pr backstore attr to toggle PR support

[David Disseldorp]

The new emulate_pr backstore attribute allows for Persistent Reservation
and SCSI2 RESERVE/RELEASE support to be completely disabled. This can be
useful for scenarios such as:
- Ensuring ATS (Compare & Write) usage on recent VMware ESXi initiators.
- Allowing clustered (e.g. tcm-user) backends to block such requests,
  avoiding the multi-node reservation state propagation.

When explicitly disabled, PR and RESERVE/RELEASE requests receive
Invalid Command Operation Code response sense data.

Signed-off-by: David Disseldorp 
---
 drivers/target/target_core_configfs.c | 24 ++--
 drivers/target/target_core_device.c   | 13 +
 drivers/target/target_core_pr.c   |  2 ++
 drivers/target/target_core_spc.c  |  8 
 include/target/target_core_base.h |  3 +++
 5 files changed, 44 insertions(+), 6 deletions(-)

Changes since v4:
* keep pgr_support attribute independent of emulate_pr

Changes since v3:
* rebase against current mainline

Changes since v2:
* handle target_pr_res_aptpl_metadata_store()
* use common error path for spc_parse_cdb() and passthrough_parse_cdb()
  checks
* drop erroneous TRANSPORT_FLAG_PASSTHROUGH_PGR ->
  TRANSPORT_FLAG_PASSTHROUGH changes

Changes since v1:
* block Reservation request passthrough when emulate_pr=0
* fix some style issues
* add an emulate_pr check to pgr_support_show()

diff --git a/drivers/target/target_core_configfs.c 
b/drivers/target/target_core_configfs.c
index f6b1549f4142..70b9f6755c36 100644
--- a/drivers/target/target_core_configfs.c
+++ b/drivers/target/target_core_configfs.c
@@ -532,6 +532,7 @@ DEF_CONFIGFS_ATTRIB_SHOW(emulate_tpu);
 DEF_CONFIGFS_ATTRIB_SHOW(emulate_tpws);
 DEF_CONFIGFS_ATTRIB_SHOW(emulate_caw);
 DEF_CONFIGFS_ATTRIB_SHOW(emulate_3pc);
+DEF_CONFIGFS_ATTRIB_SHOW(emulate_pr);
 DEF_CONFIGFS_ATTRIB_SHOW(pi_prot_type);
 DEF_CONFIGFS_ATTRIB_SHOW(hw_pi_prot_type);
 DEF_CONFIGFS_ATTRIB_SHOW(pi_prot_format);
@@ -592,6 +593,7 @@ static ssize_t _name##_store(struct config_item *item, 
const char *page,\
 DEF_CONFIGFS_ATTRIB_STORE_BOOL(emulate_fua_write);
 DEF_CONFIGFS_ATTRIB_STORE_BOOL(emulate_caw);
 DEF_CONFIGFS_ATTRIB_STORE_BOOL(emulate_3pc);
+DEF_CONFIGFS_ATTRIB_STORE_BOOL(emulate_pr);
 DEF_CONFIGFS_ATTRIB_STORE_BOOL(enforce_pr_isids);
 DEF_CONFIGFS_ATTRIB_STORE_BOOL(is_nonrot);
 
@@ -1116,6 +1118,7 @@ CONFIGFS_ATTR(, emulate_tpu);
 CONFIGFS_ATTR(, emulate_tpws);
 CONFIGFS_ATTR(, emulate_caw);
 CONFIGFS_ATTR(, emulate_3pc);
+CONFIGFS_ATTR(, emulate_pr);
 CONFIGFS_ATTR(, pi_prot_type);
 CONFIGFS_ATTR_RO(, hw_pi_prot_type);
 CONFIGFS_ATTR(, pi_prot_format);
@@ -1156,6 +1159,7 @@ struct configfs_attribute *sbc_attrib_attrs[] = {
_emulate_tpws,
_emulate_caw,
_emulate_3pc,
+   _emulate_pr,
_pi_prot_type,
_hw_pi_prot_type,
_pi_prot_format,
@@ -1427,6 +1431,9 @@ static ssize_t target_pr_res_holder_show(struct 
config_item *item, char *page)
struct se_device *dev = pr_to_dev(item);
int ret;
 
+   if (!dev->dev_attrib.emulate_pr)
+   return sprintf(page, "SPC_RESERVATIONS_DISABLED\n");
+
if (dev->transport->transport_flags & TRANSPORT_FLAG_PASSTHROUGH_PGR)
return sprintf(page, "Passthrough\n");
 
@@ -1567,12 +1574,14 @@ static ssize_t target_pr_res_type_show(struct 
config_item *item, char *page)
 {
struct se_device *dev = pr_to_dev(item);
 
+   if (!dev->dev_attrib.emulate_pr)
+   return sprintf(page, "SPC_RESERVATIONS_DISABLED\n");
if (dev->transport->transport_flags & TRANSPORT_FLAG_PASSTHROUGH_PGR)
return sprintf(page, "SPC_PASSTHROUGH\n");
-   else if (dev->dev_reservation_flags & DRF_SPC2_RESERVATIONS)
+   if (dev->dev_reservation_flags & DRF_SPC2_RESERVATIONS)
return sprintf(page, "SPC2_RESERVATIONS\n");
-   else
-   return sprintf(page, "SPC3_PERSISTENT_RESERVATIONS\n");
+
+   return sprintf(page, "SPC3_PERSISTENT_RESERVATIONS\n");
 }
 
 static ssize_t target_pr_res_aptpl_active_show(struct config_item *item,
@@ -1580,7 +1589,8 @@ static ssize_t target_pr_res_aptpl_active_show(struct 
config_item *item,
 {
struct se_device *dev = pr_to_dev(item);
 
-   if (dev->transport->transport_flags & TRANSPORT_FLAG_PASSTHROUGH_PGR)
+   if (!dev->dev_attrib.emulate_pr ||
+   (dev->transport->transport_flags & TRANSPORT_FLAG_PASSTHROUGH_PGR))
return 0;
 
return sprintf(page, "APTPL Bit Status: %s\n",
@@ -1592,7 +1602,8 @@ static ssize_t target_pr_res_aptpl_metadata_show(struct 
config_item *item,
 {
struct se_device *dev = pr_to_dev(item);
 
-   if (dev->transport->transport_flags & TRANSPORT_FLAG_PASSTHROUGH_PGR)
+   if (!dev->dev_attrib.emulate_pr ||
+   (dev->transport->transport_

Re: [PATCH v4] target: add emulate_pr backstore attr to toggle PR support

[David Disseldorp]

On Tue, 6 Nov 2018 19:54:17 -0600, Mike Christie wrote:

> > -   return snprintf(page, PAGE_SIZE, "%d\n",
> > -   flags & TRANSPORT_FLAG_PASSTHROUGH_PGR ? 0 : 1);
> > +   if (!da->da_dev->dev_attrib.emulate_pr ||
> > +   (flags & TRANSPORT_FLAG_PASSTHROUGH_PGR))
> > +   pgr_support = 0;
> > +  
> 
> I think we want to keep this separate still. The file tells userspace if
> PRs are supported in the backend module/device or in LIO core.
> 
> With the chunk above, if you had emulate_pr=0 and
> TRANSPORT_FLAG_PASSTHROUGH_PGR is set, userspace cannot detect what the
> backend supports. We would have to temporarily set emaulate_pr sow e can
> read the file then clear it.

Agreed, that'd be awkward and is unnecessary given the presence of both
configfs attributes. I'll send a new version which drops this hunk.

Cheers, David

[PATCH v4] target: add emulate_pr backstore attr to toggle PR support

[David Disseldorp]

The new emulate_pr backstore attribute allows for Persistent Reservation
and SCSI2 RESERVE/RELEASE support to be completely disabled. This can be
useful for scenarios such as:
- Ensuring ATS (Compare & Write) usage on recent VMware ESXi initiators.
- Allowing clustered (e.g. tcm-user) backends to block such requests,
  avoiding the need for multi-node reservation state propagation.

When explicitly disabled, PR and RESERVE/RELEASE requests receive
Invalid Command Operation Code response sense data.

Signed-off-by: David Disseldorp 
---
 drivers/target/target_core_configfs.c | 32 
 drivers/target/target_core_device.c   | 13 +
 drivers/target/target_core_pr.c   |  2 ++
 drivers/target/target_core_spc.c  |  8 
 include/target/target_core_base.h |  3 +++
 5 files changed, 50 insertions(+), 8 deletions(-)

Changes since v3:
* rebase against current mainline

Changes since v2:
* handle target_pr_res_aptpl_metadata_store()
* use common error path for spc_parse_cdb() and passthrough_parse_cdb()
  checks
* drop erroneous TRANSPORT_FLAG_PASSTHROUGH_PGR ->
  TRANSPORT_FLAG_PASSTHROUGH changes

Changes since v1:
* block Reservation request passthrough when emulate_pr=0
* fix some style issues
* add an emulate_pr check to pgr_support_show()

diff --git a/drivers/target/target_core_configfs.c 
b/drivers/target/target_core_configfs.c
index f6b1549f4142..bacb771a333e 100644
--- a/drivers/target/target_core_configfs.c
+++ b/drivers/target/target_core_configfs.c
@@ -532,6 +532,7 @@ DEF_CONFIGFS_ATTRIB_SHOW(emulate_tpu);
 DEF_CONFIGFS_ATTRIB_SHOW(emulate_tpws);
 DEF_CONFIGFS_ATTRIB_SHOW(emulate_caw);
 DEF_CONFIGFS_ATTRIB_SHOW(emulate_3pc);
+DEF_CONFIGFS_ATTRIB_SHOW(emulate_pr);
 DEF_CONFIGFS_ATTRIB_SHOW(pi_prot_type);
 DEF_CONFIGFS_ATTRIB_SHOW(hw_pi_prot_type);
 DEF_CONFIGFS_ATTRIB_SHOW(pi_prot_format);
@@ -592,6 +593,7 @@ static ssize_t _name##_store(struct config_item *item, 
const char *page,\
 DEF_CONFIGFS_ATTRIB_STORE_BOOL(emulate_fua_write);
 DEF_CONFIGFS_ATTRIB_STORE_BOOL(emulate_caw);
 DEF_CONFIGFS_ATTRIB_STORE_BOOL(emulate_3pc);
+DEF_CONFIGFS_ATTRIB_STORE_BOOL(emulate_pr);
 DEF_CONFIGFS_ATTRIB_STORE_BOOL(enforce_pr_isids);
 DEF_CONFIGFS_ATTRIB_STORE_BOOL(is_nonrot);
 
@@ -1100,9 +1102,13 @@ static ssize_t pgr_support_show(struct config_item 
*item, char *page)
 {
struct se_dev_attrib *da = to_attrib(item);
u8 flags = da->da_dev->transport->transport_flags;
+   int pgr_support = 1;
 
-   return snprintf(page, PAGE_SIZE, "%d\n",
-   flags & TRANSPORT_FLAG_PASSTHROUGH_PGR ? 0 : 1);
+   if (!da->da_dev->dev_attrib.emulate_pr ||
+   (flags & TRANSPORT_FLAG_PASSTHROUGH_PGR))
+   pgr_support = 0;
+
+   return snprintf(page, PAGE_SIZE, "%d\n", pgr_support);
 }
 
 CONFIGFS_ATTR(, emulate_model_alias);
@@ -1116,6 +1122,7 @@ CONFIGFS_ATTR(, emulate_tpu);
 CONFIGFS_ATTR(, emulate_tpws);
 CONFIGFS_ATTR(, emulate_caw);
 CONFIGFS_ATTR(, emulate_3pc);
+CONFIGFS_ATTR(, emulate_pr);
 CONFIGFS_ATTR(, pi_prot_type);
 CONFIGFS_ATTR_RO(, hw_pi_prot_type);
 CONFIGFS_ATTR(, pi_prot_format);
@@ -1156,6 +1163,7 @@ struct configfs_attribute *sbc_attrib_attrs[] = {
_emulate_tpws,
_emulate_caw,
_emulate_3pc,
+   _emulate_pr,
_pi_prot_type,
_hw_pi_prot_type,
_pi_prot_format,
@@ -1427,6 +1435,9 @@ static ssize_t target_pr_res_holder_show(struct 
config_item *item, char *page)
struct se_device *dev = pr_to_dev(item);
int ret;
 
+   if (!dev->dev_attrib.emulate_pr)
+   return sprintf(page, "SPC_RESERVATIONS_DISABLED\n");
+
if (dev->transport->transport_flags & TRANSPORT_FLAG_PASSTHROUGH_PGR)
return sprintf(page, "Passthrough\n");
 
@@ -1567,12 +1578,14 @@ static ssize_t target_pr_res_type_show(struct 
config_item *item, char *page)
 {
struct se_device *dev = pr_to_dev(item);
 
+   if (!dev->dev_attrib.emulate_pr)
+   return sprintf(page, "SPC_RESERVATIONS_DISABLED\n");
if (dev->transport->transport_flags & TRANSPORT_FLAG_PASSTHROUGH_PGR)
return sprintf(page, "SPC_PASSTHROUGH\n");
-   else if (dev->dev_reservation_flags & DRF_SPC2_RESERVATIONS)
+   if (dev->dev_reservation_flags & DRF_SPC2_RESERVATIONS)
return sprintf(page, "SPC2_RESERVATIONS\n");
-   else
-   return sprintf(page, "SPC3_PERSISTENT_RESERVATIONS\n");
+
+   return sprintf(page, "SPC3_PERSISTENT_RESERVATIONS\n");
 }
 
 static ssize_t target_pr_res_aptpl_active_show(struct config_item *item,
@@ -1580,7 +1593,8 @@ static ssize_t target_pr_res_aptpl_active_show(struct 
config_item *item,
 {
struct se_device *dev = pr_to_dev(item);
 
-   if (dev->transport->tran

Re: [RFC PATCH 0/3] target: remove some unused stats

[David Disseldorp]

On Wed, 17 Oct 2018 17:48:17 +0200, David Disseldorp wrote:

> This patchset removes a couple of unused error stat counters and a
> redundant cumulative counter.
> I've tagged this patchset RFC, as it may be considered a kernel<->user
> (configfs) API change.

Ping, any thoughts on this patchset?

Cheers, David

[RFC PATCH 2/3] target: remove unused session PDU-format-errors metric

[David Disseldorp]

Signed-off-by: David Disseldorp 
---
 drivers/target/iscsi/iscsi_target_stat.c | 14 +-
 include/target/iscsi/iscsi_target_stat.h |  2 --
 2 files changed, 1 insertion(+), 15 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target_stat.c 
b/drivers/target/iscsi/iscsi_target_stat.c
index ce60d123aa90..645c26f888cf 100644
--- a/drivers/target/iscsi/iscsi_target_stat.c
+++ b/drivers/target/iscsi/iscsi_target_stat.c
@@ -103,8 +103,7 @@ static ssize_t iscsi_stat_instance_fail_sess_show(struct 
config_item *item,
u32 sess_err_count;
 
spin_lock_bh(_err->lock);
-   sess_err_count = (sess_err->cxn_timeout_errors +
- sess_err->pdu_format_errors);
+   sess_err_count = sess_err->cxn_timeout_errors;
spin_unlock_bh(_err->lock);
 
return snprintf(page, PAGE_SIZE, "%u\n", sess_err_count);
@@ -217,23 +216,12 @@ static ssize_t iscsi_stat_sess_err_cxn_errors_show(struct 
config_item *item,
return snprintf(page, PAGE_SIZE, "%u\n", sess_err->cxn_timeout_errors);
 }
 
-static ssize_t iscsi_stat_sess_err_format_errors_show(struct config_item *item,
-   char *page)
-{
-   struct iscsi_tiqn *tiqn = iscsi_sess_err_tiqn(item);
-   struct iscsi_sess_err_stats *sess_err = >sess_err_stats;
-
-   return snprintf(page, PAGE_SIZE, "%u\n", sess_err->pdu_format_errors);
-}
-
 CONFIGFS_ATTR_RO(iscsi_stat_sess_err_, inst);
 CONFIGFS_ATTR_RO(iscsi_stat_sess_err_, cxn_errors);
-CONFIGFS_ATTR_RO(iscsi_stat_sess_err_, format_errors);
 
 static struct configfs_attribute *iscsi_stat_sess_err_attrs[] = {
_stat_sess_err_attr_inst,
_stat_sess_err_attr_cxn_errors,
-   _stat_sess_err_attr_format_errors,
NULL,
 };
 
diff --git a/include/target/iscsi/iscsi_target_stat.h 
b/include/target/iscsi/iscsi_target_stat.h
index b2d0f190483d..2e32b934a678 100644
--- a/include/target/iscsi/iscsi_target_stat.h
+++ b/include/target/iscsi/iscsi_target_stat.h
@@ -23,13 +23,11 @@ extern const struct config_item_type iscsi_stat_sess_cit;
 /* iSCSI session error types */
 #define ISCSI_SESS_ERR_UNKNOWN 0
 #define ISCSI_SESS_ERR_CXN_TIMEOUT 2
-#define ISCSI_SESS_ERR_PDU_FORMAT  3
 
 /* iSCSI session error stats */
 struct iscsi_sess_err_stats {
spinlock_t  lock;
u32 cxn_timeout_errors;
-   u32 pdu_format_errors;
u32 last_sess_failure_type;
charlast_sess_fail_rem_name[224];
 } cacheline_aligned;
-- 
2.13.7

[RFC PATCH 3/3] target: remove cumulative session errors metric

[David Disseldorp]

The cumulative session errors metric matches cxn_timeout_errors, so drop
the duplicate.

Signed-off-by: David Disseldorp 
---
 drivers/target/iscsi/iscsi_target_stat.c | 16 
 1 file changed, 16 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target_stat.c 
b/drivers/target/iscsi/iscsi_target_stat.c
index 645c26f888cf..94b6f5ad48a4 100644
--- a/drivers/target/iscsi/iscsi_target_stat.c
+++ b/drivers/target/iscsi/iscsi_target_stat.c
@@ -95,20 +95,6 @@ static ssize_t iscsi_stat_instance_sessions_show(struct 
config_item *item,
iscsi_instance_tiqn(item)->tiqn_nsessions);
 }
 
-static ssize_t iscsi_stat_instance_fail_sess_show(struct config_item *item,
-   char *page)
-{
-   struct iscsi_tiqn *tiqn = iscsi_instance_tiqn(item);
-   struct iscsi_sess_err_stats *sess_err = >sess_err_stats;
-   u32 sess_err_count;
-
-   spin_lock_bh(_err->lock);
-   sess_err_count = sess_err->cxn_timeout_errors;
-   spin_unlock_bh(_err->lock);
-
-   return snprintf(page, PAGE_SIZE, "%u\n", sess_err_count);
-}
-
 static ssize_t iscsi_stat_instance_fail_type_show(struct config_item *item,
char *page)
 {
@@ -160,7 +146,6 @@ CONFIGFS_ATTR_RO(iscsi_stat_instance_, max_ver);
 CONFIGFS_ATTR_RO(iscsi_stat_instance_, portals);
 CONFIGFS_ATTR_RO(iscsi_stat_instance_, nodes);
 CONFIGFS_ATTR_RO(iscsi_stat_instance_, sessions);
-CONFIGFS_ATTR_RO(iscsi_stat_instance_, fail_sess);
 CONFIGFS_ATTR_RO(iscsi_stat_instance_, fail_type);
 CONFIGFS_ATTR_RO(iscsi_stat_instance_, fail_rem_name);
 CONFIGFS_ATTR_RO(iscsi_stat_instance_, disc_time);
@@ -175,7 +160,6 @@ static struct configfs_attribute 
*iscsi_stat_instance_attrs[] = {
_stat_instance_attr_portals,
_stat_instance_attr_nodes,
_stat_instance_attr_sessions,
-   _stat_instance_attr_fail_sess,
_stat_instance_attr_fail_type,
_stat_instance_attr_fail_rem_name,
_stat_instance_attr_disc_time,
-- 
2.13.7

[RFC PATCH 1/3] target: remove unused session digest-errors metric

[David Disseldorp]

Signed-off-by: David Disseldorp 
---
 drivers/target/iscsi/iscsi_target_stat.c | 14 +-
 include/target/iscsi/iscsi_target_stat.h |  2 --
 2 files changed, 1 insertion(+), 15 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target_stat.c 
b/drivers/target/iscsi/iscsi_target_stat.c
index df0a39811dc2..ce60d123aa90 100644
--- a/drivers/target/iscsi/iscsi_target_stat.c
+++ b/drivers/target/iscsi/iscsi_target_stat.c
@@ -103,8 +103,7 @@ static ssize_t iscsi_stat_instance_fail_sess_show(struct 
config_item *item,
u32 sess_err_count;
 
spin_lock_bh(_err->lock);
-   sess_err_count = (sess_err->digest_errors +
- sess_err->cxn_timeout_errors +
+   sess_err_count = (sess_err->cxn_timeout_errors +
  sess_err->pdu_format_errors);
spin_unlock_bh(_err->lock);
 
@@ -209,15 +208,6 @@ static ssize_t iscsi_stat_sess_err_inst_show(struct 
config_item *item,
iscsi_sess_err_tiqn(item)->tiqn_index);
 }
 
-static ssize_t iscsi_stat_sess_err_digest_errors_show(struct config_item *item,
-   char *page)
-{
-   struct iscsi_tiqn *tiqn = iscsi_sess_err_tiqn(item);
-   struct iscsi_sess_err_stats *sess_err = >sess_err_stats;
-
-   return snprintf(page, PAGE_SIZE, "%u\n", sess_err->digest_errors);
-}
-
 static ssize_t iscsi_stat_sess_err_cxn_errors_show(struct config_item *item,
char *page)
 {
@@ -237,13 +227,11 @@ static ssize_t 
iscsi_stat_sess_err_format_errors_show(struct config_item *item,
 }
 
 CONFIGFS_ATTR_RO(iscsi_stat_sess_err_, inst);
-CONFIGFS_ATTR_RO(iscsi_stat_sess_err_, digest_errors);
 CONFIGFS_ATTR_RO(iscsi_stat_sess_err_, cxn_errors);
 CONFIGFS_ATTR_RO(iscsi_stat_sess_err_, format_errors);
 
 static struct configfs_attribute *iscsi_stat_sess_err_attrs[] = {
_stat_sess_err_attr_inst,
-   _stat_sess_err_attr_digest_errors,
_stat_sess_err_attr_cxn_errors,
_stat_sess_err_attr_format_errors,
NULL,
diff --git a/include/target/iscsi/iscsi_target_stat.h 
b/include/target/iscsi/iscsi_target_stat.h
index 4d75a2c426ca..b2d0f190483d 100644
--- a/include/target/iscsi/iscsi_target_stat.h
+++ b/include/target/iscsi/iscsi_target_stat.h
@@ -22,14 +22,12 @@ extern const struct config_item_type iscsi_stat_sess_cit;
 
 /* iSCSI session error types */
 #define ISCSI_SESS_ERR_UNKNOWN 0
-#define ISCSI_SESS_ERR_DIGEST  1
 #define ISCSI_SESS_ERR_CXN_TIMEOUT 2
 #define ISCSI_SESS_ERR_PDU_FORMAT  3
 
 /* iSCSI session error stats */
 struct iscsi_sess_err_stats {
spinlock_t  lock;
-   u32 digest_errors;
u32 cxn_timeout_errors;
u32 pdu_format_errors;
u32 last_sess_failure_type;
-- 
2.13.7

[RFC PATCH 0/3] target: remove some unused stats

[David Disseldorp]

This patchset removes a couple of unused error stat counters and a
redundant cumulative counter.
I've tagged this patchset RFC, as it may be considered a kernel<->user
(configfs) API change.

Cheers, David
---
 drivers/target/iscsi/iscsi_target_stat.c | 40 

 include/target/iscsi/iscsi_target_stat.h |  4 
 2 files changed, 44 deletions(-)

Re: [PATCH v3 4/5] target: split out helper for cxn timeout error stashing

[David Disseldorp]

On Tue, 16 Oct 2018 00:42:57 -0400, Martin K. Petersen wrote:

> > I guess I'll resend entire series in future to avoid false kbuild
> > reports.  
> 
> Yes, please. Patchwork can't handle individual patches getting updated
> either.

Okay, will do.

> Applied to 4.20/scsi-queue.

Thanks Martin.

Cheers, David

Re: [PATCH v3 4/5] target: split out helper for cxn timeout error stashing

[David Disseldorp]

On Sun, 14 Oct 2018 11:43:34 +0800, kbuild test robot wrote:

> Thank you for the patch! Yet something to improve:
> 
> [auto build test ERROR on target/master]
> [also build test ERROR on v4.19-rc7 next-20181012]
> [if your patch is applied to the wrong git tree, please drop us a note to 
> help improve the system]
...
>drivers/target/iscsi/iscsi_target_util.c: In function 
> 'iscsit_handle_nopin_response_timeout':
> >> drivers/target/iscsi/iscsi_target_util.c:902:36: error: 'sess' undeclared 
> >> (first use in this function)

This patch is a v3 respin of an individual patch within the series:
[PATCH v2 0/5] target: improve Data-Out and NOP timeout error reporting
The sess declaration is provided in [PATCH v2 3/5].

I guess I'll resend entire series in future to avoid false kbuild
reports.

Cheers, David

[PATCH v3] target: split out helper for cxn timeout error stashing

[David Disseldorp]

Replace existing nested code blocks with helper function calls.

Signed-off-by: David Disseldorp 
---
 drivers/target/iscsi/iscsi_target_erl0.c | 15 +
 drivers/target/iscsi/iscsi_target_util.c | 36 ++--
 drivers/target/iscsi/iscsi_target_util.h |  1 +
 3 files changed, 22 insertions(+), 30 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target_erl0.c 
b/drivers/target/iscsi/iscsi_target_erl0.c
index 718fe9a1b709..1193cf884a28 100644
--- a/drivers/target/iscsi/iscsi_target_erl0.c
+++ b/drivers/target/iscsi/iscsi_target_erl0.c
@@ -770,21 +770,8 @@ void iscsit_handle_time2retain_timeout(struct timer_list 
*t)
 
pr_err("Time2Retain timer expired for SID: %u, cleaning up"
" iSCSI session.\n", sess->sid);
-   {
-   struct iscsi_tiqn *tiqn = tpg->tpg_tiqn;
-
-   if (tiqn) {
-   spin_lock(>sess_err_stats.lock);
-   strcpy(tiqn->sess_err_stats.last_sess_fail_rem_name,
-   (void *)sess->sess_ops->InitiatorName);
-   tiqn->sess_err_stats.last_sess_failure_type =
-   ISCSI_SESS_ERR_CXN_TIMEOUT;
-   tiqn->sess_err_stats.cxn_timeout_errors++;
-   atomic_long_inc(>conn_timeout_errors);
-   spin_unlock(>sess_err_stats.lock);
-   }
-   }
 
+   iscsit_fill_cxn_timeout_err_stats(sess);
spin_unlock_bh(_tpg->session_lock);
iscsit_close_session(sess);
 }
diff --git a/drivers/target/iscsi/iscsi_target_util.c 
b/drivers/target/iscsi/iscsi_target_util.c
index 931c51f56435..1227872227dc 100644
--- a/drivers/target/iscsi/iscsi_target_util.c
+++ b/drivers/target/iscsi/iscsi_target_util.c
@@ -933,22 +933,7 @@ void iscsit_handle_nopin_response_timeout(struct 
timer_list *t)
conn->nopin_response_timer_flags &= ~ISCSI_TF_RUNNING;
spin_unlock_bh(>nopin_timer_lock);
 
-   {
-   struct iscsi_portal_group *tpg = conn->sess->tpg;
-   struct iscsi_tiqn *tiqn = tpg->tpg_tiqn;
-
-   if (tiqn) {
-   spin_lock_bh(>sess_err_stats.lock);
-   strcpy(tiqn->sess_err_stats.last_sess_fail_rem_name,
-   conn->sess->sess_ops->InitiatorName);
-   tiqn->sess_err_stats.last_sess_failure_type =
-   ISCSI_SESS_ERR_CXN_TIMEOUT;
-   tiqn->sess_err_stats.cxn_timeout_errors++;
-   atomic_long_inc(>sess->conn_timeout_errors);
-   spin_unlock_bh(>sess_err_stats.lock);
-   }
-   }
-
+   iscsit_fill_cxn_timeout_err_stats(sess);
iscsit_cause_connection_reinstatement(conn, 0);
iscsit_dec_conn_usage_count(conn);
 }
@@ -1407,3 +1392,22 @@ struct iscsi_tiqn *iscsit_snmp_get_tiqn(struct 
iscsi_conn *conn)
 
return tpg->tpg_tiqn;
 }
+
+void iscsit_fill_cxn_timeout_err_stats(struct iscsi_session *sess)
+{
+   struct iscsi_portal_group *tpg = sess->tpg;
+   struct iscsi_tiqn *tiqn = tpg->tpg_tiqn;
+
+   if (!tiqn)
+   return;
+
+   spin_lock_bh(>sess_err_stats.lock);
+   strlcpy(tiqn->sess_err_stats.last_sess_fail_rem_name,
+   sess->sess_ops->InitiatorName,
+   sizeof(tiqn->sess_err_stats.last_sess_fail_rem_name));
+   tiqn->sess_err_stats.last_sess_failure_type =
+   ISCSI_SESS_ERR_CXN_TIMEOUT;
+   tiqn->sess_err_stats.cxn_timeout_errors++;
+   atomic_long_inc(>conn_timeout_errors);
+   spin_unlock_bh(>sess_err_stats.lock);
+}
diff --git a/drivers/target/iscsi/iscsi_target_util.h 
b/drivers/target/iscsi/iscsi_target_util.h
index d66dfc212624..68e84803b0a1 100644
--- a/drivers/target/iscsi/iscsi_target_util.h
+++ b/drivers/target/iscsi/iscsi_target_util.h
@@ -67,5 +67,6 @@ extern int rx_data(struct iscsi_conn *, struct kvec *, int, 
int);
 extern int tx_data(struct iscsi_conn *, struct kvec *, int, int);
 extern void iscsit_collect_login_stats(struct iscsi_conn *, u8, u8);
 extern struct iscsi_tiqn *iscsit_snmp_get_tiqn(struct iscsi_conn *);
+extern void iscsit_fill_cxn_timeout_err_stats(struct iscsi_session *);
 
 #endif /*** ISCSI_TARGET_UTIL_H ***/
-- 
2.13.7

Re: [PATCH v2 4/5] target: split out helper for cxn timeout error stashing

[David Disseldorp]

On Fri, 12 Oct 2018 09:11:27 -0700, Bart Van Assche wrote:

> There have been too many problems with strcpy() and buffer overflows in the
> past. If the source and destination strings both have the same size, please
> add a BUILD_BUG_ON() statement that verifies that at compile time. If that
> not's the case, how about using strlcpy() to make it easy for anyone who
> reads the source code that no output buffer overflow will occur?

Both arrays are the same size (ISCSI_IQN_LEN). I'll change this over to
use strlcpy(), as I agree that it helps readability.

Cheers, David

[PATCH v2 0/5] target: improve Data-Out and NOP timeout error reporting

[David Disseldorp]

The following patchset converts existing Data-Out and NOP ping timeout
messages from pr_debug() to pr_error(), to reflect the seriousness of
unexpected connection termination events.

These events can be triggered using a couple of libiscsi client hacks:
https://github.com/ddiss/libiscsi/tree/hack-lio-trigger-dataout-timeout
https://github.com/ddiss/libiscsi/tree/hack-lio-trigger-nop-timeout

Cheers, David
---
Changes since v1
- split out and use a helper function for cxn timeout error stashing
- add I_T Nexus to error messages

 drivers/target/iscsi/iscsi_target_erl0.c | 15 +--
 drivers/target/iscsi/iscsi_target_erl1.c | 17 -
 drivers/target/iscsi/iscsi_target_stat.c |  4 ++--
 drivers/target/iscsi/iscsi_target_util.c | 43 
---
 drivers/target/iscsi/iscsi_target_util.h |  1 +
 include/target/iscsi/iscsi_target_core.h |  6 +++---
 include/target/iscsi/iscsi_target_stat.h |  4 ++--
 7 files changed, 45 insertions(+), 45 deletions(-)

[PATCH v2 5/5] target: stash sess_err_stats on Data-Out timeout

[David Disseldorp]

sess_err_stats are currently filled on NOP ping timeout, but not
Data-Out timeout. Stash details of Data-Out timeouts using a
ISCSI_SESS_ERR_CXN_TIMEOUT value for last_sess_failure_type.

Signed-off-by: David Disseldorp 
---
 drivers/target/iscsi/iscsi_target_erl1.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/target/iscsi/iscsi_target_erl1.c 
b/drivers/target/iscsi/iscsi_target_erl1.c
index 7f3a1a06696e..a211e8154f4c 100644
--- a/drivers/target/iscsi/iscsi_target_erl1.c
+++ b/drivers/target/iscsi/iscsi_target_erl1.c
@@ -1230,6 +1230,7 @@ void iscsit_handle_dataout_timeout(struct timer_list *t)
 
 failure:
spin_unlock_bh(>dataout_timeout_lock);
+   iscsit_fill_cxn_timeout_err_stats(sess);
iscsit_cause_connection_reinstatement(conn, 0);
iscsit_dec_conn_usage_count(conn);
 }
-- 
2.13.7

[PATCH v2 2/5] target: log Data-Out timeouts as errors

[David Disseldorp]

Data-Out timeouts resulting in connection outages should be logged as
errors. Include the I_T Nexus in the message to aid path identification.

Signed-off-by: David Disseldorp 
---
 drivers/target/iscsi/iscsi_target_erl1.c | 16 +++-
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target_erl1.c 
b/drivers/target/iscsi/iscsi_target_erl1.c
index 5efa42b939a1..7f3a1a06696e 100644
--- a/drivers/target/iscsi/iscsi_target_erl1.c
+++ b/drivers/target/iscsi/iscsi_target_erl1.c
@@ -1169,15 +1169,21 @@ void iscsit_handle_dataout_timeout(struct timer_list *t)
na = iscsit_tpg_get_node_attrib(sess);
 
if (!sess->sess_ops->ErrorRecoveryLevel) {
-   pr_debug("Unable to recover from DataOut timeout while"
-   " in ERL=0.\n");
+   pr_err("Unable to recover from DataOut timeout while"
+   " in ERL=0, closing iSCSI connection for I_T Nexus"
+   " %s,i,0x%6phN,%s,t,0x%02x\n",
+   sess->sess_ops->InitiatorName, sess->isid,
+   sess->tpg->tpg_tiqn->tiqn, (u32)sess->tpg->tpgt);
goto failure;
}
 
if (++cmd->dataout_timeout_retries == na->dataout_timeout_retries) {
-   pr_debug("Command ITT: 0x%08x exceeded max retries"
-   " for DataOUT timeout %u, closing iSCSI connection.\n",
-   cmd->init_task_tag, na->dataout_timeout_retries);
+   pr_err("Command ITT: 0x%08x exceeded max retries"
+   " for DataOUT timeout %u, closing iSCSI connection for"
+   " I_T Nexus %s,i,0x%6phN,%s,t,0x%02x\n",
+   cmd->init_task_tag, na->dataout_timeout_retries,
+   sess->sess_ops->InitiatorName, sess->isid,
+   sess->tpg->tpg_tiqn->tiqn, (u32)sess->tpg->tpgt);
goto failure;
}
 
-- 
2.13.7

[PATCH v2 4/5] target: split out helper for cxn timeout error stashing

[David Disseldorp]

Replace existing nested code blocks with helper function calls.

Signed-off-by: David Disseldorp 
---
 drivers/target/iscsi/iscsi_target_erl0.c | 15 +-
 drivers/target/iscsi/iscsi_target_util.c | 35 +---
 drivers/target/iscsi/iscsi_target_util.h |  1 +
 3 files changed, 21 insertions(+), 30 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target_erl0.c 
b/drivers/target/iscsi/iscsi_target_erl0.c
index 718fe9a1b709..1193cf884a28 100644
--- a/drivers/target/iscsi/iscsi_target_erl0.c
+++ b/drivers/target/iscsi/iscsi_target_erl0.c
@@ -770,21 +770,8 @@ void iscsit_handle_time2retain_timeout(struct timer_list 
*t)
 
pr_err("Time2Retain timer expired for SID: %u, cleaning up"
" iSCSI session.\n", sess->sid);
-   {
-   struct iscsi_tiqn *tiqn = tpg->tpg_tiqn;
-
-   if (tiqn) {
-   spin_lock(>sess_err_stats.lock);
-   strcpy(tiqn->sess_err_stats.last_sess_fail_rem_name,
-   (void *)sess->sess_ops->InitiatorName);
-   tiqn->sess_err_stats.last_sess_failure_type =
-   ISCSI_SESS_ERR_CXN_TIMEOUT;
-   tiqn->sess_err_stats.cxn_timeout_errors++;
-   atomic_long_inc(>conn_timeout_errors);
-   spin_unlock(>sess_err_stats.lock);
-   }
-   }
 
+   iscsit_fill_cxn_timeout_err_stats(sess);
spin_unlock_bh(_tpg->session_lock);
iscsit_close_session(sess);
 }
diff --git a/drivers/target/iscsi/iscsi_target_util.c 
b/drivers/target/iscsi/iscsi_target_util.c
index 931c51f56435..eacc9d5f78b7 100644
--- a/drivers/target/iscsi/iscsi_target_util.c
+++ b/drivers/target/iscsi/iscsi_target_util.c
@@ -933,22 +933,7 @@ void iscsit_handle_nopin_response_timeout(struct 
timer_list *t)
conn->nopin_response_timer_flags &= ~ISCSI_TF_RUNNING;
spin_unlock_bh(>nopin_timer_lock);
 
-   {
-   struct iscsi_portal_group *tpg = conn->sess->tpg;
-   struct iscsi_tiqn *tiqn = tpg->tpg_tiqn;
-
-   if (tiqn) {
-   spin_lock_bh(>sess_err_stats.lock);
-   strcpy(tiqn->sess_err_stats.last_sess_fail_rem_name,
-   conn->sess->sess_ops->InitiatorName);
-   tiqn->sess_err_stats.last_sess_failure_type =
-   ISCSI_SESS_ERR_CXN_TIMEOUT;
-   tiqn->sess_err_stats.cxn_timeout_errors++;
-   atomic_long_inc(>sess->conn_timeout_errors);
-   spin_unlock_bh(>sess_err_stats.lock);
-   }
-   }
-
+   iscsit_fill_cxn_timeout_err_stats(sess);
iscsit_cause_connection_reinstatement(conn, 0);
iscsit_dec_conn_usage_count(conn);
 }
@@ -1407,3 +1392,21 @@ struct iscsi_tiqn *iscsit_snmp_get_tiqn(struct 
iscsi_conn *conn)
 
return tpg->tpg_tiqn;
 }
+
+void iscsit_fill_cxn_timeout_err_stats(struct iscsi_session *sess)
+{
+   struct iscsi_portal_group *tpg = sess->tpg;
+   struct iscsi_tiqn *tiqn = tpg->tpg_tiqn;
+
+   if (!tiqn)
+   return;
+
+   spin_lock_bh(>sess_err_stats.lock);
+   strcpy(tiqn->sess_err_stats.last_sess_fail_rem_name,
+   sess->sess_ops->InitiatorName);
+   tiqn->sess_err_stats.last_sess_failure_type =
+   ISCSI_SESS_ERR_CXN_TIMEOUT;
+   tiqn->sess_err_stats.cxn_timeout_errors++;
+   atomic_long_inc(>conn_timeout_errors);
+   spin_unlock_bh(>sess_err_stats.lock);
+}
diff --git a/drivers/target/iscsi/iscsi_target_util.h 
b/drivers/target/iscsi/iscsi_target_util.h
index d66dfc212624..68e84803b0a1 100644
--- a/drivers/target/iscsi/iscsi_target_util.h
+++ b/drivers/target/iscsi/iscsi_target_util.h
@@ -67,5 +67,6 @@ extern int rx_data(struct iscsi_conn *, struct kvec *, int, 
int);
 extern int tx_data(struct iscsi_conn *, struct kvec *, int, int);
 extern void iscsit_collect_login_stats(struct iscsi_conn *, u8, u8);
 extern struct iscsi_tiqn *iscsit_snmp_get_tiqn(struct iscsi_conn *);
+extern void iscsit_fill_cxn_timeout_err_stats(struct iscsi_session *);
 
 #endif /*** ISCSI_TARGET_UTIL_H ***/
-- 
2.13.7

[PATCH v2 3/5] target: log NOP ping timeouts as errors

[David Disseldorp]

Events resulting in connection outages like this should be logged as
errors. Include the I_T Nexus in the message to aid path identification.

Signed-off-by: David Disseldorp 
---
 drivers/target/iscsi/iscsi_target_util.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target_util.c 
b/drivers/target/iscsi/iscsi_target_util.c
index 49be1e41290c..931c51f56435 100644
--- a/drivers/target/iscsi/iscsi_target_util.c
+++ b/drivers/target/iscsi/iscsi_target_util.c
@@ -915,6 +915,7 @@ static int iscsit_add_nopin(struct iscsi_conn *conn, int 
want_response)
 void iscsit_handle_nopin_response_timeout(struct timer_list *t)
 {
struct iscsi_conn *conn = from_timer(conn, t, nopin_response_timer);
+   struct iscsi_session *sess = conn->sess;
 
iscsit_inc_conn_usage_count(conn);
 
@@ -925,9 +926,10 @@ void iscsit_handle_nopin_response_timeout(struct 
timer_list *t)
return;
}
 
-   pr_debug("Did not receive response to NOPIN on CID: %hu on"
-   " SID: %u, failing connection.\n", conn->cid,
-   conn->sess->sid);
+   pr_err("Did not receive response to NOPIN on CID: %hu, failing"
+   " connection for I_T Nexus %s,i,0x%6phN,%s,t,0x%02x\n",
+   conn->cid, sess->sess_ops->InitiatorName, sess->isid,
+   sess->tpg->tpg_tiqn->tiqn, (u32)sess->tpg->tpgt);
conn->nopin_response_timer_flags &= ~ISCSI_TF_RUNNING;
spin_unlock_bh(>nopin_timer_lock);
 
-- 
2.13.7

[PATCH v2 1/5] target: use ISCSI_IQN_LEN in iscsi_target_stat

[David Disseldorp]

Move the ISCSI_IQN_LEN definition up, so that it can be used in more
places instead of a hardcoded value.

Signed-off-by: David Disseldorp 
---
 drivers/target/iscsi/iscsi_target_stat.c | 4 ++--
 include/target/iscsi/iscsi_target_core.h | 6 +++---
 include/target/iscsi/iscsi_target_stat.h | 4 ++--
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target_stat.c 
b/drivers/target/iscsi/iscsi_target_stat.c
index df0a39811dc2..bb98882bdaa7 100644
--- a/drivers/target/iscsi/iscsi_target_stat.c
+++ b/drivers/target/iscsi/iscsi_target_stat.c
@@ -328,10 +328,10 @@ static ssize_t 
iscsi_stat_tgt_attr_fail_intr_name_show(struct config_item *item,
 {
struct iscsi_tiqn *tiqn = iscsi_tgt_attr_tiqn(item);
struct iscsi_login_stats *lstat = >login_stats;
-   unsigned char buf[224];
+   unsigned char buf[ISCSI_IQN_LEN];
 
spin_lock(>lock);
-   snprintf(buf, 224, "%s", lstat->last_intr_fail_name[0] ?
+   snprintf(buf, ISCSI_IQN_LEN, "%s", lstat->last_intr_fail_name[0] ?
lstat->last_intr_fail_name : NONE);
spin_unlock(>lock);
 
diff --git a/include/target/iscsi/iscsi_target_core.h 
b/include/target/iscsi/iscsi_target_core.h
index f2e6abea8490..24c398f4a68f 100644
--- a/include/target/iscsi/iscsi_target_core.h
+++ b/include/target/iscsi/iscsi_target_core.h
@@ -25,6 +25,7 @@ struct sock;
 #define ISCSIT_TCP_BACKLOG 256
 #define ISCSI_RX_THREAD_NAME   "iscsi_trx"
 #define ISCSI_TX_THREAD_NAME   "iscsi_ttx"
+#define ISCSI_IQN_LEN  224
 
 /* struct iscsi_node_attrib sanity values */
 #define NA_DATAOUT_TIMEOUT 3
@@ -270,9 +271,9 @@ struct iscsi_conn_ops {
 };
 
 struct iscsi_sess_ops {
-   charInitiatorName[224];
+   charInitiatorName[ISCSI_IQN_LEN];
charInitiatorAlias[256];
-   charTargetName[224];
+   charTargetName[ISCSI_IQN_LEN];
charTargetAlias[256];
charTargetAddress[256];
u16 TargetPortalGroupTag;   /* [0..65535] */
@@ -855,7 +856,6 @@ struct iscsi_wwn_stat_grps {
 };
 
 struct iscsi_tiqn {
-#define ISCSI_IQN_LEN  224
unsigned char   tiqn[ISCSI_IQN_LEN];
enum tiqn_state_table   tiqn_state;
int tiqn_access_count;
diff --git a/include/target/iscsi/iscsi_target_stat.h 
b/include/target/iscsi/iscsi_target_stat.h
index 4d75a2c426ca..ff6a47209313 100644
--- a/include/target/iscsi/iscsi_target_stat.h
+++ b/include/target/iscsi/iscsi_target_stat.h
@@ -33,7 +33,7 @@ struct iscsi_sess_err_stats {
u32 cxn_timeout_errors;
u32 pdu_format_errors;
u32 last_sess_failure_type;
-   charlast_sess_fail_rem_name[224];
+   charlast_sess_fail_rem_name[ISCSI_IQN_LEN];
 } cacheline_aligned;
 
 /* iSCSI login failure types (sub oids) */
@@ -56,7 +56,7 @@ struct iscsi_login_stats {
u32 last_fail_type;
int last_intr_fail_ip_family;
struct sockaddr_storage last_intr_fail_sockaddr;
-   charlast_intr_fail_name[224];
+   charlast_intr_fail_name[ISCSI_IQN_LEN];
 } cacheline_aligned;
 
 /* iSCSI logout stats */
-- 
2.13.7

[PATCH v3] target: fix truncated PR-in ReadKeys response

[David Disseldorp]

SPC5r17 states that the contents of the ADDITIONAL LENGTH field are not
altered based on the allocation length, so always calculate and pack the
full key list length even if the list itself is truncated.

According to Maged:
  Yes it fixes the "Storage Spaces Persistent Reservation" test in the
  Windows 2016 Server Failover Cluster validation suites when having
  many connections that result in more than 8 registrations. I tested
  your patch on 4.17 with iblock.

This behaviour can be tested using the libiscsi PrinReadKeys.Truncate
test.

Cc: sta...@vger.kernel.org
Signed-off-by: David Disseldorp 
Reviewed-by: Mike Christie 
Tested-by: Maged Mokhtar 
Reviewed-by: Christoph Hellwig 
---
Changes since v2:
* drop unnecessary braces
* add Christoph's Reviewed-by

Changes since v1:
* CC stable
* mention Maged's Windows PR test fix comment in commit message
* add Reviewed-by and Tested-by tags

 drivers/target/target_core_pr.c | 15 ++-
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
index 01ac306131c1..10db5656fd5d 100644
--- a/drivers/target/target_core_pr.c
+++ b/drivers/target/target_core_pr.c
@@ -3727,11 +3727,16 @@ core_scsi3_pri_read_keys(struct se_cmd *cmd)
 * Check for overflow of 8byte PRI READ_KEYS payload and
 * next reservation key list descriptor.
 */
-   if ((add_len + 8) > (cmd->data_length - 8))
-   break;
-
-   put_unaligned_be64(pr_reg->pr_res_key, [off]);
-   off += 8;
+   if (off + 8 <= cmd->data_length) {
+   put_unaligned_be64(pr_reg->pr_res_key, [off]);
+   off += 8;
+   }
+   /*
+* SPC5r17: 6.16.2 READ KEYS service action
+* The ADDITIONAL LENGTH field indicates the number of bytes in
+* the Reservation key list. The contents of the ADDITIONAL
+* LENGTH field are not altered based on the allocation length
+*/
add_len += 8;
}
spin_unlock(>t10_pr.registration_lock);
-- 
2.13.7

Re: [PATCH v2] target: fix truncated PR-in ReadKeys response

[David Disseldorp]

cc'ing the SCSI list, as it doesn't look like anything is making it
through target-devel nowadays...
https://patchwork.kernel.org/patch/10448059/

Cheers, David

On Tue,  5 Jun 2018 12:00:25 +0200, David Disseldorp wrote:

> SPC5r17 states that the contents of the ADDITIONAL LENGTH field are not
> altered based on the allocation length, so always calculate and pack the
> full key list length even if the list itself is truncated.
> 
> According to Maged:
>   Yes it fixes the "Storage Spaces Persistent Reservation" test in the
>   Windows 2016 Server Failover Cluster validation suites when having
>   many connections that result in more than 8 registrations. I tested
>   your patch on 4.17 with iblock.
> 
> This behaviour can be tested using the libiscsi PrinReadKeys.Truncate
> test.
> 
> Cc: sta...@vger.kernel.org
> Signed-off-by: David Disseldorp 
> Reviewed-by: Mike Christie 
> Tested-by: Maged Mokhtar 
> ---
>  drivers/target/target_core_pr.c | 15 ++-
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> Changes since v1:
> * CC stable
> * mention Maged's Windows PR test fix comment in commit message
> * add Reviewed-by and Tested-by tags
> 
> diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
> index 01ac306131c1..2e865fdaa362 100644
> --- a/drivers/target/target_core_pr.c
> +++ b/drivers/target/target_core_pr.c
> @@ -3727,11 +3727,16 @@ core_scsi3_pri_read_keys(struct se_cmd *cmd)
>* Check for overflow of 8byte PRI READ_KEYS payload and
>* next reservation key list descriptor.
>*/
> - if ((add_len + 8) > (cmd->data_length - 8))
> - break;
> -
> - put_unaligned_be64(pr_reg->pr_res_key, [off]);
> - off += 8;
> + if ((off + 8) <= cmd->data_length) {
> + put_unaligned_be64(pr_reg->pr_res_key, [off]);
> + off += 8;
> + }
> + /*
> +  * SPC5r17: 6.16.2 READ KEYS service action
> +  * The ADDITIONAL LENGTH field indicates the number of bytes in
> +  * the Reservation key list. The contents of the ADDITIONAL
> +  * LENGTH field are not altered based on the allocation length
> +  */
>   add_len += 8;
>   }
>   spin_unlock(>t10_pr.registration_lock);

Re: [PATCH 01/33] TCMU PR: first commit to implement TCMU PR

[David Disseldorp]

On Mon, 18 Jun 2018 04:12:17 -0700, Christoph Hellwig wrote:

> On Sun, Jun 17, 2018 at 12:40:56PM +0800, Zhu Lingshan wrote:
> > Hello Mike and Christoph，
> > Thanks Mike's comment inspired me, if I understand this correctly, it is
> > suggested to implement this whole solution in kernel, avoid
> >  splitting PRG handling in both kernel and userspace, make it not that
> > complex. I can try to implement this by sending OSD requests
> > from kernel side, then we don't need tcmu-runner supporting is, no need to
> > use netlink to exchange information with userspace, pure kernel code, seems
> > much more simple. Do you think this may be better?  
> 
> Yes.  And SuSE actually ships such a backend (originally writen by
> Mike), although it would still need some work to get into shape.

FWIW, the kernel backend is also shipped by petasan. I'd be happy to
restart efforts with Mike to get this in shape for mainline. The current
target_core_rbd PR implementation is suboptimal in that it duplicates
quite a bit of protocol logic from target_core_pr.

Cheers, David

Re: [PATCH 1/1] target:separate tx/rx cmd_puds

[David Disseldorp]

On Thu, 5 Apr 2018 13:12:12 +0200, David Disseldorp wrote:

> > -CONFIGFS_ATTR_RO(target_stat_tgt_port_, in_cmds);
> > +CONFIGFS_ATTR_RO(target_stat_tgt_port_, tx_cmds);
> > +CONFIGFS_ATTR_RO(target_stat_tgt_port_, rx_cmds);  
> 
> I don't think the in_cmds metric should be deleted here. It could be
> calculated on the fly via tx_cmds + rx_cmds + nodata_cmds.

@Zhang Zhuoyu: How about something like the following?
https://git.samba.org/?p=ddiss/linux.git;a=commitdiff;h=73723ccf433424721830797d70cfb88d4596e0fc

...this keeps the in_cmds metric, and renames tx/rx_cmds read/write_cmds
respectively. read/write_cmds is still a bit ambiguous, as it refers to
the command data direction rather than SCSI READ/WRITE CDBs, but IMO
it's clearer, and more consistent with the read/write_mbytes metrics.

Cheers, David

Re: [PATCH 1/1] target:separate tx/rx cmd_puds

[David Disseldorp]

Hi,

The commit summary has a typo (cmd_puds). That said, this change
isn't iSCSI specific, so using "pdu" here doesn't make much sense IMO.

On Wed, 21 Mar 2018 17:52:43 +0800, Zhang Zhuoyu wrote:

> Separate tx/rx cmd_pdus in order to distinguish LUN read/write IOPS.
> 
> Signed-off-by: Zhang Zhuoyu 
> ---
>  drivers/target/target_core_stat.c  | 26 ++
>  drivers/target/target_core_transport.c | 11 ++-
>  include/target/target_core_base.h  |  3 ++-
>  3 files changed, 34 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/target/target_core_stat.c 
> b/drivers/target/target_core_stat.c
> index f0db91e..9099494 100644
> --- a/drivers/target/target_core_stat.c
> +++ b/drivers/target/target_core_stat.c
> @@ -706,7 +722,8 @@ static ssize_t 
> target_stat_tgt_port_hs_in_cmds_show(struct config_item *item,
>  CONFIGFS_ATTR_RO(target_stat_tgt_port_, indx);
>  CONFIGFS_ATTR_RO(target_stat_tgt_port_, name);
>  CONFIGFS_ATTR_RO(target_stat_tgt_port_, port_index);
> -CONFIGFS_ATTR_RO(target_stat_tgt_port_, in_cmds);
> +CONFIGFS_ATTR_RO(target_stat_tgt_port_, tx_cmds);
> +CONFIGFS_ATTR_RO(target_stat_tgt_port_, rx_cmds);

I don't think the in_cmds metric should be deleted here. It could be
calculated on the fly via tx_cmds + rx_cmds + nodata_cmds.

Cheers, David

Re: Persistent Reservation API

[David Disseldorp]

Hi Christoph,

On Tue,  4 Aug 2015 09:11:05 +0200, Christoph Hellwig wrote:

 This series adds support for a simplified persistent reservation API
 to the block layer.  The intent is that both in-kernel and userspace
 consumers can use the API instead of having to hand craft SCSI or NVMe
 command through the various pass through interfaces.  It also adds
 DM support as getting reservations through dm-multipath is a major
 pain with the current scheme.
 
 NVMe support currently isn't included as I don't have a multihost
 NVMe setup to test on, but if I can find a volunteer to test it I'm
 happy to write the code for it.
 
 The ioctl API is documented in Documentation/block/pr.txt, but to
 fully understand the concept you'll have to read up the SPC spec,
 PRs are too complicated that trying to rephrase them into different
 terminology is just going to create confusion.

Do you have any thoughts on where SCSI-2 RESERVE/RELEASE should fit into
this API, if at all? I.e. as a new enum pr_type members for
pr_reserve()/pr_release(), as separate pr_ops hooks, etc?
Similarly for PR_IN - IIUC, if LIO is to handle cluster wide PRs
for Ceph rbd via the block layer, these will all need to be supported
by block layer (and LIO backend) APIs.

Cheers, David
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Persistent Reservation API

[David Disseldorp]

On Tue, 11 Aug 2015 14:26:34 +0200, Christoph Hellwig wrote:

 if you want to add SCSI-2 RESERVE/RELEASE it would be at least
 different opcodes if not an entirely different API.  But what for?
 RESERVE/RELEASE are basially unsuable and that's why there are
 almost no users out there, and they can usually prefer SCSI-3
 semantics.

IIUC, recent versions of ESX can still fall-back to using SCSI-2
reservations[1].
With COMPARE AND WRITE supported by LIO, I expect SCSI-2 reservations
are no longer strictly needed, but they haven't been dropped yet.

 If you have a real users feel free to propose patches and I'm happy to
 review them.

Thanks, David

1. http://linux-iscsi.org/wiki/VStorage_APIs_for_Array_Integration
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html