Project

General

Profile

Bug #10012

vioblk should not accept an all-zero serial number

Added by Joshua Clulow 8 months ago. Updated 4 months ago.

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

0%

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

History

#1

Updated by Joshua Clulow 8 months ago

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

Updated by Joshua Clulow 8 months ago

  • Description updated (diff)
#3

Updated by Joshua Clulow 5 months ago

  • Assignee set to Joshua Clulow
#4

Updated by Joshua Clulow 4 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 Clulow 4 months ago

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

Also available in: Atom PDF