From 88041eaf561a3e238b7fd297ea1626ec09ecf2d1 Mon Sep 17 00:00:00 2001 From: Julian Schindel Date: Tue, 30 Jun 2026 17:27:51 +0200 Subject: [PATCH 1/8] vmm: remove `get_lock_state` function The function suffers from a TOCTOU problem. There is no guarantee that the locks set on the file at the time of the state check are the same as at the time of the locking attempt. On-behalf-of: SAP julian.schindel@sap.com Signed-off-by: Julian Schindel --- block/src/fcntl.rs | 57 +------------------------------------ virtio-devices/src/block.rs | 16 +++++------ 2 files changed, 8 insertions(+), 65 deletions(-) diff --git a/block/src/fcntl.rs b/block/src/fcntl.rs index 0cb2ee552b..e215c86818 100644 --- a/block/src/fcntl.rs +++ b/block/src/fcntl.rs @@ -24,8 +24,6 @@ use thiserror::Error; #[derive(Error, Debug)] pub enum LockError { /// The file is already locked. - /// - /// A call to [`get_lock_state`] can help to identify the reason. #[error("The file is already locked")] AlreadyLocked, /// IO error. @@ -38,6 +36,7 @@ pub enum LockError { enum FcntlArg<'a> { /// Set an OFD lock from the given lock description. F_OFD_SETLK(&'a libc::flock), + #[expect(unused, reason = "will be used in the following commits")] /// Get the first OFD lock for the given lock description. F_OFD_GETLK(&'a mut libc::flock), } @@ -74,34 +73,6 @@ impl LockType { } } -/// Describes the current state of a lock. -#[derive(Debug)] -pub enum LockState { - /// No lock set. - Unlocked, - /// Locked for reading (non-exclusive). - SharedRead, - /// Locked for writing (exclusive mode). - ExclusiveWrite, -} - -impl LockState { - fn new(value: libc::c_int) -> Self { - const F_UNLCK: libc::c_int = libc::F_UNLCK as libc::c_int; - const F_WRLCK: libc::c_int = libc::F_WRLCK as libc::c_int; - const F_RDLCK: libc::c_int = libc::F_RDLCK as libc::c_int; - match value { - F_UNLCK => Self::Unlocked, - F_WRLCK => Self::ExclusiveWrite, - F_RDLCK => Self::SharedRead, - // This is so unlikely that we want to avoid the complexity of - // coping with this error case. Can only fail if either Linux - // is broken or memory is messed up. - other => panic!("Unexpected lock state: {other}"), - } - } -} - /// The granularity of the advisory lock. /// /// The granularity has significant implications in typical cloud deployments @@ -229,29 +200,3 @@ pub fn try_acquire_lock( pub fn clear_lock(file: &Fd, granularity: LockGranularity) -> Result<(), LockError> { try_acquire_lock(file, LockType::Unlock, granularity) } - -/// Returns the current lock state using [`fcntl`] with respect to the given -/// parameters. -/// -/// # Parameters -/// - `file`: The file for which to get the lock state. -/// - `granularity`: The [`LockGranularity`]. -pub fn get_lock_state( - file: &Fd, - granularity: LockGranularity, -) -> Result { - let mut flock = get_flock(LockType::Write, granularity); - let res = fcntl(file.as_raw_fd(), FcntlArg::F_OFD_GETLK(&mut flock)); - match res { - 0 => { - let state = flock.l_type as libc::c_int; - let state = LockState::new(state); - Ok(state) - } - -1 => { - let io_error = io::Error::last_os_error(); - Err(LockError::Io(io_error)) - } - val => panic!("Unexpected return value from fcntl(): {val}"), - } -} diff --git a/virtio-devices/src/block.rs b/virtio-devices/src/block.rs index 261091e494..084514d462 100644 --- a/virtio-devices/src/block.rs +++ b/virtio-devices/src/block.rs @@ -23,7 +23,7 @@ use anyhow::anyhow; use block::async_io::{AsyncIo, AsyncIoError}; use block::disk_file::AsyncFullDiskFile; use block::error::BlockError; -use block::fcntl::{LockError, LockGranularity, LockGranularityChoice, LockType, get_lock_state}; +use block::fcntl::{LockError, LockGranularity, LockGranularityChoice, LockType}; use block::{ ExecuteAsync, ExecuteError, MAX_DISCARD_WRITE_ZEROES_SEG, Request, RequestType, VirtioBlockConfig, build_serial, fcntl, @@ -1018,14 +1018,12 @@ impl Block { ); let fd = self.disk_image.fd(); fcntl::try_acquire_lock(&fd, lock_type, granularity).map_err(|error| { - let current_lock = get_lock_state(&fd, granularity); - // Don't propagate the error to the outside, as it is not useful at all. Instead, - // we try to log additional help to the user. - if let Ok(current_lock) = current_lock { - error!("Can't get {lock_type:?} lock for {} as there is already a {current_lock:?} lock", self.disk_path.display()); - } else { - error!("Can't get {lock_type:?} lock for {}, but also can't determine the current lock state", self.disk_path.display()); - } + error!( + "Cannot acquire {lock_type:?} lock for disk image: id={},path={},granularity={granularity:?}", + self.id, + self.disk_path.display() + ); + Error::LockDiskImage { path: self.disk_path.clone(), error, From d1e04b72f215159917535946279b52946ef3f262 Mon Sep 17 00:00:00 2001 From: Julian Schindel Date: Tue, 30 Jun 2026 17:28:54 +0200 Subject: [PATCH 2/8] vmm: remove `get_` prefix Rust typically doesn't use `get_` prefixes. On-behalf-of: SAP julian.schindel@sap.com Signed-off-by: Julian Schindel --- block/src/fcntl.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/block/src/fcntl.rs b/block/src/fcntl.rs index e215c86818..993c64c825 100644 --- a/block/src/fcntl.rs +++ b/block/src/fcntl.rs @@ -144,7 +144,7 @@ impl FromStr for LockGranularityChoice { } /// Returns a [`struct@libc::flock`] structure for the whole file. -const fn get_flock(lock_type: LockType, granularity: LockGranularity) -> libc::flock { +const fn flock(lock_type: LockType, granularity: LockGranularity) -> libc::flock { libc::flock { l_type: lock_type.to_libc_val() as libc::c_short, l_whence: libc::SEEK_SET as libc::c_short, @@ -170,7 +170,7 @@ pub fn try_acquire_lock( lock_type: LockType, granularity: LockGranularity, ) -> Result<(), LockError> { - let flock = get_flock(lock_type, granularity); + let flock = flock(lock_type, granularity); loop { let res = fcntl(file.as_raw_fd(), FcntlArg::F_OFD_SETLK(&flock)); From f049994d746d1ceabd0b3dd31901213962b7fdff Mon Sep 17 00:00:00 2001 From: Julian Schindel Date: Tue, 30 Jun 2026 12:08:05 +0200 Subject: [PATCH 3/8] vmm: refactor locking functions to be methods of `LockGranularity` All functions take a `LockGranularity` already and making them part of `LockGranularity` makes it easier to add associated helpers. On-behalf-of: SAP julian.schindel@sap.com Signed-off-by: Julian Schindel --- block/src/fcntl.rs | 114 ++++++++++++++++++------------------ virtio-devices/src/block.rs | 40 +++++++------ 2 files changed, 78 insertions(+), 76 deletions(-) diff --git a/block/src/fcntl.rs b/block/src/fcntl.rs index 993c64c825..b7b27cd7c6 100644 --- a/block/src/fcntl.rs +++ b/block/src/fcntl.rs @@ -110,6 +110,62 @@ impl LockGranularity { LockGranularity::ByteRange(_, len) => len, } } + + /// Tries to acquire a lock using [`fcntl`] with respect to the given + /// parameters. + /// + /// Please note that `fcntl()` OFD locks are **advisory locks**, which do not + /// prevent to `open()` a file if a lock is already placed. + /// + /// # Parameters + /// - `file`: The file to acquire a lock for [`LockType`]. The file's state will + /// be logically mutated, but not technically. + /// - `lock_type`: The [`LockType`] + pub fn try_acquire_lock( + self, + file: &Fd, + lock_type: LockType, + ) -> Result<(), LockError> { + let flock = self.flock(lock_type); + + loop { + let res = fcntl(file.as_raw_fd(), FcntlArg::F_OFD_SETLK(&flock)); + match res { + 0 => return Ok(()), + -1 => { + let io_error = io::Error::last_os_error(); + let errno = io_error.raw_os_error().unwrap(); + match errno { + // See man page for error code: + // + libc::EAGAIN | libc::EACCES => return Err(LockError::AlreadyLocked), + libc::EINTR => continue, + _ => return Err(LockError::Io(io_error)), + } + } + val => panic!("Unexpected return value from fcntl(): {val}"), + } + } + } + + /// Clears a lock. + /// + /// # Parameters + /// - `file`: The file to clear all locks for [`LockType`]. + pub fn clear_lock(self, file: &Fd) -> Result<(), LockError> { + self.try_acquire_lock(file, LockType::Unlock) + } + + /// Returns a [`struct@libc::flock`] structure for the whole file. + const fn flock(self, lock_type: LockType) -> libc::flock { + libc::flock { + l_type: lock_type.to_libc_val() as libc::c_short, + l_whence: libc::SEEK_SET as libc::c_short, + l_start: self.l_start() as libc::c_long, + l_len: self.l_len() as libc::c_long, + l_pid: 0, /* filled by callee */ + } + } } /// User-facing choice for the lock granularity. @@ -142,61 +198,3 @@ impl FromStr for LockGranularityChoice { } } } - -/// Returns a [`struct@libc::flock`] structure for the whole file. -const fn flock(lock_type: LockType, granularity: LockGranularity) -> libc::flock { - libc::flock { - l_type: lock_type.to_libc_val() as libc::c_short, - l_whence: libc::SEEK_SET as libc::c_short, - l_start: granularity.l_start() as libc::c_long, - l_len: granularity.l_len() as libc::c_long, - l_pid: 0, /* filled by callee */ - } -} - -/// Tries to acquire a lock using [`fcntl`] with respect to the given -/// parameters. -/// -/// Please note that `fcntl()` OFD locks are **advisory locks**, which do not -/// prevent to `open()` a file if a lock is already placed. -/// -/// # Parameters -/// - `file`: The file to acquire a lock for [`LockType`]. The file's state will -/// be logically mutated, but not technically. -/// - `lock_type`: The [`LockType`] -/// - `granularity`: The [`LockGranularity`]. -pub fn try_acquire_lock( - file: &Fd, - lock_type: LockType, - granularity: LockGranularity, -) -> Result<(), LockError> { - let flock = flock(lock_type, granularity); - - loop { - let res = fcntl(file.as_raw_fd(), FcntlArg::F_OFD_SETLK(&flock)); - match res { - 0 => return Ok(()), - -1 => { - let io_error = io::Error::last_os_error(); - let errno = io_error.raw_os_error().unwrap(); - match errno { - // See man page for error code: - // - libc::EAGAIN | libc::EACCES => return Err(LockError::AlreadyLocked), - libc::EINTR => continue, - _ => return Err(LockError::Io(io_error)), - } - } - val => panic!("Unexpected return value from fcntl(): {val}"), - } - } -} - -/// Clears a lock. -/// -/// # Parameters -/// - `file`: The file to clear all locks for [`LockType`]. -/// - `granularity`: The [`LockGranularity`]. -pub fn clear_lock(file: &Fd, granularity: LockGranularity) -> Result<(), LockError> { - try_acquire_lock(file, LockType::Unlock, granularity) -} diff --git a/virtio-devices/src/block.rs b/virtio-devices/src/block.rs index 084514d462..525b7b5c77 100644 --- a/virtio-devices/src/block.rs +++ b/virtio-devices/src/block.rs @@ -26,7 +26,7 @@ use block::error::BlockError; use block::fcntl::{LockError, LockGranularity, LockGranularityChoice, LockType}; use block::{ ExecuteAsync, ExecuteError, MAX_DISCARD_WRITE_ZEROES_SEG, Request, RequestType, - VirtioBlockConfig, build_serial, fcntl, + VirtioBlockConfig, build_serial, }; use event_monitor::event; use log::{debug, error, info, warn}; @@ -1017,19 +1017,21 @@ impl Block { self.disk_path.display() ); let fd = self.disk_image.fd(); - fcntl::try_acquire_lock(&fd, lock_type, granularity).map_err(|error| { - error!( - "Cannot acquire {lock_type:?} lock for disk image: id={},path={},granularity={granularity:?}", - self.id, - self.disk_path.display() - ); + granularity + .try_acquire_lock(&fd, lock_type) + .map_err(|error| { + error!( + "Cannot acquire {lock_type:?} lock for disk image: id={},path={},granularity={granularity:?}", + self.id, + self.disk_path.display() + ); - Error::LockDiskImage { - path: self.disk_path.clone(), - error, - lock_type, - } - })?; + Error::LockDiskImage { + path: self.disk_path.clone(), + error, + lock_type, + } + })?; info!( "Acquired {lock_type:?} lock for disk image id={},path={}", self.id, @@ -1046,11 +1048,13 @@ impl Block { // Should we remove the Result to simplify the error propagation on // higher levels? let fd = self.disk_image.fd(); - fcntl::clear_lock(&fd, granularity).map_err(|error| Error::LockDiskImage { - path: self.disk_path.clone(), - error, - lock_type: LockType::Unlock, - }) + granularity + .clear_lock(&fd) + .map_err(|error| Error::LockDiskImage { + path: self.disk_path.clone(), + error, + lock_type: LockType::Unlock, + }) } fn state(&self) -> BlockState { From 1561018d19e37a17fc772beddbafa0be0dda6922 Mon Sep 17 00:00:00 2001 From: Julian Schindel Date: Tue, 30 Jun 2026 12:18:07 +0200 Subject: [PATCH 4/8] vmm: refactor `LockGranularity` for easier addition of variants By extracting the full file and file range locking, it's easier to add file lock handling that's not following this pattern. On-behalf-of: SAP julian.schindel@sap.com Signed-off-by: Julian Schindel --- block/src/fcntl.rs | 53 +++++++++++++++++++++++++++------------------- 1 file changed, 31 insertions(+), 22 deletions(-) diff --git a/block/src/fcntl.rs b/block/src/fcntl.rs index b7b27cd7c6..cae3e15598 100644 --- a/block/src/fcntl.rs +++ b/block/src/fcntl.rs @@ -97,13 +97,6 @@ pub enum LockGranularity { } impl LockGranularity { - const fn l_start(self) -> u64 { - match self { - LockGranularity::WholeFile => 0, - LockGranularity::ByteRange(start, _) => start, - } - } - const fn l_len(self) -> u64 { match self { LockGranularity::WholeFile => 0, /* EOF */ @@ -111,22 +104,15 @@ impl LockGranularity { } } - /// Tries to acquire a lock using [`fcntl`] with respect to the given - /// parameters. - /// - /// Please note that `fcntl()` OFD locks are **advisory locks**, which do not - /// prevent to `open()` a file if a lock is already placed. - /// - /// # Parameters - /// - `file`: The file to acquire a lock for [`LockType`]. The file's state will - /// be logically mutated, but not technically. - /// - `lock_type`: The [`LockType`] - pub fn try_acquire_lock( + /// Internal implementation of [`Self::try_acquire_lock`] for [`LockGranularity::WholeFile`] and + /// [`LockGranularity::ByteRange`]. + fn try_acquire_lock_file( self, file: &Fd, lock_type: LockType, + l_start: u64, ) -> Result<(), LockError> { - let flock = self.flock(lock_type); + let flock = self.flock(lock_type, l_start); loop { let res = fcntl(file.as_raw_fd(), FcntlArg::F_OFD_SETLK(&flock)); @@ -148,6 +134,29 @@ impl LockGranularity { } } + /// Tries to acquire a lock using [`fcntl`] with respect to the given + /// parameters. + /// + /// Please note that `fcntl()` OFD locks are **advisory locks**, which do not + /// prevent to `open()` a file if a lock is already placed. + /// + /// # Parameters + /// - `file`: The file to acquire a lock for [`LockType`]. The file's state will + /// be logically mutated, but not technically. + /// - `lock_type`: The [`LockType`] + pub fn try_acquire_lock( + self, + file: &Fd, + lock_type: LockType, + ) -> Result<(), LockError> { + match self { + LockGranularity::WholeFile => self.try_acquire_lock_file(file, lock_type, 0), + LockGranularity::ByteRange(start, _) => { + self.try_acquire_lock_file(file, lock_type, start) + } + } + } + /// Clears a lock. /// /// # Parameters @@ -156,12 +165,12 @@ impl LockGranularity { self.try_acquire_lock(file, LockType::Unlock) } - /// Returns a [`struct@libc::flock`] structure for the whole file. - const fn flock(self, lock_type: LockType) -> libc::flock { + /// Returns a [`struct@libc::flock`] structure. + const fn flock(self, lock_type: LockType, l_start: u64) -> libc::flock { libc::flock { l_type: lock_type.to_libc_val() as libc::c_short, l_whence: libc::SEEK_SET as libc::c_short, - l_start: self.l_start() as libc::c_long, + l_start: l_start as libc::c_long, l_len: self.l_len() as libc::c_long, l_pid: 0, /* filled by callee */ } From 3d2b9e9117473be7a3b73605a3d100581e5a1a45 Mon Sep 17 00:00:00 2001 From: Julian Schindel Date: Wed, 1 Jul 2026 09:13:59 +0200 Subject: [PATCH 5/8] vmm: move error handling into `fcntl` function Avoids repeating the retry/error logic when adding new users of the function. On-behalf-of: SAP julian.schindel@sap.com Signed-off-by: Julian Schindel --- block/src/fcntl.rs | 52 +++++++++++++++++++++++++--------------------- 1 file changed, 28 insertions(+), 24 deletions(-) diff --git a/block/src/fcntl.rs b/block/src/fcntl.rs index cae3e15598..2e3a6b55e9 100644 --- a/block/src/fcntl.rs +++ b/block/src/fcntl.rs @@ -42,12 +42,33 @@ enum FcntlArg<'a> { } /// Wrapper for [`libc::fcntl`] that properly sets the function arguments. -fn fcntl(fd: RawFd, arg: FcntlArg) -> libc::c_int { - // SAFETY: We use a valid FD. - unsafe { - match arg { - FcntlArg::F_OFD_SETLK(flock) => libc::fcntl(fd, libc::F_OFD_SETLK, flock), - FcntlArg::F_OFD_GETLK(flock) => libc::fcntl(fd, libc::F_OFD_GETLK, flock), +fn fcntl(fd: RawFd, mut arg: FcntlArg) -> Result<(), LockError> { + loop { + // SAFETY: We use a valid FD. + let result = unsafe { + match &mut arg { + FcntlArg::F_OFD_SETLK(flock) => { + libc::fcntl(fd, libc::F_OFD_SETLK, *flock as *const libc::flock) + } + FcntlArg::F_OFD_GETLK(flock) => { + libc::fcntl(fd, libc::F_OFD_GETLK, *flock as *mut libc::flock) + } + } + }; + match result { + 0 => return Ok(()), + -1 => { + let io_error = io::Error::last_os_error(); + let errno = io_error.raw_os_error().unwrap(); + match errno { + // See man page for error code: + // + libc::EAGAIN | libc::EACCES => return Err(LockError::AlreadyLocked), + libc::EINTR => continue, + _ => return Err(LockError::Io(io_error)), + } + } + val => panic!("Unexpected return value from fcntl(): {val}"), } } } @@ -114,24 +135,7 @@ impl LockGranularity { ) -> Result<(), LockError> { let flock = self.flock(lock_type, l_start); - loop { - let res = fcntl(file.as_raw_fd(), FcntlArg::F_OFD_SETLK(&flock)); - match res { - 0 => return Ok(()), - -1 => { - let io_error = io::Error::last_os_error(); - let errno = io_error.raw_os_error().unwrap(); - match errno { - // See man page for error code: - // - libc::EAGAIN | libc::EACCES => return Err(LockError::AlreadyLocked), - libc::EINTR => continue, - _ => return Err(LockError::Io(io_error)), - } - } - val => panic!("Unexpected return value from fcntl(): {val}"), - } - } + fcntl(file.as_raw_fd(), FcntlArg::F_OFD_SETLK(&flock)) } /// Tries to acquire a lock using [`fcntl`] with respect to the given From f4a7f4eb11af9098e3093e5570f20a1b3758bdbd Mon Sep 17 00:00:00 2001 From: Julian Schindel Date: Wed, 1 Jul 2026 09:14:47 +0200 Subject: [PATCH 6/8] vmm: fix safety comment for `fcntl` wrapper Reference to pointer coercion doesn't happen for variadic functions like `libc::fcntl`. While this doesn't pose a problem by itself, it removes the check whether a reference can be coerced to the appropriate pointer type or mutability. By explicitly casting to a pointer, we ensure refactors don't accidentally break the safety requirements. On-behalf-of: SAP julian.schindel@sap.com Signed-off-by: Julian Schindel --- block/src/fcntl.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/block/src/fcntl.rs b/block/src/fcntl.rs index 2e3a6b55e9..f571d50269 100644 --- a/block/src/fcntl.rs +++ b/block/src/fcntl.rs @@ -44,7 +44,10 @@ enum FcntlArg<'a> { /// Wrapper for [`libc::fcntl`] that properly sets the function arguments. fn fcntl(fd: RawFd, mut arg: FcntlArg) -> Result<(), LockError> { loop { - // SAFETY: We use a valid FD. + // SAFETY: + // - `F_OFD_SETLK` and `F_OFD_GETLK` fcntl calls handle invalid file descriptors. + // - `F_OFD_SETLK` does not modify `flock`. + // - `F_OFD_GETLK` uses a mutable pointer to `flock`. let result = unsafe { match &mut arg { FcntlArg::F_OFD_SETLK(flock) => { From c28667c421b73de1fe867834d1b28e1a8f772592 Mon Sep 17 00:00:00 2001 From: Julian Schindel Date: Tue, 30 Jun 2026 16:57:58 +0200 Subject: [PATCH 7/8] vmm: add option for QEMU compatible file locking QEMU uses locks on specific bytes for modeling file lock permissions. This commit only implements what's needed to model the permissions cloud-hypervisor currently uses. This is needed to be compatible with storage management systems that expect QEMU compatible file locking like NetApp's NFS implementation. QEMU uses two offsets from the start of the file to separate permissions and "unshared" permissions. "unshared" permissions are permissions that cannot be shared between lock holders [0] and can be considered exclusive. Starting from each offset, locks are placed with a length of one byte. Each corresponds to a specific permission [1][2]. The locking is done by first signaling intent by locking the required marker bytes. Next any conflicts with existing locks are detected and in the failure case, the locks are rolled back. Locking is done via `F_RDLCK`, which allows other parties to set the same locks and check for conflicts with `F_WRLCK`. [0]: https://github.com/qemu/qemu/blob/30e8a06b64aa58a3990ba39cb5d09531e7d265e0/block/file-posix.c#L131-L134 [1]: https://github.com/qemu/qemu/blob/30e8a06b64aa58a3990ba39cb5d09531e7d265e0/include/block/block-common.h#L392-L437 [2]: https://github.com/qemu/qemu/blob/30e8a06b64aa58a3990ba39cb5d09531e7d265e0/block/file-posix.c#L868-L940 On-behalf-of: SAP julian.schindel@sap.com Signed-off-by: Julian Schindel --- block/src/fcntl.rs | 167 +++++++++++++++++++++- docs/disk_locking.md | 11 ++ virtio-devices/src/block.rs | 12 +- vmm/src/api/openapi/cloud-hypervisor.yaml | 2 +- vmm/src/config.rs | 9 +- 5 files changed, 191 insertions(+), 10 deletions(-) diff --git a/block/src/fcntl.rs b/block/src/fcntl.rs index f571d50269..b55983354b 100644 --- a/block/src/fcntl.rs +++ b/block/src/fcntl.rs @@ -36,7 +36,6 @@ pub enum LockError { enum FcntlArg<'a> { /// Set an OFD lock from the given lock description. F_OFD_SETLK(&'a libc::flock), - #[expect(unused, reason = "will be used in the following commits")] /// Get the first OFD lock for the given lock description. F_OFD_GETLK(&'a mut libc::flock), } @@ -97,6 +96,25 @@ impl LockType { } } +/// Amount of bytes by which the first lock is offset from the start of the file. +const QEMU_LOCK_OFFSET: u64 = 100; +/// Amount of bytes by which the first unshared lock is offset from the start of the file. +/// +/// Unsharing is equivalent to marking lock as exclusive. +/// +/// # Example +/// +/// Setting `QEMU_LOCK_OFFSET` + `QEMU_READ_BYTE` indicates a reader lock that may be shared with +/// other readers. +/// Setting `QEMU_UNSHARE_LOCK_OFFSET` + `QEMU_READ_BYTE` additionally indicates, that the reader +/// lock is "unshared" (exclusive) and may not be shared with others. +const QEMU_UNSHARE_LOCK_OFFSET: u64 = 200; + +/// Read permission lock index for QEMU. +const QEMU_READ_BYTE: u64 = 0; +/// Write permission lock index for QEMU. +const QEMU_WRITE_BYTE: u64 = 1; + /// The granularity of the advisory lock. /// /// The granularity has significant implications in typical cloud deployments @@ -118,6 +136,7 @@ impl LockType { pub enum LockGranularity { WholeFile, ByteRange(u64 /* from, inclusive */, u64 /* len */), + QemuCompatible, } impl LockGranularity { @@ -125,6 +144,8 @@ impl LockGranularity { match self { LockGranularity::WholeFile => 0, /* EOF */ LockGranularity::ByteRange(_, len) => len, + // QEMU uses multiple one byte long locks. + LockGranularity::QemuCompatible => 1, } } @@ -136,11 +157,100 @@ impl LockGranularity { lock_type: LockType, l_start: u64, ) -> Result<(), LockError> { - let flock = self.flock(lock_type, l_start); + let flock = self.flock(lock_type.to_libc_val(), l_start); fcntl(file.as_raw_fd(), FcntlArg::F_OFD_SETLK(&flock)) } + /// Releases all locks not required for `lock_type`. + /// + /// Used to roll back a lock acquisition attempt to a previously acquired lock. + fn release_unneeded_locks_qemu( + self, + file: &Fd, + lock_type: LockType, + ) -> Result<(), LockError> { + let flocks = match lock_type { + LockType::Unlock => vec![ + LockGranularity::QemuCompatible + .flock(libc::F_UNLCK, QEMU_LOCK_OFFSET + QEMU_READ_BYTE), + LockGranularity::QemuCompatible + .flock(libc::F_UNLCK, QEMU_LOCK_OFFSET + QEMU_WRITE_BYTE), + LockGranularity::QemuCompatible + .flock(libc::F_UNLCK, QEMU_UNSHARE_LOCK_OFFSET + QEMU_READ_BYTE), + LockGranularity::QemuCompatible + .flock(libc::F_UNLCK, QEMU_UNSHARE_LOCK_OFFSET + QEMU_WRITE_BYTE), + ], + LockType::Write => vec![], + LockType::Read => vec![ + LockGranularity::QemuCompatible + .flock(libc::F_UNLCK, QEMU_LOCK_OFFSET + QEMU_WRITE_BYTE), + ], + }; + + let mut first_error = None; + for flock in flocks { + if let Err(error) = fcntl(file.as_raw_fd(), FcntlArg::F_OFD_SETLK(&flock)) { + first_error.get_or_insert(error); + } + } + if let Some(first_error) = first_error { + return Err(first_error); + } + Ok(()) + } + + /// Internal implementation of [`Self::try_acquire_lock`] for [`LockGranularity::QemuCompatible`]. + fn try_acquire_lock_qemu( + self, + file: &Fd, + lock_type: LockType, + current_lock_status: LockType, + ) -> Result<(), LockError> { + let flocks = match lock_type { + LockType::Unlock => vec![ + LockGranularity::QemuCompatible + .flock(libc::F_UNLCK, QEMU_LOCK_OFFSET + QEMU_READ_BYTE), + LockGranularity::QemuCompatible + .flock(libc::F_UNLCK, QEMU_LOCK_OFFSET + QEMU_WRITE_BYTE), + LockGranularity::QemuCompatible + .flock(libc::F_UNLCK, QEMU_UNSHARE_LOCK_OFFSET + QEMU_READ_BYTE), + LockGranularity::QemuCompatible + .flock(libc::F_UNLCK, QEMU_UNSHARE_LOCK_OFFSET + QEMU_WRITE_BYTE), + ], + LockType::Write => vec![ + LockGranularity::QemuCompatible + .flock(libc::F_RDLCK, QEMU_LOCK_OFFSET + QEMU_READ_BYTE), + LockGranularity::QemuCompatible + .flock(libc::F_RDLCK, QEMU_LOCK_OFFSET + QEMU_WRITE_BYTE), + LockGranularity::QemuCompatible + .flock(libc::F_RDLCK, QEMU_UNSHARE_LOCK_OFFSET + QEMU_WRITE_BYTE), + ], + LockType::Read => vec![ + LockGranularity::QemuCompatible + .flock(libc::F_RDLCK, QEMU_LOCK_OFFSET + QEMU_READ_BYTE), + LockGranularity::QemuCompatible + .flock(libc::F_RDLCK, QEMU_UNSHARE_LOCK_OFFSET + QEMU_WRITE_BYTE), + ], + }; + + for flock in flocks { + if let Err(error) = fcntl(file.as_raw_fd(), FcntlArg::F_OFD_SETLK(&flock)) { + if let LockType::Unlock = lock_type { + return Err(error); + } + let _ = self.release_unneeded_locks_qemu(file, current_lock_status); + return Err(error); + } + } + + if let Err(error) = self.check_lock_success_qemu(file, lock_type) { + let _ = self.release_unneeded_locks_qemu(file, current_lock_status); + return Err(error); + } + Ok(()) + } + /// Tries to acquire a lock using [`fcntl`] with respect to the given /// parameters. /// @@ -151,16 +261,22 @@ impl LockGranularity { /// - `file`: The file to acquire a lock for [`LockType`]. The file's state will /// be logically mutated, but not technically. /// - `lock_type`: The [`LockType`] + /// - `current_lock_status`: Already held locks on this `file`. + /// Used for [`LockGranularity::QemuCompatible`] to roll back to if locking fails. pub fn try_acquire_lock( self, file: &Fd, lock_type: LockType, + current_lock_status: LockType, ) -> Result<(), LockError> { match self { LockGranularity::WholeFile => self.try_acquire_lock_file(file, lock_type, 0), LockGranularity::ByteRange(start, _) => { self.try_acquire_lock_file(file, lock_type, start) } + LockGranularity::QemuCompatible => { + self.try_acquire_lock_qemu(file, lock_type, current_lock_status) + } } } @@ -169,13 +285,49 @@ impl LockGranularity { /// # Parameters /// - `file`: The file to clear all locks for [`LockType`]. pub fn clear_lock(self, file: &Fd) -> Result<(), LockError> { - self.try_acquire_lock(file, LockType::Unlock) + self.try_acquire_lock(file, LockType::Unlock, LockType::Unlock) + } + + /// Checks whether any conflicting locks are set. + /// + /// Returns an error if a conflicting lock is set. + fn check_lock_success_qemu( + &self, + file: &Fd, + lock_type: LockType, + ) -> Result<(), LockError> { + let flocks = match lock_type { + LockType::Unlock => vec![], + LockType::Write => vec![ + LockGranularity::QemuCompatible + .flock(libc::F_WRLCK, QEMU_UNSHARE_LOCK_OFFSET + QEMU_READ_BYTE), + LockGranularity::QemuCompatible + .flock(libc::F_WRLCK, QEMU_UNSHARE_LOCK_OFFSET + QEMU_WRITE_BYTE), + LockGranularity::QemuCompatible + .flock(libc::F_WRLCK, QEMU_LOCK_OFFSET + QEMU_WRITE_BYTE), + ], + LockType::Read => vec![ + LockGranularity::QemuCompatible + .flock(libc::F_WRLCK, QEMU_UNSHARE_LOCK_OFFSET + QEMU_READ_BYTE), + LockGranularity::QemuCompatible + .flock(libc::F_WRLCK, QEMU_LOCK_OFFSET + QEMU_WRITE_BYTE), + ], + }; + + for mut flock in flocks { + fcntl(file.as_raw_fd(), FcntlArg::F_OFD_GETLK(&mut flock))?; + + if flock.l_type as libc::c_int != libc::F_UNLCK { + return Err(LockError::AlreadyLocked); + } + } + Ok(()) } /// Returns a [`struct@libc::flock`] structure. - const fn flock(self, lock_type: LockType, l_start: u64) -> libc::flock { + const fn flock(self, lock_type: libc::c_int, l_start: u64) -> libc::flock { libc::flock { - l_type: lock_type.to_libc_val() as libc::c_short, + l_type: lock_type as libc::c_short, l_whence: libc::SEEK_SET as libc::c_short, l_start: l_start as libc::c_long, l_len: self.l_len() as libc::c_long, @@ -196,11 +348,13 @@ pub enum LockGranularityChoice { ByteRange, /// Whole-file lock (l_start=0, l_len=0) - original OFD whole-file lock behavior. Full, + /// Locking scheme that mimics QEMU's marker byte based locking scheme. + QemuCompatible, } /// Error returned when parsing a [`LockGranularityChoice`] from a string. #[derive(Error, Debug)] -#[error("Invalid lock granularity value: {0}, expected 'byte-range' or 'full'")] +#[error("Invalid lock granularity value: {0}, expected 'byte-range', 'full' or 'qemu-compatible'")] pub struct LockGranularityParseError(String); impl FromStr for LockGranularityChoice { @@ -210,6 +364,7 @@ impl FromStr for LockGranularityChoice { match s { "byte-range" => Ok(LockGranularityChoice::ByteRange), "full" => Ok(LockGranularityChoice::Full), + "qemu-compatible" => Ok(Self::QemuCompatible), _ => Err(LockGranularityParseError(s.to_owned())), } } diff --git a/docs/disk_locking.md b/docs/disk_locking.md index adb8956a08..546f5ffea7 100644 --- a/docs/disk_locking.md +++ b/docs/disk_locking.md @@ -24,10 +24,21 @@ The `lock_granularity` parameter controls how the lock is placed on the disk image: ``` +--disk path=/bar.img,lock_granularity=qemu-compatible --disk path=/foo.img,lock_granularity=byte-range --disk path=/bar.img,lock_granularity=full ``` +### `qemu-compatible` + +Mimics QEMU's file locking behavior. Only locks marker-bytes to express +QEMU's file locking semantics. + +For read-only disks, this translates to QEMU's `BLK_PERM_CONSISTENT_READ` +with `BLK_PERM_WRITE` unshared. +For read-write disks, this translates to QEMU's `BLK_PERM_CONSISTENT_READ` +and `BLK_PERM_WRITE` with `BLK_PERM_WRITE` unshared. + ### `byte-range` (default) Locks the byte range `[0, physical_file_size)`. The physical file size diff --git a/virtio-devices/src/block.rs b/virtio-devices/src/block.rs index 525b7b5c77..edb3650ab0 100644 --- a/virtio-devices/src/block.rs +++ b/virtio-devices/src/block.rs @@ -771,6 +771,9 @@ pub struct Block { queue_affinity: BTreeMap>, disable_sector0_writes: bool, lock_granularity_choice: LockGranularityChoice, + /// The current lock status. + // There is no way to query what locks are already held, so we need to cache that information. + held_lock: LockType, device_status: Arc, active_request_count: Arc, draining_active_requests: Arc, @@ -937,6 +940,7 @@ impl Block { queue_affinity, disable_sector0_writes, lock_granularity_choice: lock_granularity, + held_lock: LockType::Unlock, device_status: Arc::new(AtomicU8::new(0)), active_request_count: Arc::new(AtomicUsize::new(0)), draining_active_requests: Arc::new(AtomicBool::new(false)), @@ -1001,6 +1005,7 @@ impl Block { } } } + LockGranularityChoice::QemuCompatible => LockGranularity::QemuCompatible, } } @@ -1018,7 +1023,7 @@ impl Block { ); let fd = self.disk_image.fd(); granularity - .try_acquire_lock(&fd, lock_type) + .try_acquire_lock(&fd, lock_type, self.held_lock) .map_err(|error| { error!( "Cannot acquire {lock_type:?} lock for disk image: id={},path={},granularity={granularity:?}", @@ -1032,6 +1037,7 @@ impl Block { lock_type, } })?; + self.held_lock = lock_type; info!( "Acquired {lock_type:?} lock for disk image id={},path={}", self.id, @@ -1054,7 +1060,9 @@ impl Block { path: self.disk_path.clone(), error, lock_type: LockType::Unlock, - }) + })?; + self.held_lock = LockType::Unlock; + Ok(()) } fn state(&self) -> BlockState { diff --git a/vmm/src/api/openapi/cloud-hypervisor.yaml b/vmm/src/api/openapi/cloud-hypervisor.yaml index fd5ccec531..3786c3cc4b 100644 --- a/vmm/src/api/openapi/cloud-hypervisor.yaml +++ b/vmm/src/api/openapi/cloud-hypervisor.yaml @@ -983,7 +983,7 @@ components: LockGranularity: type: string - enum: [ByteRange, Full] + enum: [ByteRange, Full, QemuCompatible] default: ByteRange DiskConfig: diff --git a/vmm/src/config.rs b/vmm/src/config.rs index 29cf8de56f..c0ab9aaa6f 100644 --- a/vmm/src/config.rs +++ b/vmm/src/config.rs @@ -1380,7 +1380,7 @@ impl DiskConfig { rate_limit_group=,\ queue_affinity=,\ serial=,backing_files=on|off,sparse=on|off,\ - image_type=,lock_granularity=byte-range|full"; + image_type=,lock_granularity=byte-range|full|qemu-compatible"; pub fn parse(disk: &str) -> Result { let mut parser = OptionParser::new(); @@ -4222,6 +4222,13 @@ mod unit_tests { ..disk_fixture() } ); + assert_eq!( + DiskConfig::parse("path=/path/to_file,lock_granularity=qemu-compatible")?, + DiskConfig { + lock_granularity: LockGranularityChoice::QemuCompatible, + ..disk_fixture() + } + ); assert_eq!( DiskConfig::parse("path=/path/to_file,queue_affinity=[0@[1],1@[2],2@[3,4],3@[5-8]]")?, DiskConfig { From 28aab6cafd2f08756aad27e4469cd02e8dc1170b Mon Sep 17 00:00:00 2001 From: Julian Schindel Date: Wed, 1 Jul 2026 12:42:26 +0200 Subject: [PATCH 8/8] vmm: set `QemuCompatible` to the default `LockGranularityChoice` On-behalf-of: SAP julian.schindel@sap.com Signed-off-by: Julian Schindel --- block/src/fcntl.rs | 2 +- docs/disk_locking.md | 4 ++-- vmm/src/api/openapi/cloud-hypervisor.yaml | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/block/src/fcntl.rs b/block/src/fcntl.rs index b55983354b..f5cb626c00 100644 --- a/block/src/fcntl.rs +++ b/block/src/fcntl.rs @@ -344,11 +344,11 @@ impl LockGranularity { #[derive(Clone, Copy, Debug, Default, PartialEq, Eq, serde::Deserialize, serde::Serialize)] pub enum LockGranularityChoice { /// Byte-range lock covering [0, size). - #[default] ByteRange, /// Whole-file lock (l_start=0, l_len=0) - original OFD whole-file lock behavior. Full, /// Locking scheme that mimics QEMU's marker byte based locking scheme. + #[default] QemuCompatible, } diff --git a/docs/disk_locking.md b/docs/disk_locking.md index 546f5ffea7..e205031e3a 100644 --- a/docs/disk_locking.md +++ b/docs/disk_locking.md @@ -29,7 +29,7 @@ disk image: --disk path=/bar.img,lock_granularity=full ``` -### `qemu-compatible` +### `qemu-compatible` (default) Mimics QEMU's file locking behavior. Only locks marker-bytes to express QEMU's file locking semantics. @@ -39,7 +39,7 @@ with `BLK_PERM_WRITE` unshared. For read-write disks, this translates to QEMU's `BLK_PERM_CONSISTENT_READ` and `BLK_PERM_WRITE` with `BLK_PERM_WRITE` unshared. -### `byte-range` (default) +### `byte-range` Locks the byte range `[0, physical_file_size)`. The physical file size is evaluated once at startup; if the file grows after the lock is diff --git a/vmm/src/api/openapi/cloud-hypervisor.yaml b/vmm/src/api/openapi/cloud-hypervisor.yaml index 3786c3cc4b..ad612b9514 100644 --- a/vmm/src/api/openapi/cloud-hypervisor.yaml +++ b/vmm/src/api/openapi/cloud-hypervisor.yaml @@ -984,7 +984,7 @@ components: LockGranularity: type: string enum: [ByteRange, Full, QemuCompatible] - default: ByteRange + default: QemuCompatible DiskConfig: type: object