Rudimentary NVMe emulation fuzzer - #966
Conversation
This tries doing a bunch of random operations against an NVMe device and checks the operations against a limited model of what the results of those operations should be. The initial stab at this is what caught #965, and it caught a bug in an intermediate state of #953 (which other phd tests did notice anyway). This fuzzing would probably be best with actual I/O operations mixed in, and I think that *should* be relatively straightforward to add from here., but as-is it's useful! This would probably be best phrased as a `cargo-fuzz` test to at least get coverage-guided fuzzing. Because of the statefulness of NVMe I think either way we'd want the model of expected device state and a pick-actions-then-run execution to further guide `cargo-fuzz` into useful parts of the device state. The initial approach at this allowed for device reset and migration at arbitrary times via a separate thread. When that required synchronizing the model of device state it was effectively interleaved with "guest" operations on the device, and in practice admin commands are serialized by the `NvmeCtrl` state lock anyway. It may be more interesting to revisit with concurrent I/O operations on submission/completion queues.
|
|
||
| let mut rng = Pcg64::seed_from_u64(seed); | ||
|
|
||
| for _ in 0..1_000 { |
There was a problem hiding this comment.
cargo test gets through this in about 4 seconds, but cargo test --release is .. almost immediate. I'd been running this at 100k iterations instead, there. Even a thousand seems like an OK place to be for CI?
| /// any point as `PciNvme` technically does. (In practice, reset immediately | ||
| /// locks the inner `NvmeCtrl` to do the reset, for administrative options it is | ||
| /// effectively serialized anyway.) | ||
| struct FuzzCtx { |
There was a problem hiding this comment.
I'd talked with Patrick just a bit about the idea of having a more composable fuzzing framework as part of Propolis (the library). the implementation in this file is simultaneously:
- an NVMe driver, which is driven ~randomly to exercise
propolis/src/hw/nvme - a model of the NVMe controller state, based on the driver's operations (what queues are created, is the device initialized, etc)
- a model of the synthesis of NVMe spec and controller model: given the current state, what NVMe operations should have what outcomes, and what state transitions are permissible from the current state?
you could swap out the word "NVMe" above for any other devices (the chipset would be interesting!). a reasonable thing to want would be fuzzing a pair of NVMe devices concurrently on the same PCI bridge. or poking a disk and NIC concurrently, or configuring a pair of NVMe devices to write on each others' queues, or ...
in the limit this seems to me like assembling an increasingly chaotic VM and configuring how chaotic it should be. I don't plan on adjusting this in that direction right now, but it seems like an interesting future direction this could take.
There was a problem hiding this comment.
I like this idea, but I also agree that we should try to get this in before generalizing it.
jordanhendricks
left a comment
jordanhendricks
left a comment
There was a problem hiding this comment.
I gave this a look and broadly it looks good, but I think I'm lacking some understanding of how we intend to use this. Is the idea that we will run a fuzzer in CI? Or as a standalone tool?
| /// any point as `PciNvme` technically does. (In practice, reset immediately | ||
| /// locks the inner `NvmeCtrl` to do the reset, for administrative options it is | ||
| /// effectively serialized anyway.) | ||
| struct FuzzCtx { |
There was a problem hiding this comment.
I like this idea, but I also agree that we should try to get this in before generalizing it.
| // 64 MB feels like a reasonable (but very tiny!) size for a test | ||
| // disk. |
There was a problem hiding this comment.
are there compelling reasons to vary the block size and size of the test disk, in future?
There was a problem hiding this comment.
block size definitely, disk size less probably? since then we've gotten a new parameter on PciNvme for "does the device have a volatile write cache", which won't affect disk semantics and we should be able to change arbitrarily.
but changing these generally I'm not sure how to fit in quite yet.. probably want that kind of fuzzing in an enclosing loop that tries 1k operations or whatever, rather than (for example) Init possibly changing the controller like this. in my mind the current sequence of operations is a stand-in for a single guest operating on a single disk, so changing those parameters are some other variable?
| // I/O submission/completion queues are interleaved (for fun more than | ||
| // anything else). With 256kb of memory for queues we can have | ||
| // up to 64 I/O queues in the form of 32 submission and completion | ||
| // queues. |
There was a problem hiding this comment.
turbo nitpick: mayhaps we could have a const IO_QUEUE_REGION_SIZE = 256 and derive the 32 et al from that (and use that 256 in the address calculation above)? that way there's one constant to mess with and everything else just works.
There was a problem hiding this comment.
since the size of this region is a function of the queue size and queue count I turned this around a bit, there's now an IO_QUEUEPAIR_MAX that with the queue size const drives IO_QUEUES_SIZE and places IO_QUEUES_BASE. since I was messing with the const there's now const asserts that the region size and placement is some constants, so that the hex values you might see in debugging are written out somewhere in the source..
dafaa36 to
cb63659
Compare
d5c6b3e to
8fc9550
Compare
so! I'm planning on merging this imminently as I've since expanded on this a bit. to your question from ... last year ..., my hope is that we should always be able to do 1k Thingies to a disk in CI, and if not that's a remarkable problem. but whatever we're doing in CI should be pretty short. we might want to concurrently I also hope we can use this to build a standalone tool that runs longer (or more exotic) configurations. but the part in CI here is intended to be pretty short and really a smoke-test that things basically work. |
This would be the sort of thing I'd be thinking about following up with (eventually). strong endorse |
4 participants
This tries doing a bunch of random operations against an NVMe device and checks the operations against a limited model of what the results of those operations should be.
The initial stab at this is what caught #965, and it caught a bug in an intermediate state of #953 (which other phd tests did notice anyway). This fuzzing would probably be best with actual I/O operations mixed in, and I think that should be relatively straightforward to add from here., but as-is it's useful!
This would probably be best phrased as a
cargo-fuzztest to at least get coverage-guided fuzzing. Because of the statefulness of NVMe I think either way we'd want the model of expected device state and a pick-actions-then-run execution to further guidecargo-fuzzinto useful parts of the device state.The initial approach at this allowed for device reset and migration at arbitrary times via a separate thread. When that required synchronizing the model of device state it was effectively interleaved with "guest" operations on the device, and in practice admin commands are serialized by the
NvmeCtrlstate lock anyway. It may be more interesting to revisit with concurrent I/O operations on submission/completion queues.