| Message ID | 20181119210636.22979-4-ddiss@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target: user configurable T10 Vendor ID | expand |
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", > + &to_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. > +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. > + strncpy(buf, page, sizeof(buf)); Isn't strncpy() deprecated? How about using strlcpy() instead? > + /* > + * 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'. > + > + /* > + * 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. Thanks, Bart.
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", > > + &to_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
On Tue, 2018-11-20 at 19:00 +0100, David Disseldorp wrote: > 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? Sure, I would welcome that change. Thanks, Bart.
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
On Wed, 2018-11-28 at 16:37 +0100, David Disseldorp wrote: > 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. 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. Thanks, Bart.
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
On Wed, 2018-11-28 at 17:28 +0100, David Disseldorp wrote: > 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. Hi David, 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? 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]; - /* scsiLuVendorId */ - for (i = 0; i < sizeof(dev->t10_wwn.vendor); 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, "%-16s\n", dev->t10_wwn.vendor); } [ ... ] static void scsi_dump_inquiry(struct se_device *dev) { struct t10_wwn *wwn = &dev->t10_wwn; - char buf[17]; - int i, device_type; + int device_type = dev->transport->get_device_type(dev); + /* * Print Linux/SCSI style INQUIRY formatting to the kernel ring buffer */ - for (i = 0; i < 8; i++) - if (wwn->vendor[i] >= 0x20) - buf[i] = wwn->vendor[i]; - else - buf[i] = ' '; - buf[i] = '\0'; - pr_debug(" Vendor: %s\n", buf); - - for (i = 0; i < 16; i++) - if (wwn->model[i] >= 0x20) - buf[i] = wwn->model[i]; - else - buf[i] = ' '; - buf[i] = '\0'; - pr_debug(" Model: %s\n", buf); - - for (i = 0; i < 4; i++) - if (wwn->revision[i] >= 0x20) - buf[i] = wwn->revision[i]; - else - buf[i] = ' '; - buf[i] = '\0'; - pr_debug(" Revision: %s\n", buf); - - device_type = dev->transport->get_device_type(dev); + pr_debug(" Vendor: %-8s\n", wwn->vendor); + pr_debug(" Model: %-16s\n", wwn->model); + pr_debug(" Revision: %-4s\n", wwn->revision); pr_debug(" Type: %s ", scsi_device_type(device_type)); } Thanks, Bart.
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
On Wed, 2018-11-28 at 17:44 +0100, David Disseldorp wrote: > 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? Sure, that sounds fine to me. Bart.
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", + &to_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[] = { + &target_wwn_attr_vendor_id, &target_wwn_attr_vpd_unit_serial, &target_wwn_attr_vpd_protocol_identifier, &target_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];
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 <ddiss@suse.de> --- drivers/target/target_core_configfs.c | 55 +++++++++++++++++++++++++++++++++++ include/target/target_core_base.h | 3 +- 2 files changed, 57 insertions(+), 1 deletion(-)