Project

General

Profile

Bug #10012

vioblk should not accept an all-zero serial number

Added by Joshua M. Clulow 9 months ago. Updated 3 days ago.

Status:
Closed
Priority:
Normal
Category:
driver - device drivers
Start date:
2018-11-28
Due date:
% Done:

100%

Estimated time:
Difficulty:
Medium
Tags:

Description

While I still believe blkdev should provide device ID fabrication and in-label persistence (as sd, xdf, cmdk, etc, do; see #9985) for drivers that are not able to come up with a useful devid themselves, it's also true that a devid isn't strictly required. Furthermore, having more than one device in the system with the same devid presents many challenges; e.g., diskinfo (using libdiskmgmt) will list at most one of the Virtio block devices in the system, because it has deduplicated them (in error) by devid.

Some basic tests demonstrate that we could at least detect obviously invalid serial numbers (say, all zeroes or all ones) and refuse to turn these into a devid. Doing so in a test environment allows diskinfo to list all of the virtual block devices, and ZFS still appears to work (the devid property is correctly omitted from the vdev configuration nvlist).

(Note, an all-zeroes serial number presently becomes devid id1,kdev@A~~~~~~~~~~~~~~~~~~~~)


Related issues

Related to illumos gate - Bug #9985: blkdev devices can have an invalid devidNew2018-11-17

Actions
Related to illumos gate - Bug #10623: ZFS should be more aggressive in updating vdev devidNew2019-03-31

Actions
Related to illumos gate - Bug #10005: Cannot Fix pool misconfiguration because other top level device is raidz.Closed2018-11-22

Actions
Related to illumos gate - Feature #11329: improved Virtio frameworkClosed

Actions

History

#1

Updated by Joshua M. Clulow 9 months ago

  • Related to Bug #9985: blkdev devices can have an invalid devid added
#2

Updated by Joshua M. Clulow 9 months ago

  • Description updated (diff)
#3

Updated by Joshua M. Clulow 6 months ago

  • Assignee set to Joshua M. Clulow
#4

Updated by Joshua M. Clulow 5 months ago

The current draft version of the Virtio specific version 1.1 is available at:

http://docs.oasis-open.org/virtio/virtio/v1.1/virtio-v1.1.html

As of the time of writing this comment, the specification rather distressingly still omits the VIRTIO_BLK_T_GET_ID command from its description of the the operation of the Virtio Block Device (Section 5.2).

There is evidence of an attempt in 2012 to get the command into the specification, though it's not clear why it still has not appeared. The text from that attempt (with a little cleaning up) says:

When VIRTIO_BLK_T_GET_ID is issued, the device identifier, up to 20 bytes, is written to the buffer. The identifier should be interpreted as an ASCII string. It is terminated with a zero byte, unless it is exactly 20 bytes long.

Given this string is to be interpreted as ASCII, our currently handling is not sufficient: we perform the operation to read the ID using the target char *, allowing the device to write whatever it wants. We then pass this to ddi_devid_init() with the fixed length of 20 bytes -- even if it would have been a shorter string!

It seems appropriate instead to read the value from the device, then examine each returned byte to ensure it is printable ASCII. The string should be terminated at the first zero byte, or after 20 bytes; nonprintable characters prior to the first zero should be replaced with something printable; e.g., ?. In the case where a serial of 20 zero bytes is returned, this will result in an empty string; an empty string cannot be used to create an ATA-style devid.

#5

Updated by Joshua M. Clulow 5 months ago

  • Related to Bug #10623: ZFS should be more aggressive in updating vdev devid added
#6

Updated by Till Wegmüller 12 days ago

  • Related to Bug #10005: Cannot Fix pool misconfiguration because other top level device is raidz. added
#7

Updated by Joshua M. Clulow 4 days ago

#8

Updated by Electric Monk 3 days ago

  • Status changed from New to Closed
  • % Done changed from 0 to 100

git commit f8296c60994fb27105f37ac6f75661e4a6bdbab7

commit  f8296c60994fb27105f37ac6f75661e4a6bdbab7
Author: Joshua M. Clulow <josh@sysmgr.org>
Date:   2019-08-16T17:37:47.000Z

    11329 improved Virtio framework
    10012 vioblk should not accept an all-zero serial number
    7366 vioif happily creates rx descriptors until it consumes all memory
    Reviewed by: Robert Mustacchi <rm@joyent.com>
    Approved by: Richard Lowe <richlowe@richlowe.net>

Also available in: Atom PDF