From bcf4792b5359967d06378c93df11d5a262cfb99d Mon Sep 17 00:00:00 2001 From: Matt LaFayette Date: Mon, 6 Jan 2025 16:42:26 -0800 Subject: [PATCH] Review feedback: * Don't promote as many things to `pub`, and make it explicit when that happens. * .div_ceil rather than doing it manually * Arbitrary fields in enums. * cargo xtask fmt fix. --- vm/devices/storage/storvsp/Cargo.toml | 3 +++ vm/devices/storage/storvsp/fuzz/Cargo.toml | 2 +- .../storage/storvsp/fuzz/fuzz_storvsp.rs | 24 +++++++------------ vm/devices/storage/storvsp/src/lib.rs | 16 +++++++++---- .../storage/storvsp/src/test_helpers.rs | 2 +- 5 files changed, 26 insertions(+), 21 deletions(-) diff --git a/vm/devices/storage/storvsp/Cargo.toml b/vm/devices/storage/storvsp/Cargo.toml index 95fb087fe..ce446c412 100644 --- a/vm/devices/storage/storvsp/Cargo.toml +++ b/vm/devices/storage/storvsp/Cargo.toml @@ -12,6 +12,9 @@ ioperf = ["dep:disklayer_ram"] # Enable generating arbitrary values of types useful for fuzzing. arbitrary = ["dep:arbitrary"] +# Expose some implementation details publicly, used for fuzzing. +fuzz_helpers = [] + [dependencies] arbitrary = { workspace = true, optional = true, features = ["derive"] } diff --git a/vm/devices/storage/storvsp/fuzz/Cargo.toml b/vm/devices/storage/storvsp/fuzz/Cargo.toml index 33d2a3bba..24f1d4b03 100644 --- a/vm/devices/storage/storvsp/fuzz/Cargo.toml +++ b/vm/devices/storage/storvsp/fuzz/Cargo.toml @@ -15,7 +15,7 @@ guestmem.workspace = true pal_async.workspace = true scsi_defs = {workspace = true, features = ["arbitrary"]} scsidisk.workspace = true -storvsp = {workspace = true, features = ["arbitrary"]} +storvsp = {workspace = true, features = ["arbitrary", "fuzz_helpers"]} storvsp_resources = {workspace = true, features = ["arbitrary"]} vmbus_async.workspace = true vmbus_channel.workspace = true diff --git a/vm/devices/storage/storvsp/fuzz/fuzz_storvsp.rs b/vm/devices/storage/storvsp/fuzz/fuzz_storvsp.rs index a16dcb42d..84e1b483b 100644 --- a/vm/devices/storage/storvsp/fuzz/fuzz_storvsp.rs +++ b/vm/devices/storage/storvsp/fuzz/fuzz_storvsp.rs @@ -27,11 +27,10 @@ use zerocopy::AsBytes; use zerocopy::FromZeroes; #[derive(Arbitrary)] -pub enum StorvspFuzzAction { - SendDataPacket, +enum StorvspFuzzAction { SendReadWritePacket, - SendRawPacket, - ReadCompletion + SendRawPacket(FuzzOutgoingPacketType), + ReadCompletion, } #[derive(Arbitrary)] @@ -61,9 +60,7 @@ async fn send_gpa_direct_packet( let start_page: u64 = gpa_start / PAGE_SIZE as u64; let end_page = start_page .checked_add(byte_len.try_into()?) - .and_then(|v| v.checked_add(PAGE_SIZE as u64)) - .and_then(|v| v.checked_sub(1)) - .and_then(|v| v.checked_div(PAGE_SIZE as u64)) + .map(|v| v.div_ceil(PAGE_SIZE as u64)) .ok_or(arbitrary::Error::IncorrectFormat)?; let gpns: Vec = (start_page..end_page).collect(); @@ -126,7 +123,6 @@ async fn send_arbitrary_readwrite_packet( scsi_req.payload[0..10].copy_from_slice(cdb.as_bytes()); - // send the gpa packet send_gpa_direct_packet( guest, &[packet.as_bytes(), scsi_req.as_bytes()], @@ -172,15 +168,10 @@ fn do_fuzz(u: &mut Unstructured<'_>) -> Result<(), anyhow::Error> { while !u.is_empty() { let action = u.arbitrary::()?; match action { - StorvspFuzzAction::SendDataPacket => { - let packet = u.arbitrary::()?; - let _ = guest.send_data_packet_sync(&[packet.as_bytes()]).await; - } StorvspFuzzAction::SendReadWritePacket => { send_arbitrary_readwrite_packet(u, &mut guest).await?; } - StorvspFuzzAction::SendRawPacket => { - let packet_type = u.arbitrary()?; + StorvspFuzzAction::SendRawPacket(packet_type) => { match packet_type { FuzzOutgoingPacketType::AnyOutgoingPacket => { let packet_types = [ @@ -188,10 +179,13 @@ fn do_fuzz(u: &mut Unstructured<'_>) -> Result<(), anyhow::Error> { OutgoingPacketType::InBandWithCompletion, OutgoingPacketType::Completion, ]; + let payload = u.arbitrary::()?; + // TODO: [use-arbitrary-input] (send a byte blob of arbitrary length rather + // than a fixed-size arbitrary packet) let packet = OutgoingPacket { transaction_id: u.arbitrary()?, packet_type: *u.choose(&packet_types)?, - payload: &[&[0u8; 0]], // TODO: [use-arbitrary-input] + payload: &[payload.as_bytes()], // TODO: [use-arbitrary-input] }; guest.queue.split().1.write(packet).await?; diff --git a/vm/devices/storage/storvsp/src/lib.rs b/vm/devices/storage/storvsp/src/lib.rs index 6454f879d..f6ecfaec6 100644 --- a/vm/devices/storage/storvsp/src/lib.rs +++ b/vm/devices/storage/storvsp/src/lib.rs @@ -6,10 +6,18 @@ #[cfg(feature = "ioperf")] pub mod ioperf; +#[cfg(feature = "fuzz_helpers")] pub mod protocol; +#[cfg(feature = "fuzz_helpers")] +pub mod test_helpers; + +#[cfg(not(feature = "fuzz_helpers"))] +mod protocol; +#[cfg(not(feature = "fuzz_helpers"))] +mod test_helpers; + pub mod resolver; mod save_restore; -pub mod test_helpers; use crate::ring::gparange::GpnList; use crate::ring::gparange::MultiPagedRangeBuf; @@ -274,7 +282,7 @@ impl Future for ScsiRequest { } #[derive(Debug, Error)] -pub enum WorkerError { +enum WorkerError { #[error("packet error")] PacketError(#[source] PacketError), #[error("queue error")] @@ -284,7 +292,7 @@ pub enum WorkerError { } #[derive(Debug, Error)] -pub enum PacketError { +enum PacketError { #[error("Not transactional")] NotTransactional, #[error("Unrecognized operation {0:?}")] @@ -1544,7 +1552,7 @@ impl ScsiControllerDisk { } } -pub struct ScsiControllerState { +struct ScsiControllerState { disks: RwLock>, rescan_notification_source: Mutex>>, } diff --git a/vm/devices/storage/storvsp/src/test_helpers.rs b/vm/devices/storage/storvsp/src/test_helpers.rs index 80366c5dd..12bfb8468 100644 --- a/vm/devices/storage/storvsp/src/test_helpers.rs +++ b/vm/devices/storage/storvsp/src/test_helpers.rs @@ -43,7 +43,7 @@ pub struct TestWorker { } impl TestWorker { - pub async fn teardown(self) -> Result<(), WorkerError> { + pub(crate) async fn teardown(self) -> Result<(), WorkerError> { self.task.await }