VirtIO SCSI: validate seg_max from hypervisor

[vangyzen]

Summary

This is the seatbelt mentioned in D20529.

Until r349278, bhyve presented a seg_max to the guest that was too large.
Detect this case and clamp it to the virtqueue size. Otherwise, we would
fail the "too many segments to enqueue" assertion in virtqueue_enqueue().

I hit this by running a guest with a MAXPHYS of 256 KB.

Test Plan

OneFS now boots instead of failing the assertion and panicking.

virtio_pci0: <VirtIO PCI SCSI adapter> port 0x2000-0x203f mem 0xc0000000-0xc0001fff irq 16 at device 4.0 on pci0
vtscsi0: <VirtIO SCSI Adapter> on virtio_pci0
vtscsi0: clamping seg_max (66 64)

Vanilla FreeBSD head seems unaffected.

[cem]

sys/dev/virtio/pci/virtio_pci.c

486–487 ↗

(On Diff #58810)

Is there any non-stateful way to do this (ditto virtio_mmio)?

sys/dev/virtio/scsi/virtio_scsi.c

454–455 ↗

(On Diff #58810)

does s == 0 really mean unlimited? Or no-such-queue?

[vangyzen]

• add declaration of vtmmio_virtqueue_size()

[vangyzen]

sys/dev/virtio/pci/virtio_pci.c

486–487 ↗

(On Diff #58810)

I wish. I don't know of one. According to my understanding of the spec, a read from the QUEUE_NUM register refers to the queue selected by the QUEUE_SEL register. (Ditto for mmio.)

See section 4.1.4.3 of http://docs.oasis-open.org/virtio/virtio/v1.0/virtio-v1.0.html

sys/dev/virtio/scsi/virtio_scsi.c

454–455 ↗

(On Diff #58810)

"A 0 means the queue is unavailable." The spec is not clear on whether the device specifies this to the driver or vice versa. I'm playing it safe.

[bryanv]

I don't think this is the right approach. Don't add a new virtqueue_size bus interface. Use virtqueue_size() after the allocate VQ call. VirtIO V1 supports VQ resizing that I would like to support later so for cases like this we'd need additional checking after the allocate anyways.

Ultimately this is a device bug. I'm rather uneasy trying to make something broken work instead of failing the attach.

sys/dev/virtio/pci/virtio_pci.c

486–487 ↗

(On Diff #58810)

No. The right section for this version of VirtIO is 4.1.4.8.

sys/dev/virtio/scsi/virtio_scsi.c

478 ↗

(On Diff #58812)

This is potentially artificially limiting when indirect descriptors are available.

[bryanv]

sys/dev/virtio/pci/virtio_pci.c

486–487 ↗

(On Diff #58810)

Oh looks like I forgot to save the draft after editing this comment:

The select is stateful but should only happen during single-threaded attach/detach. However, the notify is stateless.

[vangyzen]

• Simply detect the case and fail to attach, as @bryanv suggests.

[vangyzen]

Mark: Sorry, I @-mentioned you by mistake.

Bryan: Thanks for your very helpful feedback. This is the first time I've looked at virtio. I think I've now implemented your suggestion.

[bryanv]

sys/dev/virtio/scsi/virtio_scsi.c

314 ↗

(On Diff #58855)

vtscsi_maximum_segments() has a check for indirect descriptors and I think that applies here too. Consider the degenerate case of virtqueue_size() == 1 with indirect: it can still enqueue VIRTIO_MAX_INDIRECT segments. I think the bhyve bug would have gone unnoticed if indirect was supported because the driver would have clamped the maximum segments to the max indirect limit.

Some of the code around indirect can be a little confusing. We'll always use it if it was negotiated and the count used in VQ_ALLOC_INFO_INIT warrants. Perhaps the return on line 243 of virtqueue.c should be an error instead but I believe all drivers are well behaved.

Hoist this check into a vtscsi_check_sizes() (or whatever) to match the surrounding code pattern.

After thinking about it a bit more, I'm fine too with adjusting down the maximum segments like your original patch instead of failing to attach. Whenever I get around to virtqueue resizing I doubt I would keep the original size around to compare to. I think here we can get away with changing vtscsi_max_nsegs after the fact at the cost of a bit of unused memory.

[vangyzen]

sys/dev/virtio/scsi/virtio_scsi.c

314 ↗

(On Diff #58855)

Thanks for the continual feedback and for reconsidering. I really would like to make it work. I'll re-post my original patch for that.

478 ↗

(On Diff #58812)

I could just move this into an else clause, since only bhyve has shown this problem so far, and it doesn't support indirect.

[vangyzen]

• revert to previous approach of clamping seg_max
• do not artificially limit seg_max when indirect is available

[bryanv]

Sorry I wasn't clearer: use virtqueue_size() after the allocate to do the check. Do not add a new virtqueue_size bus interface.

[vangyzen]

• Clamp seg_max after allocating virtqueues.

[vangyzen]

Sorry I wasn't clearer: use virtqueue_size() after the allocate to do the check. Do not add a new virtqueue_size bus interface.

No worries.