From 5a0b6f2d06761a6178dd32d2ee96aa40ebace512 Mon Sep 17 00:00:00 2001 From: Rob Bradford Date: Sun, 15 Feb 2026 11:25:53 +0000 Subject: [PATCH] vmm: Improve resiliency of image type handling Add an image_type to DiskConfig to specify the image type. If none is specified autodetect the image type but disable potentially unsafe behaviour in the QCOW2 backend by disabling the backing file support. If the image type is autodetected then fix it in the config so that it will be persistant across reboots and migrations/snapshot & restores. This also handles the case where the image type was not specified as part of the disk configuration. Signed-off-by: Rob Bradford (cherry picked from commit 6f2357c14ed83582fc085bc74acfe3a270e6320e) --- Cargo.lock | 1 + block/src/lib.rs | 37 ++++++++++++- cloud-hypervisor/Cargo.toml | 1 + cloud-hypervisor/src/main.rs | 14 ++--- cloud-hypervisor/tests/integration.rs | 64 ++++++++++++++++++----- vmm/src/api/openapi/cloud-hypervisor.yaml | 3 ++ vmm/src/config.rs | 21 +++++++- vmm/src/device_manager.rs | 44 ++++++++++++++-- vmm/src/vm_config.rs | 3 ++ 9 files changed, 161 insertions(+), 27 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 62bbf709f..099bf6b5b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -421,6 +421,7 @@ version = "50.0.0" dependencies = [ "anyhow", "api_client", + "block", "clap", "dhat", "dirs", diff --git a/block/src/lib.rs b/block/src/lib.rs index 72210302a..afd4b0dd7 100644 --- a/block/src/lib.rs +++ b/block/src/lib.rs @@ -30,12 +30,13 @@ pub mod vhdx_sync; use std::alloc::{Layout, alloc_zeroed, dealloc}; use std::collections::VecDeque; -use std::fmt::Debug; +use std::fmt::{self, Debug}; use std::fs::File; use std::io::{self, IoSlice, IoSliceMut, Read, Seek, SeekFrom, Write}; use std::os::linux::fs::MetadataExt; use std::os::unix::io::AsRawFd; use std::path::Path; +use std::str::FromStr; use std::time::Instant; use std::{cmp, result}; @@ -788,12 +789,44 @@ pub trait AsyncAdaptor { } } -#[derive(PartialEq, Eq, Debug)] +#[derive(Serialize, Deserialize, Clone, Copy, Debug, PartialEq, Eq, Default)] pub enum ImageType { FixedVhd, Qcow2, Raw, Vhdx, + #[default] + Unknown, +} + +impl fmt::Display for ImageType { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + ImageType::FixedVhd => write!(f, "vhd"), + ImageType::Qcow2 => write!(f, "qcow2"), + ImageType::Raw => write!(f, "raw"), + ImageType::Vhdx => write!(f, "vhdx"), + ImageType::Unknown => write!(f, "unknown"), + } + } +} + +pub enum ImageTypeParseError { + InvalidValue(String), +} + +impl FromStr for ImageType { + type Err = ImageTypeParseError; + + fn from_str(s: &str) -> Result { + match s.to_lowercase().as_str() { + "vhd" => Ok(ImageType::FixedVhd), + "qcow2" => Ok(ImageType::Qcow2), + "raw" => Ok(ImageType::Raw), + "vhdx" => Ok(ImageType::Vhdx), + _ => Err(ImageTypeParseError::InvalidValue(s.to_string())), + } + } } const QCOW_MAGIC: u32 = 0x5146_49fb; diff --git a/cloud-hypervisor/Cargo.toml b/cloud-hypervisor/Cargo.toml index 426a52263..c38ae2231 100644 --- a/cloud-hypervisor/Cargo.toml +++ b/cloud-hypervisor/Cargo.toml @@ -41,6 +41,7 @@ vmm-sys-util = { workspace = true } zbus = { version = "5.7.1", optional = true } [dev-dependencies] +block = { path = "../block" } dirs = { workspace = true } net_util = { path = "../net_util" } serde_json = { workspace = true } diff --git a/cloud-hypervisor/src/main.rs b/cloud-hypervisor/src/main.rs index d08293b6e..82d3ce05c 100644 --- a/cloud-hypervisor/src/main.rs +++ b/cloud-hypervisor/src/main.rs @@ -1199,14 +1199,14 @@ mod unit_tests { "--kernel", "/path/to/kernel", "--disk", - "path=/path/to/disk/1", + "path=/path/to/disk/1,image_type=raw", "path=/path/to/disk/2", ], r#"{ "payload": {"kernel": "/path/to/kernel"}, "disks": [ - {"path": "/path/to/disk/1"}, - {"path": "/path/to/disk/2"} + {"path": "/path/to/disk/1", "image_type": "Raw"}, + {"path": "/path/to/disk/2", "image_type": "Unknown"} ] }"#, true, @@ -1217,8 +1217,8 @@ mod unit_tests { "--kernel", "/path/to/kernel", "--disk", - "path=/path/to/disk/1", - "path=/path/to/disk/2", + "path=/path/to/disk/1,image_type=raw", + "path=/path/to/disk/2,image_type=qcow2", ], r#"{ "payload": {"kernel": "/path/to/kernel"}, @@ -1280,8 +1280,8 @@ mod unit_tests { r#"{ "payload": {"kernel": "/path/to/kernel"}, "disks": [ - {"path": "/path/to/disk/1", "rate_limit_group": "group0"}, - {"path": "/path/to/disk/2", "rate_limit_group": "group0"} + {"path": "/path/to/disk/1", "rate_limit_group": "group0", "image_type": "Unknown"}, + {"path": "/path/to/disk/2", "rate_limit_group": "group0", "image_type": "Unknown"} ], "rate_limit_groups": [ {"id": "group0", "rate_limiter_config": {"bandwidth": {"size": 1000, "one_time_burst": 0, "refill_time": 100}}} diff --git a/cloud-hypervisor/tests/integration.rs b/cloud-hypervisor/tests/integration.rs index 386e71a3f..77df84fb1 100644 --- a/cloud-hypervisor/tests/integration.rs +++ b/cloud-hypervisor/tests/integration.rs @@ -2526,6 +2526,8 @@ mod common_parallel { use std::fs::OpenOptions; use std::io::SeekFrom; + use block::ImageType; + use crate::*; #[test] @@ -3172,7 +3174,7 @@ mod common_parallel { guest.disk_config.disk(DiskType::CloudInit).unwrap() ) .as_str(), - format!("path={test_disk_path},pci_segment=15").as_str(), + format!("path={test_disk_path},pci_segment=15,image_type=raw").as_str(), ]) .capture_output() .default_net(); @@ -3413,6 +3415,7 @@ mod common_parallel { disable_aio: bool, verify_os_disk: bool, backing_files: bool, + image_type: ImageType, ) { let disk_config = UbuntuDiskConfig::new(image_name.to_string()); let guest = Guest::new(Box::new(disk_config)); @@ -3433,9 +3436,9 @@ mod common_parallel { .args([ "--disk", format!( - "path={},backing_files={}", + "path={},backing_files={},image_type={image_type}", guest.disk_config.disk(DiskType::OperatingSystem).unwrap(), - if backing_files { "on"} else {"off"} + if backing_files { "on"} else {"off"}, ) .as_str(), format!( @@ -3505,17 +3508,17 @@ mod common_parallel { #[test] fn test_virtio_block_io_uring() { - _test_virtio_block(FOCAL_IMAGE_NAME, false, true, false, false); + _test_virtio_block(FOCAL_IMAGE_NAME, false, true, false, false, ImageType::Raw); } #[test] fn test_virtio_block_aio() { - _test_virtio_block(FOCAL_IMAGE_NAME, true, false, false, false); + _test_virtio_block(FOCAL_IMAGE_NAME, true, false, false, false, ImageType::Raw); } #[test] fn test_virtio_block_sync() { - _test_virtio_block(FOCAL_IMAGE_NAME, true, true, false, false); + _test_virtio_block(FOCAL_IMAGE_NAME, true, true, false, false, ImageType::Raw); } /// Uses `qemu-img check` to verify disk image consistency. @@ -3551,17 +3554,38 @@ mod common_parallel { #[test] fn test_virtio_block_qcow2() { - _test_virtio_block(JAMMY_IMAGE_NAME_QCOW2, false, false, true, false); + _test_virtio_block( + JAMMY_IMAGE_NAME_QCOW2, + false, + false, + true, + false, + ImageType::Qcow2, + ); } #[test] fn test_virtio_block_qcow2_zlib() { - _test_virtio_block(JAMMY_IMAGE_NAME_QCOW2_ZLIB, false, false, true, false); + _test_virtio_block( + JAMMY_IMAGE_NAME_QCOW2_ZLIB, + false, + false, + true, + false, + ImageType::Qcow2, + ); } #[test] fn test_virtio_block_qcow2_zstd() { - _test_virtio_block(JAMMY_IMAGE_NAME_QCOW2_ZSTD, false, false, true, false); + _test_virtio_block( + JAMMY_IMAGE_NAME_QCOW2_ZSTD, + false, + false, + true, + false, + ImageType::Qcow2, + ); } #[test] @@ -3572,6 +3596,7 @@ mod common_parallel { false, true, true, + ImageType::Qcow2, ); } @@ -3583,6 +3608,7 @@ mod common_parallel { false, true, true, + ImageType::Qcow2, ); } @@ -3608,7 +3634,14 @@ mod common_parallel { .output() .expect("Expect generating VHD image from RAW image"); - _test_virtio_block(FOCAL_IMAGE_NAME_VHD, false, false, false, false); + _test_virtio_block( + FOCAL_IMAGE_NAME_VHD, + false, + false, + false, + false, + ImageType::FixedVhd, + ); } #[test] @@ -3632,7 +3665,14 @@ mod common_parallel { .output() .expect("Expect generating dynamic VHDx image from RAW image"); - _test_virtio_block(FOCAL_IMAGE_NAME_VHDX, false, false, true, false); + _test_virtio_block( + FOCAL_IMAGE_NAME_VHDX, + false, + false, + true, + false, + ImageType::Vhdx, + ); } #[test] @@ -4697,7 +4737,7 @@ mod common_parallel { guest.disk_config.disk(DiskType::CloudInit).unwrap() ) .as_str(), - format!("path={}", vfio_disk_path.to_str().unwrap()).as_str(), + format!("path={},image_type=raw", vfio_disk_path.to_str().unwrap()).as_str(), format!("path={},iommu=on,readonly=true", blk_file_path.to_str().unwrap()).as_str(), ]) .args([ diff --git a/vmm/src/api/openapi/cloud-hypervisor.yaml b/vmm/src/api/openapi/cloud-hypervisor.yaml index d10832101..aca4cc28e 100644 --- a/vmm/src/api/openapi/cloud-hypervisor.yaml +++ b/vmm/src/api/openapi/cloud-hypervisor.yaml @@ -944,6 +944,9 @@ components: backing_files: type: boolean default: false + image_type: + type: enum ["FixedVhd", "Qcow2", "Raw", "Vhdx"] + NetConfig: type: object diff --git a/vmm/src/config.rs b/vmm/src/config.rs index e1f9213d3..ed909362a 100644 --- a/vmm/src/config.rs +++ b/vmm/src/config.rs @@ -10,6 +10,7 @@ use std::path::PathBuf; use std::result; use std::str::FromStr; +use block::ImageType; use clap::ArgMatches; use log::{debug, warn}; use option_parser::{ @@ -1093,7 +1094,8 @@ impl DiskConfig { ops_size=,ops_one_time_burst=,ops_refill_time=,\ id=,pci_segment=,rate_limit_group=,\ queue_affinity=,\ - serial=,backing_files=on|off"; + serial=,backing_files=on|off,\ + image_type="; pub fn parse(disk: &str) -> Result { let mut parser = OptionParser::new(); @@ -1119,7 +1121,9 @@ impl DiskConfig { .add("serial") .add("rate_limit_group") .add("queue_affinity") - .add("backing_files"); + .add("backing_files") + .add("image_type"); + parser.parse(disk).map_err(Error::ParseDisk)?; let path = parser.get("path").map(PathBuf::from); @@ -1204,12 +1208,22 @@ impl DiskConfig { }) .collect() }); + let backing_files = parser .convert::("backing_files") .map_err(Error::ParseDisk)? .unwrap_or(Toggle(false)) .0; + let image_type = if vhost_socket.is_none() { + parser + .convert::("image_type") + .map_err(Error::ParseDisk)? + .unwrap_or(ImageType::Unknown) + } else { + ImageType::Unknown + }; + let bw_tb_config = if bw_size != 0 && bw_refill_time != 0 { Some(TokenBucketConfig { size: bw_size, @@ -1255,6 +1269,7 @@ impl DiskConfig { serial, queue_affinity, backing_files, + image_type, }) } @@ -3423,6 +3438,7 @@ mod unit_tests { serial: None, queue_affinity: None, backing_files: false, + image_type: ImageType::Unknown, } } @@ -3445,6 +3461,7 @@ mod unit_tests { path: None, vhost_socket: Some(String::from("/tmp/sock")), vhost_user: true, + image_type: ImageType::Unknown, ..disk_fixture() } ); diff --git a/vmm/src/device_manager.rs b/vmm/src/device_manager.rs index 9b2a32455..8ca636ab8 100644 --- a/vmm/src/device_manager.rs +++ b/vmm/src/device_manager.rs @@ -674,6 +674,15 @@ pub enum DeviceManagerError { /// Disk resizing failed. #[error("Disk resize error")] DiskResize(#[source] virtio_devices::block::Error), + + /// Disk image type does not match expected type. + #[error( + "Disk image type does not match expected type: specified = {specified}, detected = {detected}" + )] + DiskImageTypeMismatch { + specified: ImageType, + detected: ImageType, + }, } pub type DeviceManagerResult = result::Result; @@ -2654,14 +2663,40 @@ impl DeviceManager { .clone(), ) .map_err(DeviceManagerError::Disk)?; - let image_type = - detect_image_type(&mut file).map_err(DeviceManagerError::DetectImageType)?; - if image_type != ImageType::Qcow2 && disk_cfg.backing_files { + let detected_image_type = + detect_image_type(&mut file).map_err(DeviceManagerError::DetectImageType)?; + if disk_cfg.image_type == ImageType::Unknown { + warn!( + "No image_type specified - detected as {detected_image_type}. \ + Configuration updated to persist type across reboots and migrations." + ); + + if detected_image_type != ImageType::Raw { + warn!( + "Non-raw image type detected. In the future it will be necessary \ + to specify image_type for non-raw files." + ); + } + + if detected_image_type == ImageType::Qcow2 && disk_cfg.backing_files { + warn!("QCOW2 image type autodetected. Disabling backing files"); + disk_cfg.backing_files = false; + } + + disk_cfg.image_type = detected_image_type; + } else if disk_cfg.image_type != detected_image_type { + return Err(DeviceManagerError::DiskImageTypeMismatch { + specified: disk_cfg.image_type, + detected: detected_image_type, + }); + } + + if disk_cfg.image_type != ImageType::Qcow2 && disk_cfg.backing_files { warn!("Enabling backing_files option only applies for QCOW2 files"); } - let image = match image_type { + let image = match disk_cfg.image_type { ImageType::FixedVhd => { // Use asynchronous backend relying on io_uring if the // syscalls are supported. @@ -2725,6 +2760,7 @@ impl DeviceManager { .map_err(DeviceManagerError::CreateFixedVhdxDiskSync)?, ) as Box } + ImageType::Unknown => unreachable!(), }; let rate_limit_group = diff --git a/vmm/src/vm_config.rs b/vmm/src/vm_config.rs index a71cca7be..fc625c0c7 100644 --- a/vmm/src/vm_config.rs +++ b/vmm/src/vm_config.rs @@ -8,6 +8,7 @@ use std::path::{Path, PathBuf}; use std::str::FromStr; use std::{fs, result}; +use block::ImageType; use log::{debug, warn}; use net_util::MacAddr; use serde::{Deserialize, Serialize}; @@ -286,6 +287,8 @@ pub struct DiskConfig { pub queue_affinity: Option>, #[serde(default)] pub backing_files: bool, + #[serde(default)] + pub image_type: ImageType, } impl ApplyLandlock for DiskConfig {