From 5921c375cc65fda3e87bbf8e57ed0eaecc6e603a Mon Sep 17 00:00:00 2001 From: Frederick Mayle Date: Tue, 10 Sep 2024 11:42:07 -0700 Subject: [PATCH] disk: add option to disable file locking The recent changes to make crosvm lock nested disk files is running afoul of Android Virtualization Framework's SELinux policies. The file locking doesn't add much value in that environement because all the shared files are read-only, so add an opt-out for locking to buy us time to re-engineer the policies. BUG=b:330911976 Change-Id: I0b35732978e946a2331507d6061729d53955a8d3 Reviewed-on: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/5849284 Reviewed-by: Daniel Verkamp Commit-Queue: Frederick Mayle --- devices/src/virtio/block/mod.rs | 74 +++++++++++++++++++++++++ devices/src/virtio/block/sys/linux.rs | 1 + devices/src/virtio/block/sys/windows.rs | 1 + devices/src/virtio/scsi/mod.rs | 10 ++++ devices/src/virtio/scsi/sys/linux.rs | 1 + disk/src/composite.rs | 1 + disk/src/disk.rs | 2 + disk/src/qcow/mod.rs | 4 ++ disk/src/sys/linux.rs | 16 +++--- disk/src/sys/windows.rs | 5 +- fuzz/fuzz_targets/qcow_fuzzer.rs | 1 + src/main.rs | 2 + 12 files changed, 110 insertions(+), 8 deletions(-) diff --git a/devices/src/virtio/block/mod.rs b/devices/src/virtio/block/mod.rs index b06f311155..3b4cf640a3 100644 --- a/devices/src/virtio/block/mod.rs +++ b/devices/src/virtio/block/mod.rs @@ -22,6 +22,9 @@ pub use asynchronous::BlockAsync; fn block_option_sparse_default() -> bool { true } +fn block_option_lock_default() -> bool { + true +} fn block_option_block_size_default() -> u32 { 512 } @@ -93,6 +96,9 @@ pub struct DiskOption { // camel_case variant allowed for backward compatibility. #[serde(default, alias = "o_direct")] pub direct: bool, + /// Whether to lock the disk files. Uses flock on Unix and FILE_SHARE_* flags on Windows. + #[serde(default = "block_option_lock_default")] + pub lock: bool, // camel_case variant allowed for backward compatibility. #[serde(default = "block_option_block_size_default", alias = "block_size")] pub block_size: u32, @@ -141,6 +147,7 @@ impl Default for DiskOption { root: false, sparse: block_option_sparse_default(), direct: false, + lock: block_option_lock_default(), block_size: block_option_block_size_default(), id: None, #[cfg(windows)] @@ -200,6 +207,7 @@ mod tests { root: false, sparse: true, direct: false, + lock: true, block_size: 512, id: None, #[cfg(windows)] @@ -222,6 +230,7 @@ mod tests { root: false, sparse: true, direct: false, + lock: true, block_size: 512, id: None, #[cfg(windows)] @@ -244,6 +253,7 @@ mod tests { root: false, sparse: true, direct: false, + lock: true, block_size: 512, id: None, #[cfg(windows)] @@ -266,6 +276,7 @@ mod tests { root: false, sparse: true, direct: false, + lock: true, block_size: 512, id: None, #[cfg(windows)] @@ -288,6 +299,7 @@ mod tests { root: true, sparse: true, direct: false, + lock: true, block_size: 512, id: None, #[cfg(windows)] @@ -310,6 +322,7 @@ mod tests { root: false, sparse: true, direct: false, + lock: true, block_size: 512, id: None, #[cfg(windows)] @@ -330,6 +343,7 @@ mod tests { root: false, sparse: false, direct: false, + lock: true, block_size: 512, id: None, #[cfg(windows)] @@ -352,6 +366,7 @@ mod tests { root: false, sparse: true, direct: true, + lock: true, block_size: 512, id: None, #[cfg(windows)] @@ -374,6 +389,7 @@ mod tests { root: false, sparse: true, direct: true, + lock: true, block_size: 512, id: None, #[cfg(windows)] @@ -396,6 +412,7 @@ mod tests { root: false, sparse: true, direct: false, + lock: true, block_size: 128, id: None, #[cfg(windows)] @@ -418,6 +435,7 @@ mod tests { root: false, sparse: true, direct: false, + lock: true, block_size: 128, id: None, async_executor: None, @@ -442,6 +460,7 @@ mod tests { root: false, sparse: true, direct: false, + lock: true, block_size: 512, id: None, io_concurrency: NonZeroU32::new(4).unwrap(), @@ -461,6 +480,7 @@ mod tests { root: false, sparse: true, direct: false, + lock: true, block_size: 512, id: None, io_concurrency: NonZeroU32::new(1).unwrap(), @@ -482,6 +502,7 @@ mod tests { root: false, sparse: true, direct: false, + lock: true, block_size: 512, id: None, io_concurrency: NonZeroU32::new(1).unwrap(), @@ -509,6 +530,7 @@ mod tests { root: false, sparse: true, direct: false, + lock: true, block_size: 512, id: Some(*b"DISK\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"), #[cfg(windows)] @@ -544,6 +566,7 @@ mod tests { root: false, sparse: true, direct: false, + lock: true, block_size: 512, id: None, #[cfg(windows)] @@ -566,6 +589,7 @@ mod tests { root: false, sparse: true, direct: false, + lock: true, block_size: 512, id: None, #[cfg(windows)] @@ -588,6 +612,7 @@ mod tests { root: false, sparse: true, direct: false, + lock: true, block_size: 512, id: None, #[cfg(windows)] @@ -604,6 +629,51 @@ mod tests { } ); + // lock=true + let params = from_block_arg("/path/to/disk.img,lock=true").unwrap(); + assert_eq!( + params, + DiskOption { + path: "/path/to/disk.img".into(), + read_only: false, + root: false, + sparse: true, + direct: false, + lock: true, + block_size: 512, + id: None, + #[cfg(windows)] + io_concurrency: NonZeroU32::new(1).unwrap(), + multiple_workers: false, + async_executor: None, + packed_queue: false, + bootindex: None, + pci_address: None, + } + ); + // lock=false + let params = from_block_arg("/path/to/disk.img,lock=false").unwrap(); + assert_eq!( + params, + DiskOption { + path: "/path/to/disk.img".into(), + read_only: false, + root: false, + sparse: true, + direct: false, + lock: false, + block_size: 512, + id: None, + #[cfg(windows)] + io_concurrency: NonZeroU32::new(1).unwrap(), + multiple_workers: false, + async_executor: None, + packed_queue: false, + bootindex: None, + pci_address: None, + } + ); + // All together let params = from_block_arg(&format!( "/some/path.img,block_size=256,ro,root,sparse=false,id=DISK_LABEL\ @@ -618,6 +688,7 @@ mod tests { root: true, sparse: false, direct: true, + lock: true, block_size: 256, id: Some(*b"DISK_LABEL\0\0\0\0\0\0\0\0\0\0"), #[cfg(windows)] @@ -644,6 +715,7 @@ mod tests { root: false, sparse: true, direct: false, + lock: true, block_size: 512, id: None, #[cfg(windows)] @@ -665,6 +737,7 @@ mod tests { root: false, sparse: true, direct: false, + lock: true, block_size: 512, id: Some(*b"BLK\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"), #[cfg(windows)] @@ -686,6 +759,7 @@ mod tests { root: false, sparse: true, direct: false, + lock: true, block_size: 512, id: Some(*b"QWERTYUIOPASDFGHJKL:"), #[cfg(windows)] diff --git a/devices/src/virtio/block/sys/linux.rs b/devices/src/virtio/block/sys/linux.rs index b8de72d10c..1dd20b2c54 100644 --- a/devices/src/virtio/block/sys/linux.rs +++ b/devices/src/virtio/block/sys/linux.rs @@ -31,6 +31,7 @@ impl DiskOption { is_sparse_file: self.sparse, is_overlapped: false, is_direct: self.direct, + lock: self.lock, depth: 0, }) .context("open_disk_file failed") diff --git a/devices/src/virtio/block/sys/windows.rs b/devices/src/virtio/block/sys/windows.rs index be30b92793..a977b7d25c 100644 --- a/devices/src/virtio/block/sys/windows.rs +++ b/devices/src/virtio/block/sys/windows.rs @@ -28,6 +28,7 @@ impl DiskOption { ExecutorKind::SysVariants(ExecutorKindSys::Overlapped { .. }) ), is_direct: self.direct, + lock: self.lock, depth: 0, })?) } diff --git a/devices/src/virtio/scsi/mod.rs b/devices/src/virtio/scsi/mod.rs index e0034bf010..8a81580f87 100644 --- a/devices/src/virtio/scsi/mod.rs +++ b/devices/src/virtio/scsi/mod.rs @@ -16,6 +16,9 @@ mod device; pub use device::Controller; pub use device::DiskConfig; +fn scsi_option_lock_default() -> bool { + true +} fn scsi_option_block_size_default() -> u32 { 512 } @@ -29,6 +32,9 @@ pub struct ScsiOption { // Indicates whether the device is ready only. #[serde(default, rename = "ro")] pub read_only: bool, + /// Whether to lock the disk files. Uses flock on Unix and FILE_SHARE_* flags on Windows. + #[serde(default = "scsi_option_lock_default")] + pub lock: bool, // The block size of the device. #[serde(default = "scsi_option_block_size_default")] pub block_size: u32, @@ -54,6 +60,7 @@ mod tests { ScsiOption { path: Path::new("/path/to/image").to_path_buf(), read_only: false, + lock: scsi_option_lock_default(), block_size: 512, root: false, } @@ -65,6 +72,7 @@ mod tests { ScsiOption { path: Path::new("/path/to/image").to_path_buf(), read_only: true, + lock: scsi_option_lock_default(), block_size: 512, root: false, } @@ -76,6 +84,7 @@ mod tests { ScsiOption { path: Path::new("/path/to/image").to_path_buf(), read_only: false, + lock: scsi_option_lock_default(), block_size: 1024, root: false, } @@ -88,6 +97,7 @@ mod tests { ScsiOption { path: Path::new("/path/to/image").to_path_buf(), read_only: false, + lock: scsi_option_lock_default(), block_size: 1024, root: true, } diff --git a/devices/src/virtio/scsi/sys/linux.rs b/devices/src/virtio/scsi/sys/linux.rs index cf2bdaac62..4211d2a186 100644 --- a/devices/src/virtio/scsi/sys/linux.rs +++ b/devices/src/virtio/scsi/sys/linux.rs @@ -16,6 +16,7 @@ impl ScsiOption { is_sparse_file: true, is_overlapped: false, is_direct: false, + lock: self.lock, depth: 0, }) .context("open_disk_file failed") diff --git a/disk/src/composite.rs b/disk/src/composite.rs index 4e007aefbf..ac64d8211b 100644 --- a/disk/src/composite.rs +++ b/disk/src/composite.rs @@ -231,6 +231,7 @@ impl CompositeDiskFile { // TODO: Should pass `params.is_overlapped` through here. Needs testing. is_overlapped: false, is_direct: params.is_direct, + lock: params.lock, depth: params.depth + 1, }) .map_err(|e| Error::DiskError(Box::new(e)))?, diff --git a/disk/src/disk.rs b/disk/src/disk.rs index 262309c7c0..324dcdc2db 100644 --- a/disk/src/disk.rs +++ b/disk/src/disk.rs @@ -270,6 +270,8 @@ pub struct DiskFileParams { pub is_overlapped: bool, // Whether to disable OS page caches / buffering. pub is_direct: bool, + // Whether to lock the file. + pub lock: bool, // The nesting depth of the file. Used to avoid infinite recursion. Users outside the disk // crate should set this to zero. pub depth: u32, diff --git a/disk/src/qcow/mod.rs b/disk/src/qcow/mod.rs index ae537fd5d3..1e5a0e2855 100644 --- a/disk/src/qcow/mod.rs +++ b/disk/src/qcow/mod.rs @@ -409,6 +409,7 @@ fn max_refcount_clusters(refcount_order: u32, cluster_size: u32, num_clusters: u /// is_sparse_file: false, /// is_overlapped: false, /// is_direct: false, +/// lock: true, /// depth: 0, /// }).expect("Can't open qcow file"); /// let mut buf = [0u8; 12]; @@ -486,6 +487,7 @@ impl QcowFile { // TODO: Should pass `params.is_overlapped` through here. Needs testing. is_overlapped: false, is_direct: params.is_direct, + lock: params.lock, depth: params.depth + 1, }) .map_err(|e| Error::BackingFileOpen(Box::new(e)))?; @@ -642,6 +644,7 @@ impl QcowFile { // TODO: Should pass `params.is_overlapped` through here. Needs testing. is_overlapped: false, is_direct: params.is_direct, + lock: params.lock, depth: params.depth + 1, }) .map_err(|e| Error::BackingFileOpen(Box::new(e)))?; @@ -1720,6 +1723,7 @@ mod tests { is_sparse_file: false, is_overlapped: false, is_direct: false, + lock: true, depth: 0, } } diff --git a/disk/src/sys/linux.rs b/disk/src/sys/linux.rs index 3ac31b5caa..42bcb999a7 100644 --- a/disk/src/sys/linux.rs +++ b/disk/src/sys/linux.rs @@ -22,13 +22,15 @@ pub fn open_raw_disk_image(params: &DiskFileParams) -> Result { let raw_image = base::open_file_or_duplicate(¶ms.path, &options) .map_err(|e| Error::OpenFile(params.path.display().to_string(), e))?; - // Lock the disk image to prevent other crosvm instances from using it. - let lock_op = if params.is_read_only { - base::FlockOperation::LockShared - } else { - base::FlockOperation::LockExclusive - }; - base::flock(&raw_image, lock_op, true).map_err(Error::LockFileFailure)?; + if params.lock { + // Lock the disk image to prevent other crosvm instances from using it. + let lock_op = if params.is_read_only { + base::FlockOperation::LockShared + } else { + base::FlockOperation::LockExclusive + }; + base::flock(&raw_image, lock_op, true).map_err(Error::LockFileFailure)?; + } // If O_DIRECT is requested, set the flag via fcntl. It is not done at // open_file_or_reuse time because it will reuse existing fd and will diff --git a/disk/src/sys/windows.rs b/disk/src/sys/windows.rs index c484f3b1bc..99f413d01f 100644 --- a/disk/src/sys/windows.rs +++ b/disk/src/sys/windows.rs @@ -32,7 +32,10 @@ impl SingleFileDisk { pub fn open_raw_disk_image(params: &DiskFileParams) -> Result { let mut options = File::options(); options.read(true).write(!params.is_read_only); - options.share_mode(FILE_SHARE_READ | FILE_SHARE_WRITE); + if params.lock { + // We only prevent file deletion and renaming right now. + options.share_mode(FILE_SHARE_READ | FILE_SHARE_WRITE); + } let mut flags = 0; if params.is_direct { diff --git a/fuzz/fuzz_targets/qcow_fuzzer.rs b/fuzz/fuzz_targets/qcow_fuzzer.rs index 6f9b9e98a7..e06af8212d 100644 --- a/fuzz/fuzz_targets/qcow_fuzzer.rs +++ b/fuzz/fuzz_targets/qcow_fuzzer.rs @@ -38,6 +38,7 @@ fuzz_target!(|bytes| { is_sparse_file: false, is_overlapped: false, is_direct: false, + lock: true, depth: 0, }, ) { diff --git a/src/main.rs b/src/main.rs index 4f65133f4a..7e4f2af88c 100644 --- a/src/main.rs +++ b/src/main.rs @@ -458,6 +458,7 @@ fn create_composite(cmd: cmdline::CreateCompositeCommand) -> std::result::Result is_sparse_file: true, is_overlapped: false, is_direct: false, + lock: true, depth: 0, }) .map_err(|e| error!("Failed to create DiskFile instance: {}", e))? @@ -522,6 +523,7 @@ fn create_qcow2(cmd: cmdline::CreateQcow2Command) -> std::result::Result<(), ()> is_sparse_file: false, is_overlapped: false, is_direct: false, + lock: true, depth: 0, }; match (cmd.size, cmd.backing_file) {