mirrors/crosvm
1
0
Fork 0
mirror of https://chromium.googlesource.com/crosvm/crosvm synced 2024-10-22 20:36:34 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions

disk: add option to disable file locking

Browse source 
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 <dverkamp@chromium.org>
Commit-Queue: Frederick Mayle <fmayle@google.com>
This commit is contained in:
Frederick Mayle 2024-09-10 11:42:07 -07:00 committed by crosvm LUCI
parent 044fe65c71
commit 5921c375cc
12 changed files with 110 additions and 8 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

74
devices/src/virtio/block/mod.rs
View file

@ -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)]

1
devices/src/virtio/block/sys/linux.rs
View file

@ -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")

1
devices/src/virtio/block/sys/windows.rs
View file

@ -28,6 +28,7 @@ impl DiskOption {
ExecutorKind::SysVariants(ExecutorKindSys::Overlapped { .. })
),
is_direct: self.direct,
lock: self.lock,
depth: 0,
})?)
}

10
devices/src/virtio/scsi/mod.rs
View file

@ -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,
}

1
devices/src/virtio/scsi/sys/linux.rs
View file

@ -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")

1
disk/src/composite.rs
View file

@ -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)))?,

2
disk/src/disk.rs
View file

@ -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,

4
disk/src/qcow/mod.rs
View file

@ -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,
}
}

16
disk/src/sys/linux.rs
View file

@ -22,13 +22,15 @@ pub fn open_raw_disk_image(params: &DiskFileParams) -> Result<File> {
let raw_image = base::open_file_or_duplicate(&params.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

5
disk/src/sys/windows.rs
View file

@ -32,7 +32,10 @@ impl SingleFileDisk {
pub fn open_raw_disk_image(params: &DiskFileParams) -> Result<File> {
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 {

1
fuzz/fuzz_targets/qcow_fuzzer.rs
View file

@ -38,6 +38,7 @@ fuzz_target!(|bytes| {
is_sparse_file: false,
is_overlapped: false,
is_direct: false,
lock: true,
depth: 0,
},
) {

2
src/main.rs
View file

@ -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) {