Re: [PATCH] scsi: t10-pi: Return correct ref tag when queue has no integrity profile
On 12/4/18 6:31 PM, Martin K. Petersen wrote:
Commit ddd0bc756983 ("block: move ref_tag calculation func to the block
layer") moved ref tag calculation from SCSI to a library function. However,
this change broke returning the correct ref tag for devices operating in
DIF mode since these do not have an associated block integrity profile.
This in turn caused read/write failures on PI-formatted disks attached to
an mpt3sas controller.
Fixes: ddd0bc756983 ("block: move ref_tag calculation func to the block layer")
Cc: sta...@vger.kernel.org # 4.19+
Reported-by: John Garry
Signed-off-by: Martin K. Petersen
---
include/linux/t10-pi.h | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/include/linux/t10-pi.h b/include/linux/t10-pi.h
index b9626aa7e90c..3e2a80cc7b56 100644
--- a/include/linux/t10-pi.h
+++ b/include/linux/t10-pi.h
@@ -39,12 +39,13 @@ struct t10_pi_tuple {
static inline u32 t10_pi_ref_tag(struct request *rq)
{
+ unsigned int shift = ilog2(queue_logical_block_size(rq->q));
+
#ifdef CONFIG_BLK_DEV_INTEGRITY
- return blk_rq_pos(rq) >>
- (rq->q->integrity.interval_exp - 9) & 0x;
-#else
- return -1U;
+ if (rq->q->integrity.interval_exp)
+ shift = rq->q->integrity.interval_exp;
#endif
+ return blk_rq_pos(rq) >> (shift - SECTOR_SHIFT) & 0x;
}
Since the return value of this function is 'u32', can the ' &
0x' be left out?
Thanks,
Bart.
98IvH6E5wF5vcK8DK6
238.144.170.101106.119.122.156 ¿ª ¸÷ Àà ÔöÖµ Ë°Õý ¹æ Õæ ƱQQ:2211261333 ÁÖ ³Ì£º13632225663(΢ÐÅͬºÅ)
[PATCH] scsi: t10-pi: Return correct ref tag when queue has no integrity profile
Commit ddd0bc756983 ("block: move ref_tag calculation func to the block
layer") moved ref tag calculation from SCSI to a library function. However,
this change broke returning the correct ref tag for devices operating in
DIF mode since these do not have an associated block integrity profile.
This in turn caused read/write failures on PI-formatted disks attached to
an mpt3sas controller.
Fixes: ddd0bc756983 ("block: move ref_tag calculation func to the block layer")
Cc: sta...@vger.kernel.org # 4.19+
Reported-by: John Garry
Signed-off-by: Martin K. Petersen
---
include/linux/t10-pi.h | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/include/linux/t10-pi.h b/include/linux/t10-pi.h
index b9626aa7e90c..3e2a80cc7b56 100644
--- a/include/linux/t10-pi.h
+++ b/include/linux/t10-pi.h
@@ -39,12 +39,13 @@ struct t10_pi_tuple {
static inline u32 t10_pi_ref_tag(struct request *rq)
{
+ unsigned int shift = ilog2(queue_logical_block_size(rq->q));
+
#ifdef CONFIG_BLK_DEV_INTEGRITY
- return blk_rq_pos(rq) >>
- (rq->q->integrity.interval_exp - 9) & 0x;
-#else
- return -1U;
+ if (rq->q->integrity.interval_exp)
+ shift = rq->q->integrity.interval_exp;
#endif
+ return blk_rq_pos(rq) >> (shift - SECTOR_SHIFT) & 0x;
}
extern const struct blk_integrity_profile t10_pi_type1_crc;
--
2.19.2
UBSAN: Undefined behaviour in drivers/scsi/megaraid/megaraid_sas_fp.c:117:32
UBSAN: Undefined behaviour in drivers/scsi/megaraid/megaraid_sas_fp.c:117:32
index 255 is out of range for type 'MR_LD_SPAN_MAP [1]'
This commit 51087a8617fe (megaraid_sas : Extended VD support) defined those,
struct MR_FW_RAID_MAP {
u8 ldTgtIdToLd[MAX_RAIDMAP_LOGICAL_DRIVES+\
MAX_RAIDMAP_VIEWS];
struct MR_LD_SPAN_MAP ldSpanMap[1];
struct MR_FW_RAID_MAP_ALL {
struct MR_FW_RAID_MAP raidMap;
struct MR_LD_RAID *MR_LdRaidGet(u32 ld, struct MR_DRV_RAID_MAP_ALL *map)
return >raidMap.ldSpanMap[ld].ldRaid;
Then, there are several paths could trigger that undefined behavior due to
out-of-bound access.
mr_update_load_balance_params
for (ldCount = 0; ldCount < MAX_LOGICAL_DRIVES_EXT;\
ldCount++;
ld = MR_TargetIdToLdGet(ldCount, drv_map);
raid = MR_LdRaidGet(ld, drv_map)
megasas_build_io_fusion
megasas_build_ld_nonrw_fusion
ld = MR_TargetIdToLdGet(device_id, local_map_ptr);
raid = MR_LdRaidGet(ld, local_map_ptr);
Any clue?
Re: [PATCH v6 3/5] target: add device vendor_id configfs attribute
On Tue, 2018-12-04 at 16:26 +0300, Roman Bolshakov wrote: > wrt PATCH 5 in the series. Should we allow to set vendor_id for for > pscsi? I think we should allow that. Bart.
Re: [PATCH v6 3/5] target: add device vendor_id configfs attribute
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
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
Re: [PATCH v6 3/5] target: add device vendor_id configfs attribute
On Tue, Dec 04, 2018 at 03:13:51PM +0300, Roman Bolshakov wrote: > On Tue, Dec 04, 2018 at 11:12:36AM +0100, David Disseldorp wrote: > > The vendor_id attribute will allow for the modification of the T10 > > Vendor Identification string returned in inquiry responses. Its value > > can be viewed and modified via the ConfigFS path at: > > target/core/$backstore/$name/wwn/vendor_id > > > > "LIO-ORG" remains the default value, which is set when the backstore > > device is enabled. > > > > Signed-off-by: David Disseldorp > > Reviewed-by: Bryant G. Ly > > Reviewed-by: Lee Duncan > > Reviewed-by: Hannes Reinecke > > --- > > drivers/target/target_core_configfs.c | 48 > > +++ > > 1 file changed, 48 insertions(+) > > > > diff --git a/drivers/target/target_core_configfs.c > > b/drivers/target/target_core_configfs.c > > index 34872f24e8bf..67303c3f9cb4 100644 > > --- a/drivers/target/target_core_configfs.c > > +++ b/drivers/target/target_core_configfs.c > > + > > + /* Assume ASCII encoding. Strip any newline added from userspace. */ > > + BUILD_BUG_ON(sizeof(dev->t10_wwn.vendor) != INQUIRY_VENDOR_LEN + 1); > > + strlcpy(dev->t10_wwn.vendor, strstrip(buf), > > + sizeof(dev->t10_wwn.vendor)); > > + > > Should we allow non-ASCII characters? It's okay to strip final newline > for convenience. A simple loop would ensure the rest is conformant to > SPC. EINVAL can be returned otherwise. > > And for fuzzy purposes there could be a special backstore that does all > sorts of nasty things. > > According to SPC 4.3.1: > ASCII data fields shall contain only ASCII printable characters (i.e., > code values 20h to 7Eh) and may be terminated with one or more ASCII > null (00h) characters. > > 3.3.10 shall > keyword indicating a mandatory requirement > Note 1 to entry: Designers are required to implement all such > interoperability with other products that conform to this standard. > > Thank you, > Roman wrt PATCH 5 in the series. Should we allow to set vendor_id for for pscsi? target_transport_configure sets t10_wwn fields for pscsi, but but an attempt to set vendor_id will overwrite the value recieved from scsi_device. And (please correct me if I'm wrong) it's not used anywhere except statistics config_group. That means in the actual INQUIRY commands it will still return vendor_id of the underlying storage. If that's that's true, this means an attempt to set vendor_id will be succesful but it won't do what's intended for. Perhaps -ENOSYS or an error code of your preference could be returned if device has TRANSPORT_FLAG_PASSTHROUGH. Roman
Re: [PATCH v6 5/5] target: perform t10_wwn ID initialisation in target_alloc_device()
On Tue, Dec 04, 2018 at 11:12:38AM +0100, David Disseldorp wrote: > Initialise the t10_wwn vendor, model and revision defaults when a > device is allocated instead of when it's enabled. This ensures that > custom vendor or model strings set prior to enablement are not later > overwritten with default values. > > The TRANSPORT_FLAG_PASSTHROUGH conditional can be dropped for the > following reasons: > - target_core_pscsi overwrites the defaults in the > pscsi_configure_device() callback. > + the contents is then only used for ConfigFS via > $pscsi_dev/info, $pscsi_dev/statistics/scsi_lu/vend, etc. > - target_core_user doesn't touch the defaults, nor are they used for > anything outside of ConfigFS. > > Signed-off-by: David Disseldorp > --- > drivers/target/target_core_device.c | 27 ++- > 1 file changed, 10 insertions(+), 17 deletions(-) > Reviewed-by: Roman Bolshakov Thanks, Roman
Re: [PATCH v6 4/5] target: remove hardcoded T10 Vendor ID in INQUIRY response
On Tue, Dec 04, 2018 at 11:12:37AM +0100, David Disseldorp wrote: > Use the value stored in t10_wwn.vendor, which defaults to "LIO-ORG", but > can be reconfigured via the vendor_id ConfigFS attribute. > > Signed-off-by: David Disseldorp > Reviewed-by: Bryant G. Ly > Reviewed-by: Lee Duncan > Reviewed-by: Hannes Reinecke > --- > drivers/target/target_core_spc.c | 8 +--- > 1 file changed, 5 insertions(+), 3 deletions(-) > Reviewed-by: Roman Bolshakov Thank you, Roman
Re: [PATCH v6 3/5] target: add device vendor_id configfs attribute
On Tue, Dec 04, 2018 at 11:12:36AM +0100, David Disseldorp wrote: > The vendor_id attribute will allow for the modification of the T10 > Vendor Identification string returned in inquiry responses. Its value > can be viewed and modified via the ConfigFS path at: > target/core/$backstore/$name/wwn/vendor_id > > "LIO-ORG" remains the default value, which is set when the backstore > device is enabled. > > Signed-off-by: David Disseldorp > Reviewed-by: Bryant G. Ly > Reviewed-by: Lee Duncan > Reviewed-by: Hannes Reinecke > --- > drivers/target/target_core_configfs.c | 48 > +++ > 1 file changed, 48 insertions(+) > > diff --git a/drivers/target/target_core_configfs.c > b/drivers/target/target_core_configfs.c > index 34872f24e8bf..67303c3f9cb4 100644 > --- a/drivers/target/target_core_configfs.c > +++ b/drivers/target/target_core_configfs.c > + > + /* Assume ASCII encoding. Strip any newline added from userspace. */ > + BUILD_BUG_ON(sizeof(dev->t10_wwn.vendor) != INQUIRY_VENDOR_LEN + 1); > + strlcpy(dev->t10_wwn.vendor, strstrip(buf), > + sizeof(dev->t10_wwn.vendor)); > + Should we allow non-ASCII characters? It's okay to strip final newline for convenience. A simple loop would ensure the rest is conformant to SPC. EINVAL can be returned otherwise. And for fuzzy purposes there could be a special backstore that does all sorts of nasty things. According to SPC 4.3.1: ASCII data fields shall contain only ASCII printable characters (i.e., code values 20h to 7Eh) and may be terminated with one or more ASCII null (00h) characters. 3.3.10 shall keyword indicating a mandatory requirement Note 1 to entry: Designers are required to implement all such interoperability with other products that conform to this standard. Thank you, Roman
Re: [PATCH v5 2/5] target: consistently null-terminate t10_wwn strings
On Sat, Dec 01, 2018 at 12:34:20AM +0100, David Disseldorp wrote:
> In preparation for supporting user provided vendor strings, add an extra
> byte to the vendor, model and revision arrays in struct t10_wwn. This
> ensures that the full INQUIRY data can be carried in the arrays along
> with a null-terminator.
>
> Change a number of array readers and writers so that they account for
> explicit null-termination:
> - The pscsi_set_inquiry_info() and emulate_model_alias_store() codepaths
> don't currently explicitly null-terminate; fix this.
> - Existing t10_wwn field dumps use for-loops which step over
> null-terminators for right-padding.
> + Use printf with width specifiers instead.
>
> Signed-off-by: David Disseldorp
> ---
> drivers/target/target_core_configfs.c | 14 +++---
> drivers/target/target_core_device.c | 49
> ---
> drivers/target/target_core_pscsi.c| 18 -
> drivers/target/target_core_spc.c | 7 ++---
> drivers/target/target_core_stat.c | 32 +--
> include/target/target_core_base.h | 14 +++---
> 6 files changed, 61 insertions(+), 73 deletions(-)
>
> diff --git a/drivers/target/target_core_configfs.c
> b/drivers/target/target_core_configfs.c
> index f6b1549f4142..34872f24e8bf 100644
> --- a/drivers/target/target_core_configfs.c
> +++ b/drivers/target/target_core_configfs.c
> @@ -613,12 +613,17 @@ static void dev_set_t10_wwn_model_alias(struct
> se_device *dev)
> const char *configname;
>
> configname = config_item_name(>dev_group.cg_item);
> - if (strlen(configname) >= 16) {
> + if (strlen(configname) >= INQUIRY_MODEL_LEN) {
> pr_warn("dev[%p]: Backstore name '%s' is too long for "
> "INQUIRY_MODEL, truncating to 16 bytes\n", dev,
The warning (which I understand predates your patch) is misleading, it
should mention truncation to 15 instead of 16 bytes and your comment
just below explains this.
> configname);
> }
> - snprintf(>t10_wwn.model[0], 16, "%s", configname);
> + /*
> + * XXX We can't use sizeof(dev->t10_wwn.model) (INQUIRY_MODEL_LEN + 1)
> + * here without potentially breaking existing setups, so continue to
> + * truncate one byte shorter than what can be carried in INQUIRY.
> + */
> + strlcpy(dev->t10_wwn.model, configname, INQUIRY_MODEL_LEN);
> }
>
> diff --git a/drivers/target/target_core_device.c
> b/drivers/target/target_core_device.c
> index 47b5ef153135..5512871f50e4 100644
> --- a/drivers/target/target_core_device.c
> +++ b/drivers/target/target_core_device.c
> @@ -1008,12 +989,16 @@ int target_configure_device(struct se_device *dev)
>* anything virtual (IBLOCK, FILEIO, RAMDISK), but not for TCM/pSCSI
>* passthrough because this is being provided by the backend LLD.
>*/
> + BUILD_BUG_ON(sizeof(dev->t10_wwn.vendor) != INQUIRY_VENDOR_LEN + 1);
> + BUILD_BUG_ON(sizeof(dev->t10_wwn.model) != INQUIRY_MODEL_LEN + 1);
> + BUILD_BUG_ON(sizeof(dev->t10_wwn.revision) != INQUIRY_REVISION_LEN + 1);
I'm sorry I'm missing something. Why BUILD_BUG_ON is added in many
places?
> diff --git a/drivers/target/target_core_pscsi.c
> b/drivers/target/target_core_pscsi.c
> index 47d76c862014..1002829f2038 100644
> --- a/drivers/target/target_core_pscsi.c
> +++ b/drivers/target/target_core_pscsi.c
> @@ -190,9 +190,15 @@ pscsi_set_inquiry_info(struct scsi_device *sdev, struct
> t10_wwn *wwn)
> /*
>* Use sdev->inquiry from drivers/scsi/scsi_scan.c:scsi_alloc_sdev()
>*/
> - memcpy(>vendor[0], [8], sizeof(wwn->vendor));
> - memcpy(>model[0], [16], sizeof(wwn->model));
> - memcpy(>revision[0], [32], sizeof(wwn->revision));
> + BUILD_BUG_ON(sizeof(wwn->vendor) != INQUIRY_VENDOR_LEN + 1);
> + snprintf(wwn->vendor, sizeof(wwn->vendor),
> + "%." __stringify(INQUIRY_VENDOR_LEN) "s", [8]);
> + BUILD_BUG_ON(sizeof(wwn->model) != INQUIRY_MODEL_LEN + 1);
> + snprintf(wwn->model, sizeof(wwn->model),
> + "%." __stringify(INQUIRY_MODEL_LEN) "s", [16]);
> + BUILD_BUG_ON(sizeof(wwn->revision) != INQUIRY_REVISION_LEN + 1);
> + snprintf(wwn->revision, sizeof(wwn->revision),
> + "%." __stringify(INQUIRY_REVISION_LEN) "s", [32]);
> }
>
The parts of the sdev->inquiry have been already right-padded with
spaces by scsi_sanitize_inquiry_string in scsi_probe_lun. Thus, it's
enough to replace sizeof with the new length definitions. Also, it's
possible to use sdev->model,vendor,rev pointers like in
pscsi_show_configfs_dev_params instead of explicit offsets [8],
[16], [32].
> static int
> @@ -826,21 +832,21 @@ static ssize_t pscsi_show_configfs_dev_params(struct
> se_device *dev, char *b)
> if (sd) {
> bl += sprintf(b + bl, "");
> bl += sprintf(b + bl, "Vendor: ");
> - for (i = 0; i < 8; i++) {
> +
[PATCH v6 2/5] target: consistently null-terminate t10_wwn strings
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, 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) !=
[PATCH v6 3/5] target: add device vendor_id configfs attribute
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
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
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()
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
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 1/5] target: use consistent left-aligned ASCII INQUIRY data
On Sat, Dec 01, 2018 at 12:34:19AM +0100, David Disseldorp wrote: > spc5r17.pdf specifies: > 4.3.1 ASCII data field requirements > ASCII data fields shall contain only ASCII printable characters (i.e., > code values 20h to 7Eh) and may be terminated with one or more ASCII > null (00h) characters. > ASCII data fields described as being left-aligned shall have any > unused bytes at the end of the field (i.e., highest offset) and the > unused bytes shall be filled with ASCII space characters (20h). > > LIO currently space-pads the T10 VENDOR IDENTIFICATION and PRODUCT > IDENTIFICATION fields in the standard INQUIRY data. However, the > PRODUCT REVISION LEVEL field in the standard INQUIRY data as well as the > T10 VENDOR IDENTIFICATION field in the INQUIRY Device Identification VPD > Page are zero-terminated/zero-padded. > > Fix this inconsistency by using space-padding for all of the above > fields. > > Signed-off-by: David Disseldorp > Reviewed-by: Christoph Hellwig > Reviewed-by: Bryant G. Ly > Reviewed-by: Lee Duncan > --- > drivers/target/target_core_spc.c | 17 - > 1 file changed, 12 insertions(+), 5 deletions(-) > Reviewed-by: Roman Bolshakov Best regards, Roman
