Project

General

Profile

Bug #9985

blkdev devices can have an invalid devid

Added by Joshua M. Clulow 8 months ago. Updated 5 months ago.

Status:
New
Priority:
Normal
Category:
kernel
Start date:
2018-11-17
Due date:
% Done:

0%

Estimated time:
Difficulty:
Medium
Tags:

Description

A hypervisor may expose Virtio emulated block devices to a guest. We have a driver for these devices, vioblk, which is based on the blkdev framework.

Unfortunately, the Virtio specification has (at least historically) not required the hypervisor to provide a unique serial number or identifier of any kind for each block device, and at least some common hypervisors elect not to do so. The way this is currently wired up, vioblk will, when there is no serial number, generate the same devid each time: id1,kdev@A~~~~~~~~~~~~~~~~~~~~. This confuses many other parts of the system (e.g., the disk management library) which expect a devid to uniquely identify the media behind a particular device.

Other disk devices (including ancient IDE disks, and the Xen emulated xdf device) will fabricate a devid if none is available naturally. This devid may be persisted to disk if the disk is correctly labelled with room for devid storage, which means the devid will persistently identify the media (even if the /devices path changes), as intended.

It would appear that we should put the same fabrication logic into blkdev, so that devices using that framework will correctly fabricate a devid if one does not naturally exist for the attached media.


Related issues

Related to illumos gate - Bug #10012: vioblk should not accept an all-zero serial numberNew2018-11-28

Actions

History

#1

Updated by Garrett D'Amore 8 months ago

If this is a problem, I think it isn't in blkdev.

blkdev is intended to support optional device IDs, by having the o_devid_init entry point be optional. If an instance (the underlying driver which in this case is vioblk) cannot provide a meaningful devid it is not supposed to have that entry point fleshed out.

Having said that, in vioblk's case it may be the case that sometimes it can provide one, but sometimes it can't. The way the blkdev code is written, if vioblk cannot determine the dev id to use, then it should return DDI_FAILURE for that entry point. This is not fatal to attaching the device, but it will cause blkdev to refrain from attempting to create a device ID.

Assuming that this is done properly, then creation of the device id on disk (in the label somewhere) is supposed to be done automatically by the cmlb framework. cmlb only does that if the disk is an EFI labeled drive, or carries the VTOC. It probably specifically fails to do that for "disks" (blkdev in this case) that do not have one of those two labels on them.

So, start first by looking at vioblk, not blkdev.

#2

Updated by Garrett D'Amore 8 months ago

Well, I might have to eat my words.

cmlb has a function for this called "cmlb_get_devid_block", but it winds up getting called by sd. So some work is needed there.

Having said all that, maybe vioblk could use a device id based on some fabrication of the virtual host and disk? I'm not sure. But we could use that function too.

#3

Updated by Garrett D'Amore 8 months ago

Btw, anything which assumes a block device will have a device id is fundamentally borked. Device IDs are only supposed to be required on devices that can be moved or are multipathing capable. This doesn't describe a virtual disk.

Code that has some assumption and fails to operate on a device missing a device id is misdesigned, unless somehow that code is only meant to deal with multipathed disks, or something like that. It may be better to actually fix the code that is breaking in the absence of these than to try to jimmy some kind of device ID on something that actually isn't a real physical device!

#4

Updated by Joshua M. Clulow 8 months ago

Yes, I did actually look at all of those things before filing the ticket. There is effectively identical device ID fabrication (and in-label persistence) logic in at least four disk drivers:

  • sd (which I believe is almost everything except NVMe and Virtio in practice)
  • xdf (the Xen equivalent of vioblk)
  • cmdk (legacy Intel IDE)
  • dad (legacy SPARC IDE?)

While it's not strictly required to have a device ID for every block device, I think uniformity here is probably not a bad thing when it doesn't really cost us anything. In particular, I would note that virtual disks can indeed be moved. Depending on the hypervisor, adding or removing, e.g., a NIC can change the PCI enumeration order -- and thus the /devices path -- of block devices in the system. That's part of what makes #7119 such an acute problem in guests.

The devid fabrication and persistence logic is somewhat unfortunately copy-pasted all over the place. There are a few macros that appear to be the beginning of trying to commonise this infrastructure, but I think we can do a lot better. I've been knocking together a rough prototype of a pair of functions with signatures like:

int ddi_devid_disk_encode(const ddi_devid_t *did, uint8_t *buf, size_t bufsz);
int ddi_devid_disk_decode(const uint8_t *buf, size_t bufsz, ddi_devid_t **did);

These would provide consistent marshalling and unmarshalling of the on-disk DDI format described today by direct use of struct dk_devid, and the DKD_GETCHKSUM() and DKD_FORMCHKSUM() macros -- without requiring everybody to reimplement the same boiler plate.

We'd then use this logic in blkdev for framework consumers that either don't provide an ID callback, or where the ID callback returns DDI_FAILURE. We'll also want to attempt subsequent generation of a devid after labelling (i.e., as part of a successful DKIOCSETEFI, DKIOCSVTOC, or DKIOCSEXTVTOC operation).

Note that this would only end up generating an in-label devid in cases where, as you note, the operator has labelled the disk with either a VTOC or an EFI label. If you use your blkdev device with the faked up whole-device label that gets provided as part of the CMLB_FAKE_LABEL_ONE_PARTITION flag to cmlb_attach() , then no devid will be generated or written to disk.

#5

Updated by Joshua M. Clulow 8 months ago

  • Related to Bug #10012: vioblk should not accept an all-zero serial number added
#6

Updated by Joshua M. Clulow 5 months ago

  • Assignee set to Joshua M. Clulow

Also available in: Atom PDF