Re: [PATCH] scsi: t10-pi: Return correct ref tag when queue has no integrity profile
Bart, > Had you considered to use lower_32_bits() instead of "0x"? > That would to avoid that reviewers have to count the 'f'-s to verify > correctness of t10_pi_ref_tag(). I hadn't. I guess I tend to think of lower_32_bits() as something you do to pointers, not to block numbers. -- Martin K. Petersen Oracle Linux Engineering
BUG in copy_page_to_iter() when iscsi sets ENABLE_CLUSTERING
I recently found what I believe is a bug, and I'd appreciate feedback
on if that is correct, and if so how to proceed.
BACKGROUND
Recently Christoph Hellwig sent an email to driver maintainers for
drivers that set ".use_clustering" to DISABLE_CLUSTERING in their SCSI
Host templates, asking if the setting could be changed to
ENABLE_CLUSTERING.
As part of answering that question, I set ENABLE_CLUSTERING in
drivers/scsi/iscsi_tcp.c and tested both the iscsi initiator and
target.
As a reminder, setting ENABLE_CLUSTERING means that adjacent bio-s can
be merged. This can make IO faster, but it means that drivers must be
able to deal with IOs that cross page boundaries, since bio merges can
create such IOs.
RESULTS
The iscsi initiator code can handle ENABLE_CLUSTERING just fine, but
the iscsi target code fails. It seems to assume that IOs do *NOT*
cross a page boundary.
The problem is in iscsi lib/iov_iter.c, in the functions
copy_page_to_iter() and page_copy_sane() (see below for how to
reproduce):
>> static inline bool page_copy_sane(struct page *page, size_t offset, size_t n)
>> {
>> struct page *head = compound_head(page);
>> size_t v = n + offset + page_address(page) - page_address(head);
>>
>> if (likely(n <= v && v <= (PAGE_SIZE << compound_order(head
>> return true;
>> WARN_ON(1);
>> return false;
>> }
>>
>> size_t copy_page_to_iter(struct page *page, size_t offset, size_t bytes,
>> struct iov_iter *i)
>> {
>> if (unlikely(!page_copy_sane(page, offset, bytes)))
>> return 0;
>> if (i->type & (ITER_BVEC|ITER_KVEC)) {
>> void *kaddr = kmap_atomic(page);
>> size_t wanted = copy_to_iter(kaddr + offset, bytes, i);
>> kunmap_atomic(kaddr);
>> return wanted;
>> } else if (unlikely(iov_iter_is_discard(i)))
>> return bytes;
>> else if (likely(!iov_iter_is_pipe(i)))
>> return copy_page_to_iter_iovec(page, offset, bytes, i);
>> else
>> return copy_page_to_iter_pipe(page, offset, bytes, i);
>> }
Causing the following WARN_ON stack trace (repeatedly):
>> ...
>> [ 78.644559] WARNING: CPU: 0 PID: 2192 at lib/iov_iter.c:830
>> copy_page_to_iter+0x1a6/0x2e0
>> [ 78.644561] Modules linked in: iscsi_tcp(E) libiscsi_tcp(E) libiscsi(E)
>> scsi_transport_iscsi(E) rfcomm(E) iscsi_target_mod(E) target_core_pscsi(E)
>> target_core_file(E) target_core_iblock(E) target_core_user(E) uio(E)
>> target_core_mod(E) configfs(E) af_packet(E) iscsi_ibft(E)
>> iscsi_boot_sysfs(E) vmw_vsock_vmci_transport(E) vsock(E) bnep(E) fuse(E)
>> crct10dif_pclmul(E) crc32_pclmul(E) ghash_clmulni_intel(E) xfs(E)
>> aesni_intel(E) snd_ens1371(E) aes_x86_64(E) snd_ac97_codec(E) crypto_simd(E)
>> cryptd(E) ac97_bus(E) glue_helper(E) snd_rawmidi(E) vmw_balloon(E)
>> snd_seq_device(E) snd_pcm(E) pcspkr(E) snd_timer(E) snd(E) uvcvideo(E)
>> btusb(E) videobuf2_vmalloc(E) btrtl(E) videobuf2_memops(E) videobuf2_v4l2(E)
>> btbcm(E) btintel(E) videodev(E) bluetooth(E) videobuf2_common(E) vmw_vmci(E)
>> ecdh_generic(E) rfkill(E) soundcore(E) mptctl(E) gameport(E) joydev(E)
>> i2c_piix4(E) e1000(E) ac(E) button(E) btrfs(E) libcrc32c(E) xor(E)
>> raid6_pq(E) hid_generic(E) usbhid(E) sr_mod(E) cdrom(E) ata_generic(E)
>> [ 78.644583] crc32c_intel(E) serio_raw(E) mptspi(E) scsi_transport_spi(E)
>> mptscsih(E) ata_piix(E) uhci_hcd(E) ehci_pci(E) ehci_hcd(E) vmwgfx(E)
>> drm_kms_helper(E) syscopyarea(E) sysfillrect(E) sysimgblt(E) fb_sys_fops(E)
>> usbcore(E) ttm(E) mptbase(E) drm(E) sg(E) dm_multipath(E) dm_mod(E)
>> scsi_dh_rdac(E) scsi_dh_emc(E) scsi_dh_alua(E)
>> [ 78.644593] CPU: 0 PID: 2192 Comm: iscsi_trx Tainted: GE
>> 4.20.0-rc4-1-default+ #1 openSUSE Tumbleweed (unreleased)
>> [ 78.644593] Hardware name: VMware, Inc. VMware Virtual Platform/440BX
>> Desktop Reference Platform, BIOS 6.00 05/19/2017
>> [ 78.644595] RIP: 0010:copy_page_to_iter+0x1a6/0x2e0
>> [ 78.644596] Code: 0c 24 48 98 49 89 ce 48 89 c2 49 29 c6 48 29 ca 4d 01
>> f4 48 01 d5 48 85 c0 0f 85 71 ff ff ff 48 85 ed 0f 84 68 ff ff ff eb b2 <0f>
>> 0b 45 31 ed eb 8d bf 01 00 00 00 e8 d9 1e c5 ff 65 4c 8b 34 25
>> [ 78.644596] RSP: 0018:a04b41eefbe0 EFLAGS: 00010206
>> [ 78.644597] RAX: f7acc40de180 RBX: a04b41eefd80 RCX:
>> 0028a014
>> [ 78.644598] RDX: 2000 RSI: 1000 RDI:
>> f7acc000
>> [ 78.644598] RBP: f7acc40de180 R08: 8f1703786000 R09:
>> 2000
>> [ 78.644599] R10: 02c0 R11: 8f16b3f31400 R12:
>>
>> [ 78.644600] R13: 2000 R14: 0008 R15:
>> 3800
>> [ 78.644601] FS: () GS:8f1739c0()
>> knlGS:
>> [ 78.644601] CS: 0010 DS: ES: CR0: 80050033
>> [ 78.644602] CR2:
Re: DISABLE_CLUSTERING in scsi drivers
On 11/21/18 1:41 AM, Christoph Hellwig wrote:
> Hi all,
>
> you in the To list maintain or wrote SCSI drivers that set the
> DISABLE_CLUSTERING flag, which basically disable merges of any
> bio segments. We already have the actual max_segment size limit
> to say which length a segment should have, independent of merged
> or originally created, so this limit generally should rarely if
> ever be required, and mostly is an old cut an paste error.
>
> Can you go over your drivers and check if it could be removed?
>
I have tested changing this to ENABLE_CLUSTERING in iscsi_tcp.c. This
code is used for both the iscsi initiator and the target.
On the initiator side, there is no problem with enabling clustering of bios.
But on the target side, the code seems to assume that IOs do not cross
page boundaries, resulting in WARN_ON messages and failed IO when
ENABLE_CLUSTERING is set:
> [ 78.644595] RIP: 0010:copy_page_to_iter+0x1a6/0x2e0
> [ 78.644596] Code: 0c 24 48 98 49 89 ce 48 89 c2 49 29 c6 48 29 ca 4d 01 f4
> 48 01 d5 48 85 c0 0f 85 71 ff ff ff 48 85 ed 0f 84 68 ff ff ff eb b2 <0f> 0b
> 45 31 ed eb 8d bf 01 00 00 00 e8 d9 1e c5 ff 65 4c 8b 34 25
> [ 78.644596] RSP: 0018:a04b41eefbe0 EFLAGS: 00010206
> [ 78.644597] RAX: f7acc40de180 RBX: a04b41eefd80 RCX:
> 0028a014
> [ 78.644598] RDX: 2000 RSI: 1000 RDI:
> f7acc000
> [ 78.644598] RBP: f7acc40de180 R08: 8f1703786000 R09:
> 2000
> [ 78.644599] R10: 02c0 R11: 8f16b3f31400 R12:
>
> [ 78.644600] R13: 2000 R14: 0008 R15:
> 3800
> [ 78.644601] FS: () GS:8f1739c0()
> knlGS:
> [ 78.644601] CS: 0010 DS: ES: CR0: 80050033
> [ 78.644602] CR2: 7f0f54772000 CR3: 00012e670004 CR4:
> 003606f0
> [ 78.644624] DR0: DR1: DR2:
>
> [ 78.644625] DR3: DR6: fffe0ff0 DR7:
> 0400
> [ 78.644625] Call Trace:
> [ 78.644631] skb_copy_datagram_iter+0x16f/0x270
> [ 78.644633] tcp_recvmsg+0x223/0xc30
> [ 78.644637] inet_recvmsg+0x4b/0xe0
> [ 78.644644] iscsit_do_rx_data.constprop.9+0x89/0x110 [iscsi_target_mod]
> [ 78.644650] rx_data+0x58/0x70 [iscsi_target_mod]
> [ 78.644654] iscsit_get_rx_pdu+0xa7b/0xd20 [iscsi_target_mod]
> [ 78.644657] ? preempt_count_sub+0x43/0x50
> [ 78.644659] ? _raw_spin_unlock_irq+0x22/0x40
> [ 78.644663] ? iscsi_target_tx_thread+0x1d0/0x1d0 [iscsi_target_mod]
> [ 78.644667] ? iscsi_target_rx_thread+0x71/0xd0 [iscsi_target_mod]
> [ 78.644670] iscsi_target_rx_thread+0x71/0xd0 [iscsi_target_mod]
> [ 78.644672] kthread+0x116/0x130
> [ 78.644673] ? kthread_create_worker_on_cpu+0x40/0x40
> [ 78.644675] ret_from_fork+0x24/0x50
> [ 78.644678] ---[ end trace a0d6d5ab0e8625ee ]---
>
The problem seems to be in iov_iter.c:copy_page_to_iter(), where it
first calls page_copy_sane(), which makes sure the copy does not cross a
page boundary:
> static inline bool page_copy_sane(struct page *page, size_t offset, size_t n)
> {
> struct page *head = compound_head(page);
> size_t v = n + offset + page_address(page) - page_address(head);
>
> if (likely(n <= v && v <= (PAGE_SIZE << compound_order(head
> return true;
> WARN_ON(1);
> return false;
> }
I will submit a separate BUG email about this to the linux-scsi and
target-devel mailing lists, since it's not clear to me how to fix this.
In the mean time, the iscsi_tcp.c file still needs DISABLE_CLUSTERING.
--
Lee
"Deviants will be sacrificed to ensure group solidarity."
Re: [PATCH] scsi: t10-pi: Return correct ref tag when queue has no integrity profile
On 12/5/18 6:04 AM, Martin K. Petersen wrote: Since the return value of this function is 'u32', can the ' & 0x' be left out? Absolutely, and I almost zapped it. However, I decided to leave it to emphasize the point that the reference tag is truncated to a 32-bit value. To me, this is more obvious than having to backtrack and spot the u32 in the function definition. I generally appreciate some sort of commentary around a return statement if the value deviates from the ordinary. The parentheses around the shift value irk me but had to leave those in place to silence gcc. Hi Martin, Had you considered to use lower_32_bits() instead of "0x"? That would to avoid that reviewers have to count the 'f'-s to verify correctness of t10_pi_ref_tag(). Thanks, Bart.
Re: [PATCH] scsi: t10-pi: Return correct ref tag when queue has no integrity profile
Hi Bart, > Since the return value of this function is 'u32', can the ' & > 0x' be left out? Absolutely, and I almost zapped it. However, I decided to leave it to emphasize the point that the reference tag is truncated to a 32-bit value. To me, this is more obvious than having to backtrack and spot the u32 in the function definition. I generally appreciate some sort of commentary around a return statement if the value deviates from the ordinary. The parentheses around the shift value irk me but had to leave those in place to silence gcc. -- Martin K. Petersen Oracle Linux Engineering
[PATCH v7 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
---
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()
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
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_device(struct se_device *dev)
* passthrough because this is being provided by the backend LLD.
*/
if (!(dev->transport->transport_flags & TRANSPORT_FLAG_PASSTHROUGH)) {
- strncpy(>t10_wwn.vendor[0], "LIO-ORG", 8);
-
[PATCH v7 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 v7 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 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(-)
