From dc9bb53de131460bdf32eda5b6fc42d83e874f5b Mon Sep 17 00:00:00 2001 From: Daman Mulye Date: Wed, 24 Jun 2026 09:23:26 -0700 Subject: [PATCH 01/21] . --- vm/devices/support/fs/fuse/src/protocol.rs | 16 +- vm/devices/virtio/virtio_resources/src/lib.rs | 16 + vm/devices/virtio/virtiofs/src/inode.rs | 35 +- vm/devices/virtio/virtiofs/src/lib.rs | 451 +++++++++++++++++- vm/devices/virtio/virtiofs/src/resolver.rs | 14 + vm/devices/virtio/virtiofs/src/section.rs | 2 +- vm/devices/virtio/virtiofs/src/util.rs | 2 +- 7 files changed, 499 insertions(+), 37 deletions(-) diff --git a/vm/devices/support/fs/fuse/src/protocol.rs b/vm/devices/support/fs/fuse/src/protocol.rs index 636f926539..22d6578dc5 100644 --- a/vm/devices/support/fs/fuse/src/protocol.rs +++ b/vm/devices/support/fs/fuse/src/protocol.rs @@ -65,9 +65,23 @@ pub struct fuse_attr { pub gid: u32, pub rdev: u32, pub blksize: u32, - pub padding: u32, + /// Attribute flags (`FUSE_ATTR_*`). Named `padding` in older kernels; valid + /// for `FUSE_ATTR_SUBMOUNT` only once the `FUSE_SUBMOUNTS` capability has + /// been negotiated at init time. + pub flags: u32, } +/* + * Flags for `fuse_attr.flags` + * + * FUSE_ATTR_SUBMOUNT: object is a submount root; the kernel auto-mounts a + * distinct submount (with its own st_dev) at this location. Requires the + * `FUSE_SUBMOUNTS` capability to have been negotiated at init. + * FUSE_ATTR_DAX: enable per-inode DAX for this object. + */ +pub const FUSE_ATTR_SUBMOUNT: u32 = 1 << 0; +pub const FUSE_ATTR_DAX: u32 = 1 << 1; + #[repr(C)] #[derive(Debug, IntoBytes, Immutable, KnownLayout, FromBytes)] pub struct fuse_sx_time { diff --git a/vm/devices/virtio/virtio_resources/src/lib.rs b/vm/devices/virtio/virtio_resources/src/lib.rs index e6338c1662..95368411df 100644 --- a/vm/devices/virtio/virtio_resources/src/lib.rs +++ b/vm/devices/virtio/virtio_resources/src/lib.rs @@ -60,6 +60,22 @@ pub mod fs { SectionFs { root_path: String, }, + /// Expose multiple host folders behind a single device, each as a named + /// child of a synthetic root. Lets one virtio-fs device (one tag, one + /// PCI/MMIO footprint) serve many shares. + Aggregate { + roots: Vec, + }, + } + + /// A single host folder exposed as a named child of a [`VirtioFsBackend::Aggregate`]. + #[derive(MeshPayload)] + pub struct VirtioFsAggregateRoot { + /// Synthetic top-level directory name; the guest bind-mounts + /// `/` onto the user's target path. + pub name: String, + pub root_path: String, + pub mount_options: String, } impl ResourceId for VirtioFsHandle { diff --git a/vm/devices/virtio/virtiofs/src/inode.rs b/vm/devices/virtio/virtiofs/src/inode.rs index f1e03e8c37..5831123a61 100644 --- a/vm/devices/virtio/virtiofs/src/inode.rs +++ b/vm/devices/virtio/virtiofs/src/inode.rs @@ -18,6 +18,10 @@ use std::sync::atomic::Ordering; /// Implements inode callbacks for virtio-fs. pub struct VirtioFsInode { volume: Arc, + /// Identifies which aggregated volume this inode belongs to. Inode numbers + /// are only unique within a volume, so this is needed to key the stable + /// inode-number map when a single file system exposes multiple roots. + volume_id: u32, path: RwLock, lookup_count: AtomicU64, inode_nr: lx::ino_t, @@ -25,16 +29,26 @@ pub struct VirtioFsInode { impl VirtioFsInode { /// Create a new inode for the specified path. - pub fn new(volume: Arc, path: PathBuf) -> lx::Result<(Self, lx::Stat)> { + pub fn new( + volume: Arc, + volume_id: u32, + path: PathBuf, + ) -> lx::Result<(Self, lx::Stat)> { let stat = volume.lstat(&path)?; - let inode = Self::with_attr(volume, path, &stat); + let inode = Self::with_attr(volume, volume_id, path, &stat); Ok((inode, stat)) } /// Create a new inode for the specified path, with previously retrieved attributes. - pub fn with_attr(volume: Arc, path: PathBuf, stat: &lx::Stat) -> Self { + pub fn with_attr( + volume: Arc, + volume_id: u32, + path: PathBuf, + stat: &lx::Stat, + ) -> Self { Self { volume, + volume_id, path: RwLock::new(path), lookup_count: AtomicU64::new(1), inode_nr: stat.inode_nr, @@ -48,6 +62,11 @@ impl VirtioFsInode { self.inode_nr } + /// Return the identifier of the aggregated volume this inode belongs to. + pub fn volume_id(&self) -> u32 { + self.volume_id + } + /// Increments the lookup count. pub fn lookup(&self, new_path: PathBuf) { self.lookup_count.fetch_add(1, Ordering::AcqRel); @@ -89,7 +108,7 @@ impl VirtioFsInode { /// Performs a lookup for a child of this inode. pub fn lookup_child(&self, name: &LxStr) -> lx::Result<(VirtioFsInode, fuse_attr)> { let path = self.child_path(name)?; - let (inode, stat) = VirtioFsInode::new(Arc::clone(&self.volume), path)?; + let (inode, stat) = VirtioFsInode::new(Arc::clone(&self.volume), self.volume_id, path)?; let attr = util::stat_to_fuse_attr(&stat); Ok((inode, attr)) } @@ -138,7 +157,7 @@ impl VirtioFsInode { let flags = (flags as i32) | lx::O_CREAT | lx::O_NOFOLLOW; let file = self.volume.open(&path, flags, Some(options))?; let stat = file.fstat()?.into(); - let inode = Self::with_attr(Arc::clone(&self.volume), path, &stat); + let inode = Self::with_attr(Arc::clone(&self.volume), self.volume_id, path, &stat); let attr = util::stat_to_fuse_attr(&stat); Ok((inode, attr, file)) } @@ -156,7 +175,7 @@ impl VirtioFsInode { .volume .mkdir_stat(&path, LxCreateOptions::new(mode, uid, gid))?; - let inode = Self::with_attr(Arc::clone(&self.volume), path, &stat); + let inode = Self::with_attr(Arc::clone(&self.volume), self.volume_id, path, &stat); let attr = util::stat_to_fuse_attr(&stat); Ok((inode, attr)) } @@ -177,7 +196,7 @@ impl VirtioFsInode { device_id as usize, )?; - let inode = Self::with_attr(Arc::clone(&self.volume), path, &stat); + let inode = Self::with_attr(Arc::clone(&self.volume), self.volume_id, path, &stat); let attr = util::stat_to_fuse_attr(&stat); Ok((inode, attr)) } @@ -197,7 +216,7 @@ impl VirtioFsInode { LxCreateOptions::new(lx::S_IFLNK | 0o777, uid, gid), )?; - let inode = Self::with_attr(Arc::clone(&self.volume), path, &stat); + let inode = Self::with_attr(Arc::clone(&self.volume), self.volume_id, path, &stat); let attr = util::stat_to_fuse_attr(&stat); Ok((inode, attr)) } diff --git a/vm/devices/virtio/virtiofs/src/lib.rs b/vm/devices/virtio/virtiofs/src/lib.rs index 10f23a142a..aaac159506 100644 --- a/vm/devices/virtio/virtiofs/src/lib.rs +++ b/vm/devices/virtio/virtiofs/src/lib.rs @@ -30,6 +30,7 @@ use std::path::Path; use std::path::PathBuf; use std::sync::Arc; use std::time::Duration; +use zerocopy::FromZeros; // TODO: Make these configurable. // FUSE likes to spam getattr a lot, so having a small timeout on the attributes avoids excessive @@ -42,11 +43,69 @@ const ATTRIBUTE_TIMEOUT: Duration = Duration::from_millis(1); // update the path. const ENTRY_TIMEOUT: Duration = Duration::from_secs(0); -/// Implementation of the virtio-fs file system. -pub struct VirtioFs { +/// Reserved file handle returned by `open_dir` on the synthetic aggregate root. +/// +/// `read_dir`/`read_dir_plus`/`release_dir` recognize this sentinel and service +/// it from the root registry rather than the (real-file) handle map. `u64::MAX` +/// can never collide with a real handle because `HandleMap` allocates starting +/// at 1 and only increments. +const SYNTHETIC_ROOT_FH: u64 = u64::MAX; + +/// Linux `DT_DIR` directory-entry type, used for the synthetic root's children. +const DT_DIR: u32 = 4; + +/// A single host folder exposed as a named child of the synthetic aggregate root. +struct RootEntry { + /// Name of the synthetic top-level directory. Chosen by the caller; the guest + /// bind-mounts `/` onto the user's target path. + name: String, + volume: Arc, + /// Stable identifier disambiguating per-volume inode numbers. + volume_id: u32, +} + +/// Registry of aggregated roots for an aggregate-mode [`VirtioFs`]. +struct RootRegistry { + entries: Vec, + next_volume_id: u32, +} + +impl RootRegistry { + fn new() -> Self { + // Volume id 0 is reserved for a direct-mode single root, so aggregated + // roots start at 1. + Self { + entries: Vec::new(), + next_volume_id: 1, + } + } +} + +/// Shared mutable state behind a [`VirtioFs`] handle. +struct VirtioFsInner { inodes: RwLock, files: RwLock>>, + /// Aggregated roots; empty and unused in direct (single-root) mode. + roots: RwLock, readonly: bool, + /// When true, node 1 is a synthetic directory whose children are the entries + /// in `roots`. When false, node 1 is a real inode at a single volume root + /// (legacy single-share behavior). + aggregate: bool, + /// When true, child roots are advertised with `FUSE_ATTR_SUBMOUNT` so the + /// guest kernel gives each share its own `st_dev`. Only meaningful in + /// aggregate mode and only honored once `FUSE_SUBMOUNTS` is negotiated. + submounts: bool, +} + +/// Implementation of the virtio-fs file system. +/// +/// Cheaply clonable: all state lives behind an `Arc`, so a device host can keep +/// a handle to call [`VirtioFs::add_root`]/[`VirtioFs::remove_root`] after the +/// device has been constructed and handed to a [`fuse::Session`]. +#[derive(Clone)] +pub struct VirtioFs { + inner: Arc, } impl Fuse for VirtioFs { @@ -68,6 +127,12 @@ impl Fuse for VirtioFs { if info.capable2() & FUSE_DIRECT_IO_ALLOW_MMAP_FLAG2 != 0 { info.want2 |= FUSE_DIRECT_IO_ALLOW_MMAP_FLAG2; } + + // In aggregate mode, advertise submounts so the guest gives each child + // root its own st_dev when crossing into it. + if self.inner.submounts && info.capable() & FUSE_SUBMOUNTS != 0 { + info.want |= FUSE_SUBMOUNTS; + } } fn get_attr(&self, request: &Request, flags: u32, fh: u64) -> lx::Result { @@ -77,6 +142,8 @@ impl Fuse for VirtioFs { let attr = if flags & FUSE_GETATTR_FH != 0 { let file = self.get_file(fh)?; file.get_attr()? + } else if self.is_synthetic_root(node_id) { + Self::synthetic_root_attr() } else { let inode = self.get_inode(node_id)?; inode.get_attr()? @@ -91,7 +158,7 @@ impl Fuse for VirtioFs { fh: u64, getattr_flags: u32, flags: StatxFlags, - _mask: lx::StatExMask, + mask: lx::StatExMask, ) -> lx::Result { let node_id = request.node_id(); // If a file handle is specified, get the attributes from the open file. This is faster on @@ -99,6 +166,8 @@ impl Fuse for VirtioFs { let statx = if getattr_flags & FUSE_GETATTR_FH != 0 { let file = self.get_file(fh)?; file.get_statx()? + } else if self.is_synthetic_root(node_id) { + Self::synthetic_root_statx(mask) } else { let inode = self.get_inode(node_id)?; inode.get_statx()? @@ -133,6 +202,9 @@ impl Fuse for VirtioFs { } fn lookup(&self, request: &Request, name: &lx::LxStr) -> lx::Result { + if self.is_synthetic_root(request.node_id()) { + return self.lookup_synthetic_root(name); + } let inode = self.get_inode(request.node_id())?; self.lookup_helper(&inode, name) } @@ -140,7 +212,7 @@ impl Fuse for VirtioFs { fn forget(&self, node_id: u64, lookup_count: u64) { // This must be done under lock so an inode can't be resurrected between the lookup count // reaching zero and removing it from the list. - let mut inodes = self.inodes.write(); + let mut inodes = self.inner.inodes.write(); if let Some(inode) = inodes.get(node_id) { if inode.forget(node_id, lookup_count) == 0 { tracing::trace!(node_id, "Removing inode"); @@ -283,21 +355,35 @@ impl Fuse for VirtioFs { } fn open_dir(&self, request: &Request, flags: u32) -> lx::Result { + if self.is_synthetic_root(request.node_id()) { + // The synthetic root has no backing handle; hand out a sentinel that + // read_dir/read_dir_plus/release_dir recognize. + return Ok(fuse_open_out::new(SYNTHETIC_ROOT_FH, 0)); + } // There is no special handling for directories, so just call open. self.open(request, flags) } fn read_dir(&self, _request: &Request, arg: &fuse_read_in) -> lx::Result> { + if arg.fh == SYNTHETIC_ROOT_FH { + return self.read_synthetic_root_dir(arg.offset, arg.size, false); + } let file = self.get_file(arg.fh)?; file.read_dir(self, arg.offset, arg.size, false) } fn read_dir_plus(&self, _request: &Request, arg: &fuse_read_in) -> lx::Result> { + if arg.fh == SYNTHETIC_ROOT_FH { + return self.read_synthetic_root_dir(arg.offset, arg.size, true); + } let file = self.get_file(arg.fh)?; file.read_dir(self, arg.offset, arg.size, true) } fn release_dir(&self, request: &Request, arg: &fuse_release_in) -> lx::Result<()> { + if arg.fh == SYNTHETIC_ROOT_FH { + return Ok(()); + } self.release(request, arg) } @@ -319,11 +405,18 @@ impl Fuse for VirtioFs { ) -> lx::Result<()> { let inode = self.get_inode(request.node_id())?; let new_inode = self.get_inode(new_dir)?; + // A rename cannot cross aggregated volume boundaries. + if inode.volume_id() != new_inode.volume_id() { + return Err(lx::Error::EXDEV); + } self.check_writable()?; inode.rename(name, &new_inode, new_name, flags) } fn statfs(&self, request: &Request) -> lx::Result { + if self.is_synthetic_root(request.node_id()) { + return Ok(fuse_kstatfs::new(0, 0, 0, 0, 0, 512, 255, 512)); + } let inode = self.get_inode(request.node_id())?; inode.stat_fs() } @@ -388,15 +481,15 @@ impl Fuse for VirtioFs { fn destroy(&self) { // To get the file system ready for re-mount, clean out any open files and leaked inodes. - self.files.write().clear(); - self.inodes.write().clear(); + self.inner.files.write().clear(); + self.inner.inodes.write().clear(); } } impl VirtioFs { /// Check if the filesystem is readonly and return EROFS if so. fn check_writable(&self) -> lx::Result<()> { - if self.readonly { + if self.inner.readonly { Err(lx::Error::EROFS) } else { Ok(()) @@ -405,7 +498,7 @@ impl VirtioFs { /// Check whether the open flags are permitted on a read-only filesystem. fn check_open_readonly(&self, inode: &VirtioFsInode, flags: u32) -> lx::Result<()> { - if !self.readonly { + if !self.inner.readonly { return Ok(()); } @@ -445,16 +538,239 @@ impl VirtioFs { } else { lxutil::LxVolume::new(root_path) }?; - let mut inodes = InodeMap::new(volume.supports_stable_file_id()); - let (root_inode, _) = VirtioFsInode::new(Arc::new(volume), PathBuf::new())?; + let mut inodes = InodeMap::new(volume.supports_stable_file_id(), false); + let (root_inode, _) = VirtioFsInode::new(Arc::new(volume), 0, PathBuf::new())?; assert!(inodes.insert(root_inode).1 == FUSE_ROOT_ID); Ok(Self { - inodes: RwLock::new(inodes), - files: RwLock::new(HandleMap::new()), - readonly, + inner: Arc::new(VirtioFsInner { + inodes: RwLock::new(inodes), + files: RwLock::new(HandleMap::new()), + roots: RwLock::new(RootRegistry::new()), + readonly, + aggregate: false, + submounts: false, + }), }) } + /// Create a new, empty aggregate virtio-fs. + /// + /// Node 1 is a synthetic, read-only directory; use [`Self::add_root`] to + /// expose host folders as named children. When `submounts` is set, each + /// child is advertised with `FUSE_ATTR_SUBMOUNT` so the guest kernel gives + /// it a distinct `st_dev` (only honored once `FUSE_SUBMOUNTS` is negotiated). + pub fn new_aggregate(readonly: bool, submounts: bool) -> Self { + Self { + inner: Arc::new(VirtioFsInner { + // Inode numbers are deduplicated per volume (see `InodeMap`), so + // enable the stable-id map and key it by `(volume_id, ino)`. + inodes: RwLock::new(InodeMap::new(true, true)), + files: RwLock::new(HandleMap::new()), + roots: RwLock::new(RootRegistry::new()), + readonly, + aggregate: true, + submounts, + }), + } + } + + /// Expose a host folder as a named child of the synthetic root. + /// + /// Only valid in aggregate mode. Returns `EEXIST` if a root with the same + /// name already exists, or `EINVAL` on a direct-mode file system. + pub fn add_root( + &self, + name: &str, + root_path: impl AsRef, + mount_options: Option<&LxVolumeOptions>, + ) -> lx::Result<()> { + if !self.inner.aggregate { + return Err(lx::Error::EINVAL); + } + + let mut roots = self.inner.roots.write(); + if roots.entries.iter().any(|e| e.name == name) { + return Err(lx::Error::EEXIST); + } + + let volume = if let Some(mount_options) = mount_options { + mount_options.new_volume(root_path) + } else { + lxutil::LxVolume::new(root_path) + }?; + let volume_id = roots.next_volume_id; + roots.next_volume_id += 1; + roots.entries.push(RootEntry { + name: name.to_string(), + volume: Arc::new(volume), + volume_id, + }); + Ok(()) + } + + /// Remove a previously added root by name. + /// + /// In-flight inodes beneath the root remain valid until the guest forgets + /// them (each holds its own volume reference); the name simply stops + /// appearing in the synthetic root. Returns `ENOENT` if no such root exists. + pub fn remove_root(&self, name: &str) -> lx::Result<()> { + if !self.inner.aggregate { + return Err(lx::Error::EINVAL); + } + + let mut roots = self.inner.roots.write(); + let before = roots.entries.len(); + roots.entries.retain(|e| e.name != name); + if roots.entries.len() == before { + Err(lx::Error::ENOENT) + } else { + Ok(()) + } + } + + /// Returns true if `node_id` refers to the synthetic aggregate root. + fn is_synthetic_root(&self, node_id: u64) -> bool { + self.inner.aggregate && node_id == FUSE_ROOT_ID + } + + /// Attributes of the synthetic aggregate root directory. + fn synthetic_root_attr() -> fuse_attr { + let mut attr = fuse_attr::new_zeroed(); + attr.ino = FUSE_ROOT_ID; + attr.mode = lx::S_IFDIR | 0o555; + attr.nlink = 2; + attr.blksize = 512; + attr + } + + /// Extended attributes of the synthetic aggregate root directory. + fn synthetic_root_statx(mask: lx::StatExMask) -> fuse_statx { + let mut sx = fuse_statx::new_zeroed(); + sx.mask = mask.into_bits(); + sx.mode = (lx::S_IFDIR | 0o555) as u16; + sx.nlink = 2; + sx.ino = FUSE_ROOT_ID; + sx.blksize = 512; + sx + } + + /// Looks up a named child of the synthetic root, returning an entry for the + /// corresponding volume's real root inode. + fn lookup_synthetic_root(&self, name: &lx::LxStr) -> lx::Result { + let name_bytes = name.as_bytes(); + let (volume, volume_id) = { + let roots = self.inner.roots.read(); + let entry = roots + .entries + .iter() + .find(|e| e.name.as_bytes() == name_bytes) + .ok_or(lx::Error::ENOENT)?; + (Arc::clone(&entry.volume), entry.volume_id) + }; + + let (inode, stat) = VirtioFsInode::new(volume, volume_id, PathBuf::new())?; + let (_, node_id) = self.insert_inode(inode); + let mut attr = util::stat_to_fuse_attr(&stat); + if self.inner.submounts { + attr.flags |= FUSE_ATTR_SUBMOUNT; + } + Ok(fuse_entry_out::new( + node_id, + ENTRY_TIMEOUT, + ATTRIBUTE_TIMEOUT, + attr, + )) + } + + /// Reads the synthetic root directory, listing `.`, `..`, and each root. + fn read_synthetic_root_dir(&self, offset: u64, size: u32, plus: bool) -> lx::Result> { + let mut buffer = Vec::with_capacity(size as usize); + // `offset` is the cookie of the next entry to emit (0 at start of stream). + // Entry 0 => ".", 1 => "..", 2.. => roots[index - 2]. + let mut index = offset; + loop { + let next = index + 1; + let fit = match index { + 0 => self.write_synthetic_dot(&mut buffer, ".", next, plus), + 1 => self.write_synthetic_dot(&mut buffer, "..", next, plus), + n => { + let root = { + let roots = self.inner.roots.read(); + roots.entries.get((n - 2) as usize).map(|e| { + (e.name.clone(), Arc::clone(&e.volume), e.volume_id) + }) + }; + let Some((name, volume, volume_id)) = root else { + break; + }; + self.write_root_entry(&mut buffer, &name, volume, volume_id, next, plus)? + } + }; + if !fit { + break; + } + index += 1; + } + Ok(buffer) + } + + /// Writes a synthetic `.`/`..` entry. These never carry a real node ID, so + /// the kernel will not issue a forget for them. + fn write_synthetic_dot( + &self, + buffer: &mut Vec, + name: &str, + next_off: u64, + plus: bool, + ) -> bool { + if plus { + if !buffer.check_dir_entry_plus(name) { + return false; + } + let mut entry = fuse_entry_out::new_zeroed(); + entry.attr.ino = FUSE_ROOT_ID; + entry.attr.mode = lx::S_IFDIR | 0o555; + buffer.dir_entry_plus(name, next_off, entry) + } else { + buffer.dir_entry(name, FUSE_ROOT_ID, next_off, DT_DIR) + } + } + + /// Writes a directory entry for an aggregated root. + fn write_root_entry( + &self, + buffer: &mut Vec, + name: &str, + volume: Arc, + volume_id: u32, + next_off: u64, + plus: bool, + ) -> lx::Result { + if plus { + if !buffer.check_dir_entry_plus(name) { + return Ok(false); + } + // readdirplus performs a lookup on each entry, incrementing its + // lookup count, so create/insert the root inode here. + let (inode, stat) = VirtioFsInode::new(volume, volume_id, PathBuf::new())?; + let (_, node_id) = self.insert_inode(inode); + let mut attr = util::stat_to_fuse_attr(&stat); + if self.inner.submounts { + attr.flags |= FUSE_ATTR_SUBMOUNT; + } + let entry = fuse_entry_out::new(node_id, ENTRY_TIMEOUT, ATTRIBUTE_TIMEOUT, attr); + Ok(buffer.dir_entry_plus(name, next_off, entry)) + } else { + // Plain readdir: report the directory using the volume root's real + // inode number, falling back to the volume id if it is inaccessible. + let ino = volume + .lstat(&PathBuf::new()) + .map(|s| s.inode_nr) + .unwrap_or(volume_id as lx::ino_t); + Ok(buffer.dir_entry(name, ino, next_off, DT_DIR)) + } + } + /// Perform lookup on a specified directory inode. fn lookup_helper(&self, inode: &VirtioFsInode, name: &lx::LxStr) -> lx::Result { let (new_inode, attr) = inode.lookup_child(name)?; @@ -476,7 +792,7 @@ impl VirtioFs { /// Retrieve the inode with the specified node ID. fn get_inode(&self, node_id: u64) -> lx::Result> { - self.inodes.read().get(node_id).ok_or_else(|| { + self.inner.inodes.read().get(node_id).ok_or_else(|| { tracing::warn!(node_id, "request for unknown inode"); lx::Error::EINVAL }) @@ -487,12 +803,12 @@ impl VirtioFs { /// If the file system supports stable inode numbers and an inode already existed with this /// number, the existing inode is returned, not the passed in one. fn insert_inode(&self, inode: VirtioFsInode) -> (Arc, u64) { - self.inodes.write().insert(inode) + self.inner.inodes.write().insert(inode) } /// Retrieve the file object with the specified file handle. fn get_file(&self, fh: u64) -> lx::Result> { - let files = self.files.read(); + let files = self.inner.files.read(); let file = files.get(fh).ok_or_else(|| { tracing::warn!(fh, "Request for unknown file"); lx::Error::EBADF @@ -503,12 +819,12 @@ impl VirtioFs { /// Insert a new file object, and return the assigned file handle. fn insert_file(&self, file: VirtioFsFile) -> u64 { - self.files.write().insert(Arc::new(file)) + self.inner.files.write().insert(Arc::new(file)) } /// Remove the file with the specified node ID. fn remove_file(&self, fh: u64) { - self.files.write().remove(fh); + self.inner.files.write().remove(fh); } } @@ -575,21 +891,27 @@ impl HandleMap { /// globally unique, whereas inode numbers are per-volume. struct InodeMap { inodes_by_node_id: HandleMap>, - inodes_by_inode_nr: Option, u64)>>, + inodes_by_inode_nr: Option, u64)>>, + /// When true, node 1 is synthetic and not stored in this map, so node IDs + /// are allocated starting at 2 and `clear` does not preserve a real root. + aggregate: bool, } impl InodeMap { /// Create a new `InodeMap`. - pub fn new(supports_stable_file_id: bool) -> Self { - // TODO: Once multiple volumes are supported, the inodes_by_inode_nr map should be per - // volume. + pub fn new(supports_stable_file_id: bool, aggregate: bool) -> Self { Self { - inodes_by_node_id: HandleMap::new(), + inodes_by_node_id: if aggregate { + HandleMap::starting_at(FUSE_ROOT_ID + 1) + } else { + HandleMap::new() + }, inodes_by_inode_nr: if supports_stable_file_id { Some(HashMap::new()) } else { None }, + aggregate, } } @@ -603,7 +925,7 @@ impl InodeMap { pub fn insert(&mut self, inode: VirtioFsInode) -> (Arc, u64) { // If stable inode numbers are supported, look for the inode by its number. if let Some(inodes_by_inode_nr) = self.inodes_by_inode_nr.as_mut() { - match inodes_by_inode_nr.entry(inode.inode_nr()) { + match inodes_by_inode_nr.entry((inode.volume_id(), inode.inode_nr())) { Entry::Occupied(entry) => { // Inode found; increment its count and return the existing FUSE node ID. let new_path = inode.clone_path(); @@ -631,12 +953,23 @@ impl InodeMap { pub fn remove(&mut self, node_id: u64) { let inode = self.inodes_by_node_id.remove(node_id).unwrap(); if let Some(inodes_by_inode_nr) = self.inodes_by_inode_nr.as_mut() { - inodes_by_inode_nr.remove(&inode.inode_nr()); + inodes_by_inode_nr.remove(&(inode.volume_id(), inode.inode_nr())); } } /// Clears the map, preserving the root inode. pub fn clear(&mut self) { + if self.aggregate { + // Node 1 is synthetic and not stored here; drop everything and resume + // allocating node IDs after the reserved root id. + self.inodes_by_node_id.clear(); + self.inodes_by_node_id.next_handle = FUSE_ROOT_ID + 1; + if let Some(inodes_by_inode_nr) = self.inodes_by_inode_nr.as_mut() { + inodes_by_inode_nr.clear(); + } + return; + } + let root_inode = Arc::clone(self.inodes_by_node_id.get(FUSE_ROOT_ID).unwrap()); self.inodes_by_node_id.clear(); @@ -646,7 +979,73 @@ impl InodeMap { // Clear the inode number map if it's supported. if let Some(inodes_by_inode_nr) = self.inodes_by_inode_nr.as_mut() { inodes_by_inode_nr.clear(); - inodes_by_inode_nr.insert(root_inode.inode_nr(), (root_inode, FUSE_ROOT_ID)); + inodes_by_inode_nr.insert( + (root_inode.volume_id(), root_inode.inode_nr()), + (root_inode, FUSE_ROOT_ID), + ); } } } + +#[cfg(test)] +mod aggregate_tests { + use super::*; + + #[test] + fn aggregate_root_registry() { + let a = tempfile::tempdir().unwrap(); + let b = tempfile::tempdir().unwrap(); + let fs = VirtioFs::new_aggregate(false, true); + + fs.add_root("share_a", a.path(), None).unwrap(); + fs.add_root("share_b", b.path(), None).unwrap(); + + // Duplicate names are rejected. + assert_eq!( + fs.add_root("share_a", a.path(), None).unwrap_err(), + lx::Error::EEXIST + ); + + // Each root gets a distinct, non-zero volume id (0 is reserved for + // direct mode). + { + let roots = fs.inner.roots.read(); + assert_eq!(roots.entries.len(), 2); + assert_ne!(roots.entries[0].volume_id, 0); + assert_ne!(roots.entries[0].volume_id, roots.entries[1].volume_id); + } + + // Removal drops only the named root. + fs.remove_root("share_a").unwrap(); + assert_eq!( + fs.remove_root("share_a").unwrap_err(), + lx::Error::ENOENT + ); + assert_eq!(fs.inner.roots.read().entries.len(), 1); + } + + #[test] + fn add_root_rejected_in_direct_mode() { + let a = tempfile::tempdir().unwrap(); + let fs = VirtioFs::new(a.path(), None).unwrap(); + assert_eq!( + fs.add_root("x", a.path(), None).unwrap_err(), + lx::Error::EINVAL + ); + assert_eq!(fs.remove_root("x").unwrap_err(), lx::Error::EINVAL); + } + + #[test] + fn synthetic_root_node_ids_start_after_root() { + // In aggregate mode the synthetic root occupies FUSE_ROOT_ID, so the + // first real inode inserted must be allocated a higher id. + let a = tempfile::tempdir().unwrap(); + let fs = VirtioFs::new_aggregate(false, false); + fs.add_root("share", a.path(), None).unwrap(); + let entry = fs + .lookup_synthetic_root(lx::LxStr::from_bytes(b"share")) + .unwrap(); + assert!(entry.nodeid > FUSE_ROOT_ID); + } +} + diff --git a/vm/devices/virtio/virtiofs/src/resolver.rs b/vm/devices/virtio/virtiofs/src/resolver.rs index 1f0484ece9..f774e5a636 100644 --- a/vm/devices/virtio/virtiofs/src/resolver.rs +++ b/vm/devices/virtio/virtiofs/src/resolver.rs @@ -59,6 +59,20 @@ impl ResolveResource for VirtioFsResolver { VirtioFsBackend::SectionFs { .. } => { anyhow::bail!("section fs not supported on this platform") } + VirtioFsBackend::Aggregate { roots } => { + let fs = VirtioFs::new_aggregate(false, true); + for root in roots { + fs.add_root( + &root.name, + &root.root_path, + Some(&LxVolumeOptions::from_option_string(&root.mount_options)), + ) + .map_err(|e| { + anyhow::anyhow!("failed to add virtiofs root {}: {:?}", root.name, e) + })?; + } + VirtioFsDevice::new(input.driver_source, &resource.tag, fs, 0, None) + } }; Ok(device.into()) } diff --git a/vm/devices/virtio/virtiofs/src/section.rs b/vm/devices/virtio/virtiofs/src/section.rs index 8999689b44..2c84c4ba99 100644 --- a/vm/devices/virtio/virtiofs/src/section.rs +++ b/vm/devices/virtio/virtiofs/src/section.rs @@ -59,7 +59,7 @@ fn get_attr_with_cur_time() -> fuse::protocol::fuse_attr { gid: 0, rdev: 0, blksize: 0, - padding: 0, + flags: 0, } } diff --git a/vm/devices/virtio/virtiofs/src/util.rs b/vm/devices/virtio/virtiofs/src/util.rs index e5e86596b5..bf19409986 100644 --- a/vm/devices/virtio/virtiofs/src/util.rs +++ b/vm/devices/virtio/virtiofs/src/util.rs @@ -26,7 +26,7 @@ pub fn stat_to_fuse_attr(stat: &lx::Stat) -> fuse_attr { rdev: stat.device_nr_special as u32, // This is `usize` on x64 and `u32` on arm64, avoid a warning. blksize: stat.block_size as _, - padding: 0, + flags: 0, } } From 749a586ca376f2d45b7b52fa5f2bf1f6cbefb38a Mon Sep 17 00:00:00 2001 From: Daman Mulye Date: Thu, 25 Jun 2026 13:28:03 -0700 Subject: [PATCH 02/21] minor cleanup --- vm/devices/virtio/virtiofs/src/aggregate.rs | 494 ++++++++++++++++++++ vm/devices/virtio/virtiofs/src/file.rs | 12 +- vm/devices/virtio/virtiofs/src/inode.rs | 66 ++- vm/devices/virtio/virtiofs/src/lib.rs | 326 +------------ 4 files changed, 567 insertions(+), 331 deletions(-) create mode 100644 vm/devices/virtio/virtiofs/src/aggregate.rs diff --git a/vm/devices/virtio/virtiofs/src/aggregate.rs b/vm/devices/virtio/virtiofs/src/aggregate.rs new file mode 100644 index 0000000000..b47f7dabfa --- /dev/null +++ b/vm/devices/virtio/virtiofs/src/aggregate.rs @@ -0,0 +1,494 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +//! Aggregate (multi-root) virtio-fs. +//! +//! An aggregate device exposes a synthetic, read-only root directory whose +//! named children are independent host folders ("roots"). Each child is +//! advertised to the guest with `FUSE_ATTR_SUBMOUNT` (when negotiated) so it +//! gets its own `st_dev`. This module owns all of the aggregate-only state and +//! the [`VirtioFs`] methods that operate on it; the core (direct-mode) file +//! system lives in the crate root. + +use crate::ATTRIBUTE_TIMEOUT; +use crate::ENTRY_TIMEOUT; +use crate::VirtioFs; +use crate::inode; +use crate::inode::VirtioFsInode; +use fuse::DirEntryWriter; +use fuse::protocol::*; +use lxutil::LxVolumeOptions; +use parking_lot::RwLock; +use std::path::Path; +use std::path::PathBuf; +use std::sync::Arc; +use std::sync::atomic::AtomicBool; +use std::sync::atomic::Ordering; +use zerocopy::FromZeros; + +/// Reserved file handle returned by `open_dir` on the synthetic aggregate root. +/// +/// `read_dir`/`read_dir_plus`/`release_dir` recognize this sentinel and service +/// it from the root registry rather than the (real-file) handle map. `u64::MAX` +/// can never collide with a real handle because `HandleMap` allocates starting +/// at 1 and only increments. +pub(crate) const SYNTHETIC_ROOT_FH: u64 = u64::MAX; + +/// Linux `DT_DIR` directory-entry type, used for the synthetic root's children. +const DT_DIR: u32 = 4; + +/// A single host folder exposed as a named child of the synthetic aggregate root. +struct RootEntry { + /// Name of the synthetic top-level directory. Chosen by the caller; the guest + /// bind-mounts `/` onto the user's target path. + name: String, + volume: Arc, + /// Stable identifier disambiguating per-volume inode numbers. + volume_id: u32, +} + +/// Registry of aggregated roots for an aggregate-mode [`VirtioFs`]. +struct RootRegistry { + entries: Vec, + next_volume_id: u32, +} + +impl RootRegistry { + fn new() -> Self { + // Volume id 0 is reserved for a direct-mode single root, so aggregated + // roots start at 1. + Self { + entries: Vec::new(), + next_volume_id: 1, + } + } +} + +/// State that only exists for an aggregate-mode [`VirtioFs`]. +/// +/// When present, node 1 is a synthetic directory whose children are the entries +/// in `roots`; when absent (direct mode), node 1 is a real inode at a single +/// volume root (legacy single-share behavior). +pub(crate) struct AggregateState { + /// Aggregated roots exposed as named children of the synthetic root. + roots: RwLock, + /// When true, child roots are advertised with `FUSE_ATTR_SUBMOUNT` so the + /// guest kernel gives each share its own `st_dev`. Only honored once + /// `FUSE_SUBMOUNTS` is negotiated. + submounts: bool, + /// Set once the owning device host begins tearing the aggregate down (see + /// [`VirtioFs::begin_teardown`]). After this, [`VirtioFs::add_root`] fails + /// fast with `EAGAIN` rather than appending a root to a doomed device. + tearing_down: AtomicBool, +} + +impl AggregateState { + pub(crate) fn new(submounts: bool) -> Self { + Self { + roots: RwLock::new(RootRegistry::new()), + submounts, + tearing_down: AtomicBool::new(false), + } + } +} + +/// Aggregate-mode operations on [`VirtioFs`]. The crate-root `Fuse` +/// implementation dispatches the synthetic-root cases to the `pub(crate)` +/// helpers here. +impl VirtioFs { + /// Expose a host folder as a named child of the synthetic root. + /// + /// Only valid in aggregate mode. Returns: + /// - `EINVAL` on a direct-mode file system, or if the root's `readonly` + /// setting does not match the aggregate's (every child must agree, since + /// write permission is enforced device-wide). + /// - `EAGAIN` if the device has begun tearing down (see + /// [`Self::begin_teardown`]). + /// - `EEXIST` if a root with the same name already exists. + pub fn add_root( + &self, + name: &str, + root_path: impl AsRef, + mount_options: Option<&LxVolumeOptions>, + ) -> lx::Result<()> { + let Some(aggregate) = &self.inner.aggregate else { + return Err(lx::Error::EINVAL); + }; + + // Every child must share the aggregate's readonly setting: write + // permission is checked against the device-wide `readonly` flag, so a + // mismatched root would be silently mis-enforced. + let child_readonly = mount_options.is_some_and(|o| o.is_readonly()); + if child_readonly != self.inner.readonly { + return Err(lx::Error::EINVAL); + } + + // Fast-fail before paying for volume construction if the device is + // already tearing down. Re-checked under the lock below to close the + // race with a concurrent `begin_teardown`. + if aggregate.tearing_down.load(Ordering::Acquire) { + return Err(lx::Error::EAGAIN); + } + + let mut roots = aggregate.roots.write(); + if aggregate.tearing_down.load(Ordering::Acquire) { + return Err(lx::Error::EAGAIN); + } + if roots.entries.iter().any(|e| e.name == name) { + return Err(lx::Error::EEXIST); + } + + let volume = if let Some(mount_options) = mount_options { + mount_options.new_volume(root_path) + } else { + lxutil::LxVolume::new(root_path) + }?; + let volume_id = roots.next_volume_id; + roots.next_volume_id += 1; + roots.entries.push(RootEntry { + name: name.to_string(), + volume: Arc::new(volume), + volume_id, + }); + Ok(()) + } + + /// Signal that the owning device host has begun tearing the aggregate + /// device down. After this, [`Self::add_root`] rejects new roots with + /// `EAGAIN`, so an in-flight add cannot append a root to a device that is + /// going away. No-op for direct-mode file systems. + /// + /// The running device keeps serving existing inodes until it is fully + /// dropped; this only stops further roots from being added. + pub fn begin_teardown(&self) { + if let Some(aggregate) = &self.inner.aggregate { + aggregate.tearing_down.store(true, Ordering::Release); + } + } + + /// Remove a previously added root by name. + /// + /// In-flight inodes beneath the root remain valid until the guest forgets + /// them (each holds its own volume reference); the name simply stops + /// appearing in the synthetic root. Returns `ENOENT` if no such root exists. + pub fn remove_root(&self, name: &str) -> lx::Result<()> { + let Some(aggregate) = &self.inner.aggregate else { + return Err(lx::Error::EINVAL); + }; + + let mut roots = aggregate.roots.write(); + let before = roots.entries.len(); + roots.entries.retain(|e| e.name != name); + if roots.entries.len() == before { + Err(lx::Error::ENOENT) + } else { + Ok(()) + } + } + + /// Returns true if `node_id` refers to the synthetic aggregate root. + pub(crate) fn is_synthetic_root(&self, node_id: u64) -> bool { + self.inner.aggregate.is_some() && node_id == FUSE_ROOT_ID + } + + /// Whether aggregate children should be advertised with + /// `FUSE_ATTR_SUBMOUNT`. Always false in direct mode. + pub(crate) fn submounts(&self) -> bool { + self.inner.aggregate.as_ref().is_some_and(|a| a.submounts) + } + + /// Attributes of the synthetic aggregate root directory. + pub(crate) fn synthetic_root_attr() -> fuse_attr { + let mut attr = fuse_attr::new_zeroed(); + attr.ino = FUSE_ROOT_ID; + attr.mode = lx::S_IFDIR | 0o555; + attr.nlink = 2; + attr.blksize = 512; + attr + } + + /// Extended attributes of the synthetic aggregate root directory. + pub(crate) fn synthetic_root_statx(mask: lx::StatExMask) -> fuse_statx { + let mut sx = fuse_statx::new_zeroed(); + sx.mask = mask.into_bits(); + sx.mode = (lx::S_IFDIR | 0o555) as u16; + sx.nlink = 2; + sx.ino = FUSE_ROOT_ID; + sx.blksize = 512; + sx + } + + /// Looks up a named child of the synthetic root, returning an entry for the + /// corresponding volume's real root inode. + pub(crate) fn lookup_synthetic_root(&self, name: &lx::LxStr) -> lx::Result { + let Some(aggregate) = &self.inner.aggregate else { + return Err(lx::Error::ENOENT); + }; + let name_bytes = name.as_bytes(); + let (volume, volume_id) = { + let roots = aggregate.roots.read(); + let entry = roots + .entries + .iter() + .find(|e| e.name.as_bytes() == name_bytes) + .ok_or(lx::Error::ENOENT)?; + (Arc::clone(&entry.volume), entry.volume_id) + }; + + let (inode, stat) = VirtioFsInode::new(volume, volume_id, PathBuf::new())?; + let mut attr = inode.attr_from_stat(&stat); + let (_, node_id) = self.insert_inode(inode); + if self.submounts() { + attr.flags |= FUSE_ATTR_SUBMOUNT; + } + Ok(fuse_entry_out::new( + node_id, + ENTRY_TIMEOUT, + ATTRIBUTE_TIMEOUT, + attr, + )) + } + + /// Reads the synthetic root directory, listing `.`, `..`, and each root. + pub(crate) fn read_synthetic_root_dir( + &self, + offset: u64, + size: u32, + plus: bool, + ) -> lx::Result> { + let Some(aggregate) = &self.inner.aggregate else { + return Ok(Vec::new()); + }; + let mut buffer = Vec::with_capacity(size as usize); + // `offset` is the cookie of the next entry to emit (0 at start of stream). + // Entry 0 => ".", 1 => "..", 2.. => roots[index - 2]. + let mut index = offset; + loop { + let next = index + 1; + let fit = match index { + 0 => self.write_synthetic_dot(&mut buffer, ".", next, plus), + 1 => self.write_synthetic_dot(&mut buffer, "..", next, plus), + n => { + let root = { + let roots = aggregate.roots.read(); + roots + .entries + .get((n - 2) as usize) + .map(|e| (e.name.clone(), Arc::clone(&e.volume), e.volume_id)) + }; + let Some((name, volume, volume_id)) = root else { + break; + }; + self.write_root_entry(&mut buffer, &name, volume, volume_id, next, plus)? + } + }; + if !fit { + break; + } + index += 1; + } + Ok(buffer) + } + + /// Writes a synthetic `.`/`..` entry. These never carry a real node ID, so + /// the kernel will not issue a forget for them. + fn write_synthetic_dot( + &self, + buffer: &mut Vec, + name: &str, + next_off: u64, + plus: bool, + ) -> bool { + if plus { + if !buffer.check_dir_entry_plus(name) { + return false; + } + let mut entry = fuse_entry_out::new_zeroed(); + entry.attr.ino = FUSE_ROOT_ID; + entry.attr.mode = lx::S_IFDIR | 0o555; + buffer.dir_entry_plus(name, next_off, entry) + } else { + buffer.dir_entry(name, FUSE_ROOT_ID, next_off, DT_DIR) + } + } + + /// Writes a directory entry for an aggregated root. + fn write_root_entry( + &self, + buffer: &mut Vec, + name: &str, + volume: Arc, + volume_id: u32, + next_off: u64, + plus: bool, + ) -> lx::Result { + if plus { + if !buffer.check_dir_entry_plus(name) { + return Ok(false); + } + // readdirplus performs a lookup on each entry, incrementing its + // lookup count, so create/insert the root inode here. + let (inode, stat) = VirtioFsInode::new(volume, volume_id, PathBuf::new())?; + let mut attr = inode.attr_from_stat(&stat); + let (_, node_id) = self.insert_inode(inode); + if self.submounts() { + attr.flags |= FUSE_ATTR_SUBMOUNT; + } + let entry = fuse_entry_out::new(node_id, ENTRY_TIMEOUT, ATTRIBUTE_TIMEOUT, attr); + Ok(buffer.dir_entry_plus(name, next_off, entry)) + } else { + // Plain readdir: report the directory using the volume root's real + // inode number (namespaced to its volume), falling back to the + // volume id if it is inaccessible. + let raw = volume + .lstat(&PathBuf::new()) + .map(|s| s.inode_nr) + .unwrap_or(volume_id as lx::ino_t); + let ino = inode::namespace_ino(volume_id, raw); + Ok(buffer.dir_entry(name, ino, next_off, DT_DIR)) + } + } +} + +#[cfg(test)] +mod tests { + use crate::VirtioFs; + use crate::inode; + use lxutil::LxVolumeOptions; + + #[test] + fn aggregate_root_registry() { + let a = tempfile::tempdir().unwrap(); + let b = tempfile::tempdir().unwrap(); + let fs = VirtioFs::new_aggregate(false, true); + + fs.add_root("share_a", a.path(), None).unwrap(); + fs.add_root("share_b", b.path(), None).unwrap(); + + // Duplicate names are rejected. + assert_eq!( + fs.add_root("share_a", a.path(), None).unwrap_err(), + lx::Error::EEXIST + ); + + // Each root gets a distinct, non-zero volume id (0 is reserved for + // direct mode). + { + let aggregate = fs.inner.aggregate.as_ref().unwrap(); + let roots = aggregate.roots.read(); + assert_eq!(roots.entries.len(), 2); + assert_ne!(roots.entries[0].volume_id, 0); + assert_ne!(roots.entries[0].volume_id, roots.entries[1].volume_id); + } + + // Removal drops only the named root. + fs.remove_root("share_a").unwrap(); + assert_eq!(fs.remove_root("share_a").unwrap_err(), lx::Error::ENOENT); + assert_eq!( + fs.inner + .aggregate + .as_ref() + .unwrap() + .roots + .read() + .entries + .len(), + 1 + ); + } + + #[test] + fn add_root_rejected_in_direct_mode() { + let a = tempfile::tempdir().unwrap(); + let fs = VirtioFs::new(a.path(), None).unwrap(); + assert_eq!( + fs.add_root("x", a.path(), None).unwrap_err(), + lx::Error::EINVAL + ); + assert_eq!(fs.remove_root("x").unwrap_err(), lx::Error::EINVAL); + } + + #[test] + fn synthetic_root_node_ids_start_after_root() { + // In aggregate mode the synthetic root occupies FUSE_ROOT_ID, so the + // first real inode inserted must be allocated a higher id. + let a = tempfile::tempdir().unwrap(); + let fs = VirtioFs::new_aggregate(false, false); + fs.add_root("share", a.path(), None).unwrap(); + let entry = fs + .lookup_synthetic_root(lx::LxStr::from_bytes(b"share")) + .unwrap(); + assert!(entry.nodeid > fuse::protocol::FUSE_ROOT_ID); + } + + #[test] + fn inode_namespacing_avoids_cross_volume_collisions() { + // Direct mode (volume id 0) is the identity transform. + assert_eq!(inode::namespace_ino(0, 42), 42); + assert_eq!(inode::namespace_ino(0, 0), 0); + + // The same raw inode number on two different volumes maps to two + // different reported numbers, so siblings never alias. + let raw = 2; // e.g. the root inode of two freshly-formatted volumes + assert_ne!( + inode::namespace_ino(1, raw), + inode::namespace_ino(2, raw), + "sibling volumes must not collide" + ); + + // Within a single volume the transform is a bijection, so distinct + // files keep distinct inode numbers (preserving hard-link identity). + assert_ne!(inode::namespace_ino(1, 10), inode::namespace_ino(1, 11)); + } + + #[test] + fn add_root_enforces_uniform_readonly() { + let a = tempfile::tempdir().unwrap(); + let mut ro = LxVolumeOptions::default(); + ro.readonly(true); + let mut rw = LxVolumeOptions::default(); + rw.readonly(false); + + // A read-write aggregate rejects a readonly child. + let rw_fs = VirtioFs::new_aggregate(false, false); + assert_eq!( + rw_fs.add_root("ro_child", a.path(), Some(&ro)).unwrap_err(), + lx::Error::EINVAL + ); + rw_fs.add_root("rw_child", a.path(), Some(&rw)).unwrap(); + + // A readonly aggregate rejects a read-write child. + let ro_fs = VirtioFs::new_aggregate(true, false); + assert_eq!( + ro_fs.add_root("rw_child", a.path(), Some(&rw)).unwrap_err(), + lx::Error::EINVAL + ); + ro_fs.add_root("ro_child", a.path(), Some(&ro)).unwrap(); + } + + #[test] + fn add_root_rejected_after_teardown() { + let a = tempfile::tempdir().unwrap(); + let fs = VirtioFs::new_aggregate(false, false); + fs.add_root("before", a.path(), None).unwrap(); + + fs.begin_teardown(); + + // Once tearing down, no further roots can be added. + assert_eq!( + fs.add_root("after", a.path(), None).unwrap_err(), + lx::Error::EAGAIN + ); + assert_eq!( + fs.inner + .aggregate + .as_ref() + .unwrap() + .roots + .read() + .entries + .len(), + 1 + ); + } +} diff --git a/vm/devices/virtio/virtiofs/src/file.rs b/vm/devices/virtio/virtiofs/src/file.rs index e8c0a6e43e..c2a9337bbf 100644 --- a/vm/devices/virtio/virtiofs/src/file.rs +++ b/vm/devices/virtio/virtiofs/src/file.rs @@ -31,13 +31,13 @@ impl VirtioFsFile { /// Gets the attributes of the open file. pub fn get_attr(&self) -> lx::Result { let stat = self.file.read().fstat()?.into(); - Ok(util::stat_to_fuse_attr(&stat)) + Ok(self.inode.attr_from_stat(&stat)) } /// Gets the statx details for the open file. pub fn get_statx(&self) -> lx::Result { let statx = self.file.read().fstat()?; - Ok(util::statx_to_fuse_statx(&statx)) + Ok(self.inode.statx_from(&statx)) } /// Sets the attributes of the open file. @@ -74,7 +74,9 @@ impl VirtioFsFile { ) -> lx::Result> { let mut buffer = Vec::with_capacity(size as usize); let mut entry_count: u32 = 0; - let self_inode_nr = self.inode.inode_nr(); + // Report the directory's own inode number namespaced to its volume, so + // `.`/`..` agree with the number reported by lookup/getattr. + let self_inode_nr = self.inode.namespaced_ino(self.inode.inode_nr()); let mut file = self.file.write(); file.read_dir(offset as lx::off_t, |entry| { entry_count += 1; @@ -130,7 +132,9 @@ impl VirtioFsFile { entry_count -= 1; return Ok(true); } - entry.inode_nr + // Children share this directory's volume, so namespace the + // entry's inode number to match lookup/readdirplus. + self.inode.namespaced_ino(entry.inode_nr) }; Ok(buffer.dir_entry( diff --git a/vm/devices/virtio/virtiofs/src/inode.rs b/vm/devices/virtio/virtiofs/src/inode.rs index 5831123a61..d682290565 100644 --- a/vm/devices/virtio/virtiofs/src/inode.rs +++ b/vm/devices/virtio/virtiofs/src/inode.rs @@ -15,12 +15,36 @@ use std::sync::Arc; use std::sync::atomic::AtomicU64; use std::sync::atomic::Ordering; +/// Multiplier used to spread a volume id across the 64-bit inode space when +/// namespacing inode numbers (see [`namespace_ino`]). It is the 64-bit +/// golden-ratio constant, chosen because it has good bit-mixing properties. +const INO_NAMESPACE_MULTIPLIER: u64 = 0x9E37_79B9_7F4A_7C15; + +/// Folds a volume's namespace into a raw host inode number so that the same +/// inode number reported from two different aggregated volumes no longer +/// collides under the single shared FUSE superblock. +/// +/// `volume_id == 0` is the direct (single-root) mode, which returns `raw` +/// unchanged. For aggregated volumes (`volume_id != 0`) the transform is an +/// XOR by a per-volume constant, which is a bijection: distinct files within a +/// volume keep distinct inode numbers (preserving hard-link identity), and it +/// never introduces a within-volume collision. Sibling volumes get distinct +/// keys, so cross-share `(st_dev, st_ino)` aliasing is avoided even when the +/// guest never instantiates a submount. +pub(crate) fn namespace_ino(volume_id: u32, raw: lx::ino_t) -> lx::ino_t { + if volume_id == 0 { + return raw; + } + raw ^ (volume_id as u64).wrapping_mul(INO_NAMESPACE_MULTIPLIER) +} + /// Implements inode callbacks for virtio-fs. pub struct VirtioFsInode { volume: Arc, /// Identifies which aggregated volume this inode belongs to. Inode numbers /// are only unique within a volume, so this is needed to key the stable - /// inode-number map when a single file system exposes multiple roots. + /// inode-number map when a single file system exposes multiple roots, and + /// to namespace reported inode numbers (see [`namespace_ino`]). volume_id: u32, path: RwLock, lookup_count: AtomicU64, @@ -67,6 +91,28 @@ impl VirtioFsInode { self.volume_id } + /// Applies this inode's volume namespace to a raw host inode number (see + /// [`namespace_ino`]). + pub(crate) fn namespaced_ino(&self, raw: lx::ino_t) -> lx::ino_t { + namespace_ino(self.volume_id, raw) + } + + /// Builds a `fuse_attr` from a stat, namespacing the reported inode number + /// to this inode's volume so that aggregated siblings never alias. + pub(crate) fn attr_from_stat(&self, stat: &lx::Stat) -> fuse_attr { + let mut attr = util::stat_to_fuse_attr(stat); + attr.ino = self.namespaced_ino(attr.ino); + attr + } + + /// Builds a `fuse_statx` from a statx, namespacing the reported inode + /// number to this inode's volume. + pub(crate) fn statx_from(&self, statx: &lx::StatEx) -> fuse_statx { + let mut sx = util::statx_to_fuse_statx(statx); + sx.ino = self.namespaced_ino(sx.ino); + sx + } + /// Increments the lookup count. pub fn lookup(&self, new_path: PathBuf) { self.lookup_count.fetch_add(1, Ordering::AcqRel); @@ -109,20 +155,20 @@ impl VirtioFsInode { pub fn lookup_child(&self, name: &LxStr) -> lx::Result<(VirtioFsInode, fuse_attr)> { let path = self.child_path(name)?; let (inode, stat) = VirtioFsInode::new(Arc::clone(&self.volume), self.volume_id, path)?; - let attr = util::stat_to_fuse_attr(&stat); + let attr = inode.attr_from_stat(&stat); Ok((inode, attr)) } /// Retrieves the attributes of this inode. pub fn get_attr(&self) -> lx::Result { let stat = self.volume.lstat(&*self.get_path())?; - Ok(util::stat_to_fuse_attr(&stat)) + Ok(self.attr_from_stat(&stat)) } /// Retrieves the extended attributes of this inode. pub fn get_statx(&self) -> lx::Result { let statx = self.volume.statx(&*self.get_path())?; - Ok(util::statx_to_fuse_statx(&statx)) + Ok(self.statx_from(&statx)) } /// Sets the attributes of this inode. @@ -133,7 +179,7 @@ impl VirtioFsInode { // depending on the attributes being set. Lxutil takes care of that on Windows (and Linux // does it naturally). let stat = self.volume.set_attr_stat(&*self.get_path(), attr)?; - Ok(util::stat_to_fuse_attr(&stat)) + Ok(self.attr_from_stat(&stat)) } /// Opens the inode, creating a file object. @@ -158,7 +204,7 @@ impl VirtioFsInode { let file = self.volume.open(&path, flags, Some(options))?; let stat = file.fstat()?.into(); let inode = Self::with_attr(Arc::clone(&self.volume), self.volume_id, path, &stat); - let attr = util::stat_to_fuse_attr(&stat); + let attr = inode.attr_from_stat(&stat); Ok((inode, attr, file)) } @@ -176,7 +222,7 @@ impl VirtioFsInode { .mkdir_stat(&path, LxCreateOptions::new(mode, uid, gid))?; let inode = Self::with_attr(Arc::clone(&self.volume), self.volume_id, path, &stat); - let attr = util::stat_to_fuse_attr(&stat); + let attr = inode.attr_from_stat(&stat); Ok((inode, attr)) } @@ -197,7 +243,7 @@ impl VirtioFsInode { )?; let inode = Self::with_attr(Arc::clone(&self.volume), self.volume_id, path, &stat); - let attr = util::stat_to_fuse_attr(&stat); + let attr = inode.attr_from_stat(&stat); Ok((inode, attr)) } @@ -217,7 +263,7 @@ impl VirtioFsInode { )?; let inode = Self::with_attr(Arc::clone(&self.volume), self.volume_id, path, &stat); - let attr = util::stat_to_fuse_attr(&stat); + let attr = inode.attr_from_stat(&stat); Ok((inode, attr)) } @@ -225,7 +271,7 @@ impl VirtioFsInode { pub fn link(&self, name: &LxStr, target: &VirtioFsInode) -> lx::Result { let path = self.child_path(name)?; let stat = self.volume.link_stat(&*target.get_path(), path)?; - Ok(util::stat_to_fuse_attr(&stat)) + Ok(self.attr_from_stat(&stat)) } /// Reads the target of the symbolic link, if this inode is a symbolic link. diff --git a/vm/devices/virtio/virtiofs/src/lib.rs b/vm/devices/virtio/virtiofs/src/lib.rs index aaac159506..e3c00d33a5 100644 --- a/vm/devices/virtio/virtiofs/src/lib.rs +++ b/vm/devices/virtio/virtiofs/src/lib.rs @@ -4,6 +4,7 @@ #![expect(missing_docs)] #![cfg(any(windows, target_os = "linux"))] +mod aggregate; mod file; mod inode; #[cfg(test)] @@ -18,6 +19,8 @@ mod virtio_util; #[cfg(windows)] pub use section::SectionFs; +use aggregate::AggregateState; +use aggregate::SYNTHETIC_ROOT_FH; use file::VirtioFsFile; use fuse::protocol::*; use fuse::*; @@ -30,7 +33,6 @@ use std::path::Path; use std::path::PathBuf; use std::sync::Arc; use std::time::Duration; -use zerocopy::FromZeros; // TODO: Make these configurable. // FUSE likes to spam getattr a lot, so having a small timeout on the attributes avoids excessive @@ -43,59 +45,14 @@ const ATTRIBUTE_TIMEOUT: Duration = Duration::from_millis(1); // update the path. const ENTRY_TIMEOUT: Duration = Duration::from_secs(0); -/// Reserved file handle returned by `open_dir` on the synthetic aggregate root. -/// -/// `read_dir`/`read_dir_plus`/`release_dir` recognize this sentinel and service -/// it from the root registry rather than the (real-file) handle map. `u64::MAX` -/// can never collide with a real handle because `HandleMap` allocates starting -/// at 1 and only increments. -const SYNTHETIC_ROOT_FH: u64 = u64::MAX; - -/// Linux `DT_DIR` directory-entry type, used for the synthetic root's children. -const DT_DIR: u32 = 4; - -/// A single host folder exposed as a named child of the synthetic aggregate root. -struct RootEntry { - /// Name of the synthetic top-level directory. Chosen by the caller; the guest - /// bind-mounts `/` onto the user's target path. - name: String, - volume: Arc, - /// Stable identifier disambiguating per-volume inode numbers. - volume_id: u32, -} - -/// Registry of aggregated roots for an aggregate-mode [`VirtioFs`]. -struct RootRegistry { - entries: Vec, - next_volume_id: u32, -} - -impl RootRegistry { - fn new() -> Self { - // Volume id 0 is reserved for a direct-mode single root, so aggregated - // roots start at 1. - Self { - entries: Vec::new(), - next_volume_id: 1, - } - } -} - /// Shared mutable state behind a [`VirtioFs`] handle. struct VirtioFsInner { inodes: RwLock, files: RwLock>>, - /// Aggregated roots; empty and unused in direct (single-root) mode. - roots: RwLock, readonly: bool, - /// When true, node 1 is a synthetic directory whose children are the entries - /// in `roots`. When false, node 1 is a real inode at a single volume root - /// (legacy single-share behavior). - aggregate: bool, - /// When true, child roots are advertised with `FUSE_ATTR_SUBMOUNT` so the - /// guest kernel gives each share its own `st_dev`. Only meaningful in - /// aggregate mode and only honored once `FUSE_SUBMOUNTS` is negotiated. - submounts: bool, + /// `None` in direct (single-root) mode; `Some` only for aggregate devices, + /// carrying the synthetic root's registry and lifecycle state. + aggregate: Option, } /// Implementation of the virtio-fs file system. @@ -130,7 +87,7 @@ impl Fuse for VirtioFs { // In aggregate mode, advertise submounts so the guest gives each child // root its own st_dev when crossing into it. - if self.inner.submounts && info.capable() & FUSE_SUBMOUNTS != 0 { + if self.submounts() && info.capable() & FUSE_SUBMOUNTS != 0 { info.want |= FUSE_SUBMOUNTS; } } @@ -545,10 +502,8 @@ impl VirtioFs { inner: Arc::new(VirtioFsInner { inodes: RwLock::new(inodes), files: RwLock::new(HandleMap::new()), - roots: RwLock::new(RootRegistry::new()), readonly, - aggregate: false, - submounts: false, + aggregate: None, }), }) } @@ -566,212 +521,12 @@ impl VirtioFs { // enable the stable-id map and key it by `(volume_id, ino)`. inodes: RwLock::new(InodeMap::new(true, true)), files: RwLock::new(HandleMap::new()), - roots: RwLock::new(RootRegistry::new()), readonly, - aggregate: true, - submounts, + aggregate: Some(AggregateState::new(submounts)), }), } } - /// Expose a host folder as a named child of the synthetic root. - /// - /// Only valid in aggregate mode. Returns `EEXIST` if a root with the same - /// name already exists, or `EINVAL` on a direct-mode file system. - pub fn add_root( - &self, - name: &str, - root_path: impl AsRef, - mount_options: Option<&LxVolumeOptions>, - ) -> lx::Result<()> { - if !self.inner.aggregate { - return Err(lx::Error::EINVAL); - } - - let mut roots = self.inner.roots.write(); - if roots.entries.iter().any(|e| e.name == name) { - return Err(lx::Error::EEXIST); - } - - let volume = if let Some(mount_options) = mount_options { - mount_options.new_volume(root_path) - } else { - lxutil::LxVolume::new(root_path) - }?; - let volume_id = roots.next_volume_id; - roots.next_volume_id += 1; - roots.entries.push(RootEntry { - name: name.to_string(), - volume: Arc::new(volume), - volume_id, - }); - Ok(()) - } - - /// Remove a previously added root by name. - /// - /// In-flight inodes beneath the root remain valid until the guest forgets - /// them (each holds its own volume reference); the name simply stops - /// appearing in the synthetic root. Returns `ENOENT` if no such root exists. - pub fn remove_root(&self, name: &str) -> lx::Result<()> { - if !self.inner.aggregate { - return Err(lx::Error::EINVAL); - } - - let mut roots = self.inner.roots.write(); - let before = roots.entries.len(); - roots.entries.retain(|e| e.name != name); - if roots.entries.len() == before { - Err(lx::Error::ENOENT) - } else { - Ok(()) - } - } - - /// Returns true if `node_id` refers to the synthetic aggregate root. - fn is_synthetic_root(&self, node_id: u64) -> bool { - self.inner.aggregate && node_id == FUSE_ROOT_ID - } - - /// Attributes of the synthetic aggregate root directory. - fn synthetic_root_attr() -> fuse_attr { - let mut attr = fuse_attr::new_zeroed(); - attr.ino = FUSE_ROOT_ID; - attr.mode = lx::S_IFDIR | 0o555; - attr.nlink = 2; - attr.blksize = 512; - attr - } - - /// Extended attributes of the synthetic aggregate root directory. - fn synthetic_root_statx(mask: lx::StatExMask) -> fuse_statx { - let mut sx = fuse_statx::new_zeroed(); - sx.mask = mask.into_bits(); - sx.mode = (lx::S_IFDIR | 0o555) as u16; - sx.nlink = 2; - sx.ino = FUSE_ROOT_ID; - sx.blksize = 512; - sx - } - - /// Looks up a named child of the synthetic root, returning an entry for the - /// corresponding volume's real root inode. - fn lookup_synthetic_root(&self, name: &lx::LxStr) -> lx::Result { - let name_bytes = name.as_bytes(); - let (volume, volume_id) = { - let roots = self.inner.roots.read(); - let entry = roots - .entries - .iter() - .find(|e| e.name.as_bytes() == name_bytes) - .ok_or(lx::Error::ENOENT)?; - (Arc::clone(&entry.volume), entry.volume_id) - }; - - let (inode, stat) = VirtioFsInode::new(volume, volume_id, PathBuf::new())?; - let (_, node_id) = self.insert_inode(inode); - let mut attr = util::stat_to_fuse_attr(&stat); - if self.inner.submounts { - attr.flags |= FUSE_ATTR_SUBMOUNT; - } - Ok(fuse_entry_out::new( - node_id, - ENTRY_TIMEOUT, - ATTRIBUTE_TIMEOUT, - attr, - )) - } - - /// Reads the synthetic root directory, listing `.`, `..`, and each root. - fn read_synthetic_root_dir(&self, offset: u64, size: u32, plus: bool) -> lx::Result> { - let mut buffer = Vec::with_capacity(size as usize); - // `offset` is the cookie of the next entry to emit (0 at start of stream). - // Entry 0 => ".", 1 => "..", 2.. => roots[index - 2]. - let mut index = offset; - loop { - let next = index + 1; - let fit = match index { - 0 => self.write_synthetic_dot(&mut buffer, ".", next, plus), - 1 => self.write_synthetic_dot(&mut buffer, "..", next, plus), - n => { - let root = { - let roots = self.inner.roots.read(); - roots.entries.get((n - 2) as usize).map(|e| { - (e.name.clone(), Arc::clone(&e.volume), e.volume_id) - }) - }; - let Some((name, volume, volume_id)) = root else { - break; - }; - self.write_root_entry(&mut buffer, &name, volume, volume_id, next, plus)? - } - }; - if !fit { - break; - } - index += 1; - } - Ok(buffer) - } - - /// Writes a synthetic `.`/`..` entry. These never carry a real node ID, so - /// the kernel will not issue a forget for them. - fn write_synthetic_dot( - &self, - buffer: &mut Vec, - name: &str, - next_off: u64, - plus: bool, - ) -> bool { - if plus { - if !buffer.check_dir_entry_plus(name) { - return false; - } - let mut entry = fuse_entry_out::new_zeroed(); - entry.attr.ino = FUSE_ROOT_ID; - entry.attr.mode = lx::S_IFDIR | 0o555; - buffer.dir_entry_plus(name, next_off, entry) - } else { - buffer.dir_entry(name, FUSE_ROOT_ID, next_off, DT_DIR) - } - } - - /// Writes a directory entry for an aggregated root. - fn write_root_entry( - &self, - buffer: &mut Vec, - name: &str, - volume: Arc, - volume_id: u32, - next_off: u64, - plus: bool, - ) -> lx::Result { - if plus { - if !buffer.check_dir_entry_plus(name) { - return Ok(false); - } - // readdirplus performs a lookup on each entry, incrementing its - // lookup count, so create/insert the root inode here. - let (inode, stat) = VirtioFsInode::new(volume, volume_id, PathBuf::new())?; - let (_, node_id) = self.insert_inode(inode); - let mut attr = util::stat_to_fuse_attr(&stat); - if self.inner.submounts { - attr.flags |= FUSE_ATTR_SUBMOUNT; - } - let entry = fuse_entry_out::new(node_id, ENTRY_TIMEOUT, ATTRIBUTE_TIMEOUT, attr); - Ok(buffer.dir_entry_plus(name, next_off, entry)) - } else { - // Plain readdir: report the directory using the volume root's real - // inode number, falling back to the volume id if it is inaccessible. - let ino = volume - .lstat(&PathBuf::new()) - .map(|s| s.inode_nr) - .unwrap_or(volume_id as lx::ino_t); - Ok(buffer.dir_entry(name, ino, next_off, DT_DIR)) - } - } - - /// Perform lookup on a specified directory inode. fn lookup_helper(&self, inode: &VirtioFsInode, name: &lx::LxStr) -> lx::Result { let (new_inode, attr) = inode.lookup_child(name)?; let (_, new_inode_nr) = self.insert_inode(new_inode); @@ -986,66 +741,3 @@ impl InodeMap { } } } - -#[cfg(test)] -mod aggregate_tests { - use super::*; - - #[test] - fn aggregate_root_registry() { - let a = tempfile::tempdir().unwrap(); - let b = tempfile::tempdir().unwrap(); - let fs = VirtioFs::new_aggregate(false, true); - - fs.add_root("share_a", a.path(), None).unwrap(); - fs.add_root("share_b", b.path(), None).unwrap(); - - // Duplicate names are rejected. - assert_eq!( - fs.add_root("share_a", a.path(), None).unwrap_err(), - lx::Error::EEXIST - ); - - // Each root gets a distinct, non-zero volume id (0 is reserved for - // direct mode). - { - let roots = fs.inner.roots.read(); - assert_eq!(roots.entries.len(), 2); - assert_ne!(roots.entries[0].volume_id, 0); - assert_ne!(roots.entries[0].volume_id, roots.entries[1].volume_id); - } - - // Removal drops only the named root. - fs.remove_root("share_a").unwrap(); - assert_eq!( - fs.remove_root("share_a").unwrap_err(), - lx::Error::ENOENT - ); - assert_eq!(fs.inner.roots.read().entries.len(), 1); - } - - #[test] - fn add_root_rejected_in_direct_mode() { - let a = tempfile::tempdir().unwrap(); - let fs = VirtioFs::new(a.path(), None).unwrap(); - assert_eq!( - fs.add_root("x", a.path(), None).unwrap_err(), - lx::Error::EINVAL - ); - assert_eq!(fs.remove_root("x").unwrap_err(), lx::Error::EINVAL); - } - - #[test] - fn synthetic_root_node_ids_start_after_root() { - // In aggregate mode the synthetic root occupies FUSE_ROOT_ID, so the - // first real inode inserted must be allocated a higher id. - let a = tempfile::tempdir().unwrap(); - let fs = VirtioFs::new_aggregate(false, false); - fs.add_root("share", a.path(), None).unwrap(); - let entry = fs - .lookup_synthetic_root(lx::LxStr::from_bytes(b"share")) - .unwrap(); - assert!(entry.nodeid > FUSE_ROOT_ID); - } -} - From f9f55ed6d1dfa881b8af29f9d921a7c951e25e4c Mon Sep 17 00:00:00 2001 From: Daman Mulye Date: Thu, 25 Jun 2026 13:28:17 -0700 Subject: [PATCH 03/21] make FAT/exFAT WSL tests pass --- .../support/fs/lxutil/src/windows/fs.rs | 36 ++++----- .../support/fs/lxutil/src/windows/mod.rs | 80 ++++++++++++++++++- .../support/fs/lxutil/src/windows/util.rs | 45 ++++++----- 3 files changed, 123 insertions(+), 38 deletions(-) diff --git a/vm/devices/support/fs/lxutil/src/windows/fs.rs b/vm/devices/support/fs/lxutil/src/windows/fs.rs index dad942543f..bff2949a8d 100644 --- a/vm/devices/support/fs/lxutil/src/windows/fs.rs +++ b/vm/devices/support/fs/lxutil/src/windows/fs.rs @@ -295,13 +295,24 @@ pub fn analyze_delete_error(error: lx::Error, file_handle: &OwnedHandle) -> Dele { DeleteErrorAction::ReturnError(error) } else if error.value() == lx::EIO { - // EIO can come from STATUS_CANNOT_DELETE, which for directories means - // the directory is not empty. Check if this is a directory and return - // ENOTEMPTY in that case. - if is_directory(file_handle) { - DeleteErrorAction::ReturnError(lx::Error::ENOTEMPTY) - } else { - DeleteErrorAction::ReturnError(error) + // EIO can come from STATUS_CANNOT_DELETE. Inspect the file's attributes + // to decide how to proceed: + // - a non-empty directory surfaces as STATUS_CANNOT_DELETE, so map it to + // ENOTEMPTY; + // - a read-only file cannot be deleted via the non-POSIX + // FileDispositionInformation path, so clear the read-only attribute and + // retry; + // - anything else (including a genuine I/O failure, or a handle without + // FILE_READ_ATTRIBUTES) is returned as-is so the read-only attribute is + // not disturbed. + match util::query_information_file::(file_handle) { + Ok(info) if info.FileAttributes & W32Fs::FILE_ATTRIBUTE_DIRECTORY.0 != 0 => { + DeleteErrorAction::ReturnError(lx::Error::ENOTEMPTY) + } + Ok(info) if info.FileAttributes & W32Fs::FILE_ATTRIBUTE_READONLY.0 != 0 => { + DeleteErrorAction::TryReadOnlyWorkaround + } + _ => DeleteErrorAction::ReturnError(error), } } else { DeleteErrorAction::TryReadOnlyWorkaround @@ -322,17 +333,6 @@ pub fn delete_file(fs_context: &FsContext, file_handle: &OwnedHandle) -> lx::Res } } -/// Check if a file handle refers to a directory. -fn is_directory(file_handle: &OwnedHandle) -> bool { - if let Ok(info) = - util::query_information_file::(file_handle) - { - info.FileAttributes & W32Fs::FILE_ATTRIBUTE_DIRECTORY.0 != 0 - } else { - false - } -} - pub fn delete_file_core(fs_context: &FsContext, file_handle: &OwnedHandle) -> lx::Result<()> { if fs_context .compatibility_flags diff --git a/vm/devices/support/fs/lxutil/src/windows/mod.rs b/vm/devices/support/fs/lxutil/src/windows/mod.rs index 3622f89837..a21cc75e11 100644 --- a/vm/devices/support/fs/lxutil/src/windows/mod.rs +++ b/vm/devices/support/fs/lxutil/src/windows/mod.rs @@ -405,6 +405,64 @@ impl LxVolume { Default::default(), )?; + // NT's POSIX rename (used by NTFS) natively enforces several Linux rename + // semantics. The classic non-POSIX path used by FAT/exFAT does not, so + // replicate them explicitly here. NTFS is left untouched: it returns the + // same errors from NT directly. + if !self + .state + .fs_context + .compatibility_flags + .supports_posix_unlink_rename() + { + let is_dir = |h: &OwnedHandle| -> lx::Result { + Ok( + util::query_information_file::(h)? + .FileAttributes + & W32Fs::FILE_ATTRIBUTE_DIRECTORY.0 + != 0, + ) + }; + let source_is_dir = is_dir(&handle)?; + + // Linux returns EINVAL when renaming a directory into one of its own + // descendants; NT's classic rename instead reports STATUS_ACCESS_DENIED + // (-> EACCES). `Path::starts_with` compares whole path components, so + // "a/bc" is not treated as a descendant of "a/b". Only directories can + // have descendants, so non-directory sources are unaffected. + // N.B. The comparison is case-sensitive, so a case-only descendant such + // as renaming "a/b" into "a/B/c" on a case-insensitive volume is not + // caught here; NT still rejects it (as EACCES) so no data is lost. + if source_is_dir && new_path != path && new_path.starts_with(path) { + return Err(lx::Error::EINVAL); + } + + // Linux forbids replacing a file with a directory (ENOTDIR) or a + // directory with a file (EISDIR). The classic non-POSIX path would + // instead silently supersede a file with a directory and report only a + // generic collision for the reverse, so detect type-incompatible targets + // up front before the destructive rename is attempted. + // N.B. `open_file` always sets FILE_OPEN_REPARSE_POINT, so a symlink + // target is classified as the (non-directory) reparse point itself, + // matching Linux. On case-insensitive volumes a case-only rename + // (foo -> FOO) opens the same object as source and target, yielding + // equal types and thus no false error. + match self.open_file(new_path, W32Fs::FILE_READ_ATTRIBUTES, Default::default()) { + Ok(target_handle) => { + let target_is_dir = is_dir(&target_handle)?; + if source_is_dir && !target_is_dir { + return Err(lx::Error::ENOTDIR); + } + if !source_is_dir && target_is_dir { + return Err(lx::Error::EISDIR); + } + } + // A missing target is fine; the rename creates it. + Err(err) if err.value() == lx::ENOENT => {} + Err(err) => return Err(err), + } + } + let flags = fs::RenameFlags::default(); let error = match fs::rename(&handle, &self.root, new_path, &self.state.fs_context, flags) { Ok(_) => return Ok(()), @@ -425,7 +483,11 @@ impl LxVolume { .compatibility_flags .supports_posix_unlink_rename() { - match self.open_file(new_path, W32Fs::DELETE, Default::default()) { + match self.open_file( + new_path, + W32Fs::FILE_READ_ATTRIBUTES | W32Fs::DELETE, + Default::default(), + ) { Ok(target_handle) => self.delete_file(&target_handle)?, Err(err) => { // ENOENT means the rename can proceed. @@ -700,6 +762,22 @@ impl LxVolume { assert!(path.is_relative() && !path.as_os_str().is_empty()); self.check_sandbox_enforcement(path)?; + // Symlinks are always implemented as reparse points (either an LX + // symlink reparse tag or an NT symlink). Filesystems that don't support + // reparse points (e.g. FAT) cannot store a symlink at all, so report + // EPERM up front rather than creating the file and then failing to set + // the reparse point (which would surface as EOPNOTSUPP). This matches + // the conventional Linux errno for symlink() on such filesystems and + // mirrors the hard link check in link_helper. + if !self + .state + .fs_context + .compatibility_flags + .supports_reparse_points() + { + return Err(lx::Error::EPERM); + } + // Convert the target to its native Windows format. let win_target = Path::from_lx(target); diff --git a/vm/devices/support/fs/lxutil/src/windows/util.rs b/vm/devices/support/fs/lxutil/src/windows/util.rs index 41f2fe71e8..126c32f299 100644 --- a/vm/devices/support/fs/lxutil/src/windows/util.rs +++ b/vm/devices/support/fs/lxutil/src/windows/util.rs @@ -155,29 +155,32 @@ fn get_token_for_access_check() -> lx::Result { let mut client_token_raw = Foundation::HANDLE::default(); let mut duplicate_token_raw = Foundation::HANDLE::default(); // SAFETY: Calling Win32 API as documented. - let result = unsafe { - check_status(FileSystem::NtOpenThreadToken( + // + // N.B. The raw NTSTATUS is inspected directly (rather than going through + // `check_status`) so that `STATUS_NO_TOKEN` can be distinguished from + // other failures. `check_status` maps the NTSTATUS to an `lx::Error` + // errno, which cannot be compared against an NTSTATUS constant. + let status = unsafe { + FileSystem::NtOpenThreadToken( Threading::GetCurrentThread(), Security::TOKEN_QUERY.0 | Security::TOKEN_DUPLICATE.0, true, &mut client_token_raw, - )) + ) }; - if let Err(e) = result { - // Use the process token if there's no token. - if e.value() == Foundation::STATUS_NO_TOKEN.0 { - // SAFETY: Calling Win32 API as documented. - unsafe { - let _ = check_status(FileSystem::NtOpenProcessToken( - Threading::GetCurrentProcess(), - Security::TOKEN_QUERY.0 | Security::TOKEN_DUPLICATE.0, - &mut client_token_raw, - ))?; - } - } else { - return Err(e); + if status == Foundation::STATUS_NO_TOKEN { + // The thread is not impersonating, so use the process token instead. + // SAFETY: Calling Win32 API as documented. + unsafe { + let _ = check_status(FileSystem::NtOpenProcessToken( + Threading::GetCurrentProcess(), + Security::TOKEN_QUERY.0 | Security::TOKEN_DUPLICATE.0, + &mut client_token_raw, + ))?; } + } else { + let _ = check_status(status)?; } // Create the RAII handle now so it gets closed on drop if we need @@ -213,7 +216,10 @@ fn get_token_for_access_check() -> lx::Result { }, EffectiveOnly: false, }; - let mut object_attributes = Wdk::Foundation::OBJECT_ATTRIBUTES::default(); + let mut object_attributes = Wdk::Foundation::OBJECT_ATTRIBUTES { + Length: size_of::() as u32, + ..Default::default() + }; object_attributes.SecurityQualityOfService = ptr::from_ref::(&security_qos).cast(); @@ -731,13 +737,14 @@ pub fn set_information_file( // SAFETY: Calling NtSetInformationFile as documented. unsafe { - let _ = check_status(FileSystem::NtSetInformationFile( + let status = FileSystem::NtSetInformationFile( Foundation::HANDLE(handle.as_raw_handle()), &mut iosb, buf.cast(), len.try_into().unwrap(), info.file_information_class(), - ))?; + ); + let _ = check_status(status)?; Ok(()) } From dc8629208472ba877f4a12a33d2101c5208b2d62 Mon Sep 17 00:00:00 2001 From: Daman Mulye Date: Thu, 25 Jun 2026 14:15:43 -0700 Subject: [PATCH 04/21] clean-up naming --- vm/devices/support/fs/fuse/src/protocol.rs | 3 - vm/devices/virtio/virtio_resources/src/lib.rs | 9 +- vm/devices/virtio/virtiofs/src/aggregate.rs | 149 +++++++++--------- vm/devices/virtio/virtiofs/src/resolver.rs | 14 +- 4 files changed, 87 insertions(+), 88 deletions(-) diff --git a/vm/devices/support/fs/fuse/src/protocol.rs b/vm/devices/support/fs/fuse/src/protocol.rs index 22d6578dc5..c15fa95aaf 100644 --- a/vm/devices/support/fs/fuse/src/protocol.rs +++ b/vm/devices/support/fs/fuse/src/protocol.rs @@ -65,9 +65,6 @@ pub struct fuse_attr { pub gid: u32, pub rdev: u32, pub blksize: u32, - /// Attribute flags (`FUSE_ATTR_*`). Named `padding` in older kernels; valid - /// for `FUSE_ATTR_SUBMOUNT` only once the `FUSE_SUBMOUNTS` capability has - /// been negotiated at init time. pub flags: u32, } diff --git a/vm/devices/virtio/virtio_resources/src/lib.rs b/vm/devices/virtio/virtio_resources/src/lib.rs index 95368411df..29609a74e0 100644 --- a/vm/devices/virtio/virtio_resources/src/lib.rs +++ b/vm/devices/virtio/virtio_resources/src/lib.rs @@ -64,15 +64,16 @@ pub mod fs { /// child of a synthetic root. Lets one virtio-fs device (one tag, one /// PCI/MMIO footprint) serve many shares. Aggregate { - roots: Vec, + children: Vec, }, } /// A single host folder exposed as a named child of a [`VirtioFsBackend::Aggregate`]. #[derive(MeshPayload)] - pub struct VirtioFsAggregateRoot { - /// Synthetic top-level directory name; the guest bind-mounts - /// `/` onto the user's target path. + pub struct VirtioFsAggregateChild { + /// Name of this child's directory under the aggregate's synthetic root; + /// the guest bind-mounts `/` onto the user's + /// target path. pub name: String, pub root_path: String, pub mount_options: String, diff --git a/vm/devices/virtio/virtiofs/src/aggregate.rs b/vm/devices/virtio/virtiofs/src/aggregate.rs index b47f7dabfa..c7a168834f 100644 --- a/vm/devices/virtio/virtiofs/src/aggregate.rs +++ b/vm/devices/virtio/virtiofs/src/aggregate.rs @@ -4,7 +4,7 @@ //! Aggregate (multi-root) virtio-fs. //! //! An aggregate device exposes a synthetic, read-only root directory whose -//! named children are independent host folders ("roots"). Each child is +//! named children are independent host folders. Each child is //! advertised to the guest with `FUSE_ATTR_SUBMOUNT` (when negotiated) so it //! gets its own `st_dev`. This module owns all of the aggregate-only state and //! the [`VirtioFs`] methods that operate on it; the core (direct-mode) file @@ -38,25 +38,26 @@ pub(crate) const SYNTHETIC_ROOT_FH: u64 = u64::MAX; const DT_DIR: u32 = 4; /// A single host folder exposed as a named child of the synthetic aggregate root. -struct RootEntry { - /// Name of the synthetic top-level directory. Chosen by the caller; the guest - /// bind-mounts `/` onto the user's target path. +struct ChildEntry { + /// Name of this child's directory under the synthetic root. Chosen by the + /// caller; the guest bind-mounts `/` onto the user's + /// target path. name: String, volume: Arc, /// Stable identifier disambiguating per-volume inode numbers. volume_id: u32, } -/// Registry of aggregated roots for an aggregate-mode [`VirtioFs`]. -struct RootRegistry { - entries: Vec, +/// Registry of aggregated children for an aggregate-mode [`VirtioFs`]. +struct ChildRegistry { + entries: Vec, next_volume_id: u32, } -impl RootRegistry { +impl ChildRegistry { fn new() -> Self { // Volume id 0 is reserved for a direct-mode single root, so aggregated - // roots start at 1. + // children start at 1. Self { entries: Vec::new(), next_volume_id: 1, @@ -67,25 +68,25 @@ impl RootRegistry { /// State that only exists for an aggregate-mode [`VirtioFs`]. /// /// When present, node 1 is a synthetic directory whose children are the entries -/// in `roots`; when absent (direct mode), node 1 is a real inode at a single +/// in `children`; when absent (direct mode), node 1 is a real inode at a single /// volume root (legacy single-share behavior). pub(crate) struct AggregateState { - /// Aggregated roots exposed as named children of the synthetic root. - roots: RwLock, - /// When true, child roots are advertised with `FUSE_ATTR_SUBMOUNT` so the + /// Aggregated children exposed under the synthetic root. + children: RwLock, + /// When true, children are advertised with `FUSE_ATTR_SUBMOUNT` so the /// guest kernel gives each share its own `st_dev`. Only honored once /// `FUSE_SUBMOUNTS` is negotiated. submounts: bool, /// Set once the owning device host begins tearing the aggregate down (see - /// [`VirtioFs::begin_teardown`]). After this, [`VirtioFs::add_root`] fails - /// fast with `EAGAIN` rather than appending a root to a doomed device. + /// [`VirtioFs::begin_teardown`]). After this, [`VirtioFs::add_child`] fails + /// fast with `EAGAIN` rather than appending a child to a doomed device. tearing_down: AtomicBool, } impl AggregateState { pub(crate) fn new(submounts: bool) -> Self { Self { - roots: RwLock::new(RootRegistry::new()), + children: RwLock::new(ChildRegistry::new()), submounts, tearing_down: AtomicBool::new(false), } @@ -99,13 +100,13 @@ impl VirtioFs { /// Expose a host folder as a named child of the synthetic root. /// /// Only valid in aggregate mode. Returns: - /// - `EINVAL` on a direct-mode file system, or if the root's `readonly` + /// - `EINVAL` on a direct-mode file system, or if the child's `readonly` /// setting does not match the aggregate's (every child must agree, since /// write permission is enforced device-wide). /// - `EAGAIN` if the device has begun tearing down (see /// [`Self::begin_teardown`]). - /// - `EEXIST` if a root with the same name already exists. - pub fn add_root( + /// - `EEXIST` if a child with the same name already exists. + pub fn add_child( &self, name: &str, root_path: impl AsRef, @@ -117,7 +118,7 @@ impl VirtioFs { // Every child must share the aggregate's readonly setting: write // permission is checked against the device-wide `readonly` flag, so a - // mismatched root would be silently mis-enforced. + // mismatched child would be silently mis-enforced. let child_readonly = mount_options.is_some_and(|o| o.is_readonly()); if child_readonly != self.inner.readonly { return Err(lx::Error::EINVAL); @@ -130,11 +131,11 @@ impl VirtioFs { return Err(lx::Error::EAGAIN); } - let mut roots = aggregate.roots.write(); + let mut children = aggregate.children.write(); if aggregate.tearing_down.load(Ordering::Acquire) { return Err(lx::Error::EAGAIN); } - if roots.entries.iter().any(|e| e.name == name) { + if children.entries.iter().any(|e| e.name == name) { return Err(lx::Error::EEXIST); } @@ -143,9 +144,9 @@ impl VirtioFs { } else { lxutil::LxVolume::new(root_path) }?; - let volume_id = roots.next_volume_id; - roots.next_volume_id += 1; - roots.entries.push(RootEntry { + let volume_id = children.next_volume_id; + children.next_volume_id += 1; + children.entries.push(ChildEntry { name: name.to_string(), volume: Arc::new(volume), volume_id, @@ -154,32 +155,32 @@ impl VirtioFs { } /// Signal that the owning device host has begun tearing the aggregate - /// device down. After this, [`Self::add_root`] rejects new roots with - /// `EAGAIN`, so an in-flight add cannot append a root to a device that is + /// device down. After this, [`Self::add_child`] rejects new children with + /// `EAGAIN`, so an in-flight add cannot append a child to a device that is /// going away. No-op for direct-mode file systems. /// /// The running device keeps serving existing inodes until it is fully - /// dropped; this only stops further roots from being added. + /// dropped; this only stops further children from being added. pub fn begin_teardown(&self) { if let Some(aggregate) = &self.inner.aggregate { aggregate.tearing_down.store(true, Ordering::Release); } } - /// Remove a previously added root by name. + /// Remove a previously added child by name. /// - /// In-flight inodes beneath the root remain valid until the guest forgets + /// In-flight inodes beneath the child remain valid until the guest forgets /// them (each holds its own volume reference); the name simply stops - /// appearing in the synthetic root. Returns `ENOENT` if no such root exists. - pub fn remove_root(&self, name: &str) -> lx::Result<()> { + /// appearing in the synthetic root. Returns `ENOENT` if no such child exists. + pub fn remove_child(&self, name: &str) -> lx::Result<()> { let Some(aggregate) = &self.inner.aggregate else { return Err(lx::Error::EINVAL); }; - let mut roots = aggregate.roots.write(); - let before = roots.entries.len(); - roots.entries.retain(|e| e.name != name); - if roots.entries.len() == before { + let mut children = aggregate.children.write(); + let before = children.entries.len(); + children.entries.retain(|e| e.name != name); + if children.entries.len() == before { Err(lx::Error::ENOENT) } else { Ok(()) @@ -226,8 +227,8 @@ impl VirtioFs { }; let name_bytes = name.as_bytes(); let (volume, volume_id) = { - let roots = aggregate.roots.read(); - let entry = roots + let children = aggregate.children.read(); + let entry = children .entries .iter() .find(|e| e.name.as_bytes() == name_bytes) @@ -249,7 +250,7 @@ impl VirtioFs { )) } - /// Reads the synthetic root directory, listing `.`, `..`, and each root. + /// Reads the synthetic root directory, listing `.`, `..`, and each child. pub(crate) fn read_synthetic_root_dir( &self, offset: u64, @@ -261,7 +262,7 @@ impl VirtioFs { }; let mut buffer = Vec::with_capacity(size as usize); // `offset` is the cookie of the next entry to emit (0 at start of stream). - // Entry 0 => ".", 1 => "..", 2.. => roots[index - 2]. + // Entry 0 => ".", 1 => "..", 2.. => children[index - 2]. let mut index = offset; loop { let next = index + 1; @@ -269,17 +270,17 @@ impl VirtioFs { 0 => self.write_synthetic_dot(&mut buffer, ".", next, plus), 1 => self.write_synthetic_dot(&mut buffer, "..", next, plus), n => { - let root = { - let roots = aggregate.roots.read(); - roots + let child = { + let children = aggregate.children.read(); + children .entries .get((n - 2) as usize) .map(|e| (e.name.clone(), Arc::clone(&e.volume), e.volume_id)) }; - let Some((name, volume, volume_id)) = root else { + let Some((name, volume, volume_id)) = child else { break; }; - self.write_root_entry(&mut buffer, &name, volume, volume_id, next, plus)? + self.write_child_entry(&mut buffer, &name, volume, volume_id, next, plus)? } }; if !fit { @@ -312,8 +313,8 @@ impl VirtioFs { } } - /// Writes a directory entry for an aggregated root. - fn write_root_entry( + /// Writes a directory entry for an aggregated child. + fn write_child_entry( &self, buffer: &mut Vec, name: &str, @@ -357,39 +358,39 @@ mod tests { use lxutil::LxVolumeOptions; #[test] - fn aggregate_root_registry() { + fn aggregate_child_registry() { let a = tempfile::tempdir().unwrap(); let b = tempfile::tempdir().unwrap(); let fs = VirtioFs::new_aggregate(false, true); - fs.add_root("share_a", a.path(), None).unwrap(); - fs.add_root("share_b", b.path(), None).unwrap(); + fs.add_child("share_a", a.path(), None).unwrap(); + fs.add_child("share_b", b.path(), None).unwrap(); // Duplicate names are rejected. assert_eq!( - fs.add_root("share_a", a.path(), None).unwrap_err(), + fs.add_child("share_a", a.path(), None).unwrap_err(), lx::Error::EEXIST ); - // Each root gets a distinct, non-zero volume id (0 is reserved for + // Each child gets a distinct, non-zero volume id (0 is reserved for // direct mode). { let aggregate = fs.inner.aggregate.as_ref().unwrap(); - let roots = aggregate.roots.read(); - assert_eq!(roots.entries.len(), 2); - assert_ne!(roots.entries[0].volume_id, 0); - assert_ne!(roots.entries[0].volume_id, roots.entries[1].volume_id); + let children = aggregate.children.read(); + assert_eq!(children.entries.len(), 2); + assert_ne!(children.entries[0].volume_id, 0); + assert_ne!(children.entries[0].volume_id, children.entries[1].volume_id); } - // Removal drops only the named root. - fs.remove_root("share_a").unwrap(); - assert_eq!(fs.remove_root("share_a").unwrap_err(), lx::Error::ENOENT); + // Removal drops only the named child. + fs.remove_child("share_a").unwrap(); + assert_eq!(fs.remove_child("share_a").unwrap_err(), lx::Error::ENOENT); assert_eq!( fs.inner .aggregate .as_ref() .unwrap() - .roots + .children .read() .entries .len(), @@ -398,14 +399,14 @@ mod tests { } #[test] - fn add_root_rejected_in_direct_mode() { + fn add_child_rejected_in_direct_mode() { let a = tempfile::tempdir().unwrap(); let fs = VirtioFs::new(a.path(), None).unwrap(); assert_eq!( - fs.add_root("x", a.path(), None).unwrap_err(), + fs.add_child("x", a.path(), None).unwrap_err(), lx::Error::EINVAL ); - assert_eq!(fs.remove_root("x").unwrap_err(), lx::Error::EINVAL); + assert_eq!(fs.remove_child("x").unwrap_err(), lx::Error::EINVAL); } #[test] @@ -414,7 +415,7 @@ mod tests { // first real inode inserted must be allocated a higher id. let a = tempfile::tempdir().unwrap(); let fs = VirtioFs::new_aggregate(false, false); - fs.add_root("share", a.path(), None).unwrap(); + fs.add_child("share", a.path(), None).unwrap(); let entry = fs .lookup_synthetic_root(lx::LxStr::from_bytes(b"share")) .unwrap(); @@ -442,7 +443,7 @@ mod tests { } #[test] - fn add_root_enforces_uniform_readonly() { + fn add_child_enforces_uniform_readonly() { let a = tempfile::tempdir().unwrap(); let mut ro = LxVolumeOptions::default(); ro.readonly(true); @@ -452,31 +453,31 @@ mod tests { // A read-write aggregate rejects a readonly child. let rw_fs = VirtioFs::new_aggregate(false, false); assert_eq!( - rw_fs.add_root("ro_child", a.path(), Some(&ro)).unwrap_err(), + rw_fs.add_child("ro_child", a.path(), Some(&ro)).unwrap_err(), lx::Error::EINVAL ); - rw_fs.add_root("rw_child", a.path(), Some(&rw)).unwrap(); + rw_fs.add_child("rw_child", a.path(), Some(&rw)).unwrap(); // A readonly aggregate rejects a read-write child. let ro_fs = VirtioFs::new_aggregate(true, false); assert_eq!( - ro_fs.add_root("rw_child", a.path(), Some(&rw)).unwrap_err(), + ro_fs.add_child("rw_child", a.path(), Some(&rw)).unwrap_err(), lx::Error::EINVAL ); - ro_fs.add_root("ro_child", a.path(), Some(&ro)).unwrap(); + ro_fs.add_child("ro_child", a.path(), Some(&ro)).unwrap(); } #[test] - fn add_root_rejected_after_teardown() { + fn add_child_rejected_after_teardown() { let a = tempfile::tempdir().unwrap(); let fs = VirtioFs::new_aggregate(false, false); - fs.add_root("before", a.path(), None).unwrap(); + fs.add_child("before", a.path(), None).unwrap(); fs.begin_teardown(); - // Once tearing down, no further roots can be added. + // Once tearing down, no further children can be added. assert_eq!( - fs.add_root("after", a.path(), None).unwrap_err(), + fs.add_child("after", a.path(), None).unwrap_err(), lx::Error::EAGAIN ); assert_eq!( @@ -484,7 +485,7 @@ mod tests { .aggregate .as_ref() .unwrap() - .roots + .children .read() .entries .len(), diff --git a/vm/devices/virtio/virtiofs/src/resolver.rs b/vm/devices/virtio/virtiofs/src/resolver.rs index f774e5a636..e21d69a89a 100644 --- a/vm/devices/virtio/virtiofs/src/resolver.rs +++ b/vm/devices/virtio/virtiofs/src/resolver.rs @@ -59,16 +59,16 @@ impl ResolveResource for VirtioFsResolver { VirtioFsBackend::SectionFs { .. } => { anyhow::bail!("section fs not supported on this platform") } - VirtioFsBackend::Aggregate { roots } => { + VirtioFsBackend::Aggregate { children } => { let fs = VirtioFs::new_aggregate(false, true); - for root in roots { - fs.add_root( - &root.name, - &root.root_path, - Some(&LxVolumeOptions::from_option_string(&root.mount_options)), + for child in children { + fs.add_child( + &child.name, + &child.root_path, + Some(&LxVolumeOptions::from_option_string(&child.mount_options)), ) .map_err(|e| { - anyhow::anyhow!("failed to add virtiofs root {}: {:?}", root.name, e) + anyhow::anyhow!("failed to add virtiofs root {}: {:?}", child.name, e) })?; } VirtioFsDevice::new(input.driver_source, &resource.tag, fs, 0, None) From 6d1d36b725dd7a24c16e386105a958f303ff268e Mon Sep 17 00:00:00 2001 From: Daman Mulye Date: Thu, 25 Jun 2026 14:15:59 -0700 Subject: [PATCH 05/21] cache computed inode_no for readability --- vm/devices/virtio/virtiofs/src/file.rs | 2 +- vm/devices/virtio/virtiofs/src/inode.rs | 37 +++++++++++++++++++------ 2 files changed, 29 insertions(+), 10 deletions(-) diff --git a/vm/devices/virtio/virtiofs/src/file.rs b/vm/devices/virtio/virtiofs/src/file.rs index c2a9337bbf..84a3896397 100644 --- a/vm/devices/virtio/virtiofs/src/file.rs +++ b/vm/devices/virtio/virtiofs/src/file.rs @@ -76,7 +76,7 @@ impl VirtioFsFile { let mut entry_count: u32 = 0; // Report the directory's own inode number namespaced to its volume, so // `.`/`..` agree with the number reported by lookup/getattr. - let self_inode_nr = self.inode.namespaced_ino(self.inode.inode_nr()); + let self_inode_nr = self.inode.namespaced_inode_nr(); let mut file = self.file.write(); file.read_dir(offset as lx::off_t, |entry| { entry_count += 1; diff --git a/vm/devices/virtio/virtiofs/src/inode.rs b/vm/devices/virtio/virtiofs/src/inode.rs index d682290565..ff81842c18 100644 --- a/vm/devices/virtio/virtiofs/src/inode.rs +++ b/vm/devices/virtio/virtiofs/src/inode.rs @@ -49,6 +49,10 @@ pub struct VirtioFsInode { path: RwLock, lookup_count: AtomicU64, inode_nr: lx::ino_t, + /// This inode's number as reported to the guest: its host inode number + /// ([`Self::inode_nr`]) folded into its volume's namespace (see + /// [`namespace_ino`]). + namespaced_inode_nr: lx::ino_t, } impl VirtioFsInode { @@ -76,6 +80,7 @@ impl VirtioFsInode { path: RwLock::new(path), lookup_count: AtomicU64::new(1), inode_nr: stat.inode_nr, + namespaced_inode_nr: namespace_ino(volume_id, stat.inode_nr), } } @@ -91,25 +96,37 @@ impl VirtioFsInode { self.volume_id } - /// Applies this inode's volume namespace to a raw host inode number (see - /// [`namespace_ino`]). + /// This inode's own number as reported to the guest: its host inode number + /// folded into its volume's namespace (see [`namespace_ino`]). Fixed for + /// the inode's lifetime. + pub(crate) fn namespaced_inode_nr(&self) -> lx::ino_t { + self.namespaced_inode_nr + } + + /// Namespaces a raw host inode number from this inode's volume (see + /// [`namespace_ino`]). For numbers belonging to *other* inodes in the same + /// volume (e.g. readdir child entries); for this inode's own number use + /// [`Self::namespaced_inode_nr`]. pub(crate) fn namespaced_ino(&self, raw: lx::ino_t) -> lx::ino_t { namespace_ino(self.volume_id, raw) } - /// Builds a `fuse_attr` from a stat, namespacing the reported inode number - /// to this inode's volume so that aggregated siblings never alias. + /// Builds a `fuse_attr` from a stat *of this inode*, reporting its cached + /// namespaced inode number so that aggregated siblings never alias. + /// + /// `stat` must describe this inode; to report a different inode's + /// attributes (e.g. a hard-link target) call this on that inode. pub(crate) fn attr_from_stat(&self, stat: &lx::Stat) -> fuse_attr { let mut attr = util::stat_to_fuse_attr(stat); - attr.ino = self.namespaced_ino(attr.ino); + attr.ino = self.namespaced_inode_nr; attr } - /// Builds a `fuse_statx` from a statx, namespacing the reported inode - /// number to this inode's volume. + /// Builds a `fuse_statx` from a statx *of this inode*, reporting its cached + /// namespaced inode number. pub(crate) fn statx_from(&self, statx: &lx::StatEx) -> fuse_statx { let mut sx = util::statx_to_fuse_statx(statx); - sx.ino = self.namespaced_ino(sx.ino); + sx.ino = self.namespaced_inode_nr; sx } @@ -271,7 +288,9 @@ impl VirtioFsInode { pub fn link(&self, name: &LxStr, target: &VirtioFsInode) -> lx::Result { let path = self.child_path(name)?; let stat = self.volume.link_stat(&*target.get_path(), path)?; - Ok(self.attr_from_stat(&stat)) + // The reply describes the shared (target) inode, so namespace via the + // target rather than this directory inode. + Ok(target.attr_from_stat(&stat)) } /// Reads the target of the symbolic link, if this inode is a symbolic link. From 93d96c31d515d8b194984d986a3a58824b905810 Mon Sep 17 00:00:00 2001 From: Daman Mulye Date: Thu, 25 Jun 2026 14:58:04 -0700 Subject: [PATCH 06/21] edge case --- vm/devices/virtio/virtiofs/src/lib.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/vm/devices/virtio/virtiofs/src/lib.rs b/vm/devices/virtio/virtiofs/src/lib.rs index e3c00d33a5..bf0c4d745b 100644 --- a/vm/devices/virtio/virtiofs/src/lib.rs +++ b/vm/devices/virtio/virtiofs/src/lib.rs @@ -56,10 +56,6 @@ struct VirtioFsInner { } /// Implementation of the virtio-fs file system. -/// -/// Cheaply clonable: all state lives behind an `Arc`, so a device host can keep -/// a handle to call [`VirtioFs::add_root`]/[`VirtioFs::remove_root`] after the -/// device has been constructed and handed to a [`fuse::Session`]. #[derive(Clone)] pub struct VirtioFs { inner: Arc, @@ -95,8 +91,9 @@ impl Fuse for VirtioFs { fn get_attr(&self, request: &Request, flags: u32, fh: u64) -> lx::Result { let node_id = request.node_id(); // If a file handle is specified, get the attributes from the open file. This is faster on - // Windows and works if the file was deleted. - let attr = if flags & FUSE_GETATTR_FH != 0 { + // Windows and works if the file was deleted. The synthetic root's directory handle has no + // backing file, so fall through to the node-based branch for it. + let attr = if flags & FUSE_GETATTR_FH != 0 && fh != SYNTHETIC_ROOT_FH { let file = self.get_file(fh)?; file.get_attr()? } else if self.is_synthetic_root(node_id) { @@ -119,8 +116,9 @@ impl Fuse for VirtioFs { ) -> lx::Result { let node_id = request.node_id(); // If a file handle is specified, get the attributes from the open file. This is faster on - // Windows and works if the file was deleted. - let statx = if getattr_flags & FUSE_GETATTR_FH != 0 { + // Windows and works if the file was deleted. The synthetic root's directory handle has no + // backing file, so fall through to the node-based branch for it. + let statx = if getattr_flags & FUSE_GETATTR_FH != 0 && fh != SYNTHETIC_ROOT_FH { let file = self.get_file(fh)?; file.get_statx()? } else if self.is_synthetic_root(node_id) { @@ -646,6 +644,8 @@ impl HandleMap { /// globally unique, whereas inode numbers are per-volume. struct InodeMap { inodes_by_node_id: HandleMap>, + // If stable inode numbers are supported, this maps `(volume_id, inode_nr)` to the + // corresponding inode and its FUSE node ID. inodes_by_inode_nr: Option, u64)>>, /// When true, node 1 is synthetic and not stored in this map, so node IDs /// are allocated starting at 2 and `clear` does not preserve a real root. From 53b0ef30d9352e4626cd280188bb0d6340644989 Mon Sep 17 00:00:00 2001 From: Daman Mulye Date: Thu, 25 Jun 2026 16:40:00 -0700 Subject: [PATCH 07/21] Handle both ro and non-ro shares in the same synth root --- vm/devices/virtio/virtiofs/src/aggregate.rs | 105 ++++++++++---------- vm/devices/virtio/virtiofs/src/file.rs | 5 + vm/devices/virtio/virtiofs/src/inode.rs | 29 ++++-- vm/devices/virtio/virtiofs/src/lib.rs | 79 +++++++++------ vm/devices/virtio/virtiofs/src/resolver.rs | 2 +- 5 files changed, 132 insertions(+), 88 deletions(-) diff --git a/vm/devices/virtio/virtiofs/src/aggregate.rs b/vm/devices/virtio/virtiofs/src/aggregate.rs index c7a168834f..454db9a84a 100644 --- a/vm/devices/virtio/virtiofs/src/aggregate.rs +++ b/vm/devices/virtio/virtiofs/src/aggregate.rs @@ -46,6 +46,9 @@ struct ChildEntry { volume: Arc, /// Stable identifier disambiguating per-volume inode numbers. volume_id: u32, + /// Whether this child volume is read-only. Per child, so shares under one + /// aggregate device may differ; forced on when the device is read-only. + readonly: bool, } /// Registry of aggregated children for an aggregate-mode [`VirtioFs`]. @@ -99,10 +102,11 @@ impl AggregateState { impl VirtioFs { /// Expose a host folder as a named child of the synthetic root. /// + /// Each child carries its own read-only setting (from `mount_options`), so + /// shares under one aggregate device may differ. + /// /// Only valid in aggregate mode. Returns: - /// - `EINVAL` on a direct-mode file system, or if the child's `readonly` - /// setting does not match the aggregate's (every child must agree, since - /// write permission is enforced device-wide). + /// - `EINVAL` on a direct-mode file system. /// - `EAGAIN` if the device has begun tearing down (see /// [`Self::begin_teardown`]). /// - `EEXIST` if a child with the same name already exists. @@ -112,17 +116,13 @@ impl VirtioFs { root_path: impl AsRef, mount_options: Option<&LxVolumeOptions>, ) -> lx::Result<()> { - let Some(aggregate) = &self.inner.aggregate else { + let Some(aggregate) = self.inner.aggregate() else { return Err(lx::Error::EINVAL); }; - // Every child must share the aggregate's readonly setting: write - // permission is checked against the device-wide `readonly` flag, so a - // mismatched child would be silently mis-enforced. - let child_readonly = mount_options.is_some_and(|o| o.is_readonly()); - if child_readonly != self.inner.readonly { - return Err(lx::Error::EINVAL); - } + // Each child carries its own read-only setting, so shares under one + // aggregate device may differ. + let readonly = mount_options.is_some_and(|o| o.is_readonly()); // Fast-fail before paying for volume construction if the device is // already tearing down. Re-checked under the lock below to close the @@ -150,6 +150,7 @@ impl VirtioFs { name: name.to_string(), volume: Arc::new(volume), volume_id, + readonly, }); Ok(()) } @@ -162,7 +163,7 @@ impl VirtioFs { /// The running device keeps serving existing inodes until it is fully /// dropped; this only stops further children from being added. pub fn begin_teardown(&self) { - if let Some(aggregate) = &self.inner.aggregate { + if let Some(aggregate) = self.inner.aggregate() { aggregate.tearing_down.store(true, Ordering::Release); } } @@ -173,7 +174,7 @@ impl VirtioFs { /// them (each holds its own volume reference); the name simply stops /// appearing in the synthetic root. Returns `ENOENT` if no such child exists. pub fn remove_child(&self, name: &str) -> lx::Result<()> { - let Some(aggregate) = &self.inner.aggregate else { + let Some(aggregate) = self.inner.aggregate() else { return Err(lx::Error::EINVAL); }; @@ -189,13 +190,13 @@ impl VirtioFs { /// Returns true if `node_id` refers to the synthetic aggregate root. pub(crate) fn is_synthetic_root(&self, node_id: u64) -> bool { - self.inner.aggregate.is_some() && node_id == FUSE_ROOT_ID + self.inner.aggregate().is_some() && node_id == FUSE_ROOT_ID } /// Whether aggregate children should be advertised with /// `FUSE_ATTR_SUBMOUNT`. Always false in direct mode. pub(crate) fn submounts(&self) -> bool { - self.inner.aggregate.as_ref().is_some_and(|a| a.submounts) + self.inner.aggregate().is_some_and(|a| a.submounts) } /// Attributes of the synthetic aggregate root directory. @@ -222,21 +223,21 @@ impl VirtioFs { /// Looks up a named child of the synthetic root, returning an entry for the /// corresponding volume's real root inode. pub(crate) fn lookup_synthetic_root(&self, name: &lx::LxStr) -> lx::Result { - let Some(aggregate) = &self.inner.aggregate else { + let Some(aggregate) = self.inner.aggregate() else { return Err(lx::Error::ENOENT); }; let name_bytes = name.as_bytes(); - let (volume, volume_id) = { + let (volume, volume_id, readonly) = { let children = aggregate.children.read(); let entry = children .entries .iter() .find(|e| e.name.as_bytes() == name_bytes) .ok_or(lx::Error::ENOENT)?; - (Arc::clone(&entry.volume), entry.volume_id) + (Arc::clone(&entry.volume), entry.volume_id, entry.readonly) }; - let (inode, stat) = VirtioFsInode::new(volume, volume_id, PathBuf::new())?; + let (inode, stat) = VirtioFsInode::new(volume, volume_id, readonly, PathBuf::new())?; let mut attr = inode.attr_from_stat(&stat); let (_, node_id) = self.insert_inode(inode); if self.submounts() { @@ -257,7 +258,7 @@ impl VirtioFs { size: u32, plus: bool, ) -> lx::Result> { - let Some(aggregate) = &self.inner.aggregate else { + let Some(aggregate) = self.inner.aggregate() else { return Ok(Vec::new()); }; let mut buffer = Vec::with_capacity(size as usize); @@ -272,15 +273,22 @@ impl VirtioFs { n => { let child = { let children = aggregate.children.read(); - children - .entries - .get((n - 2) as usize) - .map(|e| (e.name.clone(), Arc::clone(&e.volume), e.volume_id)) + children.entries.get((n - 2) as usize).map(|e| { + (e.name.clone(), Arc::clone(&e.volume), e.volume_id, e.readonly) + }) }; - let Some((name, volume, volume_id)) = child else { + let Some((name, volume, volume_id, readonly)) = child else { break; }; - self.write_child_entry(&mut buffer, &name, volume, volume_id, next, plus)? + self.write_child_entry( + &mut buffer, + &name, + volume, + volume_id, + readonly, + next, + plus, + )? } }; if !fit { @@ -320,6 +328,7 @@ impl VirtioFs { name: &str, volume: Arc, volume_id: u32, + readonly: bool, next_off: u64, plus: bool, ) -> lx::Result { @@ -329,7 +338,7 @@ impl VirtioFs { } // readdirplus performs a lookup on each entry, incrementing its // lookup count, so create/insert the root inode here. - let (inode, stat) = VirtioFsInode::new(volume, volume_id, PathBuf::new())?; + let (inode, stat) = VirtioFsInode::new(volume, volume_id, readonly, PathBuf::new())?; let mut attr = inode.attr_from_stat(&stat); let (_, node_id) = self.insert_inode(inode); if self.submounts() { @@ -361,7 +370,7 @@ mod tests { fn aggregate_child_registry() { let a = tempfile::tempdir().unwrap(); let b = tempfile::tempdir().unwrap(); - let fs = VirtioFs::new_aggregate(false, true); + let fs = VirtioFs::new_aggregate(true); fs.add_child("share_a", a.path(), None).unwrap(); fs.add_child("share_b", b.path(), None).unwrap(); @@ -375,7 +384,7 @@ mod tests { // Each child gets a distinct, non-zero volume id (0 is reserved for // direct mode). { - let aggregate = fs.inner.aggregate.as_ref().unwrap(); + let aggregate = fs.inner.aggregate().unwrap(); let children = aggregate.children.read(); assert_eq!(children.entries.len(), 2); assert_ne!(children.entries[0].volume_id, 0); @@ -387,8 +396,7 @@ mod tests { assert_eq!(fs.remove_child("share_a").unwrap_err(), lx::Error::ENOENT); assert_eq!( fs.inner - .aggregate - .as_ref() + .aggregate() .unwrap() .children .read() @@ -414,7 +422,7 @@ mod tests { // In aggregate mode the synthetic root occupies FUSE_ROOT_ID, so the // first real inode inserted must be allocated a higher id. let a = tempfile::tempdir().unwrap(); - let fs = VirtioFs::new_aggregate(false, false); + let fs = VirtioFs::new_aggregate(false); fs.add_child("share", a.path(), None).unwrap(); let entry = fs .lookup_synthetic_root(lx::LxStr::from_bytes(b"share")) @@ -443,34 +451,30 @@ mod tests { } #[test] - fn add_child_enforces_uniform_readonly() { + fn add_child_allows_per_child_readonly() { let a = tempfile::tempdir().unwrap(); let mut ro = LxVolumeOptions::default(); ro.readonly(true); let mut rw = LxVolumeOptions::default(); rw.readonly(false); - // A read-write aggregate rejects a readonly child. - let rw_fs = VirtioFs::new_aggregate(false, false); - assert_eq!( - rw_fs.add_child("ro_child", a.path(), Some(&ro)).unwrap_err(), - lx::Error::EINVAL - ); - rw_fs.add_child("rw_child", a.path(), Some(&rw)).unwrap(); - - // A readonly aggregate rejects a read-write child. - let ro_fs = VirtioFs::new_aggregate(true, false); - assert_eq!( - ro_fs.add_child("rw_child", a.path(), Some(&rw)).unwrap_err(), - lx::Error::EINVAL - ); - ro_fs.add_child("ro_child", a.path(), Some(&ro)).unwrap(); + // A writable aggregate lets each child pick its own readonly setting. + let fs = VirtioFs::new_aggregate(false); + fs.add_child("ro_child", a.path(), Some(&ro)).unwrap(); + fs.add_child("rw_child", a.path(), Some(&rw)).unwrap(); + + let aggregate = fs.inner.aggregate().unwrap(); + let children = aggregate.children.read(); + let ro_entry = children.entries.iter().find(|e| e.name == "ro_child").unwrap(); + let rw_entry = children.entries.iter().find(|e| e.name == "rw_child").unwrap(); + assert!(ro_entry.readonly); + assert!(!rw_entry.readonly); } #[test] fn add_child_rejected_after_teardown() { let a = tempfile::tempdir().unwrap(); - let fs = VirtioFs::new_aggregate(false, false); + let fs = VirtioFs::new_aggregate(false); fs.add_child("before", a.path(), None).unwrap(); fs.begin_teardown(); @@ -482,8 +486,7 @@ mod tests { ); assert_eq!( fs.inner - .aggregate - .as_ref() + .aggregate() .unwrap() .children .read() diff --git a/vm/devices/virtio/virtiofs/src/file.rs b/vm/devices/virtio/virtiofs/src/file.rs index 84a3896397..983ca67f2d 100644 --- a/vm/devices/virtio/virtiofs/src/file.rs +++ b/vm/devices/virtio/virtiofs/src/file.rs @@ -28,6 +28,11 @@ impl VirtioFsFile { } } + /// The inode backing this open file. + pub fn inode(&self) -> &VirtioFsInode { + &self.inode + } + /// Gets the attributes of the open file. pub fn get_attr(&self) -> lx::Result { let stat = self.file.read().fstat()?.into(); diff --git a/vm/devices/virtio/virtiofs/src/inode.rs b/vm/devices/virtio/virtiofs/src/inode.rs index ff81842c18..1ba9cd15b4 100644 --- a/vm/devices/virtio/virtiofs/src/inode.rs +++ b/vm/devices/virtio/virtiofs/src/inode.rs @@ -53,6 +53,10 @@ pub struct VirtioFsInode { /// ([`Self::inode_nr`]) folded into its volume's namespace (see /// [`namespace_ino`]). namespaced_inode_nr: lx::ino_t, + /// Whether this inode's volume is read-only. Carried per inode so write + /// permission can be enforced per share in an aggregate device (each child + /// volume may differ). Inherited by descendants from their parent. + readonly: bool, } impl VirtioFsInode { @@ -60,10 +64,11 @@ impl VirtioFsInode { pub fn new( volume: Arc, volume_id: u32, + readonly: bool, path: PathBuf, ) -> lx::Result<(Self, lx::Stat)> { let stat = volume.lstat(&path)?; - let inode = Self::with_attr(volume, volume_id, path, &stat); + let inode = Self::with_attr(volume, volume_id, readonly, path, &stat); Ok((inode, stat)) } @@ -71,6 +76,7 @@ impl VirtioFsInode { pub fn with_attr( volume: Arc, volume_id: u32, + readonly: bool, path: PathBuf, stat: &lx::Stat, ) -> Self { @@ -81,6 +87,7 @@ impl VirtioFsInode { lookup_count: AtomicU64::new(1), inode_nr: stat.inode_nr, namespaced_inode_nr: namespace_ino(volume_id, stat.inode_nr), + readonly, } } @@ -96,6 +103,11 @@ impl VirtioFsInode { self.volume_id } + /// Whether this inode's volume is read-only. + pub fn readonly(&self) -> bool { + self.readonly + } + /// This inode's own number as reported to the guest: its host inode number /// folded into its volume's namespace (see [`namespace_ino`]). Fixed for /// the inode's lifetime. @@ -171,7 +183,8 @@ impl VirtioFsInode { /// Performs a lookup for a child of this inode. pub fn lookup_child(&self, name: &LxStr) -> lx::Result<(VirtioFsInode, fuse_attr)> { let path = self.child_path(name)?; - let (inode, stat) = VirtioFsInode::new(Arc::clone(&self.volume), self.volume_id, path)?; + let (inode, stat) = + VirtioFsInode::new(Arc::clone(&self.volume), self.volume_id, self.readonly, path)?; let attr = inode.attr_from_stat(&stat); Ok((inode, attr)) } @@ -220,7 +233,8 @@ impl VirtioFsInode { let flags = (flags as i32) | lx::O_CREAT | lx::O_NOFOLLOW; let file = self.volume.open(&path, flags, Some(options))?; let stat = file.fstat()?.into(); - let inode = Self::with_attr(Arc::clone(&self.volume), self.volume_id, path, &stat); + let inode = + Self::with_attr(Arc::clone(&self.volume), self.volume_id, self.readonly, path, &stat); let attr = inode.attr_from_stat(&stat); Ok((inode, attr, file)) } @@ -238,7 +252,8 @@ impl VirtioFsInode { .volume .mkdir_stat(&path, LxCreateOptions::new(mode, uid, gid))?; - let inode = Self::with_attr(Arc::clone(&self.volume), self.volume_id, path, &stat); + let inode = + Self::with_attr(Arc::clone(&self.volume), self.volume_id, self.readonly, path, &stat); let attr = inode.attr_from_stat(&stat); Ok((inode, attr)) } @@ -259,7 +274,8 @@ impl VirtioFsInode { device_id as usize, )?; - let inode = Self::with_attr(Arc::clone(&self.volume), self.volume_id, path, &stat); + let inode = + Self::with_attr(Arc::clone(&self.volume), self.volume_id, self.readonly, path, &stat); let attr = inode.attr_from_stat(&stat); Ok((inode, attr)) } @@ -279,7 +295,8 @@ impl VirtioFsInode { LxCreateOptions::new(lx::S_IFLNK | 0o777, uid, gid), )?; - let inode = Self::with_attr(Arc::clone(&self.volume), self.volume_id, path, &stat); + let inode = + Self::with_attr(Arc::clone(&self.volume), self.volume_id, self.readonly, path, &stat); let attr = inode.attr_from_stat(&stat); Ok((inode, attr)) } diff --git a/vm/devices/virtio/virtiofs/src/lib.rs b/vm/devices/virtio/virtiofs/src/lib.rs index bf0c4d745b..59d70ce8b0 100644 --- a/vm/devices/virtio/virtiofs/src/lib.rs +++ b/vm/devices/virtio/virtiofs/src/lib.rs @@ -49,10 +49,30 @@ const ENTRY_TIMEOUT: Duration = Duration::from_secs(0); struct VirtioFsInner { inodes: RwLock, files: RwLock>>, - readonly: bool, - /// `None` in direct (single-root) mode; `Some` only for aggregate devices, - /// carrying the synthetic root's registry and lifecycle state. - aggregate: Option, + mode: VirtioFsMode, +} + +/// Distinguishes a single-share device from a multi-share aggregate. +/// +/// The read-only setting is not tracked here: in direct mode it is baked into +/// the inodes (each carries its volume's setting), and in aggregate mode each +/// child carries its own (see [`AggregateState`]). +enum VirtioFsMode { + /// Single share: node 1 is a real inode at the volume root. + Direct, + /// Multi-share: node 1 is a synthetic directory whose children are + /// independent host folders. + Aggregate(AggregateState), +} + +impl VirtioFsInner { + /// The aggregate state, or `None` for a direct (single-share) device. + fn aggregate(&self) -> Option<&AggregateState> { + match &self.mode { + VirtioFsMode::Aggregate(state) => Some(state), + VirtioFsMode::Direct => None, + } + } } /// Implementation of the virtio-fs file system. @@ -140,7 +160,7 @@ impl Fuse for VirtioFs { let file = self.get_file(arg.fh)?; // Block truncation and other modifications on readonly filesystems if arg.valid & !(FATTR_FH | FATTR_LOCKOWNER) != 0 { - self.check_writable()?; + self.check_writable(file.inode())?; } file.set_attr(arg, request.uid())?; file.get_attr()? @@ -148,7 +168,7 @@ impl Fuse for VirtioFs { let inode = self.get_inode(node_id)?; // Block truncation and other modifications on readonly filesystems if arg.valid & !(FATTR_FH | FATTR_LOCKOWNER) != 0 { - self.check_writable()?; + self.check_writable(&inode)?; } inode.set_attr(arg, request.uid())? }; @@ -193,7 +213,7 @@ impl Fuse for VirtioFs { arg: &fuse_create_in, ) -> lx::Result { let inode = self.get_inode(request.node_id())?; - self.check_writable()?; + self.check_writable(&inode)?; let (new_inode, attr, file) = inode.create(name, arg.flags, arg.mode, request.uid(), request.gid())?; @@ -216,7 +236,7 @@ impl Fuse for VirtioFs { arg: &fuse_mkdir_in, ) -> lx::Result { let inode = self.get_inode(request.node_id())?; - self.check_writable()?; + self.check_writable(&inode)?; let (new_inode, attr) = inode.mkdir(name, arg.mode, request.uid(), request.gid())?; let (_, node_id) = self.insert_inode(new_inode); Ok(fuse_entry_out::new( @@ -234,7 +254,7 @@ impl Fuse for VirtioFs { arg: &fuse_mknod_in, ) -> lx::Result { let inode = self.get_inode(request.node_id())?; - self.check_writable()?; + self.check_writable(&inode)?; let (new_inode, attr) = inode.mknod(name, arg.mode, request.uid(), request.gid(), arg.rdev)?; @@ -254,7 +274,7 @@ impl Fuse for VirtioFs { target: &lx::LxStr, ) -> lx::Result { let inode = self.get_inode(request.node_id())?; - self.check_writable()?; + self.check_writable(&inode)?; let (new_inode, attr) = inode.symlink(name, target, request.uid(), request.gid())?; let (_, node_id) = self.insert_inode(new_inode); @@ -269,7 +289,7 @@ impl Fuse for VirtioFs { fn link(&self, request: &Request, name: &lx::LxStr, target: u64) -> lx::Result { let inode = self.get_inode(request.node_id())?; let target_inode = self.get_inode(target)?; - self.check_writable()?; + self.check_writable(&inode)?; let attr = inode.link(name, &target_inode)?; // Increment the lookup count since we're returning an entry for this inode. @@ -300,7 +320,7 @@ impl Fuse for VirtioFs { fn write(&self, request: &Request, arg: &fuse_write_in, data: &[u8]) -> lx::Result { let file = self.get_file(arg.fh)?; - self.check_writable()?; + self.check_writable(file.inode())?; file.write(data, arg.offset, request.uid()) } @@ -364,7 +384,7 @@ impl Fuse for VirtioFs { if inode.volume_id() != new_inode.volume_id() { return Err(lx::Error::EXDEV); } - self.check_writable()?; + self.check_writable(&inode)?; inode.rename(name, &new_inode, new_name, flags) } @@ -409,7 +429,7 @@ impl Fuse for VirtioFs { flags: u32, ) -> lx::Result<()> { let inode = self.get_inode(request.node_id())?; - self.check_writable()?; + self.check_writable(&inode)?; inode.set_xattr(name, value, flags) } @@ -430,7 +450,7 @@ impl Fuse for VirtioFs { fn remove_xattr(&self, request: &Request, name: &lx::LxStr) -> lx::Result<()> { let inode = self.get_inode(request.node_id())?; - self.check_writable()?; + self.check_writable(&inode)?; inode.remove_xattr(name) } @@ -442,9 +462,9 @@ impl Fuse for VirtioFs { } impl VirtioFs { - /// Check if the filesystem is readonly and return EROFS if so. - fn check_writable(&self) -> lx::Result<()> { - if self.inner.readonly { + /// Check if the inode's volume is readonly and return EROFS if so. + fn check_writable(&self, inode: &VirtioFsInode) -> lx::Result<()> { + if inode.readonly() { Err(lx::Error::EROFS) } else { Ok(()) @@ -453,7 +473,7 @@ impl VirtioFs { /// Check whether the open flags are permitted on a read-only filesystem. fn check_open_readonly(&self, inode: &VirtioFsInode, flags: u32) -> lx::Result<()> { - if !self.inner.readonly { + if !inode.readonly() { return Ok(()); } @@ -494,33 +514,32 @@ impl VirtioFs { lxutil::LxVolume::new(root_path) }?; let mut inodes = InodeMap::new(volume.supports_stable_file_id(), false); - let (root_inode, _) = VirtioFsInode::new(Arc::new(volume), 0, PathBuf::new())?; + let (root_inode, _) = VirtioFsInode::new(Arc::new(volume), 0, readonly, PathBuf::new())?; assert!(inodes.insert(root_inode).1 == FUSE_ROOT_ID); Ok(Self { inner: Arc::new(VirtioFsInner { inodes: RwLock::new(inodes), files: RwLock::new(HandleMap::new()), - readonly, - aggregate: None, + mode: VirtioFsMode::Direct, }), }) } /// Create a new, empty aggregate virtio-fs. /// - /// Node 1 is a synthetic, read-only directory; use [`Self::add_root`] to - /// expose host folders as named children. When `submounts` is set, each - /// child is advertised with `FUSE_ATTR_SUBMOUNT` so the guest kernel gives - /// it a distinct `st_dev` (only honored once `FUSE_SUBMOUNTS` is negotiated). - pub fn new_aggregate(readonly: bool, submounts: bool) -> Self { + /// Node 1 is a synthetic, read-only directory; use [`Self::add_child`] to + /// expose host folders as named children, each with its own read-only + /// setting. When `submounts` is set, each child is advertised with + /// `FUSE_ATTR_SUBMOUNT` so the guest kernel gives it a distinct `st_dev` + /// (only honored once `FUSE_SUBMOUNTS` is negotiated). + pub fn new_aggregate(submounts: bool) -> Self { Self { inner: Arc::new(VirtioFsInner { // Inode numbers are deduplicated per volume (see `InodeMap`), so // enable the stable-id map and key it by `(volume_id, ino)`. inodes: RwLock::new(InodeMap::new(true, true)), files: RwLock::new(HandleMap::new()), - readonly, - aggregate: Some(AggregateState::new(submounts)), + mode: VirtioFsMode::Aggregate(AggregateState::new(submounts)), }), } } @@ -539,7 +558,7 @@ impl VirtioFs { /// Removes a file or directory. fn unlink_helper(&self, request: &Request, name: &lx::LxStr, flags: i32) -> lx::Result<()> { let inode = self.get_inode(request.node_id())?; - self.check_writable()?; + self.check_writable(&inode)?; inode.unlink(name, flags) } diff --git a/vm/devices/virtio/virtiofs/src/resolver.rs b/vm/devices/virtio/virtiofs/src/resolver.rs index e21d69a89a..154bdd26c7 100644 --- a/vm/devices/virtio/virtiofs/src/resolver.rs +++ b/vm/devices/virtio/virtiofs/src/resolver.rs @@ -60,7 +60,7 @@ impl ResolveResource for VirtioFsResolver { anyhow::bail!("section fs not supported on this platform") } VirtioFsBackend::Aggregate { children } => { - let fs = VirtioFs::new_aggregate(false, true); + let fs = VirtioFs::new_aggregate(true); for child in children { fs.add_child( &child.name, From 00a0a6710ec1b206c0a90e4b93364173327c8c93 Mon Sep 17 00:00:00 2001 From: Daman Mulye Date: Thu, 25 Jun 2026 16:45:12 -0700 Subject: [PATCH 08/21] revert FAT/exFAT changes for now --- .../support/fs/lxutil/src/windows/fs.rs | 36 ++++----- .../support/fs/lxutil/src/windows/mod.rs | 80 +------------------ .../support/fs/lxutil/src/windows/util.rs | 45 +++++------ 3 files changed, 38 insertions(+), 123 deletions(-) diff --git a/vm/devices/support/fs/lxutil/src/windows/fs.rs b/vm/devices/support/fs/lxutil/src/windows/fs.rs index bff2949a8d..dad942543f 100644 --- a/vm/devices/support/fs/lxutil/src/windows/fs.rs +++ b/vm/devices/support/fs/lxutil/src/windows/fs.rs @@ -295,24 +295,13 @@ pub fn analyze_delete_error(error: lx::Error, file_handle: &OwnedHandle) -> Dele { DeleteErrorAction::ReturnError(error) } else if error.value() == lx::EIO { - // EIO can come from STATUS_CANNOT_DELETE. Inspect the file's attributes - // to decide how to proceed: - // - a non-empty directory surfaces as STATUS_CANNOT_DELETE, so map it to - // ENOTEMPTY; - // - a read-only file cannot be deleted via the non-POSIX - // FileDispositionInformation path, so clear the read-only attribute and - // retry; - // - anything else (including a genuine I/O failure, or a handle without - // FILE_READ_ATTRIBUTES) is returned as-is so the read-only attribute is - // not disturbed. - match util::query_information_file::(file_handle) { - Ok(info) if info.FileAttributes & W32Fs::FILE_ATTRIBUTE_DIRECTORY.0 != 0 => { - DeleteErrorAction::ReturnError(lx::Error::ENOTEMPTY) - } - Ok(info) if info.FileAttributes & W32Fs::FILE_ATTRIBUTE_READONLY.0 != 0 => { - DeleteErrorAction::TryReadOnlyWorkaround - } - _ => DeleteErrorAction::ReturnError(error), + // EIO can come from STATUS_CANNOT_DELETE, which for directories means + // the directory is not empty. Check if this is a directory and return + // ENOTEMPTY in that case. + if is_directory(file_handle) { + DeleteErrorAction::ReturnError(lx::Error::ENOTEMPTY) + } else { + DeleteErrorAction::ReturnError(error) } } else { DeleteErrorAction::TryReadOnlyWorkaround @@ -333,6 +322,17 @@ pub fn delete_file(fs_context: &FsContext, file_handle: &OwnedHandle) -> lx::Res } } +/// Check if a file handle refers to a directory. +fn is_directory(file_handle: &OwnedHandle) -> bool { + if let Ok(info) = + util::query_information_file::(file_handle) + { + info.FileAttributes & W32Fs::FILE_ATTRIBUTE_DIRECTORY.0 != 0 + } else { + false + } +} + pub fn delete_file_core(fs_context: &FsContext, file_handle: &OwnedHandle) -> lx::Result<()> { if fs_context .compatibility_flags diff --git a/vm/devices/support/fs/lxutil/src/windows/mod.rs b/vm/devices/support/fs/lxutil/src/windows/mod.rs index a21cc75e11..3622f89837 100644 --- a/vm/devices/support/fs/lxutil/src/windows/mod.rs +++ b/vm/devices/support/fs/lxutil/src/windows/mod.rs @@ -405,64 +405,6 @@ impl LxVolume { Default::default(), )?; - // NT's POSIX rename (used by NTFS) natively enforces several Linux rename - // semantics. The classic non-POSIX path used by FAT/exFAT does not, so - // replicate them explicitly here. NTFS is left untouched: it returns the - // same errors from NT directly. - if !self - .state - .fs_context - .compatibility_flags - .supports_posix_unlink_rename() - { - let is_dir = |h: &OwnedHandle| -> lx::Result { - Ok( - util::query_information_file::(h)? - .FileAttributes - & W32Fs::FILE_ATTRIBUTE_DIRECTORY.0 - != 0, - ) - }; - let source_is_dir = is_dir(&handle)?; - - // Linux returns EINVAL when renaming a directory into one of its own - // descendants; NT's classic rename instead reports STATUS_ACCESS_DENIED - // (-> EACCES). `Path::starts_with` compares whole path components, so - // "a/bc" is not treated as a descendant of "a/b". Only directories can - // have descendants, so non-directory sources are unaffected. - // N.B. The comparison is case-sensitive, so a case-only descendant such - // as renaming "a/b" into "a/B/c" on a case-insensitive volume is not - // caught here; NT still rejects it (as EACCES) so no data is lost. - if source_is_dir && new_path != path && new_path.starts_with(path) { - return Err(lx::Error::EINVAL); - } - - // Linux forbids replacing a file with a directory (ENOTDIR) or a - // directory with a file (EISDIR). The classic non-POSIX path would - // instead silently supersede a file with a directory and report only a - // generic collision for the reverse, so detect type-incompatible targets - // up front before the destructive rename is attempted. - // N.B. `open_file` always sets FILE_OPEN_REPARSE_POINT, so a symlink - // target is classified as the (non-directory) reparse point itself, - // matching Linux. On case-insensitive volumes a case-only rename - // (foo -> FOO) opens the same object as source and target, yielding - // equal types and thus no false error. - match self.open_file(new_path, W32Fs::FILE_READ_ATTRIBUTES, Default::default()) { - Ok(target_handle) => { - let target_is_dir = is_dir(&target_handle)?; - if source_is_dir && !target_is_dir { - return Err(lx::Error::ENOTDIR); - } - if !source_is_dir && target_is_dir { - return Err(lx::Error::EISDIR); - } - } - // A missing target is fine; the rename creates it. - Err(err) if err.value() == lx::ENOENT => {} - Err(err) => return Err(err), - } - } - let flags = fs::RenameFlags::default(); let error = match fs::rename(&handle, &self.root, new_path, &self.state.fs_context, flags) { Ok(_) => return Ok(()), @@ -483,11 +425,7 @@ impl LxVolume { .compatibility_flags .supports_posix_unlink_rename() { - match self.open_file( - new_path, - W32Fs::FILE_READ_ATTRIBUTES | W32Fs::DELETE, - Default::default(), - ) { + match self.open_file(new_path, W32Fs::DELETE, Default::default()) { Ok(target_handle) => self.delete_file(&target_handle)?, Err(err) => { // ENOENT means the rename can proceed. @@ -762,22 +700,6 @@ impl LxVolume { assert!(path.is_relative() && !path.as_os_str().is_empty()); self.check_sandbox_enforcement(path)?; - // Symlinks are always implemented as reparse points (either an LX - // symlink reparse tag or an NT symlink). Filesystems that don't support - // reparse points (e.g. FAT) cannot store a symlink at all, so report - // EPERM up front rather than creating the file and then failing to set - // the reparse point (which would surface as EOPNOTSUPP). This matches - // the conventional Linux errno for symlink() on such filesystems and - // mirrors the hard link check in link_helper. - if !self - .state - .fs_context - .compatibility_flags - .supports_reparse_points() - { - return Err(lx::Error::EPERM); - } - // Convert the target to its native Windows format. let win_target = Path::from_lx(target); diff --git a/vm/devices/support/fs/lxutil/src/windows/util.rs b/vm/devices/support/fs/lxutil/src/windows/util.rs index 126c32f299..41f2fe71e8 100644 --- a/vm/devices/support/fs/lxutil/src/windows/util.rs +++ b/vm/devices/support/fs/lxutil/src/windows/util.rs @@ -155,32 +155,29 @@ fn get_token_for_access_check() -> lx::Result { let mut client_token_raw = Foundation::HANDLE::default(); let mut duplicate_token_raw = Foundation::HANDLE::default(); // SAFETY: Calling Win32 API as documented. - // - // N.B. The raw NTSTATUS is inspected directly (rather than going through - // `check_status`) so that `STATUS_NO_TOKEN` can be distinguished from - // other failures. `check_status` maps the NTSTATUS to an `lx::Error` - // errno, which cannot be compared against an NTSTATUS constant. - let status = unsafe { - FileSystem::NtOpenThreadToken( + let result = unsafe { + check_status(FileSystem::NtOpenThreadToken( Threading::GetCurrentThread(), Security::TOKEN_QUERY.0 | Security::TOKEN_DUPLICATE.0, true, &mut client_token_raw, - ) + )) }; - if status == Foundation::STATUS_NO_TOKEN { - // The thread is not impersonating, so use the process token instead. - // SAFETY: Calling Win32 API as documented. - unsafe { - let _ = check_status(FileSystem::NtOpenProcessToken( - Threading::GetCurrentProcess(), - Security::TOKEN_QUERY.0 | Security::TOKEN_DUPLICATE.0, - &mut client_token_raw, - ))?; + if let Err(e) = result { + // Use the process token if there's no token. + if e.value() == Foundation::STATUS_NO_TOKEN.0 { + // SAFETY: Calling Win32 API as documented. + unsafe { + let _ = check_status(FileSystem::NtOpenProcessToken( + Threading::GetCurrentProcess(), + Security::TOKEN_QUERY.0 | Security::TOKEN_DUPLICATE.0, + &mut client_token_raw, + ))?; + } + } else { + return Err(e); } - } else { - let _ = check_status(status)?; } // Create the RAII handle now so it gets closed on drop if we need @@ -216,10 +213,7 @@ fn get_token_for_access_check() -> lx::Result { }, EffectiveOnly: false, }; - let mut object_attributes = Wdk::Foundation::OBJECT_ATTRIBUTES { - Length: size_of::() as u32, - ..Default::default() - }; + let mut object_attributes = Wdk::Foundation::OBJECT_ATTRIBUTES::default(); object_attributes.SecurityQualityOfService = ptr::from_ref::(&security_qos).cast(); @@ -737,14 +731,13 @@ pub fn set_information_file( // SAFETY: Calling NtSetInformationFile as documented. unsafe { - let status = FileSystem::NtSetInformationFile( + let _ = check_status(FileSystem::NtSetInformationFile( Foundation::HANDLE(handle.as_raw_handle()), &mut iosb, buf.cast(), len.try_into().unwrap(), info.file_information_class(), - ); - let _ = check_status(status)?; + ))?; Ok(()) } From e59df7471bc4b232612c678686767f4b6b14185f Mon Sep 17 00:00:00 2001 From: Daman Mulye Date: Thu, 25 Jun 2026 16:53:59 -0700 Subject: [PATCH 09/21] fmt and clippy --- vm/devices/virtio/virtiofs/src/aggregate.rs | 37 ++++++++--------- vm/devices/virtio/virtiofs/src/inode.rs | 44 ++++++++++++++++----- 2 files changed, 53 insertions(+), 28 deletions(-) diff --git a/vm/devices/virtio/virtiofs/src/aggregate.rs b/vm/devices/virtio/virtiofs/src/aggregate.rs index 454db9a84a..f2a9396066 100644 --- a/vm/devices/virtio/virtiofs/src/aggregate.rs +++ b/vm/devices/virtio/virtiofs/src/aggregate.rs @@ -274,7 +274,12 @@ impl VirtioFs { let child = { let children = aggregate.children.read(); children.entries.get((n - 2) as usize).map(|e| { - (e.name.clone(), Arc::clone(&e.volume), e.volume_id, e.readonly) + ( + e.name.clone(), + Arc::clone(&e.volume), + e.volume_id, + e.readonly, + ) }) }; let Some((name, volume, volume_id, readonly)) = child else { @@ -351,7 +356,7 @@ impl VirtioFs { // inode number (namespaced to its volume), falling back to the // volume id if it is inaccessible. let raw = volume - .lstat(&PathBuf::new()) + .lstat(PathBuf::new()) .map(|s| s.inode_nr) .unwrap_or(volume_id as lx::ino_t); let ino = inode::namespace_ino(volume_id, raw); @@ -395,13 +400,7 @@ mod tests { fs.remove_child("share_a").unwrap(); assert_eq!(fs.remove_child("share_a").unwrap_err(), lx::Error::ENOENT); assert_eq!( - fs.inner - .aggregate() - .unwrap() - .children - .read() - .entries - .len(), + fs.inner.aggregate().unwrap().children.read().entries.len(), 1 ); } @@ -465,8 +464,16 @@ mod tests { let aggregate = fs.inner.aggregate().unwrap(); let children = aggregate.children.read(); - let ro_entry = children.entries.iter().find(|e| e.name == "ro_child").unwrap(); - let rw_entry = children.entries.iter().find(|e| e.name == "rw_child").unwrap(); + let ro_entry = children + .entries + .iter() + .find(|e| e.name == "ro_child") + .unwrap(); + let rw_entry = children + .entries + .iter() + .find(|e| e.name == "rw_child") + .unwrap(); assert!(ro_entry.readonly); assert!(!rw_entry.readonly); } @@ -485,13 +492,7 @@ mod tests { lx::Error::EAGAIN ); assert_eq!( - fs.inner - .aggregate() - .unwrap() - .children - .read() - .entries - .len(), + fs.inner.aggregate().unwrap().children.read().entries.len(), 1 ); } diff --git a/vm/devices/virtio/virtiofs/src/inode.rs b/vm/devices/virtio/virtiofs/src/inode.rs index 1ba9cd15b4..b82a8e006d 100644 --- a/vm/devices/virtio/virtiofs/src/inode.rs +++ b/vm/devices/virtio/virtiofs/src/inode.rs @@ -183,8 +183,12 @@ impl VirtioFsInode { /// Performs a lookup for a child of this inode. pub fn lookup_child(&self, name: &LxStr) -> lx::Result<(VirtioFsInode, fuse_attr)> { let path = self.child_path(name)?; - let (inode, stat) = - VirtioFsInode::new(Arc::clone(&self.volume), self.volume_id, self.readonly, path)?; + let (inode, stat) = VirtioFsInode::new( + Arc::clone(&self.volume), + self.volume_id, + self.readonly, + path, + )?; let attr = inode.attr_from_stat(&stat); Ok((inode, attr)) } @@ -233,8 +237,13 @@ impl VirtioFsInode { let flags = (flags as i32) | lx::O_CREAT | lx::O_NOFOLLOW; let file = self.volume.open(&path, flags, Some(options))?; let stat = file.fstat()?.into(); - let inode = - Self::with_attr(Arc::clone(&self.volume), self.volume_id, self.readonly, path, &stat); + let inode = Self::with_attr( + Arc::clone(&self.volume), + self.volume_id, + self.readonly, + path, + &stat, + ); let attr = inode.attr_from_stat(&stat); Ok((inode, attr, file)) } @@ -252,8 +261,13 @@ impl VirtioFsInode { .volume .mkdir_stat(&path, LxCreateOptions::new(mode, uid, gid))?; - let inode = - Self::with_attr(Arc::clone(&self.volume), self.volume_id, self.readonly, path, &stat); + let inode = Self::with_attr( + Arc::clone(&self.volume), + self.volume_id, + self.readonly, + path, + &stat, + ); let attr = inode.attr_from_stat(&stat); Ok((inode, attr)) } @@ -274,8 +288,13 @@ impl VirtioFsInode { device_id as usize, )?; - let inode = - Self::with_attr(Arc::clone(&self.volume), self.volume_id, self.readonly, path, &stat); + let inode = Self::with_attr( + Arc::clone(&self.volume), + self.volume_id, + self.readonly, + path, + &stat, + ); let attr = inode.attr_from_stat(&stat); Ok((inode, attr)) } @@ -295,8 +314,13 @@ impl VirtioFsInode { LxCreateOptions::new(lx::S_IFLNK | 0o777, uid, gid), )?; - let inode = - Self::with_attr(Arc::clone(&self.volume), self.volume_id, self.readonly, path, &stat); + let inode = Self::with_attr( + Arc::clone(&self.volume), + self.volume_id, + self.readonly, + path, + &stat, + ); let attr = inode.attr_from_stat(&stat); Ok((inode, attr)) } From e3058b1cd58b344df00bee32c01644c2568aa80d Mon Sep 17 00:00:00 2001 From: Daman Mulye Date: Fri, 26 Jun 2026 09:47:18 -0700 Subject: [PATCH 10/21] feedback --- vm/devices/virtio/virtiofs/src/aggregate.rs | 28 ++++++++++++-- vm/devices/virtio/virtiofs/src/lib.rs | 43 +++++++++++++++++++++ 2 files changed, 67 insertions(+), 4 deletions(-) diff --git a/vm/devices/virtio/virtiofs/src/aggregate.rs b/vm/devices/virtio/virtiofs/src/aggregate.rs index f2a9396066..aa58e9e1b9 100644 --- a/vm/devices/virtio/virtiofs/src/aggregate.rs +++ b/vm/devices/virtio/virtiofs/src/aggregate.rs @@ -106,7 +106,8 @@ impl VirtioFs { /// shares under one aggregate device may differ. /// /// Only valid in aggregate mode. Returns: - /// - `EINVAL` on a direct-mode file system. + /// - `EINVAL` on a direct-mode file system, or if `name` is empty, + /// reserved (`.`/`..`), or contains `/` or `\0`. /// - `EAGAIN` if the device has begun tearing down (see /// [`Self::begin_teardown`]). /// - `EEXIST` if a child with the same name already exists. @@ -120,6 +121,17 @@ impl VirtioFs { return Err(lx::Error::EINVAL); }; + // Reject names that are empty, reserved ("." / ".."), or contain + // characters invalid in a POSIX directory entry ('/' or '\0'). + if name.is_empty() + || name == "." + || name == ".." + || name.contains('/') + || name.contains('\0') + { + return Err(lx::Error::EINVAL); + } + // Each child carries its own read-only setting, so shares under one // aggregate device may differ. let readonly = mount_options.is_some_and(|o| o.is_readonly()); @@ -212,7 +224,13 @@ impl VirtioFs { /// Extended attributes of the synthetic aggregate root directory. pub(crate) fn synthetic_root_statx(mask: lx::StatExMask) -> fuse_statx { let mut sx = fuse_statx::new_zeroed(); - sx.mask = mask.into_bits(); + let returned_mask = lx::StatExMask::new() + .with_file_type(true) + .with_mode(true) + .with_nlink(true) + .with_ino(true) + .into_bits(); + sx.mask = mask.into_bits() & returned_mask; sx.mode = (lx::S_IFDIR | 0o555) as u16; sx.nlink = 2; sx.ino = FUSE_ROOT_ID; @@ -266,7 +284,9 @@ impl VirtioFs { // Entry 0 => ".", 1 => "..", 2.. => children[index - 2]. let mut index = offset; loop { - let next = index + 1; + let Some(next) = index.checked_add(1) else { + break; + }; let fit = match index { 0 => self.write_synthetic_dot(&mut buffer, ".", next, plus), 1 => self.write_synthetic_dot(&mut buffer, "..", next, plus), @@ -299,7 +319,7 @@ impl VirtioFs { if !fit { break; } - index += 1; + index = next; } Ok(buffer) } diff --git a/vm/devices/virtio/virtiofs/src/lib.rs b/vm/devices/virtio/virtiofs/src/lib.rs index 59d70ce8b0..da7327bc77 100644 --- a/vm/devices/virtio/virtiofs/src/lib.rs +++ b/vm/devices/virtio/virtiofs/src/lib.rs @@ -154,6 +154,10 @@ impl Fuse for VirtioFs { fn set_attr(&self, request: &Request, arg: &fuse_setattr_in) -> lx::Result { let node_id = request.node_id(); + if self.is_synthetic_root(node_id) { + return Err(lx::Error::EROFS); + } + // If a file handle is specified, set the attributes on the open file. This is faster on // Windows and works if the file was deleted. let attr = if arg.valid & FATTR_FH != 0 { @@ -212,6 +216,9 @@ impl Fuse for VirtioFs { name: &lx::LxStr, arg: &fuse_create_in, ) -> lx::Result { + if self.is_synthetic_root(request.node_id()) { + return Err(lx::Error::EROFS); + } let inode = self.get_inode(request.node_id())?; self.check_writable(&inode)?; let (new_inode, attr, file) = @@ -235,6 +242,9 @@ impl Fuse for VirtioFs { name: &lx::LxStr, arg: &fuse_mkdir_in, ) -> lx::Result { + if self.is_synthetic_root(request.node_id()) { + return Err(lx::Error::EROFS); + } let inode = self.get_inode(request.node_id())?; self.check_writable(&inode)?; let (new_inode, attr) = inode.mkdir(name, arg.mode, request.uid(), request.gid())?; @@ -253,6 +263,9 @@ impl Fuse for VirtioFs { name: &lx::LxStr, arg: &fuse_mknod_in, ) -> lx::Result { + if self.is_synthetic_root(request.node_id()) { + return Err(lx::Error::EROFS); + } let inode = self.get_inode(request.node_id())?; self.check_writable(&inode)?; let (new_inode, attr) = @@ -273,6 +286,9 @@ impl Fuse for VirtioFs { name: &lx::LxStr, target: &lx::LxStr, ) -> lx::Result { + if self.is_synthetic_root(request.node_id()) { + return Err(lx::Error::EROFS); + } let inode = self.get_inode(request.node_id())?; self.check_writable(&inode)?; let (new_inode, attr) = inode.symlink(name, target, request.uid(), request.gid())?; @@ -287,6 +303,9 @@ impl Fuse for VirtioFs { } fn link(&self, request: &Request, name: &lx::LxStr, target: u64) -> lx::Result { + if self.is_synthetic_root(request.node_id()) { + return Err(lx::Error::EROFS); + } let inode = self.get_inode(request.node_id())?; let target_inode = self.get_inode(target)?; self.check_writable(&inode)?; @@ -378,6 +397,9 @@ impl Fuse for VirtioFs { new_name: &lx::LxStr, flags: u32, ) -> lx::Result<()> { + if self.is_synthetic_root(request.node_id()) || self.is_synthetic_root(new_dir) { + return Err(lx::Error::EROFS); + } let inode = self.get_inode(request.node_id())?; let new_inode = self.get_inode(new_dir)?; // A rename cannot cross aggregated volume boundaries. @@ -407,6 +429,9 @@ impl Fuse for VirtioFs { } fn get_xattr(&self, request: &Request, name: &lx::LxStr, size: u32) -> lx::Result> { + if self.is_synthetic_root(request.node_id()) { + return Err(lx::Error::ENODATA); + } let inode = self.get_inode(request.node_id())?; let mut value = vec![0u8; size as usize]; let size = inode.get_xattr(name, Some(&mut value))?; @@ -415,6 +440,9 @@ impl Fuse for VirtioFs { } fn get_xattr_size(&self, request: &Request, name: &lx::LxStr) -> lx::Result { + if self.is_synthetic_root(request.node_id()) { + return Err(lx::Error::ENODATA); + } let inode = self.get_inode(request.node_id())?; let size = inode.get_xattr(name, None)?; let size = size.try_into().map_err(|_| lx::Error::E2BIG)?; @@ -428,12 +456,18 @@ impl Fuse for VirtioFs { value: &[u8], flags: u32, ) -> lx::Result<()> { + if self.is_synthetic_root(request.node_id()) { + return Err(lx::Error::EROFS); + } let inode = self.get_inode(request.node_id())?; self.check_writable(&inode)?; inode.set_xattr(name, value, flags) } fn list_xattr(&self, request: &Request, size: u32) -> lx::Result> { + if self.is_synthetic_root(request.node_id()) { + return Ok(Vec::new()); + } let inode = self.get_inode(request.node_id())?; let mut list = vec![0u8; size as usize]; let size = inode.list_xattr(Some(&mut list))?; @@ -442,6 +476,9 @@ impl Fuse for VirtioFs { } fn list_xattr_size(&self, request: &Request) -> lx::Result { + if self.is_synthetic_root(request.node_id()) { + return Ok(0); + } let inode = self.get_inode(request.node_id())?; let size = inode.list_xattr(None)?; let size = size.try_into().map_err(|_| lx::Error::E2BIG)?; @@ -449,6 +486,9 @@ impl Fuse for VirtioFs { } fn remove_xattr(&self, request: &Request, name: &lx::LxStr) -> lx::Result<()> { + if self.is_synthetic_root(request.node_id()) { + return Err(lx::Error::EROFS); + } let inode = self.get_inode(request.node_id())?; self.check_writable(&inode)?; inode.remove_xattr(name) @@ -557,6 +597,9 @@ impl VirtioFs { /// Removes a file or directory. fn unlink_helper(&self, request: &Request, name: &lx::LxStr, flags: i32) -> lx::Result<()> { + if self.is_synthetic_root(request.node_id()) { + return Err(lx::Error::EROFS); + } let inode = self.get_inode(request.node_id())?; self.check_writable(&inode)?; inode.unlink(name, flags) From 947368552d47de08e03773cf40001f8740b39c64 Mon Sep 17 00:00:00 2001 From: Daman Mulye Date: Fri, 26 Jun 2026 10:03:46 -0700 Subject: [PATCH 11/21] update comment --- vm/devices/virtio/virtiofs/src/inode.rs | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/vm/devices/virtio/virtiofs/src/inode.rs b/vm/devices/virtio/virtiofs/src/inode.rs index b82a8e006d..286d5986a5 100644 --- a/vm/devices/virtio/virtiofs/src/inode.rs +++ b/vm/devices/virtio/virtiofs/src/inode.rs @@ -20,17 +20,14 @@ use std::sync::atomic::Ordering; /// golden-ratio constant, chosen because it has good bit-mixing properties. const INO_NAMESPACE_MULTIPLIER: u64 = 0x9E37_79B9_7F4A_7C15; -/// Folds a volume's namespace into a raw host inode number so that the same -/// inode number reported from two different aggregated volumes no longer -/// collides under the single shared FUSE superblock. +/// XORs a per-volume key into a raw host inode number to reduce the likelihood +/// of cross-volume `st_ino` collisions under the shared FUSE superblock. /// -/// `volume_id == 0` is the direct (single-root) mode, which returns `raw` -/// unchanged. For aggregated volumes (`volume_id != 0`) the transform is an -/// XOR by a per-volume constant, which is a bijection: distinct files within a -/// volume keep distinct inode numbers (preserving hard-link identity), and it -/// never introduces a within-volume collision. Sibling volumes get distinct -/// keys, so cross-share `(st_dev, st_ino)` aliasing is avoided even when the -/// guest never instantiates a submount. +/// `volume_id == 0` (direct/single-root mode) returns `raw` unchanged. For +/// aggregated volumes the transform is a bijection within each volume +/// (preserving hard-link identity), but cross-volume collisions are still +/// possible when two volumes happen to have inode numbers whose XOR equals the +/// difference of their volume keys. pub(crate) fn namespace_ino(volume_id: u32, raw: lx::ino_t) -> lx::ino_t { if volume_id == 0 { return raw; From 1441c1405bcb49645ff2d99395d8d32764078c99 Mon Sep 17 00:00:00 2001 From: Daman Mulye Date: Fri, 26 Jun 2026 10:25:23 -0700 Subject: [PATCH 12/21] clippy --- vm/devices/virtio/virtiofs/src/aggregate.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/vm/devices/virtio/virtiofs/src/aggregate.rs b/vm/devices/virtio/virtiofs/src/aggregate.rs index aa58e9e1b9..14bec35d50 100644 --- a/vm/devices/virtio/virtiofs/src/aggregate.rs +++ b/vm/devices/virtio/virtiofs/src/aggregate.rs @@ -283,10 +283,7 @@ impl VirtioFs { // `offset` is the cookie of the next entry to emit (0 at start of stream). // Entry 0 => ".", 1 => "..", 2.. => children[index - 2]. let mut index = offset; - loop { - let Some(next) = index.checked_add(1) else { - break; - }; + while let Some(next) = index.checked_add(1) { let fit = match index { 0 => self.write_synthetic_dot(&mut buffer, ".", next, plus), 1 => self.write_synthetic_dot(&mut buffer, "..", next, plus), From 9072ab3818c6918c11613b0a6a7c25f64e749ffb Mon Sep 17 00:00:00 2001 From: Daman Mulye Date: Tue, 30 Jun 2026 11:02:23 -0700 Subject: [PATCH 13/21] feedback --- vm/devices/virtio/virtiofs/src/aggregate.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/vm/devices/virtio/virtiofs/src/aggregate.rs b/vm/devices/virtio/virtiofs/src/aggregate.rs index 14bec35d50..b489d730b9 100644 --- a/vm/devices/virtio/virtiofs/src/aggregate.rs +++ b/vm/devices/virtio/virtiofs/src/aggregate.rs @@ -176,6 +176,7 @@ impl VirtioFs { /// dropped; this only stops further children from being added. pub fn begin_teardown(&self) { if let Some(aggregate) = self.inner.aggregate() { + let _children = aggregate.children.write(); aggregate.tearing_down.store(true, Ordering::Release); } } From e766122cc4326c2461574371417ac6a5af89d68a Mon Sep 17 00:00:00 2001 From: Daman Mulye Date: Tue, 30 Jun 2026 14:15:29 -0700 Subject: [PATCH 14/21] add back exFAT/FAT support + more feedback --- .../support/fs/lxutil/src/windows/fs.rs | 36 ++++++------ .../support/fs/lxutil/src/windows/mod.rs | 55 ++++++++++++++++++- .../support/fs/lxutil/src/windows/util.rs | 40 ++++++++------ vm/devices/virtio/virtiofs/src/inode.rs | 2 +- vm/devices/virtio/virtiofs/src/lib.rs | 11 +++- 5 files changed, 105 insertions(+), 39 deletions(-) diff --git a/vm/devices/support/fs/lxutil/src/windows/fs.rs b/vm/devices/support/fs/lxutil/src/windows/fs.rs index dad942543f..bff2949a8d 100644 --- a/vm/devices/support/fs/lxutil/src/windows/fs.rs +++ b/vm/devices/support/fs/lxutil/src/windows/fs.rs @@ -295,13 +295,24 @@ pub fn analyze_delete_error(error: lx::Error, file_handle: &OwnedHandle) -> Dele { DeleteErrorAction::ReturnError(error) } else if error.value() == lx::EIO { - // EIO can come from STATUS_CANNOT_DELETE, which for directories means - // the directory is not empty. Check if this is a directory and return - // ENOTEMPTY in that case. - if is_directory(file_handle) { - DeleteErrorAction::ReturnError(lx::Error::ENOTEMPTY) - } else { - DeleteErrorAction::ReturnError(error) + // EIO can come from STATUS_CANNOT_DELETE. Inspect the file's attributes + // to decide how to proceed: + // - a non-empty directory surfaces as STATUS_CANNOT_DELETE, so map it to + // ENOTEMPTY; + // - a read-only file cannot be deleted via the non-POSIX + // FileDispositionInformation path, so clear the read-only attribute and + // retry; + // - anything else (including a genuine I/O failure, or a handle without + // FILE_READ_ATTRIBUTES) is returned as-is so the read-only attribute is + // not disturbed. + match util::query_information_file::(file_handle) { + Ok(info) if info.FileAttributes & W32Fs::FILE_ATTRIBUTE_DIRECTORY.0 != 0 => { + DeleteErrorAction::ReturnError(lx::Error::ENOTEMPTY) + } + Ok(info) if info.FileAttributes & W32Fs::FILE_ATTRIBUTE_READONLY.0 != 0 => { + DeleteErrorAction::TryReadOnlyWorkaround + } + _ => DeleteErrorAction::ReturnError(error), } } else { DeleteErrorAction::TryReadOnlyWorkaround @@ -322,17 +333,6 @@ pub fn delete_file(fs_context: &FsContext, file_handle: &OwnedHandle) -> lx::Res } } -/// Check if a file handle refers to a directory. -fn is_directory(file_handle: &OwnedHandle) -> bool { - if let Ok(info) = - util::query_information_file::(file_handle) - { - info.FileAttributes & W32Fs::FILE_ATTRIBUTE_DIRECTORY.0 != 0 - } else { - false - } -} - pub fn delete_file_core(fs_context: &FsContext, file_handle: &OwnedHandle) -> lx::Result<()> { if fs_context .compatibility_flags diff --git a/vm/devices/support/fs/lxutil/src/windows/mod.rs b/vm/devices/support/fs/lxutil/src/windows/mod.rs index 3622f89837..2b7a1311ad 100644 --- a/vm/devices/support/fs/lxutil/src/windows/mod.rs +++ b/vm/devices/support/fs/lxutil/src/windows/mod.rs @@ -405,6 +405,45 @@ impl LxVolume { Default::default(), )?; + // Emulate Linux rename semantics that NTFS enforces natively but FAT/exFAT does not. + if !self + .state + .fs_context + .compatibility_flags + .supports_posix_unlink_rename() + { + let is_dir = |h: &OwnedHandle| -> lx::Result { + Ok( + util::query_information_file::(h)? + .FileAttributes + & W32Fs::FILE_ATTRIBUTE_DIRECTORY.0 + != 0, + ) + }; + let source_is_dir = is_dir(&handle)?; + + // Reject renaming a directory into its own descendant (EINVAL). + if source_is_dir && new_path != path && new_path.starts_with(path) { + return Err(lx::Error::EINVAL); + } + + // Reject cross-type replacement: file-over-directory (ENOTDIR) or directory-over-file (EISDIR). + match self.open_file(new_path, W32Fs::FILE_READ_ATTRIBUTES, Default::default()) { + Ok(target_handle) => { + let target_is_dir = is_dir(&target_handle)?; + if source_is_dir && !target_is_dir { + return Err(lx::Error::ENOTDIR); + } + if !source_is_dir && target_is_dir { + return Err(lx::Error::EISDIR); + } + } + // A missing target is fine; the rename creates it. + Err(err) if err.value() == lx::ENOENT => {} + Err(err) => return Err(err), + } + } + let flags = fs::RenameFlags::default(); let error = match fs::rename(&handle, &self.root, new_path, &self.state.fs_context, flags) { Ok(_) => return Ok(()), @@ -425,7 +464,11 @@ impl LxVolume { .compatibility_flags .supports_posix_unlink_rename() { - match self.open_file(new_path, W32Fs::DELETE, Default::default()) { + match self.open_file( + new_path, + W32Fs::FILE_READ_ATTRIBUTES | W32Fs::DELETE, + Default::default(), + ) { Ok(target_handle) => self.delete_file(&target_handle)?, Err(err) => { // ENOENT means the rename can proceed. @@ -700,6 +743,16 @@ impl LxVolume { assert!(path.is_relative() && !path.as_os_str().is_empty()); self.check_sandbox_enforcement(path)?; + // Handle filesystems that do not support reparse points. + if !self + .state + .fs_context + .compatibility_flags + .supports_reparse_points() + { + return Err(lx::Error::EPERM); + } + // Convert the target to its native Windows format. let win_target = Path::from_lx(target); diff --git a/vm/devices/support/fs/lxutil/src/windows/util.rs b/vm/devices/support/fs/lxutil/src/windows/util.rs index 41f2fe71e8..8b566ab552 100644 --- a/vm/devices/support/fs/lxutil/src/windows/util.rs +++ b/vm/devices/support/fs/lxutil/src/windows/util.rs @@ -155,29 +155,32 @@ fn get_token_for_access_check() -> lx::Result { let mut client_token_raw = Foundation::HANDLE::default(); let mut duplicate_token_raw = Foundation::HANDLE::default(); // SAFETY: Calling Win32 API as documented. - let result = unsafe { - check_status(FileSystem::NtOpenThreadToken( + // + // N.B. The raw NTSTATUS is inspected directly (rather than going through + // `check_status`) so that `STATUS_NO_TOKEN` can be distinguished from + // other failures. `check_status` maps the NTSTATUS to an `lx::Error` + // errno, which cannot be compared against an NTSTATUS constant. + let status = unsafe { + FileSystem::NtOpenThreadToken( Threading::GetCurrentThread(), Security::TOKEN_QUERY.0 | Security::TOKEN_DUPLICATE.0, true, &mut client_token_raw, - )) + ) }; - if let Err(e) = result { - // Use the process token if there's no token. - if e.value() == Foundation::STATUS_NO_TOKEN.0 { - // SAFETY: Calling Win32 API as documented. - unsafe { - let _ = check_status(FileSystem::NtOpenProcessToken( - Threading::GetCurrentProcess(), - Security::TOKEN_QUERY.0 | Security::TOKEN_DUPLICATE.0, - &mut client_token_raw, - ))?; - } - } else { - return Err(e); + if status == Foundation::STATUS_NO_TOKEN { + // The thread is not impersonating, so use the process token instead. + // SAFETY: Calling Win32 API as documented. + unsafe { + let _ = check_status(FileSystem::NtOpenProcessToken( + Threading::GetCurrentProcess(), + Security::TOKEN_QUERY.0 | Security::TOKEN_DUPLICATE.0, + &mut client_token_raw, + ))?; } + } else { + let _ = check_status(status)?; } // Create the RAII handle now so it gets closed on drop if we need @@ -213,7 +216,10 @@ fn get_token_for_access_check() -> lx::Result { }, EffectiveOnly: false, }; - let mut object_attributes = Wdk::Foundation::OBJECT_ATTRIBUTES::default(); + let mut object_attributes = Wdk::Foundation::OBJECT_ATTRIBUTES { + Length: size_of::() as u32, + ..Default::default() + }; object_attributes.SecurityQualityOfService = ptr::from_ref::(&security_qos).cast(); diff --git a/vm/devices/virtio/virtiofs/src/inode.rs b/vm/devices/virtio/virtiofs/src/inode.rs index 286d5986a5..9ed118caeb 100644 --- a/vm/devices/virtio/virtiofs/src/inode.rs +++ b/vm/devices/virtio/virtiofs/src/inode.rs @@ -37,7 +37,7 @@ pub(crate) fn namespace_ino(volume_id: u32, raw: lx::ino_t) -> lx::ino_t { /// Implements inode callbacks for virtio-fs. pub struct VirtioFsInode { - volume: Arc, + pub(crate) volume: Arc, /// Identifies which aggregated volume this inode belongs to. Inode numbers /// are only unique within a volume, so this is needed to key the stable /// inode-number map when a single file system exposes multiple roots, and diff --git a/vm/devices/virtio/virtiofs/src/lib.rs b/vm/devices/virtio/virtiofs/src/lib.rs index da7327bc77..44f689b7c3 100644 --- a/vm/devices/virtio/virtiofs/src/lib.rs +++ b/vm/devices/virtio/virtiofs/src/lib.rs @@ -740,8 +740,15 @@ impl InodeMap { /// Insert an inode into the map, returning its node ID. pub fn insert(&mut self, inode: VirtioFsInode) -> (Arc, u64) { - // If stable inode numbers are supported, look for the inode by its number. - if let Some(inodes_by_inode_nr) = self.inodes_by_inode_nr.as_mut() { + // Only consult the stable-inode dedup map when the inode's backing + // volume actually has stable file IDs. Volumes without stable IDs + // (e.g. FAT) can reuse inode numbers after rename/deletion, so + // deduplicating by (volume_id, inode_nr) would alias unrelated files. + if let Some(inodes_by_inode_nr) = self + .inodes_by_inode_nr + .as_mut() + .filter(|_| inode.volume.supports_stable_file_id()) + { match inodes_by_inode_nr.entry((inode.volume_id(), inode.inode_nr())) { Entry::Occupied(entry) => { // Inode found; increment its count and return the existing FUSE node ID. From 30e1e7fa609069493e99535ec5f6f47f3ccdda0c Mon Sep 17 00:00:00 2001 From: Daman Mulye Date: Tue, 7 Jul 2026 09:37:53 -0700 Subject: [PATCH 15/21] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- vm/devices/virtio/virtiofs/src/resolver.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/vm/devices/virtio/virtiofs/src/resolver.rs b/vm/devices/virtio/virtiofs/src/resolver.rs index 154bdd26c7..64a88cf15e 100644 --- a/vm/devices/virtio/virtiofs/src/resolver.rs +++ b/vm/devices/virtio/virtiofs/src/resolver.rs @@ -68,7 +68,11 @@ impl ResolveResource for VirtioFsResolver { Some(&LxVolumeOptions::from_option_string(&child.mount_options)), ) .map_err(|e| { - anyhow::anyhow!("failed to add virtiofs root {}: {:?}", child.name, e) + anyhow::anyhow!( + "failed to add virtiofs aggregate child '{}' (root_path='{}'): {e}", + child.name, + child.root_path + ) })?; } VirtioFsDevice::new(input.driver_source, &resource.tag, fs, 0, None) From 0680847e7e2281cfc42deda729cf4e017d39d5de Mon Sep 17 00:00:00 2001 From: Daman Mulye Date: Fri, 10 Jul 2026 14:32:56 -0700 Subject: [PATCH 16/21] Feedback + refactor + tests --- vm/devices/support/fs/fuse/src/lib.rs | 9 + vm/devices/support/fs/fuse/src/request.rs | 28 +- vm/devices/virtio/virtiofs/src/aggregate.rs | 392 +++++++----------- .../virtio/virtiofs/src/aggregate_tests.rs | 333 +++++++++++++++ vm/devices/virtio/virtiofs/src/file.rs | 18 +- vm/devices/virtio/virtiofs/src/inode.rs | 200 ++++----- vm/devices/virtio/virtiofs/src/lib.rs | 57 ++- vm/devices/virtio/virtiofs/src/resolver.rs | 2 +- 8 files changed, 657 insertions(+), 382 deletions(-) create mode 100644 vm/devices/virtio/virtiofs/src/aggregate_tests.rs diff --git a/vm/devices/support/fs/fuse/src/lib.rs b/vm/devices/support/fs/fuse/src/lib.rs index 16bd97807d..72fb3e2150 100644 --- a/vm/devices/support/fs/fuse/src/lib.rs +++ b/vm/devices/support/fs/fuse/src/lib.rs @@ -21,6 +21,7 @@ pub use reply::ReplySender; pub use request::FuseOperation; pub use request::Request; pub use request::RequestReader; +pub use request::check_name; pub use session::Session; pub use session::SessionInfo; @@ -29,6 +30,7 @@ use lx::LxString; use protocol::*; use std::time::Duration; use zerocopy::FromBytes; +use zerocopy::FromZeros; use zerocopy::Immutable; use zerocopy::IntoBytes; use zerocopy::KnownLayout; @@ -436,6 +438,13 @@ impl fuse_entry_out { attr, } } + + pub fn new_dot(ino: u64, mode: u32) -> Self { + let mut entry = Self::new_zeroed(); + entry.attr.ino = ino; + entry.attr.mode = mode; + entry + } } impl fuse_attr_out { diff --git a/vm/devices/support/fs/fuse/src/request.rs b/vm/devices/support/fs/fuse/src/request.rs index eca2d4df72..7e8b40d498 100644 --- a/vm/devices/support/fs/fuse/src/request.rs +++ b/vm/devices/support/fs/fuse/src/request.rs @@ -186,6 +186,23 @@ impl Request { } } +const NAME_MAX: usize = 255; + +pub fn check_name(name: &[u8]) -> lx::Result<()> { + if name.is_empty() + || name == b"." + || name == b".." + || name.contains(&b'/') + || name.contains(&b'\0') + { + return Err(lx::Error::EINVAL); + } + if name.len() > NAME_MAX { + return Err(lx::Error::ENAMETOOLONG); + } + Ok(()) +} + /// Helpers to parse FUSE messages. pub trait RequestReader: io::Read { /// Read until a matching byte is found. @@ -224,19 +241,10 @@ pub trait RequestReader: io::Read { Ok(lx::LxString::from_vec(buffer)) } - /// Maximum length of a file name component (NAME_MAX on Linux). - const NAME_MAX: usize = 255; - /// Read a NULL-terminated string and ensure it's a valid path name component. fn name(&mut self) -> lx::Result { let name = self.string()?; - if name.is_empty() || name == "." || name == ".." || name.as_bytes().contains(&b'/') { - return Err(lx::Error::EINVAL); - } - if name.len() > Self::NAME_MAX { - return Err(lx::Error::ENAMETOOLONG); - } - + check_name(name.as_bytes())?; Ok(name) } } diff --git a/vm/devices/virtio/virtiofs/src/aggregate.rs b/vm/devices/virtio/virtiofs/src/aggregate.rs index b489d730b9..672ae3cb1c 100644 --- a/vm/devices/virtio/virtiofs/src/aggregate.rs +++ b/vm/devices/virtio/virtiofs/src/aggregate.rs @@ -13,17 +13,18 @@ use crate::ATTRIBUTE_TIMEOUT; use crate::ENTRY_TIMEOUT; use crate::VirtioFs; -use crate::inode; +use crate::build_volume; +use crate::inode::MAX_AGGREGATE_VOLUMES; use crate::inode::VirtioFsInode; +use crate::inode::VirtioFsVolume; use fuse::DirEntryWriter; +use fuse::check_name; use fuse::protocol::*; use lxutil::LxVolumeOptions; use parking_lot::RwLock; use std::path::Path; use std::path::PathBuf; use std::sync::Arc; -use std::sync::atomic::AtomicBool; -use std::sync::atomic::Ordering; use zerocopy::FromZeros; /// Reserved file handle returned by `open_dir` on the synthetic aggregate root. @@ -34,64 +35,94 @@ use zerocopy::FromZeros; /// at 1 and only increments. pub(crate) const SYNTHETIC_ROOT_FH: u64 = u64::MAX; -/// Linux `DT_DIR` directory-entry type, used for the synthetic root's children. -const DT_DIR: u32 = 4; - /// A single host folder exposed as a named child of the synthetic aggregate root. struct ChildEntry { /// Name of this child's directory under the synthetic root. Chosen by the /// caller; the guest bind-mounts `/` onto the user's /// target path. name: String, - volume: Arc, - /// Stable identifier disambiguating per-volume inode numbers. - volume_id: u32, - /// Whether this child volume is read-only. Per child, so shares under one - /// aggregate device may differ; forced on when the device is read-only. - readonly: bool, + volume: Arc, } /// Registry of aggregated children for an aggregate-mode [`VirtioFs`]. -struct ChildRegistry { +struct AggregateRegistry { entries: Vec, next_volume_id: u32, + /// Guest inode mapping selected during FUSE initialization. + inode_mode: Option, + tearing_down: bool, +} + +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +enum InodeMode { + Namespaced, + Raw, } -impl ChildRegistry { +impl AggregateRegistry { fn new() -> Self { // Volume id 0 is reserved for a direct-mode single root, so aggregated // children start at 1. Self { entries: Vec::new(), next_volume_id: 1, + inode_mode: None, + tearing_down: false, + } + } + + fn initialize(&mut self, submounts: bool) { + let inode_mode = if submounts { + InodeMode::Raw + } else { + InodeMode::Namespaced + }; + self.inode_mode = Some(inode_mode); + for child in &mut self.entries { + child.volume = Arc::new(child.volume.with_inode_mapping(submounts)); } } + + fn reset(&mut self) { + self.inode_mode = None; + for child in &mut self.entries { + child.volume = Arc::new(child.volume.with_inode_mapping(false)); + } + } + + fn submounts_enabled(&self) -> bool { + self.inode_mode == Some(InodeMode::Raw) + } + + fn fallback_inode_mapping(&self) -> bool { + self.inode_mode == Some(InodeMode::Namespaced) + } + + fn check_can_add(&self) -> lx::Result<()> { + if self.tearing_down { + return Err(lx::Error::EAGAIN); + } + if self.fallback_inode_mapping() && self.next_volume_id > MAX_AGGREGATE_VOLUMES { + return Err(lx::Error::ENOSPC); + } + Ok(()) + } } /// State that only exists for an aggregate-mode [`VirtioFs`]. /// /// When present, node 1 is a synthetic directory whose children are the entries -/// in `children`; when absent (direct mode), node 1 is a real inode at a single +/// in `registry`; when absent (direct mode), node 1 is a real inode at a single /// volume root (legacy single-share behavior). pub(crate) struct AggregateState { - /// Aggregated children exposed under the synthetic root. - children: RwLock, - /// When true, children are advertised with `FUSE_ATTR_SUBMOUNT` so the - /// guest kernel gives each share its own `st_dev`. Only honored once - /// `FUSE_SUBMOUNTS` is negotiated. - submounts: bool, - /// Set once the owning device host begins tearing the aggregate down (see - /// [`VirtioFs::begin_teardown`]). After this, [`VirtioFs::add_child`] fails - /// fast with `EAGAIN` rather than appending a child to a doomed device. - tearing_down: AtomicBool, + /// Aggregated children and their lifecycle state. + registry: RwLock, } impl AggregateState { - pub(crate) fn new(submounts: bool) -> Self { + pub(crate) fn new() -> Self { Self { - children: RwLock::new(ChildRegistry::new()), - submounts, - tearing_down: AtomicBool::new(false), + registry: RwLock::new(AggregateRegistry::new()), } } } @@ -111,6 +142,8 @@ impl VirtioFs { /// - `EAGAIN` if the device has begun tearing down (see /// [`Self::begin_teardown`]). /// - `EEXIST` if a child with the same name already exists. + /// - `ENOSPC` if the fallback inode mapping is active and 64 children have + /// already been allocated. pub fn add_child( &self, name: &str, @@ -121,48 +154,34 @@ impl VirtioFs { return Err(lx::Error::EINVAL); }; - // Reject names that are empty, reserved ("." / ".."), or contain - // characters invalid in a POSIX directory entry ('/' or '\0'). - if name.is_empty() - || name == "." - || name == ".." - || name.contains('/') - || name.contains('\0') - { - return Err(lx::Error::EINVAL); - } - - // Each child carries its own read-only setting, so shares under one - // aggregate device may differ. - let readonly = mount_options.is_some_and(|o| o.is_readonly()); + check_name(name.as_bytes())?; // Fast-fail before paying for volume construction if the device is // already tearing down. Re-checked under the lock below to close the // race with a concurrent `begin_teardown`. - if aggregate.tearing_down.load(Ordering::Acquire) { - return Err(lx::Error::EAGAIN); + { + aggregate.registry.read().check_can_add()?; } - let mut children = aggregate.children.write(); - if aggregate.tearing_down.load(Ordering::Acquire) { - return Err(lx::Error::EAGAIN); - } - if children.entries.iter().any(|e| e.name == name) { + let (volume, readonly) = build_volume(root_path, mount_options)?; + + let mut registry = aggregate.registry.write(); + registry.check_can_add()?; + if registry.entries.iter().any(|e| e.name == name) { return Err(lx::Error::EEXIST); } - let volume = if let Some(mount_options) = mount_options { - mount_options.new_volume(root_path) - } else { - lxutil::LxVolume::new(root_path) - }?; - let volume_id = children.next_volume_id; - children.next_volume_id += 1; - children.entries.push(ChildEntry { + let volume_id = registry.next_volume_id; + registry.next_volume_id = volume_id.checked_add(1).ok_or(lx::Error::ENOSPC)?; + let use_raw_inodes = registry.submounts_enabled(); + registry.entries.push(ChildEntry { name: name.to_string(), - volume: Arc::new(volume), - volume_id, - readonly, + volume: Arc::new(VirtioFsVolume::new( + volume, + volume_id, + readonly, + use_raw_inodes, + )), }); Ok(()) } @@ -176,8 +195,7 @@ impl VirtioFs { /// dropped; this only stops further children from being added. pub fn begin_teardown(&self) { if let Some(aggregate) = self.inner.aggregate() { - let _children = aggregate.children.write(); - aggregate.tearing_down.store(true, Ordering::Release); + aggregate.registry.write().tearing_down = true; } } @@ -191,7 +209,7 @@ impl VirtioFs { return Err(lx::Error::EINVAL); }; - let mut children = aggregate.children.write(); + let mut children = aggregate.registry.write(); let before = children.entries.len(); children.entries.retain(|e| e.name != name); if children.entries.len() == before { @@ -206,24 +224,49 @@ impl VirtioFs { self.inner.aggregate().is_some() && node_id == FUSE_ROOT_ID } + pub(crate) fn is_synthetic_root_handle(&self, node_id: u64, fh: u64) -> bool { + self.is_synthetic_root(node_id) && fh == SYNTHETIC_ROOT_FH + } + + /// Finalize aggregate inode mapping after FUSE capability negotiation. + pub(crate) fn initialize_submounts(&self, capable: bool) -> bool { + let Some(aggregate) = self.inner.aggregate() else { + return false; + }; + let mut registry = aggregate.registry.write(); + registry.initialize(capable); + capable + } + + /// Return the aggregate to its pre-initialization state for a remount. + pub(crate) fn reset_submounts(&self) { + let Some(aggregate) = self.inner.aggregate() else { + return; + }; + let mut registry = aggregate.registry.write(); + registry.reset(); + } + /// Whether aggregate children should be advertised with /// `FUSE_ATTR_SUBMOUNT`. Always false in direct mode. pub(crate) fn submounts(&self) -> bool { - self.inner.aggregate().is_some_and(|a| a.submounts) + self.inner + .aggregate() + .is_some_and(|aggregate| aggregate.registry.read().submounts_enabled()) } /// Attributes of the synthetic aggregate root directory. - pub(crate) fn synthetic_root_attr() -> fuse_attr { + pub(crate) fn synthetic_root_attr(&self) -> fuse_attr { let mut attr = fuse_attr::new_zeroed(); attr.ino = FUSE_ROOT_ID; attr.mode = lx::S_IFDIR | 0o555; - attr.nlink = 2; + attr.nlink = self.synthetic_root_nlink(); attr.blksize = 512; attr } /// Extended attributes of the synthetic aggregate root directory. - pub(crate) fn synthetic_root_statx(mask: lx::StatExMask) -> fuse_statx { + pub(crate) fn synthetic_root_statx(&self, mask: lx::StatExMask) -> fuse_statx { let mut sx = fuse_statx::new_zeroed(); let returned_mask = lx::StatExMask::new() .with_file_type(true) @@ -233,12 +276,22 @@ impl VirtioFs { .into_bits(); sx.mask = mask.into_bits() & returned_mask; sx.mode = (lx::S_IFDIR | 0o555) as u16; - sx.nlink = 2; + sx.nlink = self.synthetic_root_nlink(); sx.ino = FUSE_ROOT_ID; sx.blksize = 512; sx } + fn synthetic_root_nlink(&self) -> u32 { + let child_count = self + .inner + .aggregate() + .map_or(0, |aggregate| aggregate.registry.read().entries.len()); + u32::try_from(child_count) + .unwrap_or(u32::MAX) + .saturating_add(2) + } + /// Looks up a named child of the synthetic root, returning an entry for the /// corresponding volume's real root inode. pub(crate) fn lookup_synthetic_root(&self, name: &lx::LxStr) -> lx::Result { @@ -246,17 +299,21 @@ impl VirtioFs { return Err(lx::Error::ENOENT); }; let name_bytes = name.as_bytes(); - let (volume, volume_id, readonly) = { - let children = aggregate.children.read(); + let volume = { + let children = aggregate.registry.read(); let entry = children .entries .iter() .find(|e| e.name.as_bytes() == name_bytes) .ok_or(lx::Error::ENOENT)?; - (Arc::clone(&entry.volume), entry.volume_id, entry.readonly) + Arc::clone(&entry.volume) }; - let (inode, stat) = VirtioFsInode::new(volume, volume_id, readonly, PathBuf::new())?; + self.insert_child_root_entry(volume) + } + + fn insert_child_root_entry(&self, volume: Arc) -> lx::Result { + let (inode, stat) = VirtioFsInode::new(volume, PathBuf::new())?; let mut attr = inode.attr_from_stat(&stat); let (_, node_id) = self.insert_inode(inode); if self.submounts() { @@ -290,28 +347,16 @@ impl VirtioFs { 1 => self.write_synthetic_dot(&mut buffer, "..", next, plus), n => { let child = { - let children = aggregate.children.read(); - children.entries.get((n - 2) as usize).map(|e| { - ( - e.name.clone(), - Arc::clone(&e.volume), - e.volume_id, - e.readonly, - ) - }) + let children = aggregate.registry.read(); + children + .entries + .get((n - 2) as usize) + .map(|e| (e.name.clone(), Arc::clone(&e.volume))) }; - let Some((name, volume, volume_id, readonly)) = child else { + let Some((name, volume)) = child else { break; }; - self.write_child_entry( - &mut buffer, - &name, - volume, - volume_id, - readonly, - next, - plus, - )? + self.write_child_entry(&mut buffer, &name, volume, next, plus)? } }; if !fit { @@ -335,12 +380,10 @@ impl VirtioFs { if !buffer.check_dir_entry_plus(name) { return false; } - let mut entry = fuse_entry_out::new_zeroed(); - entry.attr.ino = FUSE_ROOT_ID; - entry.attr.mode = lx::S_IFDIR | 0o555; + let entry = fuse_entry_out::new_dot(FUSE_ROOT_ID, lx::S_IFDIR | 0o555); buffer.dir_entry_plus(name, next_off, entry) } else { - buffer.dir_entry(name, FUSE_ROOT_ID, next_off, DT_DIR) + buffer.dir_entry(name, FUSE_ROOT_ID, next_off, lx::DT_DIR as u32) } } @@ -349,9 +392,7 @@ impl VirtioFs { &self, buffer: &mut Vec, name: &str, - volume: Arc, - volume_id: u32, - readonly: bool, + volume: Arc, next_off: u64, plus: bool, ) -> lx::Result { @@ -361,157 +402,22 @@ impl VirtioFs { } // readdirplus performs a lookup on each entry, incrementing its // lookup count, so create/insert the root inode here. - let (inode, stat) = VirtioFsInode::new(volume, volume_id, readonly, PathBuf::new())?; - let mut attr = inode.attr_from_stat(&stat); - let (_, node_id) = self.insert_inode(inode); - if self.submounts() { - attr.flags |= FUSE_ATTR_SUBMOUNT; - } - let entry = fuse_entry_out::new(node_id, ENTRY_TIMEOUT, ATTRIBUTE_TIMEOUT, attr); + let entry = self.insert_child_root_entry(volume)?; Ok(buffer.dir_entry_plus(name, next_off, entry)) } else { - // Plain readdir: report the directory using the volume root's real - // inode number (namespaced to its volume), falling back to the - // volume id if it is inaccessible. + // Plain readdir: report the directory using the volume root's + // guest-visible inode number, falling back to the volume id if it + // is inaccessible. let raw = volume .lstat(PathBuf::new()) .map(|s| s.inode_nr) - .unwrap_or(volume_id as lx::ino_t); - let ino = inode::namespace_ino(volume_id, raw); - Ok(buffer.dir_entry(name, ino, next_off, DT_DIR)) + .unwrap_or(volume.id() as lx::ino_t); + let ino = volume.map_inode(raw)?; + Ok(buffer.dir_entry(name, ino, next_off, lx::DT_DIR as u32)) } } } #[cfg(test)] -mod tests { - use crate::VirtioFs; - use crate::inode; - use lxutil::LxVolumeOptions; - - #[test] - fn aggregate_child_registry() { - let a = tempfile::tempdir().unwrap(); - let b = tempfile::tempdir().unwrap(); - let fs = VirtioFs::new_aggregate(true); - - fs.add_child("share_a", a.path(), None).unwrap(); - fs.add_child("share_b", b.path(), None).unwrap(); - - // Duplicate names are rejected. - assert_eq!( - fs.add_child("share_a", a.path(), None).unwrap_err(), - lx::Error::EEXIST - ); - - // Each child gets a distinct, non-zero volume id (0 is reserved for - // direct mode). - { - let aggregate = fs.inner.aggregate().unwrap(); - let children = aggregate.children.read(); - assert_eq!(children.entries.len(), 2); - assert_ne!(children.entries[0].volume_id, 0); - assert_ne!(children.entries[0].volume_id, children.entries[1].volume_id); - } - - // Removal drops only the named child. - fs.remove_child("share_a").unwrap(); - assert_eq!(fs.remove_child("share_a").unwrap_err(), lx::Error::ENOENT); - assert_eq!( - fs.inner.aggregate().unwrap().children.read().entries.len(), - 1 - ); - } - - #[test] - fn add_child_rejected_in_direct_mode() { - let a = tempfile::tempdir().unwrap(); - let fs = VirtioFs::new(a.path(), None).unwrap(); - assert_eq!( - fs.add_child("x", a.path(), None).unwrap_err(), - lx::Error::EINVAL - ); - assert_eq!(fs.remove_child("x").unwrap_err(), lx::Error::EINVAL); - } - - #[test] - fn synthetic_root_node_ids_start_after_root() { - // In aggregate mode the synthetic root occupies FUSE_ROOT_ID, so the - // first real inode inserted must be allocated a higher id. - let a = tempfile::tempdir().unwrap(); - let fs = VirtioFs::new_aggregate(false); - fs.add_child("share", a.path(), None).unwrap(); - let entry = fs - .lookup_synthetic_root(lx::LxStr::from_bytes(b"share")) - .unwrap(); - assert!(entry.nodeid > fuse::protocol::FUSE_ROOT_ID); - } - - #[test] - fn inode_namespacing_avoids_cross_volume_collisions() { - // Direct mode (volume id 0) is the identity transform. - assert_eq!(inode::namespace_ino(0, 42), 42); - assert_eq!(inode::namespace_ino(0, 0), 0); - - // The same raw inode number on two different volumes maps to two - // different reported numbers, so siblings never alias. - let raw = 2; // e.g. the root inode of two freshly-formatted volumes - assert_ne!( - inode::namespace_ino(1, raw), - inode::namespace_ino(2, raw), - "sibling volumes must not collide" - ); - - // Within a single volume the transform is a bijection, so distinct - // files keep distinct inode numbers (preserving hard-link identity). - assert_ne!(inode::namespace_ino(1, 10), inode::namespace_ino(1, 11)); - } - - #[test] - fn add_child_allows_per_child_readonly() { - let a = tempfile::tempdir().unwrap(); - let mut ro = LxVolumeOptions::default(); - ro.readonly(true); - let mut rw = LxVolumeOptions::default(); - rw.readonly(false); - - // A writable aggregate lets each child pick its own readonly setting. - let fs = VirtioFs::new_aggregate(false); - fs.add_child("ro_child", a.path(), Some(&ro)).unwrap(); - fs.add_child("rw_child", a.path(), Some(&rw)).unwrap(); - - let aggregate = fs.inner.aggregate().unwrap(); - let children = aggregate.children.read(); - let ro_entry = children - .entries - .iter() - .find(|e| e.name == "ro_child") - .unwrap(); - let rw_entry = children - .entries - .iter() - .find(|e| e.name == "rw_child") - .unwrap(); - assert!(ro_entry.readonly); - assert!(!rw_entry.readonly); - } - - #[test] - fn add_child_rejected_after_teardown() { - let a = tempfile::tempdir().unwrap(); - let fs = VirtioFs::new_aggregate(false); - fs.add_child("before", a.path(), None).unwrap(); - - fs.begin_teardown(); - - // Once tearing down, no further children can be added. - assert_eq!( - fs.add_child("after", a.path(), None).unwrap_err(), - lx::Error::EAGAIN - ); - assert_eq!( - fs.inner.aggregate().unwrap().children.read().entries.len(), - 1 - ); - } -} +#[path = "aggregate_tests.rs"] +mod tests; diff --git a/vm/devices/virtio/virtiofs/src/aggregate_tests.rs b/vm/devices/virtio/virtiofs/src/aggregate_tests.rs new file mode 100644 index 0000000000..8657beb079 --- /dev/null +++ b/vm/devices/virtio/virtiofs/src/aggregate_tests.rs @@ -0,0 +1,333 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +use super::SYNTHETIC_ROOT_FH; +use crate::VirtioFs; +use crate::inode; +use crate::inode::MAX_AGGREGATE_VOLUMES; +use fuse::protocol::FUSE_ATTR_SUBMOUNT; +use fuse::protocol::FUSE_ROOT_ID; +use lxutil::LxVolumeOptions; +use std::sync::Arc; +use test_with_tracing::test; + +#[test] +fn aggregate_child_registry() { + let a = tempfile::tempdir().unwrap(); + let b = tempfile::tempdir().unwrap(); + let fs = VirtioFs::new_aggregate(); + + fs.add_child("share_a", a.path(), None).unwrap(); + fs.add_child("share_b", b.path(), None).unwrap(); + + // Duplicate names are rejected. + assert_eq!( + fs.add_child("share_a", a.path(), None).unwrap_err(), + lx::Error::EEXIST + ); + + // Each child gets a distinct, non-zero volume id (0 is reserved for + // direct mode). + { + let aggregate = fs.inner.aggregate().unwrap(); + let children = aggregate.registry.read(); + assert_eq!(children.entries.len(), 2); + assert_ne!(children.entries[0].volume.id(), 0); + assert_ne!( + children.entries[0].volume.id(), + children.entries[1].volume.id() + ); + } + + // Removal drops only the named child. + fs.remove_child("share_a").unwrap(); + assert_eq!(fs.remove_child("share_a").unwrap_err(), lx::Error::ENOENT); + assert_eq!( + fs.inner.aggregate().unwrap().registry.read().entries.len(), + 1 + ); +} + +#[test] +fn add_child_rejected_in_direct_mode() { + let a = tempfile::tempdir().unwrap(); + let fs = VirtioFs::new(a.path(), None).unwrap(); + assert_eq!( + fs.add_child("x", a.path(), None).unwrap_err(), + lx::Error::EINVAL + ); + assert_eq!(fs.remove_child("x").unwrap_err(), lx::Error::EINVAL); +} + +#[test] +fn add_child_validates_name() { + let root = tempfile::tempdir().unwrap(); + let fs = VirtioFs::new_aggregate(); + + for name in ["", ".", "..", "a/b", "a\0b"] { + assert_eq!( + fs.add_child(name, root.path(), None).unwrap_err(), + lx::Error::EINVAL + ); + } + + fs.add_child(&"a".repeat(255), root.path(), None).unwrap(); + assert_eq!( + fs.add_child(&"b".repeat(256), root.path(), None) + .unwrap_err(), + lx::Error::ENAMETOOLONG + ); +} + +#[test] +fn synthetic_root_node_ids_start_after_root() { + // In aggregate mode the synthetic root occupies FUSE_ROOT_ID, so the + // first real inode inserted must be allocated a higher id. + let a = tempfile::tempdir().unwrap(); + let fs = VirtioFs::new_aggregate(); + fs.add_child("share", a.path(), None).unwrap(); + let entry = fs + .lookup_synthetic_root(lx::LxStr::from_bytes(b"share")) + .unwrap(); + assert!(entry.nodeid > FUSE_ROOT_ID); +} + +#[test] +fn submount_flag_requires_negotiation() { + let first = tempfile::tempdir().unwrap(); + let second = tempfile::tempdir().unwrap(); + let fs = VirtioFs::new_aggregate(); + fs.add_child("first", first.path(), None).unwrap(); + + let volume = |index: usize| { + let children = fs.inner.aggregate().unwrap().registry.read(); + Arc::clone(&children.entries[index].volume) + }; + assert_eq!( + volume(0).map_inode(u64::MAX).unwrap_err(), + lx::Error::EOVERFLOW + ); + + let entry = fs + .lookup_synthetic_root(lx::LxStr::from_bytes(b"first")) + .unwrap(); + assert_eq!(entry.attr.flags & FUSE_ATTR_SUBMOUNT, 0); + + assert!(fs.initialize_submounts(true)); + assert_eq!(volume(0).map_inode(u64::MAX).unwrap(), u64::MAX); + + fs.add_child("second", second.path(), None).unwrap(); + assert_eq!(volume(1).map_inode(u64::MAX).unwrap(), u64::MAX); + assert_ne!( + fs.lookup_synthetic_root(lx::LxStr::from_bytes(b"second")) + .unwrap() + .attr + .flags + & FUSE_ATTR_SUBMOUNT, + 0 + ); + + fs.reset_submounts(); + assert!(!fs.initialize_submounts(false)); + assert_eq!(volume(0).map_inode(u64::MAX), Err(lx::Error::EOVERFLOW)); + assert_eq!(volume(1).map_inode(u64::MAX), Err(lx::Error::EOVERFLOW)); +} + +#[test] +fn synthetic_root_handle_is_scoped_to_root() { + let aggregate = VirtioFs::new_aggregate(); + assert!(aggregate.is_synthetic_root_handle(FUSE_ROOT_ID, SYNTHETIC_ROOT_FH)); + assert!(!aggregate.is_synthetic_root_handle(FUSE_ROOT_ID + 1, SYNTHETIC_ROOT_FH)); + + let root = tempfile::tempdir().unwrap(); + let direct = VirtioFs::new(root.path(), None).unwrap(); + assert!(!direct.is_synthetic_root_handle(FUSE_ROOT_ID, SYNTHETIC_ROOT_FH)); +} + +#[test] +fn synthetic_root_link_count_includes_children() { + let first = tempfile::tempdir().unwrap(); + let second = tempfile::tempdir().unwrap(); + let fs = VirtioFs::new_aggregate(); + + assert_eq!(fs.synthetic_root_attr().nlink, 2); + assert_eq!(fs.synthetic_root_statx(lx::StatExMask::new()).nlink, 2); + + fs.add_child("first", first.path(), None).unwrap(); + fs.add_child("second", second.path(), None).unwrap(); + assert_eq!(fs.synthetic_root_attr().nlink, 4); + assert_eq!(fs.synthetic_root_statx(lx::StatExMask::new()).nlink, 4); + + fs.remove_child("first").unwrap(); + assert_eq!(fs.synthetic_root_attr().nlink, 3); + assert_eq!(fs.synthetic_root_statx(lx::StatExMask::new()).nlink, 3); +} + +#[test] +fn inode_namespacing_avoids_cross_volume_collisions() { + // Direct mode (volume id 0) is the identity transform. + assert_eq!(inode::namespace_ino(0, 42).unwrap(), 42); + assert_eq!(inode::namespace_ino(0, u64::MAX).unwrap(), u64::MAX); + + const INODE_MASK: u64 = (1 << 58) - 1; + let raw = INODE_MASK; + assert_eq!(inode::namespace_ino(1, raw).unwrap(), INODE_MASK); + assert_eq!( + inode::namespace_ino(2, raw).unwrap(), + (1 << 58) | INODE_MASK + ); + assert_eq!(inode::namespace_ino(64, raw).unwrap(), u64::MAX); + assert_eq!( + inode::namespace_ino(65, raw).unwrap_err(), + lx::Error::ENOSPC + ); + assert_eq!( + inode::namespace_ino(1, INODE_MASK + 1).unwrap_err(), + lx::Error::EOVERFLOW + ); + + // Distinct host inode numbers remain distinct within a volume. + assert_ne!( + inode::namespace_ino(1, 10).unwrap(), + inode::namespace_ino(1, 11).unwrap() + ); +} + +#[test] +fn aggregate_volume_count_is_limited_to_namespace_capacity() { + let root = tempfile::tempdir().unwrap(); + let fs = VirtioFs::new_aggregate(); + assert!(!fs.initialize_submounts(false)); + + for index in 0..MAX_AGGREGATE_VOLUMES { + fs.add_child(&format!("share_{index}"), root.path(), None) + .unwrap(); + } + + assert_eq!( + fs.add_child("one_too_many", root.path(), None).unwrap_err(), + lx::Error::ENOSPC + ); +} + +#[test] +fn aggregate_volume_count_is_not_limited_when_submounts_are_available() { + let root = tempfile::tempdir().unwrap(); + let fs = VirtioFs::new_aggregate(); + + fs.inner + .aggregate() + .unwrap() + .registry + .write() + .next_volume_id = MAX_AGGREGATE_VOLUMES + 1; + fs.add_child("pending", root.path(), None).unwrap(); + + assert!(fs.initialize_submounts(true)); + fs.add_child("negotiated", root.path(), None).unwrap(); + + let aggregate = fs.inner.aggregate().unwrap(); + let children = aggregate.registry.read(); + assert_eq!(children.entries[0].volume.id(), MAX_AGGREGATE_VOLUMES + 1); + assert_eq!(children.entries[1].volume.id(), MAX_AGGREGATE_VOLUMES + 2); + assert_eq!(children.entries[0].volume.map_inode(42).unwrap(), 42); + assert_eq!(children.entries[1].volume.map_inode(42).unwrap(), 42); + + let fallback = VirtioFs::new_aggregate(); + fallback + .inner + .aggregate() + .unwrap() + .registry + .write() + .next_volume_id = MAX_AGGREGATE_VOLUMES + 1; + assert!(!fallback.initialize_submounts(false)); + assert_eq!( + fallback.add_child("fallback", root.path(), None), + Err(lx::Error::ENOSPC) + ); +} + +#[test] +fn hard_link_rejects_cross_volume_target() { + let first = tempfile::tempdir().unwrap(); + let second = tempfile::tempdir().unwrap(); + std::fs::write(first.path().join("target"), b"data").unwrap(); + + let fs = VirtioFs::new_aggregate(); + fs.add_child("first", first.path(), None).unwrap(); + fs.add_child("second", second.path(), None).unwrap(); + + let first_root = fs + .lookup_synthetic_root(lx::LxStr::from_bytes(b"first")) + .unwrap(); + let second_root = fs + .lookup_synthetic_root(lx::LxStr::from_bytes(b"second")) + .unwrap(); + let target = fs + .lookup_helper( + &fs.get_inode(first_root.nodeid).unwrap(), + lx::LxStr::from_bytes(b"target"), + ) + .unwrap(); + + assert_eq!( + fs.get_inode(second_root.nodeid) + .unwrap() + .link( + lx::LxStr::from_bytes(b"link"), + &fs.get_inode(target.nodeid).unwrap() + ) + .unwrap_err(), + lx::Error::EXDEV + ); + assert!(!second.path().join("link").exists()); +} + +#[test] +fn add_child_allows_per_child_readonly() { + let a = tempfile::tempdir().unwrap(); + let mut ro = LxVolumeOptions::default(); + ro.readonly(true); + let mut rw = LxVolumeOptions::default(); + rw.readonly(false); + + // A writable aggregate lets each child pick its own readonly setting. + let fs = VirtioFs::new_aggregate(); + fs.add_child("ro_child", a.path(), Some(&ro)).unwrap(); + fs.add_child("rw_child", a.path(), Some(&rw)).unwrap(); + + let aggregate = fs.inner.aggregate().unwrap(); + let children = aggregate.registry.read(); + let ro_entry = children + .entries + .iter() + .find(|e| e.name == "ro_child") + .unwrap(); + let rw_entry = children + .entries + .iter() + .find(|e| e.name == "rw_child") + .unwrap(); + assert!(ro_entry.volume.readonly()); + assert!(!rw_entry.volume.readonly()); +} + +#[test] +fn add_child_rejected_after_teardown() { + let a = tempfile::tempdir().unwrap(); + let fs = VirtioFs::new_aggregate(); + fs.add_child("before", a.path(), None).unwrap(); + + fs.begin_teardown(); + + // Once tearing down, no further children can be added. + assert_eq!( + fs.add_child("after", a.path(), None).unwrap_err(), + lx::Error::EAGAIN + ); + assert_eq!( + fs.inner.aggregate().unwrap().registry.read().entries.len(), + 1 + ); +} diff --git a/vm/devices/virtio/virtiofs/src/file.rs b/vm/devices/virtio/virtiofs/src/file.rs index 983ca67f2d..df91fe06d4 100644 --- a/vm/devices/virtio/virtiofs/src/file.rs +++ b/vm/devices/virtio/virtiofs/src/file.rs @@ -11,7 +11,6 @@ use fuse::protocol::fuse_statx; use lxutil::LxFile; use parking_lot::RwLock; use std::sync::Arc; -use zerocopy::FromZeros; /// Implements file callbacks for virtio-fs. pub struct VirtioFsFile { @@ -79,9 +78,9 @@ impl VirtioFsFile { ) -> lx::Result> { let mut buffer = Vec::with_capacity(size as usize); let mut entry_count: u32 = 0; - // Report the directory's own inode number namespaced to its volume, so - // `.`/`..` agree with the number reported by lookup/getattr. - let self_inode_nr = self.inode.namespaced_inode_nr(); + // Report the directory's guest-visible inode number so `.`/`..` agree + // with the number reported by lookup/getattr. + let self_inode_nr = self.inode.guest_inode_nr(); let mut file = self.file.write(); file.read_dir(offset as lx::off_t, |entry| { entry_count += 1; @@ -104,10 +103,7 @@ impl VirtioFsFile { // If readdirplus is being used, do a lookup on all items except the . and .. entries. if plus { let fuse_entry = if entry.name == "." || entry.name == ".." { - let mut e = fuse_entry_out::new_zeroed(); - e.attr.ino = self_inode_nr; - e.attr.mode = (entry.file_type as u32) << 12; - e + fuse_entry_out::new_dot(self_inode_nr, (entry.file_type as u32) << 12) } else { if !buffer.check_dir_entry_plus(&entry.name) { return Ok(false); @@ -137,9 +133,9 @@ impl VirtioFsFile { entry_count -= 1; return Ok(true); } - // Children share this directory's volume, so namespace the - // entry's inode number to match lookup/readdirplus. - self.inode.namespaced_ino(entry.inode_nr) + // Children share this directory's volume, so apply its + // guest inode mapping to match lookup/readdirplus. + self.inode.guest_ino(entry.inode_nr)? }; Ok(buffer.dir_entry( diff --git a/vm/devices/virtio/virtiofs/src/inode.rs b/vm/devices/virtio/virtiofs/src/inode.rs index 9ed118caeb..2788cb045f 100644 --- a/vm/devices/virtio/virtiofs/src/inode.rs +++ b/vm/devices/virtio/virtiofs/src/inode.rs @@ -10,82 +10,119 @@ use lxutil::LxCreateOptions; use lxutil::LxVolume; use lxutil::PathBufExt; use parking_lot::RwLock; +use std::ops::Deref; use std::path::PathBuf; use std::sync::Arc; use std::sync::atomic::AtomicU64; use std::sync::atomic::Ordering; -/// Multiplier used to spread a volume id across the 64-bit inode space when -/// namespacing inode numbers (see [`namespace_ino`]). It is the 64-bit -/// golden-ratio constant, chosen because it has good bit-mixing properties. -const INO_NAMESPACE_MULTIPLIER: u64 = 0x9E37_79B9_7F4A_7C15; - -/// XORs a per-volume key into a raw host inode number to reduce the likelihood -/// of cross-volume `st_ino` collisions under the shared FUSE superblock. -/// -/// `volume_id == 0` (direct/single-root mode) returns `raw` unchanged. For -/// aggregated volumes the transform is a bijection within each volume -/// (preserving hard-link identity), but cross-volume collisions are still -/// possible when two volumes happen to have inode numbers whose XOR equals the -/// difference of their volume keys. -pub(crate) fn namespace_ino(volume_id: u32, raw: lx::ino_t) -> lx::ino_t { +const VOLUME_ID_BITS: u32 = 6; +const INODE_BITS: u32 = u64::BITS - VOLUME_ID_BITS; +const INODE_MASK: u64 = (1 << INODE_BITS) - 1; + +pub(crate) const MAX_AGGREGATE_VOLUMES: u32 = 1 << VOLUME_ID_BITS; + +/// Uses the high six bits to namespace the low 58 bits of a host inode number. +/// `volume_id == 0` is reserved for direct mode and returns `raw` unchanged; +/// aggregate volume IDs 1 through 64 map to namespace values 0 through 63. +pub(crate) fn namespace_ino(volume_id: u32, raw: lx::ino_t) -> lx::Result { if volume_id == 0 { - return raw; + return Ok(raw); + } + if volume_id > MAX_AGGREGATE_VOLUMES { + return Err(lx::Error::ENOSPC); + } + if raw > INODE_MASK { + return Err(lx::Error::EOVERFLOW); + } + + Ok((u64::from(volume_id - 1) << INODE_BITS) | raw) +} + +pub(crate) struct VirtioFsVolume { + volume: Arc, + id: u32, + readonly: bool, + use_raw_inodes: bool, +} + +impl VirtioFsVolume { + pub(crate) fn new(volume: LxVolume, id: u32, readonly: bool, use_raw_inodes: bool) -> Self { + Self { + volume: Arc::new(volume), + id, + readonly, + use_raw_inodes, + } + } + + pub(crate) fn id(&self) -> u32 { + self.id + } + + pub(crate) fn readonly(&self) -> bool { + self.readonly + } + + pub(crate) fn with_inode_mapping(&self, use_raw_inodes: bool) -> Self { + Self { + volume: Arc::clone(&self.volume), + id: self.id, + readonly: self.readonly, + use_raw_inodes, + } + } + + pub(crate) fn map_inode(&self, raw: lx::ino_t) -> lx::Result { + if self.use_raw_inodes { + Ok(raw) + } else { + namespace_ino(self.id, raw) + } + } +} + +impl Deref for VirtioFsVolume { + type Target = LxVolume; + + fn deref(&self) -> &Self::Target { + &self.volume } - raw ^ (volume_id as u64).wrapping_mul(INO_NAMESPACE_MULTIPLIER) } /// Implements inode callbacks for virtio-fs. pub struct VirtioFsInode { - pub(crate) volume: Arc, - /// Identifies which aggregated volume this inode belongs to. Inode numbers - /// are only unique within a volume, so this is needed to key the stable - /// inode-number map when a single file system exposes multiple roots, and - /// to namespace reported inode numbers (see [`namespace_ino`]). - volume_id: u32, + pub(crate) volume: Arc, path: RwLock, lookup_count: AtomicU64, inode_nr: lx::ino_t, /// This inode's number as reported to the guest: its host inode number - /// ([`Self::inode_nr`]) folded into its volume's namespace (see - /// [`namespace_ino`]). - namespaced_inode_nr: lx::ino_t, - /// Whether this inode's volume is read-only. Carried per inode so write - /// permission can be enforced per share in an aggregate device (each child - /// volume may differ). Inherited by descendants from their parent. - readonly: bool, + /// when submounts are negotiated, or its fallback namespaced number. + guest_inode_nr: lx::ino_t, } impl VirtioFsInode { /// Create a new inode for the specified path. - pub fn new( - volume: Arc, - volume_id: u32, - readonly: bool, - path: PathBuf, - ) -> lx::Result<(Self, lx::Stat)> { + pub fn new(volume: Arc, path: PathBuf) -> lx::Result<(Self, lx::Stat)> { let stat = volume.lstat(&path)?; - let inode = Self::with_attr(volume, volume_id, readonly, path, &stat); + let inode = Self::with_attr(volume, path, &stat)?; Ok((inode, stat)) } /// Create a new inode for the specified path, with previously retrieved attributes. pub fn with_attr( - volume: Arc, - volume_id: u32, - readonly: bool, + volume: Arc, path: PathBuf, stat: &lx::Stat, - ) -> Self { - Self { + ) -> lx::Result { + let guest_inode_nr = volume.map_inode(stat.inode_nr)?; + Ok(Self { volume, - volume_id, path: RwLock::new(path), lookup_count: AtomicU64::new(1), inode_nr: stat.inode_nr, - namespaced_inode_nr: namespace_ino(volume_id, stat.inode_nr), - readonly, - } + guest_inode_nr, + }) } /// Return the files inode number as reported by the underlying file system. @@ -97,45 +134,44 @@ impl VirtioFsInode { /// Return the identifier of the aggregated volume this inode belongs to. pub fn volume_id(&self) -> u32 { - self.volume_id + self.volume.id() } /// Whether this inode's volume is read-only. pub fn readonly(&self) -> bool { - self.readonly + self.volume.readonly() } /// This inode's own number as reported to the guest: its host inode number - /// folded into its volume's namespace (see [`namespace_ino`]). Fixed for - /// the inode's lifetime. - pub(crate) fn namespaced_inode_nr(&self) -> lx::ino_t { - self.namespaced_inode_nr + /// when submounts are negotiated, or its fallback namespaced number. Fixed + /// for the inode's lifetime. + pub(crate) fn guest_inode_nr(&self) -> lx::ino_t { + self.guest_inode_nr } - /// Namespaces a raw host inode number from this inode's volume (see - /// [`namespace_ino`]). For numbers belonging to *other* inodes in the same - /// volume (e.g. readdir child entries); for this inode's own number use - /// [`Self::namespaced_inode_nr`]. - pub(crate) fn namespaced_ino(&self, raw: lx::ino_t) -> lx::ino_t { - namespace_ino(self.volume_id, raw) + /// Maps a raw host inode number from this inode's volume for guest use. For + /// numbers belonging to *other* inodes in the same volume (e.g. readdir + /// child entries); for this inode's own number use [`Self::guest_inode_nr`]. + pub(crate) fn guest_ino(&self, raw: lx::ino_t) -> lx::Result { + self.volume.map_inode(raw) } /// Builds a `fuse_attr` from a stat *of this inode*, reporting its cached - /// namespaced inode number so that aggregated siblings never alias. + /// guest-visible inode number. /// /// `stat` must describe this inode; to report a different inode's /// attributes (e.g. a hard-link target) call this on that inode. pub(crate) fn attr_from_stat(&self, stat: &lx::Stat) -> fuse_attr { let mut attr = util::stat_to_fuse_attr(stat); - attr.ino = self.namespaced_inode_nr; + attr.ino = self.guest_inode_nr; attr } /// Builds a `fuse_statx` from a statx *of this inode*, reporting its cached - /// namespaced inode number. + /// guest-visible inode number. pub(crate) fn statx_from(&self, statx: &lx::StatEx) -> fuse_statx { let mut sx = util::statx_to_fuse_statx(statx); - sx.ino = self.namespaced_inode_nr; + sx.ino = self.guest_inode_nr; sx } @@ -180,12 +216,7 @@ impl VirtioFsInode { /// Performs a lookup for a child of this inode. pub fn lookup_child(&self, name: &LxStr) -> lx::Result<(VirtioFsInode, fuse_attr)> { let path = self.child_path(name)?; - let (inode, stat) = VirtioFsInode::new( - Arc::clone(&self.volume), - self.volume_id, - self.readonly, - path, - )?; + let (inode, stat) = VirtioFsInode::new(Arc::clone(&self.volume), path)?; let attr = inode.attr_from_stat(&stat); Ok((inode, attr)) } @@ -234,13 +265,7 @@ impl VirtioFsInode { let flags = (flags as i32) | lx::O_CREAT | lx::O_NOFOLLOW; let file = self.volume.open(&path, flags, Some(options))?; let stat = file.fstat()?.into(); - let inode = Self::with_attr( - Arc::clone(&self.volume), - self.volume_id, - self.readonly, - path, - &stat, - ); + let inode = Self::with_attr(Arc::clone(&self.volume), path, &stat)?; let attr = inode.attr_from_stat(&stat); Ok((inode, attr, file)) } @@ -258,13 +283,7 @@ impl VirtioFsInode { .volume .mkdir_stat(&path, LxCreateOptions::new(mode, uid, gid))?; - let inode = Self::with_attr( - Arc::clone(&self.volume), - self.volume_id, - self.readonly, - path, - &stat, - ); + let inode = Self::with_attr(Arc::clone(&self.volume), path, &stat)?; let attr = inode.attr_from_stat(&stat); Ok((inode, attr)) } @@ -285,13 +304,7 @@ impl VirtioFsInode { device_id as usize, )?; - let inode = Self::with_attr( - Arc::clone(&self.volume), - self.volume_id, - self.readonly, - path, - &stat, - ); + let inode = Self::with_attr(Arc::clone(&self.volume), path, &stat)?; let attr = inode.attr_from_stat(&stat); Ok((inode, attr)) } @@ -311,19 +324,16 @@ impl VirtioFsInode { LxCreateOptions::new(lx::S_IFLNK | 0o777, uid, gid), )?; - let inode = Self::with_attr( - Arc::clone(&self.volume), - self.volume_id, - self.readonly, - path, - &stat, - ); + let inode = Self::with_attr(Arc::clone(&self.volume), path, &stat)?; let attr = inode.attr_from_stat(&stat); Ok((inode, attr)) } /// Creates a new hard link as a child of this inode. pub fn link(&self, name: &LxStr, target: &VirtioFsInode) -> lx::Result { + if self.volume.id() != target.volume.id() { + return Err(lx::Error::EXDEV); + } let path = self.child_path(name)?; let stat = self.volume.link_stat(&*target.get_path(), path)?; // The reply describes the shared (target) inode, so namespace via the diff --git a/vm/devices/virtio/virtiofs/src/lib.rs b/vm/devices/virtio/virtiofs/src/lib.rs index 44f689b7c3..f5819f41ee 100644 --- a/vm/devices/virtio/virtiofs/src/lib.rs +++ b/vm/devices/virtio/virtiofs/src/lib.rs @@ -25,6 +25,7 @@ use file::VirtioFsFile; use fuse::protocol::*; use fuse::*; use inode::VirtioFsInode; +use inode::VirtioFsVolume; pub use lxutil::LxVolumeOptions; use parking_lot::RwLock; use std::collections::HashMap; @@ -75,6 +76,19 @@ impl VirtioFsInner { } } +fn build_volume( + root_path: impl AsRef, + mount_options: Option<&LxVolumeOptions>, +) -> lx::Result<(lxutil::LxVolume, bool)> { + let readonly = mount_options.is_some_and(|options| options.is_readonly()); + let volume = if let Some(mount_options) = mount_options { + mount_options.new_volume(root_path) + } else { + lxutil::LxVolume::new(root_path) + }?; + Ok((volume, readonly)) +} + /// Implementation of the virtio-fs file system. #[derive(Clone)] pub struct VirtioFs { @@ -103,7 +117,7 @@ impl Fuse for VirtioFs { // In aggregate mode, advertise submounts so the guest gives each child // root its own st_dev when crossing into it. - if self.submounts() && info.capable() & FUSE_SUBMOUNTS != 0 { + if self.initialize_submounts(info.capable() & FUSE_SUBMOUNTS != 0) { info.want |= FUSE_SUBMOUNTS; } } @@ -113,11 +127,11 @@ impl Fuse for VirtioFs { // If a file handle is specified, get the attributes from the open file. This is faster on // Windows and works if the file was deleted. The synthetic root's directory handle has no // backing file, so fall through to the node-based branch for it. - let attr = if flags & FUSE_GETATTR_FH != 0 && fh != SYNTHETIC_ROOT_FH { + let attr = if flags & FUSE_GETATTR_FH != 0 && !self.is_synthetic_root_handle(node_id, fh) { let file = self.get_file(fh)?; file.get_attr()? } else if self.is_synthetic_root(node_id) { - Self::synthetic_root_attr() + self.synthetic_root_attr() } else { let inode = self.get_inode(node_id)?; inode.get_attr()? @@ -138,11 +152,13 @@ impl Fuse for VirtioFs { // If a file handle is specified, get the attributes from the open file. This is faster on // Windows and works if the file was deleted. The synthetic root's directory handle has no // backing file, so fall through to the node-based branch for it. - let statx = if getattr_flags & FUSE_GETATTR_FH != 0 && fh != SYNTHETIC_ROOT_FH { + let statx = if getattr_flags & FUSE_GETATTR_FH != 0 + && !self.is_synthetic_root_handle(node_id, fh) + { let file = self.get_file(fh)?; file.get_statx()? } else if self.is_synthetic_root(node_id) { - Self::synthetic_root_statx(mask) + self.synthetic_root_statx(mask) } else { let inode = self.get_inode(node_id)?; inode.get_statx()? @@ -358,16 +374,16 @@ impl Fuse for VirtioFs { self.open(request, flags) } - fn read_dir(&self, _request: &Request, arg: &fuse_read_in) -> lx::Result> { - if arg.fh == SYNTHETIC_ROOT_FH { + fn read_dir(&self, request: &Request, arg: &fuse_read_in) -> lx::Result> { + if self.is_synthetic_root_handle(request.node_id(), arg.fh) { return self.read_synthetic_root_dir(arg.offset, arg.size, false); } let file = self.get_file(arg.fh)?; file.read_dir(self, arg.offset, arg.size, false) } - fn read_dir_plus(&self, _request: &Request, arg: &fuse_read_in) -> lx::Result> { - if arg.fh == SYNTHETIC_ROOT_FH { + fn read_dir_plus(&self, request: &Request, arg: &fuse_read_in) -> lx::Result> { + if self.is_synthetic_root_handle(request.node_id(), arg.fh) { return self.read_synthetic_root_dir(arg.offset, arg.size, true); } let file = self.get_file(arg.fh)?; @@ -375,7 +391,7 @@ impl Fuse for VirtioFs { } fn release_dir(&self, request: &Request, arg: &fuse_release_in) -> lx::Result<()> { - if arg.fh == SYNTHETIC_ROOT_FH { + if self.is_synthetic_root_handle(request.node_id(), arg.fh) { return Ok(()); } self.release(request, arg) @@ -498,6 +514,7 @@ impl Fuse for VirtioFs { // To get the file system ready for re-mount, clean out any open files and leaked inodes. self.inner.files.write().clear(); self.inner.inodes.write().clear(); + self.reset_submounts(); } } @@ -547,14 +564,10 @@ impl VirtioFs { root_path: impl AsRef, mount_options: Option<&LxVolumeOptions>, ) -> lx::Result { - let readonly = mount_options.is_some_and(|o| o.is_readonly()); - let volume = if let Some(mount_options) = mount_options { - mount_options.new_volume(root_path) - } else { - lxutil::LxVolume::new(root_path) - }?; + let (volume, readonly) = build_volume(root_path, mount_options)?; let mut inodes = InodeMap::new(volume.supports_stable_file_id(), false); - let (root_inode, _) = VirtioFsInode::new(Arc::new(volume), 0, readonly, PathBuf::new())?; + let volume = Arc::new(VirtioFsVolume::new(volume, 0, readonly, true)); + let (root_inode, _) = VirtioFsInode::new(volume, PathBuf::new())?; assert!(inodes.insert(root_inode).1 == FUSE_ROOT_ID); Ok(Self { inner: Arc::new(VirtioFsInner { @@ -569,17 +582,17 @@ impl VirtioFs { /// /// Node 1 is a synthetic, read-only directory; use [`Self::add_child`] to /// expose host folders as named children, each with its own read-only - /// setting. When `submounts` is set, each child is advertised with - /// `FUSE_ATTR_SUBMOUNT` so the guest kernel gives it a distinct `st_dev` - /// (only honored once `FUSE_SUBMOUNTS` is negotiated). - pub fn new_aggregate(submounts: bool) -> Self { + /// setting. During FUSE initialization, `FUSE_SUBMOUNTS` is negotiated so + /// the guest kernel can give each child a distinct `st_dev`; if unavailable, + /// guest inode numbers use the fallback volume namespace. + pub fn new_aggregate() -> Self { Self { inner: Arc::new(VirtioFsInner { // Inode numbers are deduplicated per volume (see `InodeMap`), so // enable the stable-id map and key it by `(volume_id, ino)`. inodes: RwLock::new(InodeMap::new(true, true)), files: RwLock::new(HandleMap::new()), - mode: VirtioFsMode::Aggregate(AggregateState::new(submounts)), + mode: VirtioFsMode::Aggregate(AggregateState::new()), }), } } diff --git a/vm/devices/virtio/virtiofs/src/resolver.rs b/vm/devices/virtio/virtiofs/src/resolver.rs index 64a88cf15e..4084646a6c 100644 --- a/vm/devices/virtio/virtiofs/src/resolver.rs +++ b/vm/devices/virtio/virtiofs/src/resolver.rs @@ -60,7 +60,7 @@ impl ResolveResource for VirtioFsResolver { anyhow::bail!("section fs not supported on this platform") } VirtioFsBackend::Aggregate { children } => { - let fs = VirtioFs::new_aggregate(true); + let fs = VirtioFs::new_aggregate(); for child in children { fs.add_child( &child.name, From 1011983fab665c612de3e5a315b6f49a5b1f22d6 Mon Sep 17 00:00:00 2001 From: Daman Mulye Date: Mon, 13 Jul 2026 10:35:17 -0700 Subject: [PATCH 17/21] review + consolidate tests --- vm/devices/virtio/virtiofs/src/aggregate.rs | 14 +- .../virtio/virtiofs/src/aggregate_tests.rs | 145 +++++------------- 2 files changed, 40 insertions(+), 119 deletions(-) diff --git a/vm/devices/virtio/virtiofs/src/aggregate.rs b/vm/devices/virtio/virtiofs/src/aggregate.rs index 672ae3cb1c..668d19d527 100644 --- a/vm/devices/virtio/virtiofs/src/aggregate.rs +++ b/vm/devices/virtio/virtiofs/src/aggregate.rs @@ -94,15 +94,11 @@ impl AggregateRegistry { self.inode_mode == Some(InodeMode::Raw) } - fn fallback_inode_mapping(&self) -> bool { - self.inode_mode == Some(InodeMode::Namespaced) - } - fn check_can_add(&self) -> lx::Result<()> { if self.tearing_down { return Err(lx::Error::EAGAIN); } - if self.fallback_inode_mapping() && self.next_volume_id > MAX_AGGREGATE_VOLUMES { + if self.next_volume_id > MAX_AGGREGATE_VOLUMES { return Err(lx::Error::ENOSPC); } Ok(()) @@ -142,8 +138,7 @@ impl VirtioFs { /// - `EAGAIN` if the device has begun tearing down (see /// [`Self::begin_teardown`]). /// - `EEXIST` if a child with the same name already exists. - /// - `ENOSPC` if the fallback inode mapping is active and 64 children have - /// already been allocated. + /// - `ENOSPC` if 64 children have already been allocated. pub fn add_child( &self, name: &str, @@ -406,8 +401,9 @@ impl VirtioFs { Ok(buffer.dir_entry_plus(name, next_off, entry)) } else { // Plain readdir: report the directory using the volume root's - // guest-visible inode number, falling back to the volume id if it - // is inaccessible. + // guest-visible inode number. If the root cannot be queried, use + // the volume id as a stable surrogate; mapping errors still + // propagate because the inode cannot be represented correctly. let raw = volume .lstat(PathBuf::new()) .map(|s| s.inode_nr) diff --git a/vm/devices/virtio/virtiofs/src/aggregate_tests.rs b/vm/devices/virtio/virtiofs/src/aggregate_tests.rs index 8657beb079..d8a590a4de 100644 --- a/vm/devices/virtio/virtiofs/src/aggregate_tests.rs +++ b/vm/devices/virtio/virtiofs/src/aggregate_tests.rs @@ -15,9 +15,14 @@ use test_with_tracing::test; fn aggregate_child_registry() { let a = tempfile::tempdir().unwrap(); let b = tempfile::tempdir().unwrap(); + let mut readonly = LxVolumeOptions::default(); + readonly.readonly(true); let fs = VirtioFs::new_aggregate(); - fs.add_child("share_a", a.path(), None).unwrap(); + assert_eq!(fs.synthetic_root_attr().nlink, 2); + assert_eq!(fs.synthetic_root_statx(lx::StatExMask::new()).nlink, 2); + + fs.add_child("share_a", a.path(), Some(&readonly)).unwrap(); fs.add_child("share_b", b.path(), None).unwrap(); // Duplicate names are rejected. @@ -37,7 +42,11 @@ fn aggregate_child_registry() { children.entries[0].volume.id(), children.entries[1].volume.id() ); + assert!(children.entries[0].volume.readonly()); + assert!(!children.entries[1].volume.readonly()); } + assert_eq!(fs.synthetic_root_attr().nlink, 4); + assert_eq!(fs.synthetic_root_statx(lx::StatExMask::new()).nlink, 4); // Removal drops only the named child. fs.remove_child("share_a").unwrap(); @@ -46,12 +55,19 @@ fn aggregate_child_registry() { fs.inner.aggregate().unwrap().registry.read().entries.len(), 1 ); + assert_eq!(fs.synthetic_root_attr().nlink, 3); + assert_eq!(fs.synthetic_root_statx(lx::StatExMask::new()).nlink, 3); } #[test] -fn add_child_rejected_in_direct_mode() { +fn aggregate_operations_are_scoped_to_aggregate_mode() { + let aggregate = VirtioFs::new_aggregate(); + assert!(aggregate.is_synthetic_root_handle(FUSE_ROOT_ID, SYNTHETIC_ROOT_FH)); + assert!(!aggregate.is_synthetic_root_handle(FUSE_ROOT_ID + 1, SYNTHETIC_ROOT_FH)); + let a = tempfile::tempdir().unwrap(); let fs = VirtioFs::new(a.path(), None).unwrap(); + assert!(!fs.is_synthetic_root_handle(FUSE_ROOT_ID, SYNTHETIC_ROOT_FH)); assert_eq!( fs.add_child("x", a.path(), None).unwrap_err(), lx::Error::EINVAL @@ -133,36 +149,6 @@ fn submount_flag_requires_negotiation() { assert_eq!(volume(1).map_inode(u64::MAX), Err(lx::Error::EOVERFLOW)); } -#[test] -fn synthetic_root_handle_is_scoped_to_root() { - let aggregate = VirtioFs::new_aggregate(); - assert!(aggregate.is_synthetic_root_handle(FUSE_ROOT_ID, SYNTHETIC_ROOT_FH)); - assert!(!aggregate.is_synthetic_root_handle(FUSE_ROOT_ID + 1, SYNTHETIC_ROOT_FH)); - - let root = tempfile::tempdir().unwrap(); - let direct = VirtioFs::new(root.path(), None).unwrap(); - assert!(!direct.is_synthetic_root_handle(FUSE_ROOT_ID, SYNTHETIC_ROOT_FH)); -} - -#[test] -fn synthetic_root_link_count_includes_children() { - let first = tempfile::tempdir().unwrap(); - let second = tempfile::tempdir().unwrap(); - let fs = VirtioFs::new_aggregate(); - - assert_eq!(fs.synthetic_root_attr().nlink, 2); - assert_eq!(fs.synthetic_root_statx(lx::StatExMask::new()).nlink, 2); - - fs.add_child("first", first.path(), None).unwrap(); - fs.add_child("second", second.path(), None).unwrap(); - assert_eq!(fs.synthetic_root_attr().nlink, 4); - assert_eq!(fs.synthetic_root_statx(lx::StatExMask::new()).nlink, 4); - - fs.remove_child("first").unwrap(); - assert_eq!(fs.synthetic_root_attr().nlink, 3); - assert_eq!(fs.synthetic_root_statx(lx::StatExMask::new()).nlink, 3); -} - #[test] fn inode_namespacing_avoids_cross_volume_collisions() { // Direct mode (volume id 0) is the identity transform. @@ -196,56 +182,24 @@ fn inode_namespacing_avoids_cross_volume_collisions() { #[test] fn aggregate_volume_count_is_limited_to_namespace_capacity() { let root = tempfile::tempdir().unwrap(); - let fs = VirtioFs::new_aggregate(); - assert!(!fs.initialize_submounts(false)); - - for index in 0..MAX_AGGREGATE_VOLUMES { - fs.add_child(&format!("share_{index}"), root.path(), None) - .unwrap(); + for submounts in [None, Some(false), Some(true)] { + let fs = VirtioFs::new_aggregate(); + fs.inner + .aggregate() + .unwrap() + .registry + .write() + .next_volume_id = MAX_AGGREGATE_VOLUMES; + if let Some(capable) = submounts { + assert_eq!(fs.initialize_submounts(capable), capable); + } + + fs.add_child("last_available", root.path(), None).unwrap(); + assert_eq!( + fs.add_child("one_too_many", root.path(), None), + Err(lx::Error::ENOSPC) + ); } - - assert_eq!( - fs.add_child("one_too_many", root.path(), None).unwrap_err(), - lx::Error::ENOSPC - ); -} - -#[test] -fn aggregate_volume_count_is_not_limited_when_submounts_are_available() { - let root = tempfile::tempdir().unwrap(); - let fs = VirtioFs::new_aggregate(); - - fs.inner - .aggregate() - .unwrap() - .registry - .write() - .next_volume_id = MAX_AGGREGATE_VOLUMES + 1; - fs.add_child("pending", root.path(), None).unwrap(); - - assert!(fs.initialize_submounts(true)); - fs.add_child("negotiated", root.path(), None).unwrap(); - - let aggregate = fs.inner.aggregate().unwrap(); - let children = aggregate.registry.read(); - assert_eq!(children.entries[0].volume.id(), MAX_AGGREGATE_VOLUMES + 1); - assert_eq!(children.entries[1].volume.id(), MAX_AGGREGATE_VOLUMES + 2); - assert_eq!(children.entries[0].volume.map_inode(42).unwrap(), 42); - assert_eq!(children.entries[1].volume.map_inode(42).unwrap(), 42); - - let fallback = VirtioFs::new_aggregate(); - fallback - .inner - .aggregate() - .unwrap() - .registry - .write() - .next_volume_id = MAX_AGGREGATE_VOLUMES + 1; - assert!(!fallback.initialize_submounts(false)); - assert_eq!( - fallback.add_child("fallback", root.path(), None), - Err(lx::Error::ENOSPC) - ); } #[test] @@ -284,35 +238,6 @@ fn hard_link_rejects_cross_volume_target() { assert!(!second.path().join("link").exists()); } -#[test] -fn add_child_allows_per_child_readonly() { - let a = tempfile::tempdir().unwrap(); - let mut ro = LxVolumeOptions::default(); - ro.readonly(true); - let mut rw = LxVolumeOptions::default(); - rw.readonly(false); - - // A writable aggregate lets each child pick its own readonly setting. - let fs = VirtioFs::new_aggregate(); - fs.add_child("ro_child", a.path(), Some(&ro)).unwrap(); - fs.add_child("rw_child", a.path(), Some(&rw)).unwrap(); - - let aggregate = fs.inner.aggregate().unwrap(); - let children = aggregate.registry.read(); - let ro_entry = children - .entries - .iter() - .find(|e| e.name == "ro_child") - .unwrap(); - let rw_entry = children - .entries - .iter() - .find(|e| e.name == "rw_child") - .unwrap(); - assert!(ro_entry.volume.readonly()); - assert!(!rw_entry.volume.readonly()); -} - #[test] fn add_child_rejected_after_teardown() { let a = tempfile::tempdir().unwrap(); From 444ddbb8cbde46c8f3a5c1472ae3e95873e798eb Mon Sep 17 00:00:00 2001 From: Daman Mulye Date: Wed, 15 Jul 2026 10:38:24 -0700 Subject: [PATCH 18/21] test fixes --- vm/devices/virtio/virtiofs/src/aggregate.rs | 18 ++-- .../virtio/virtiofs/src/aggregate_tests.rs | 83 ++++++------------- vm/devices/virtio/virtiofs/src/file.rs | 2 +- vm/devices/virtio/virtiofs/src/inode.rs | 64 +++++++------- vm/devices/virtio/virtiofs/src/lib.rs | 20 +++-- 5 files changed, 78 insertions(+), 109 deletions(-) diff --git a/vm/devices/virtio/virtiofs/src/aggregate.rs b/vm/devices/virtio/virtiofs/src/aggregate.rs index 668d19d527..3b3a92e26c 100644 --- a/vm/devices/virtio/virtiofs/src/aggregate.rs +++ b/vm/devices/virtio/virtiofs/src/aggregate.rs @@ -14,7 +14,6 @@ use crate::ATTRIBUTE_TIMEOUT; use crate::ENTRY_TIMEOUT; use crate::VirtioFs; use crate::build_volume; -use crate::inode::MAX_AGGREGATE_VOLUMES; use crate::inode::VirtioFsInode; use crate::inode::VirtioFsVolume; use fuse::DirEntryWriter; @@ -98,9 +97,6 @@ impl AggregateRegistry { if self.tearing_down { return Err(lx::Error::EAGAIN); } - if self.next_volume_id > MAX_AGGREGATE_VOLUMES { - return Err(lx::Error::ENOSPC); - } Ok(()) } } @@ -138,7 +134,7 @@ impl VirtioFs { /// - `EAGAIN` if the device has begun tearing down (see /// [`Self::begin_teardown`]). /// - `EEXIST` if a child with the same name already exists. - /// - `ENOSPC` if 64 children have already been allocated. + /// - `ENOSPC` if the volume-id space is exhausted (2^32 children). pub fn add_child( &self, name: &str, @@ -178,6 +174,13 @@ impl VirtioFs { use_raw_inodes, )), }); + tracing::info!( + name, + volume_id, + child_count = registry.entries.len(), + submounts = use_raw_inodes, + "added aggregate virtio-fs child" + ); Ok(()) } @@ -402,13 +405,12 @@ impl VirtioFs { } else { // Plain readdir: report the directory using the volume root's // guest-visible inode number. If the root cannot be queried, use - // the volume id as a stable surrogate; mapping errors still - // propagate because the inode cannot be represented correctly. + // the volume id as a stable surrogate. let raw = volume .lstat(PathBuf::new()) .map(|s| s.inode_nr) .unwrap_or(volume.id() as lx::ino_t); - let ino = volume.map_inode(raw)?; + let ino = volume.map_inode(raw); Ok(buffer.dir_entry(name, ino, next_off, lx::DT_DIR as u32)) } } diff --git a/vm/devices/virtio/virtiofs/src/aggregate_tests.rs b/vm/devices/virtio/virtiofs/src/aggregate_tests.rs index d8a590a4de..84087a568e 100644 --- a/vm/devices/virtio/virtiofs/src/aggregate_tests.rs +++ b/vm/devices/virtio/virtiofs/src/aggregate_tests.rs @@ -4,7 +4,6 @@ use super::SYNTHETIC_ROOT_FH; use crate::VirtioFs; use crate::inode; -use crate::inode::MAX_AGGREGATE_VOLUMES; use fuse::protocol::FUSE_ATTR_SUBMOUNT; use fuse::protocol::FUSE_ROOT_ID; use lxutil::LxVolumeOptions; @@ -119,21 +118,23 @@ fn submount_flag_requires_negotiation() { let children = fs.inner.aggregate().unwrap().registry.read(); Arc::clone(&children.entries[index].volume) }; - assert_eq!( - volume(0).map_inode(u64::MAX).unwrap_err(), - lx::Error::EOVERFLOW - ); + // Before submounts are negotiated, child inode numbers are namespaced into + // the shared superblock, so even the largest host inode maps without + // overflowing (and is no longer the identity). + assert_ne!(volume(0).map_inode(u64::MAX), u64::MAX); let entry = fs .lookup_synthetic_root(lx::LxStr::from_bytes(b"first")) .unwrap(); assert_eq!(entry.attr.flags & FUSE_ATTR_SUBMOUNT, 0); + // With submounts negotiated each child gets its own st_dev, so inode + // numbers are passed through unchanged. assert!(fs.initialize_submounts(true)); - assert_eq!(volume(0).map_inode(u64::MAX).unwrap(), u64::MAX); + assert_eq!(volume(0).map_inode(u64::MAX), u64::MAX); fs.add_child("second", second.path(), None).unwrap(); - assert_eq!(volume(1).map_inode(u64::MAX).unwrap(), u64::MAX); + assert_eq!(volume(1).map_inode(u64::MAX), u64::MAX); assert_ne!( fs.lookup_synthetic_root(lx::LxStr::from_bytes(b"second")) .unwrap() @@ -145,61 +146,29 @@ fn submount_flag_requires_negotiation() { fs.reset_submounts(); assert!(!fs.initialize_submounts(false)); - assert_eq!(volume(0).map_inode(u64::MAX), Err(lx::Error::EOVERFLOW)); - assert_eq!(volume(1).map_inode(u64::MAX), Err(lx::Error::EOVERFLOW)); + assert_ne!(volume(0).map_inode(u64::MAX), u64::MAX); + assert_ne!(volume(1).map_inode(u64::MAX), u64::MAX); } #[test] fn inode_namespacing_avoids_cross_volume_collisions() { // Direct mode (volume id 0) is the identity transform. - assert_eq!(inode::namespace_ino(0, 42).unwrap(), 42); - assert_eq!(inode::namespace_ino(0, u64::MAX).unwrap(), u64::MAX); - - const INODE_MASK: u64 = (1 << 58) - 1; - let raw = INODE_MASK; - assert_eq!(inode::namespace_ino(1, raw).unwrap(), INODE_MASK); - assert_eq!( - inode::namespace_ino(2, raw).unwrap(), - (1 << 58) | INODE_MASK - ); - assert_eq!(inode::namespace_ino(64, raw).unwrap(), u64::MAX); - assert_eq!( - inode::namespace_ino(65, raw).unwrap_err(), - lx::Error::ENOSPC - ); - assert_eq!( - inode::namespace_ino(1, INODE_MASK + 1).unwrap_err(), - lx::Error::EOVERFLOW - ); - - // Distinct host inode numbers remain distinct within a volume. - assert_ne!( - inode::namespace_ino(1, 10).unwrap(), - inode::namespace_ino(1, 11).unwrap() - ); -} - -#[test] -fn aggregate_volume_count_is_limited_to_namespace_capacity() { - let root = tempfile::tempdir().unwrap(); - for submounts in [None, Some(false), Some(true)] { - let fs = VirtioFs::new_aggregate(); - fs.inner - .aggregate() - .unwrap() - .registry - .write() - .next_volume_id = MAX_AGGREGATE_VOLUMES; - if let Some(capable) = submounts { - assert_eq!(fs.initialize_submounts(capable), capable); - } - - fs.add_child("last_available", root.path(), None).unwrap(); - assert_eq!( - fs.add_child("one_too_many", root.path(), None), - Err(lx::Error::ENOSPC) - ); - } + assert_eq!(inode::namespace_ino(0, 42), 42); + assert_eq!(inode::namespace_ino(0, u64::MAX), u64::MAX); + + // Namespacing uses the full 64-bit inode space, so even the largest host + // inode numbers map without overflowing or being rejected, and there is no + // limit on the number of volumes. + let _ = inode::namespace_ino(1, u64::MAX); + let _ = inode::namespace_ino(1000, u64::MAX); + + // The transform is a bijection within a volume: distinct host inode numbers + // stay distinct. + assert_ne!(inode::namespace_ino(1, 10), inode::namespace_ino(1, 11)); + + // The same host inode number maps to different values in different volumes, + // reducing cross-volume st_ino collisions under the shared superblock. + assert_ne!(inode::namespace_ino(1, 42), inode::namespace_ino(2, 42)); } #[test] diff --git a/vm/devices/virtio/virtiofs/src/file.rs b/vm/devices/virtio/virtiofs/src/file.rs index df91fe06d4..93753e6645 100644 --- a/vm/devices/virtio/virtiofs/src/file.rs +++ b/vm/devices/virtio/virtiofs/src/file.rs @@ -135,7 +135,7 @@ impl VirtioFsFile { } // Children share this directory's volume, so apply its // guest inode mapping to match lookup/readdirplus. - self.inode.guest_ino(entry.inode_nr)? + self.inode.guest_ino(entry.inode_nr) }; Ok(buffer.dir_entry( diff --git a/vm/devices/virtio/virtiofs/src/inode.rs b/vm/devices/virtio/virtiofs/src/inode.rs index 2788cb045f..2e618516c8 100644 --- a/vm/devices/virtio/virtiofs/src/inode.rs +++ b/vm/devices/virtio/virtiofs/src/inode.rs @@ -16,27 +16,25 @@ use std::sync::Arc; use std::sync::atomic::AtomicU64; use std::sync::atomic::Ordering; -const VOLUME_ID_BITS: u32 = 6; -const INODE_BITS: u32 = u64::BITS - VOLUME_ID_BITS; -const INODE_MASK: u64 = (1 << INODE_BITS) - 1; - -pub(crate) const MAX_AGGREGATE_VOLUMES: u32 = 1 << VOLUME_ID_BITS; - -/// Uses the high six bits to namespace the low 58 bits of a host inode number. -/// `volume_id == 0` is reserved for direct mode and returns `raw` unchanged; -/// aggregate volume IDs 1 through 64 map to namespace values 0 through 63. -pub(crate) fn namespace_ino(volume_id: u32, raw: lx::ino_t) -> lx::Result { +/// Multiplier used to spread a volume id across the 64-bit inode space when +/// namespacing inode numbers (see [`namespace_ino`]). It is the 64-bit +/// golden-ratio constant, chosen because it has good bit-mixing properties. +const INO_NAMESPACE_MULTIPLIER: u64 = 0x9E37_79B9_7F4A_7C15; + +/// XORs a per-volume key into a raw host inode number to reduce the likelihood +/// of cross-volume `st_ino` collisions under the shared FUSE superblock. +/// +/// `volume_id == 0` (direct/single-root mode) returns `raw` unchanged. For +/// aggregated volumes the transform is a bijection within each volume +/// (preserving hard-link identity) and uses the full 64-bit inode space, so it +/// never overflows and imposes no limit on the number of volumes. Cross-volume +/// collisions are still possible when two volumes happen to have inode numbers +/// whose XOR equals the difference of their volume keys. +pub(crate) fn namespace_ino(volume_id: u32, raw: lx::ino_t) -> lx::ino_t { if volume_id == 0 { - return Ok(raw); - } - if volume_id > MAX_AGGREGATE_VOLUMES { - return Err(lx::Error::ENOSPC); - } - if raw > INODE_MASK { - return Err(lx::Error::EOVERFLOW); + return raw; } - - Ok((u64::from(volume_id - 1) << INODE_BITS) | raw) + raw ^ u64::from(volume_id).wrapping_mul(INO_NAMESPACE_MULTIPLIER) } pub(crate) struct VirtioFsVolume { @@ -73,9 +71,9 @@ impl VirtioFsVolume { } } - pub(crate) fn map_inode(&self, raw: lx::ino_t) -> lx::Result { + pub(crate) fn map_inode(&self, raw: lx::ino_t) -> lx::ino_t { if self.use_raw_inodes { - Ok(raw) + raw } else { namespace_ino(self.id, raw) } @@ -105,24 +103,20 @@ impl VirtioFsInode { /// Create a new inode for the specified path. pub fn new(volume: Arc, path: PathBuf) -> lx::Result<(Self, lx::Stat)> { let stat = volume.lstat(&path)?; - let inode = Self::with_attr(volume, path, &stat)?; + let inode = Self::with_attr(volume, path, &stat); Ok((inode, stat)) } /// Create a new inode for the specified path, with previously retrieved attributes. - pub fn with_attr( - volume: Arc, - path: PathBuf, - stat: &lx::Stat, - ) -> lx::Result { - let guest_inode_nr = volume.map_inode(stat.inode_nr)?; - Ok(Self { + pub fn with_attr(volume: Arc, path: PathBuf, stat: &lx::Stat) -> Self { + let guest_inode_nr = volume.map_inode(stat.inode_nr); + Self { volume, path: RwLock::new(path), lookup_count: AtomicU64::new(1), inode_nr: stat.inode_nr, guest_inode_nr, - }) + } } /// Return the files inode number as reported by the underlying file system. @@ -152,7 +146,7 @@ impl VirtioFsInode { /// Maps a raw host inode number from this inode's volume for guest use. For /// numbers belonging to *other* inodes in the same volume (e.g. readdir /// child entries); for this inode's own number use [`Self::guest_inode_nr`]. - pub(crate) fn guest_ino(&self, raw: lx::ino_t) -> lx::Result { + pub(crate) fn guest_ino(&self, raw: lx::ino_t) -> lx::ino_t { self.volume.map_inode(raw) } @@ -265,7 +259,7 @@ impl VirtioFsInode { let flags = (flags as i32) | lx::O_CREAT | lx::O_NOFOLLOW; let file = self.volume.open(&path, flags, Some(options))?; let stat = file.fstat()?.into(); - let inode = Self::with_attr(Arc::clone(&self.volume), path, &stat)?; + let inode = Self::with_attr(Arc::clone(&self.volume), path, &stat); let attr = inode.attr_from_stat(&stat); Ok((inode, attr, file)) } @@ -283,7 +277,7 @@ impl VirtioFsInode { .volume .mkdir_stat(&path, LxCreateOptions::new(mode, uid, gid))?; - let inode = Self::with_attr(Arc::clone(&self.volume), path, &stat)?; + let inode = Self::with_attr(Arc::clone(&self.volume), path, &stat); let attr = inode.attr_from_stat(&stat); Ok((inode, attr)) } @@ -304,7 +298,7 @@ impl VirtioFsInode { device_id as usize, )?; - let inode = Self::with_attr(Arc::clone(&self.volume), path, &stat)?; + let inode = Self::with_attr(Arc::clone(&self.volume), path, &stat); let attr = inode.attr_from_stat(&stat); Ok((inode, attr)) } @@ -324,7 +318,7 @@ impl VirtioFsInode { LxCreateOptions::new(lx::S_IFLNK | 0o777, uid, gid), )?; - let inode = Self::with_attr(Arc::clone(&self.volume), path, &stat)?; + let inode = Self::with_attr(Arc::clone(&self.volume), path, &stat); let attr = inode.attr_from_stat(&stat); Ok((inode, attr)) } diff --git a/vm/devices/virtio/virtiofs/src/lib.rs b/vm/devices/virtio/virtiofs/src/lib.rs index f5819f41ee..33e64d7dba 100644 --- a/vm/devices/virtio/virtiofs/src/lib.rs +++ b/vm/devices/virtio/virtiofs/src/lib.rs @@ -115,11 +115,15 @@ impl Fuse for VirtioFs { info.want2 |= FUSE_DIRECT_IO_ALLOW_MMAP_FLAG2; } - // In aggregate mode, advertise submounts so the guest gives each child - // root its own st_dev when crossing into it. - if self.initialize_submounts(info.capable() & FUSE_SUBMOUNTS != 0) { - info.want |= FUSE_SUBMOUNTS; - } + // Aggregate children are exposed as plain subdirectories of the + // synthetic root rather than FUSE submounts. Advertising + // `FUSE_SUBMOUNTS` would make the guest auto-mount each child on a + // cloned superblock whose mountinfo root is "/"; instead consumers + // (e.g. the WSL drvfs mount-source resolver) rely on each child bind + // carrying the per-child root "/" so the originating share can + // be recovered. Initialize in the namespaced-inode mode, which keeps + // child inode numbers collision-free within the single superblock. + self.initialize_submounts(false); } fn get_attr(&self, request: &Request, flags: u32, fh: u64) -> lx::Result { @@ -582,9 +586,9 @@ impl VirtioFs { /// /// Node 1 is a synthetic, read-only directory; use [`Self::add_child`] to /// expose host folders as named children, each with its own read-only - /// setting. During FUSE initialization, `FUSE_SUBMOUNTS` is negotiated so - /// the guest kernel can give each child a distinct `st_dev`; if unavailable, - /// guest inode numbers use the fallback volume namespace. + /// setting. Children are exposed as plain subdirectories of the synthetic + /// root (not FUSE submounts), so guest inode numbers use the per-volume + /// namespace to stay collision-free within the single superblock. pub fn new_aggregate() -> Self { Self { inner: Arc::new(VirtioFsInner { From 67f1a7608ce52e1930b9c4235f0f70b20b871b96 Mon Sep 17 00:00:00 2001 From: Daman Mulye Date: Wed, 15 Jul 2026 11:59:18 -0700 Subject: [PATCH 19/21] fix --- vm/devices/virtio/virtiofs/src/aggregate.rs | 18 ++++++++-- .../virtio/virtiofs/src/aggregate_tests.rs | 33 +++++++++++++++---- vm/devices/virtio/virtiofs/src/lib.rs | 30 +++++++++-------- vm/devices/virtio/virtiofs/src/resolver.rs | 2 +- 4 files changed, 58 insertions(+), 25 deletions(-) diff --git a/vm/devices/virtio/virtiofs/src/aggregate.rs b/vm/devices/virtio/virtiofs/src/aggregate.rs index 3b3a92e26c..1b0fb19452 100644 --- a/vm/devices/virtio/virtiofs/src/aggregate.rs +++ b/vm/devices/virtio/virtiofs/src/aggregate.rs @@ -109,12 +109,19 @@ impl AggregateRegistry { pub(crate) struct AggregateState { /// Aggregated children and their lifecycle state. registry: RwLock, + /// Whether this consumer wants FUSE submounts (each child on its own cloned + /// superblock, giving it a distinct `st_dev`). Chosen by the device host at + /// construction (see [`VirtioFs::new_aggregate`]). When false, + /// `FUSE_SUBMOUNTS` is never negotiated and children stay plain + /// subdirectories of the synthetic root regardless of guest kernel support. + wants_submounts: bool, } impl AggregateState { - pub(crate) fn new() -> Self { + pub(crate) fn new(wants_submounts: bool) -> Self { Self { registry: RwLock::new(AggregateRegistry::new()), + wants_submounts, } } } @@ -227,13 +234,18 @@ impl VirtioFs { } /// Finalize aggregate inode mapping after FUSE capability negotiation. + /// + /// Submounts are enabled only when the guest kernel is `capable` *and* the + /// device host requested them at construction (see + /// [`VirtioFs::new_aggregate`]). pub(crate) fn initialize_submounts(&self, capable: bool) -> bool { let Some(aggregate) = self.inner.aggregate() else { return false; }; + let enable = capable && aggregate.wants_submounts; let mut registry = aggregate.registry.write(); - registry.initialize(capable); - capable + registry.initialize(enable); + enable } /// Return the aggregate to its pre-initialization state for a remount. diff --git a/vm/devices/virtio/virtiofs/src/aggregate_tests.rs b/vm/devices/virtio/virtiofs/src/aggregate_tests.rs index 84087a568e..3fd6835247 100644 --- a/vm/devices/virtio/virtiofs/src/aggregate_tests.rs +++ b/vm/devices/virtio/virtiofs/src/aggregate_tests.rs @@ -16,7 +16,7 @@ fn aggregate_child_registry() { let b = tempfile::tempdir().unwrap(); let mut readonly = LxVolumeOptions::default(); readonly.readonly(true); - let fs = VirtioFs::new_aggregate(); + let fs = VirtioFs::new_aggregate(true); assert_eq!(fs.synthetic_root_attr().nlink, 2); assert_eq!(fs.synthetic_root_statx(lx::StatExMask::new()).nlink, 2); @@ -60,7 +60,7 @@ fn aggregate_child_registry() { #[test] fn aggregate_operations_are_scoped_to_aggregate_mode() { - let aggregate = VirtioFs::new_aggregate(); + let aggregate = VirtioFs::new_aggregate(true); assert!(aggregate.is_synthetic_root_handle(FUSE_ROOT_ID, SYNTHETIC_ROOT_FH)); assert!(!aggregate.is_synthetic_root_handle(FUSE_ROOT_ID + 1, SYNTHETIC_ROOT_FH)); @@ -77,7 +77,7 @@ fn aggregate_operations_are_scoped_to_aggregate_mode() { #[test] fn add_child_validates_name() { let root = tempfile::tempdir().unwrap(); - let fs = VirtioFs::new_aggregate(); + let fs = VirtioFs::new_aggregate(true); for name in ["", ".", "..", "a/b", "a\0b"] { assert_eq!( @@ -99,7 +99,7 @@ fn synthetic_root_node_ids_start_after_root() { // In aggregate mode the synthetic root occupies FUSE_ROOT_ID, so the // first real inode inserted must be allocated a higher id. let a = tempfile::tempdir().unwrap(); - let fs = VirtioFs::new_aggregate(); + let fs = VirtioFs::new_aggregate(true); fs.add_child("share", a.path(), None).unwrap(); let entry = fs .lookup_synthetic_root(lx::LxStr::from_bytes(b"share")) @@ -111,7 +111,7 @@ fn synthetic_root_node_ids_start_after_root() { fn submount_flag_requires_negotiation() { let first = tempfile::tempdir().unwrap(); let second = tempfile::tempdir().unwrap(); - let fs = VirtioFs::new_aggregate(); + let fs = VirtioFs::new_aggregate(true); fs.add_child("first", first.path(), None).unwrap(); let volume = |index: usize| { @@ -150,6 +150,25 @@ fn submount_flag_requires_negotiation() { assert_ne!(volume(1).map_inode(u64::MAX), u64::MAX); } +#[test] +fn submounts_disabled_by_construction() { + // An aggregate constructed without submounts never negotiates them, even + // when the guest kernel is capable. + let root = tempfile::tempdir().unwrap(); + let fs = VirtioFs::new_aggregate(false); + fs.add_child("child", root.path(), None).unwrap(); + + assert!(!fs.initialize_submounts(true)); + assert_eq!( + fs.lookup_synthetic_root(lx::LxStr::from_bytes(b"child")) + .unwrap() + .attr + .flags + & FUSE_ATTR_SUBMOUNT, + 0 + ); +} + #[test] fn inode_namespacing_avoids_cross_volume_collisions() { // Direct mode (volume id 0) is the identity transform. @@ -177,7 +196,7 @@ fn hard_link_rejects_cross_volume_target() { let second = tempfile::tempdir().unwrap(); std::fs::write(first.path().join("target"), b"data").unwrap(); - let fs = VirtioFs::new_aggregate(); + let fs = VirtioFs::new_aggregate(true); fs.add_child("first", first.path(), None).unwrap(); fs.add_child("second", second.path(), None).unwrap(); @@ -210,7 +229,7 @@ fn hard_link_rejects_cross_volume_target() { #[test] fn add_child_rejected_after_teardown() { let a = tempfile::tempdir().unwrap(); - let fs = VirtioFs::new_aggregate(); + let fs = VirtioFs::new_aggregate(true); fs.add_child("before", a.path(), None).unwrap(); fs.begin_teardown(); diff --git a/vm/devices/virtio/virtiofs/src/lib.rs b/vm/devices/virtio/virtiofs/src/lib.rs index 33e64d7dba..5e2ff595a4 100644 --- a/vm/devices/virtio/virtiofs/src/lib.rs +++ b/vm/devices/virtio/virtiofs/src/lib.rs @@ -115,15 +115,13 @@ impl Fuse for VirtioFs { info.want2 |= FUSE_DIRECT_IO_ALLOW_MMAP_FLAG2; } - // Aggregate children are exposed as plain subdirectories of the - // synthetic root rather than FUSE submounts. Advertising - // `FUSE_SUBMOUNTS` would make the guest auto-mount each child on a - // cloned superblock whose mountinfo root is "/"; instead consumers - // (e.g. the WSL drvfs mount-source resolver) rely on each child bind - // carrying the per-child root "/" so the originating share can - // be recovered. Initialize in the namespaced-inode mode, which keeps - // child inode numbers collision-free within the single superblock. - self.initialize_submounts(false); + // In aggregate mode, advertise submounts when the guest kernel supports + // them *and* the device host requested them at construction (see + // `new_aggregate`). Submounts give each child its own `st_dev` on a + // cloned superblock, but make its mountinfo root "/". + if self.initialize_submounts(info.capable() & FUSE_SUBMOUNTS != 0) { + info.want |= FUSE_SUBMOUNTS; + } } fn get_attr(&self, request: &Request, flags: u32, fh: u64) -> lx::Result { @@ -586,17 +584,21 @@ impl VirtioFs { /// /// Node 1 is a synthetic, read-only directory; use [`Self::add_child`] to /// expose host folders as named children, each with its own read-only - /// setting. Children are exposed as plain subdirectories of the synthetic - /// root (not FUSE submounts), so guest inode numbers use the per-volume - /// namespace to stay collision-free within the single superblock. - pub fn new_aggregate() -> Self { + /// setting. + /// + /// `submounts` selects whether children are advertised with + /// `FUSE_ATTR_SUBMOUNT` (and `FUSE_SUBMOUNTS` negotiated) when the guest + /// kernel supports it, giving each child a distinct `st_dev` on its own + /// cloned superblock. Pass `false` for consumers that recover a child's + /// identity from its mountinfo root (which a submount reports as "/"). + pub fn new_aggregate(submounts: bool) -> Self { Self { inner: Arc::new(VirtioFsInner { // Inode numbers are deduplicated per volume (see `InodeMap`), so // enable the stable-id map and key it by `(volume_id, ino)`. inodes: RwLock::new(InodeMap::new(true, true)), files: RwLock::new(HandleMap::new()), - mode: VirtioFsMode::Aggregate(AggregateState::new()), + mode: VirtioFsMode::Aggregate(AggregateState::new(submounts)), }), } } diff --git a/vm/devices/virtio/virtiofs/src/resolver.rs b/vm/devices/virtio/virtiofs/src/resolver.rs index 4084646a6c..64a88cf15e 100644 --- a/vm/devices/virtio/virtiofs/src/resolver.rs +++ b/vm/devices/virtio/virtiofs/src/resolver.rs @@ -60,7 +60,7 @@ impl ResolveResource for VirtioFsResolver { anyhow::bail!("section fs not supported on this platform") } VirtioFsBackend::Aggregate { children } => { - let fs = VirtioFs::new_aggregate(); + let fs = VirtioFs::new_aggregate(true); for child in children { fs.add_child( &child.name, From 42ae4529e73be486058d9268b956d28b0b0f2f4e Mon Sep 17 00:00:00 2001 From: Daman Mulye Date: Wed, 15 Jul 2026 15:49:36 -0700 Subject: [PATCH 20/21] Remove usage of fuse_attr_submount + incorporate fixes for FAT tests --- vm/devices/support/fs/fuse/src/protocol.rs | 13 +- vm/devices/virtio/virtiofs/src/aggregate.rs | 94 +------------ .../virtio/virtiofs/src/aggregate_tests.rs | 80 +++-------- vm/devices/virtio/virtiofs/src/inode.rs | 73 +++++++--- vm/devices/virtio/virtiofs/src/lib.rs | 131 ++++++++++-------- vm/devices/virtio/virtiofs/src/resolver.rs | 2 +- vm/devices/virtio/virtiofs/src/section.rs | 2 +- vm/devices/virtio/virtiofs/src/util.rs | 2 +- 8 files changed, 153 insertions(+), 244 deletions(-) diff --git a/vm/devices/support/fs/fuse/src/protocol.rs b/vm/devices/support/fs/fuse/src/protocol.rs index c15fa95aaf..636f926539 100644 --- a/vm/devices/support/fs/fuse/src/protocol.rs +++ b/vm/devices/support/fs/fuse/src/protocol.rs @@ -65,20 +65,9 @@ pub struct fuse_attr { pub gid: u32, pub rdev: u32, pub blksize: u32, - pub flags: u32, + pub padding: u32, } -/* - * Flags for `fuse_attr.flags` - * - * FUSE_ATTR_SUBMOUNT: object is a submount root; the kernel auto-mounts a - * distinct submount (with its own st_dev) at this location. Requires the - * `FUSE_SUBMOUNTS` capability to have been negotiated at init. - * FUSE_ATTR_DAX: enable per-inode DAX for this object. - */ -pub const FUSE_ATTR_SUBMOUNT: u32 = 1 << 0; -pub const FUSE_ATTR_DAX: u32 = 1 << 1; - #[repr(C)] #[derive(Debug, IntoBytes, Immutable, KnownLayout, FromBytes)] pub struct fuse_sx_time { diff --git a/vm/devices/virtio/virtiofs/src/aggregate.rs b/vm/devices/virtio/virtiofs/src/aggregate.rs index 1b0fb19452..4c751a3d4d 100644 --- a/vm/devices/virtio/virtiofs/src/aggregate.rs +++ b/vm/devices/virtio/virtiofs/src/aggregate.rs @@ -4,9 +4,10 @@ //! Aggregate (multi-root) virtio-fs. //! //! An aggregate device exposes a synthetic, read-only root directory whose -//! named children are independent host folders. Each child is -//! advertised to the guest with `FUSE_ATTR_SUBMOUNT` (when negotiated) so it -//! gets its own `st_dev`. This module owns all of the aggregate-only state and +//! named children are independent host folders sharing a single FUSE +//! superblock. Each child's inode numbers are namespaced per volume (see +//! [`namespace_ino`](crate::inode::namespace_ino)) to avoid cross-volume +//! `st_ino` collisions. This module owns all of the aggregate-only state and //! the [`VirtioFs`] methods that operate on it; the core (direct-mode) file //! system lives in the crate root. @@ -47,17 +48,9 @@ struct ChildEntry { struct AggregateRegistry { entries: Vec, next_volume_id: u32, - /// Guest inode mapping selected during FUSE initialization. - inode_mode: Option, tearing_down: bool, } -#[derive(Clone, Copy, Debug, PartialEq, Eq)] -enum InodeMode { - Namespaced, - Raw, -} - impl AggregateRegistry { fn new() -> Self { // Volume id 0 is reserved for a direct-mode single root, so aggregated @@ -65,34 +58,10 @@ impl AggregateRegistry { Self { entries: Vec::new(), next_volume_id: 1, - inode_mode: None, tearing_down: false, } } - fn initialize(&mut self, submounts: bool) { - let inode_mode = if submounts { - InodeMode::Raw - } else { - InodeMode::Namespaced - }; - self.inode_mode = Some(inode_mode); - for child in &mut self.entries { - child.volume = Arc::new(child.volume.with_inode_mapping(submounts)); - } - } - - fn reset(&mut self) { - self.inode_mode = None; - for child in &mut self.entries { - child.volume = Arc::new(child.volume.with_inode_mapping(false)); - } - } - - fn submounts_enabled(&self) -> bool { - self.inode_mode == Some(InodeMode::Raw) - } - fn check_can_add(&self) -> lx::Result<()> { if self.tearing_down { return Err(lx::Error::EAGAIN); @@ -109,19 +78,12 @@ impl AggregateRegistry { pub(crate) struct AggregateState { /// Aggregated children and their lifecycle state. registry: RwLock, - /// Whether this consumer wants FUSE submounts (each child on its own cloned - /// superblock, giving it a distinct `st_dev`). Chosen by the device host at - /// construction (see [`VirtioFs::new_aggregate`]). When false, - /// `FUSE_SUBMOUNTS` is never negotiated and children stay plain - /// subdirectories of the synthetic root regardless of guest kernel support. - wants_submounts: bool, } impl AggregateState { - pub(crate) fn new(wants_submounts: bool) -> Self { + pub(crate) fn new() -> Self { Self { registry: RwLock::new(AggregateRegistry::new()), - wants_submounts, } } } @@ -171,21 +133,14 @@ impl VirtioFs { let volume_id = registry.next_volume_id; registry.next_volume_id = volume_id.checked_add(1).ok_or(lx::Error::ENOSPC)?; - let use_raw_inodes = registry.submounts_enabled(); registry.entries.push(ChildEntry { name: name.to_string(), - volume: Arc::new(VirtioFsVolume::new( - volume, - volume_id, - readonly, - use_raw_inodes, - )), + volume: Arc::new(VirtioFsVolume::new(volume, volume_id, readonly)), }); tracing::info!( name, volume_id, child_count = registry.entries.len(), - submounts = use_raw_inodes, "added aggregate virtio-fs child" ); Ok(()) @@ -233,38 +188,6 @@ impl VirtioFs { self.is_synthetic_root(node_id) && fh == SYNTHETIC_ROOT_FH } - /// Finalize aggregate inode mapping after FUSE capability negotiation. - /// - /// Submounts are enabled only when the guest kernel is `capable` *and* the - /// device host requested them at construction (see - /// [`VirtioFs::new_aggregate`]). - pub(crate) fn initialize_submounts(&self, capable: bool) -> bool { - let Some(aggregate) = self.inner.aggregate() else { - return false; - }; - let enable = capable && aggregate.wants_submounts; - let mut registry = aggregate.registry.write(); - registry.initialize(enable); - enable - } - - /// Return the aggregate to its pre-initialization state for a remount. - pub(crate) fn reset_submounts(&self) { - let Some(aggregate) = self.inner.aggregate() else { - return; - }; - let mut registry = aggregate.registry.write(); - registry.reset(); - } - - /// Whether aggregate children should be advertised with - /// `FUSE_ATTR_SUBMOUNT`. Always false in direct mode. - pub(crate) fn submounts(&self) -> bool { - self.inner - .aggregate() - .is_some_and(|aggregate| aggregate.registry.read().submounts_enabled()) - } - /// Attributes of the synthetic aggregate root directory. pub(crate) fn synthetic_root_attr(&self) -> fuse_attr { let mut attr = fuse_attr::new_zeroed(); @@ -324,11 +247,8 @@ impl VirtioFs { fn insert_child_root_entry(&self, volume: Arc) -> lx::Result { let (inode, stat) = VirtioFsInode::new(volume, PathBuf::new())?; - let mut attr = inode.attr_from_stat(&stat); + let attr = inode.attr_from_stat(&stat); let (_, node_id) = self.insert_inode(inode); - if self.submounts() { - attr.flags |= FUSE_ATTR_SUBMOUNT; - } Ok(fuse_entry_out::new( node_id, ENTRY_TIMEOUT, diff --git a/vm/devices/virtio/virtiofs/src/aggregate_tests.rs b/vm/devices/virtio/virtiofs/src/aggregate_tests.rs index 3fd6835247..824522517c 100644 --- a/vm/devices/virtio/virtiofs/src/aggregate_tests.rs +++ b/vm/devices/virtio/virtiofs/src/aggregate_tests.rs @@ -4,7 +4,6 @@ use super::SYNTHETIC_ROOT_FH; use crate::VirtioFs; use crate::inode; -use fuse::protocol::FUSE_ATTR_SUBMOUNT; use fuse::protocol::FUSE_ROOT_ID; use lxutil::LxVolumeOptions; use std::sync::Arc; @@ -16,7 +15,7 @@ fn aggregate_child_registry() { let b = tempfile::tempdir().unwrap(); let mut readonly = LxVolumeOptions::default(); readonly.readonly(true); - let fs = VirtioFs::new_aggregate(true); + let fs = VirtioFs::new_aggregate(); assert_eq!(fs.synthetic_root_attr().nlink, 2); assert_eq!(fs.synthetic_root_statx(lx::StatExMask::new()).nlink, 2); @@ -60,7 +59,7 @@ fn aggregate_child_registry() { #[test] fn aggregate_operations_are_scoped_to_aggregate_mode() { - let aggregate = VirtioFs::new_aggregate(true); + let aggregate = VirtioFs::new_aggregate(); assert!(aggregate.is_synthetic_root_handle(FUSE_ROOT_ID, SYNTHETIC_ROOT_FH)); assert!(!aggregate.is_synthetic_root_handle(FUSE_ROOT_ID + 1, SYNTHETIC_ROOT_FH)); @@ -77,7 +76,7 @@ fn aggregate_operations_are_scoped_to_aggregate_mode() { #[test] fn add_child_validates_name() { let root = tempfile::tempdir().unwrap(); - let fs = VirtioFs::new_aggregate(true); + let fs = VirtioFs::new_aggregate(); for name in ["", ".", "..", "a/b", "a\0b"] { assert_eq!( @@ -99,7 +98,7 @@ fn synthetic_root_node_ids_start_after_root() { // In aggregate mode the synthetic root occupies FUSE_ROOT_ID, so the // first real inode inserted must be allocated a higher id. let a = tempfile::tempdir().unwrap(); - let fs = VirtioFs::new_aggregate(true); + let fs = VirtioFs::new_aggregate(); fs.add_child("share", a.path(), None).unwrap(); let entry = fs .lookup_synthetic_root(lx::LxStr::from_bytes(b"share")) @@ -108,65 +107,18 @@ fn synthetic_root_node_ids_start_after_root() { } #[test] -fn submount_flag_requires_negotiation() { - let first = tempfile::tempdir().unwrap(); - let second = tempfile::tempdir().unwrap(); - let fs = VirtioFs::new_aggregate(true); - fs.add_child("first", first.path(), None).unwrap(); - - let volume = |index: usize| { - let children = fs.inner.aggregate().unwrap().registry.read(); - Arc::clone(&children.entries[index].volume) - }; - // Before submounts are negotiated, child inode numbers are namespaced into - // the shared superblock, so even the largest host inode maps without - // overflowing (and is no longer the identity). - assert_ne!(volume(0).map_inode(u64::MAX), u64::MAX); - - let entry = fs - .lookup_synthetic_root(lx::LxStr::from_bytes(b"first")) - .unwrap(); - assert_eq!(entry.attr.flags & FUSE_ATTR_SUBMOUNT, 0); - - // With submounts negotiated each child gets its own st_dev, so inode - // numbers are passed through unchanged. - assert!(fs.initialize_submounts(true)); - assert_eq!(volume(0).map_inode(u64::MAX), u64::MAX); - - fs.add_child("second", second.path(), None).unwrap(); - assert_eq!(volume(1).map_inode(u64::MAX), u64::MAX); - assert_ne!( - fs.lookup_synthetic_root(lx::LxStr::from_bytes(b"second")) - .unwrap() - .attr - .flags - & FUSE_ATTR_SUBMOUNT, - 0 - ); - - fs.reset_submounts(); - assert!(!fs.initialize_submounts(false)); - assert_ne!(volume(0).map_inode(u64::MAX), u64::MAX); - assert_ne!(volume(1).map_inode(u64::MAX), u64::MAX); -} - -#[test] -fn submounts_disabled_by_construction() { - // An aggregate constructed without submounts never negotiates them, even - // when the guest kernel is capable. +fn aggregate_children_namespace_inodes() { + // Under the single shared superblock, each aggregated child namespaces its + // inode numbers so that even the largest host inode maps to a value other + // than the identity transform reserved for direct mode (volume id 0). let root = tempfile::tempdir().unwrap(); - let fs = VirtioFs::new_aggregate(false); + let fs = VirtioFs::new_aggregate(); fs.add_child("child", root.path(), None).unwrap(); - - assert!(!fs.initialize_submounts(true)); - assert_eq!( - fs.lookup_synthetic_root(lx::LxStr::from_bytes(b"child")) - .unwrap() - .attr - .flags - & FUSE_ATTR_SUBMOUNT, - 0 - ); + let volume = { + let children = fs.inner.aggregate().unwrap().registry.read(); + Arc::clone(&children.entries[0].volume) + }; + assert_ne!(volume.map_inode(u64::MAX), u64::MAX); } #[test] @@ -196,7 +148,7 @@ fn hard_link_rejects_cross_volume_target() { let second = tempfile::tempdir().unwrap(); std::fs::write(first.path().join("target"), b"data").unwrap(); - let fs = VirtioFs::new_aggregate(true); + let fs = VirtioFs::new_aggregate(); fs.add_child("first", first.path(), None).unwrap(); fs.add_child("second", second.path(), None).unwrap(); @@ -229,7 +181,7 @@ fn hard_link_rejects_cross_volume_target() { #[test] fn add_child_rejected_after_teardown() { let a = tempfile::tempdir().unwrap(); - let fs = VirtioFs::new_aggregate(true); + let fs = VirtioFs::new_aggregate(); fs.add_child("before", a.path(), None).unwrap(); fs.begin_teardown(); diff --git a/vm/devices/virtio/virtiofs/src/inode.rs b/vm/devices/virtio/virtiofs/src/inode.rs index 2e618516c8..bf83a2a4ca 100644 --- a/vm/devices/virtio/virtiofs/src/inode.rs +++ b/vm/devices/virtio/virtiofs/src/inode.rs @@ -41,16 +41,14 @@ pub(crate) struct VirtioFsVolume { volume: Arc, id: u32, readonly: bool, - use_raw_inodes: bool, } impl VirtioFsVolume { - pub(crate) fn new(volume: LxVolume, id: u32, readonly: bool, use_raw_inodes: bool) -> Self { + pub(crate) fn new(volume: LxVolume, id: u32, readonly: bool) -> Self { Self { volume: Arc::new(volume), id, readonly, - use_raw_inodes, } } @@ -62,21 +60,8 @@ impl VirtioFsVolume { self.readonly } - pub(crate) fn with_inode_mapping(&self, use_raw_inodes: bool) -> Self { - Self { - volume: Arc::clone(&self.volume), - id: self.id, - readonly: self.readonly, - use_raw_inodes, - } - } - pub(crate) fn map_inode(&self, raw: lx::ino_t) -> lx::ino_t { - if self.use_raw_inodes { - raw - } else { - namespace_ino(self.id, raw) - } + namespace_ino(self.id, raw) } } @@ -88,14 +73,30 @@ impl Deref for VirtioFsVolume { } } +/// Key used to deduplicate inodes in the `InodeMap` so that repeated lookups of +/// the same host file return a single, stable FUSE node id. +/// +/// Volumes that report stable inode numbers key by `(volume_id, inode_nr)`. +/// Volumes that recycle inode numbers (FAT/exFAT) cannot trust the inode +/// number as an identity, so they key by `(volume_id, path)` instead — a given +/// path then maps to one node id across lookups even though the host inode +/// number is not reliable. +#[derive(Clone, PartialEq, Eq, Hash)] +pub(crate) enum DedupKey { + /// `(volume_id, host_inode_number)`, for stable-id volumes. + Ino(u32, lx::ino_t), + /// `(volume_id, host_path)`, for volumes that recycle inode numbers. + Path(u32, PathBuf), +} + /// Implements inode callbacks for virtio-fs. pub struct VirtioFsInode { pub(crate) volume: Arc, path: RwLock, lookup_count: AtomicU64, inode_nr: lx::ino_t, - /// This inode's number as reported to the guest: its host inode number - /// when submounts are negotiated, or its fallback namespaced number. + /// This inode's number as reported to the guest: its namespaced inode + /// number under the shared superblock. guest_inode_nr: lx::ino_t, } @@ -136,9 +137,8 @@ impl VirtioFsInode { self.volume.readonly() } - /// This inode's own number as reported to the guest: its host inode number - /// when submounts are negotiated, or its fallback namespaced number. Fixed - /// for the inode's lifetime. + /// This inode's own number as reported to the guest: its namespaced inode + /// number under the shared superblock. Fixed for the inode's lifetime. pub(crate) fn guest_inode_nr(&self) -> lx::ino_t { self.guest_inode_nr } @@ -400,6 +400,35 @@ impl VirtioFsInode { self.get_path().clone() } + /// The key used to deduplicate this inode in the `InodeMap`, or `None` if it + /// should not be deduplicated. + /// + /// Stable-id volumes key by `(volume_id, inode_nr)`. Volumes that recycle + /// inode numbers (FAT/exFAT) key by `(volume_id, path)` instead, so a given + /// path maps to a single, stable FUSE node id across lookups. A path-keyed + /// inode with an empty path (a volume root) is not deduplicated and returns + /// `None`. + pub(crate) fn dedup_key(&self) -> Option { + if self.volume.supports_stable_file_id() { + return Some(DedupKey::Ino(self.volume_id(), self.inode_nr())); + } + let path = self.get_path(); + if path.as_os_str().is_empty() { + return None; + } + Some(DedupKey::Path(self.volume_id(), path.clone())) + } + + /// The [`DedupKey::Path`] that a child named `name` of this inode would use, + /// for path-keyed (non-stable-id) volumes only. + pub(crate) fn child_path_dedup_key(&self, name: &LxStr) -> Option { + if self.volume.supports_stable_file_id() { + return None; + } + let path = self.child_path(name).ok()?; + Some(DedupKey::Path(self.volume_id(), path)) + } + /// Appends a child name to this inode's path. fn child_path(&self, name: &LxStr) -> lx::Result { // Defense in depth: the FUSE request parser already validates names, diff --git a/vm/devices/virtio/virtiofs/src/lib.rs b/vm/devices/virtio/virtiofs/src/lib.rs index 5e2ff595a4..48538cfd89 100644 --- a/vm/devices/virtio/virtiofs/src/lib.rs +++ b/vm/devices/virtio/virtiofs/src/lib.rs @@ -24,6 +24,7 @@ use aggregate::SYNTHETIC_ROOT_FH; use file::VirtioFsFile; use fuse::protocol::*; use fuse::*; +use inode::DedupKey; use inode::VirtioFsInode; use inode::VirtioFsVolume; pub use lxutil::LxVolumeOptions; @@ -114,14 +115,6 @@ impl Fuse for VirtioFs { if info.capable2() & FUSE_DIRECT_IO_ALLOW_MMAP_FLAG2 != 0 { info.want2 |= FUSE_DIRECT_IO_ALLOW_MMAP_FLAG2; } - - // In aggregate mode, advertise submounts when the guest kernel supports - // them *and* the device host requested them at construction (see - // `new_aggregate`). Submounts give each child its own `st_dev` on a - // cloned superblock, but make its mountinfo root "/". - if self.initialize_submounts(info.capable() & FUSE_SUBMOUNTS != 0) { - info.want |= FUSE_SUBMOUNTS; - } } fn get_attr(&self, request: &Request, flags: u32, fh: u64) -> lx::Result { @@ -425,7 +418,19 @@ impl Fuse for VirtioFs { return Err(lx::Error::EXDEV); } self.check_writable(&inode)?; - inode.rename(name, &new_inode, new_name, flags) + inode.rename(name, &new_inode, new_name, flags)?; + // On path-keyed volumes a rename moves data between + // paths without preserving inode identity, so detach both the source + // path (now vacated) and the destination path (its prior occupant, if + // any, was replaced) from any node ids they referenced. + let mut inodes = self.inner.inodes.write(); + if let Some(key) = inode.child_path_dedup_key(name) { + inodes.evict_dedup_key(&key); + } + if let Some(key) = new_inode.child_path_dedup_key(new_name) { + inodes.evict_dedup_key(&key); + } + Ok(()) } fn statfs(&self, request: &Request) -> lx::Result { @@ -516,7 +521,6 @@ impl Fuse for VirtioFs { // To get the file system ready for re-mount, clean out any open files and leaked inodes. self.inner.files.write().clear(); self.inner.inodes.write().clear(); - self.reset_submounts(); } } @@ -567,8 +571,8 @@ impl VirtioFs { mount_options: Option<&LxVolumeOptions>, ) -> lx::Result { let (volume, readonly) = build_volume(root_path, mount_options)?; - let mut inodes = InodeMap::new(volume.supports_stable_file_id(), false); - let volume = Arc::new(VirtioFsVolume::new(volume, 0, readonly, true)); + let mut inodes = InodeMap::new(false); + let volume = Arc::new(VirtioFsVolume::new(volume, 0, readonly)); let (root_inode, _) = VirtioFsInode::new(volume, PathBuf::new())?; assert!(inodes.insert(root_inode).1 == FUSE_ROOT_ID); Ok(Self { @@ -586,19 +590,17 @@ impl VirtioFs { /// expose host folders as named children, each with its own read-only /// setting. /// - /// `submounts` selects whether children are advertised with - /// `FUSE_ATTR_SUBMOUNT` (and `FUSE_SUBMOUNTS` negotiated) when the guest - /// kernel supports it, giving each child a distinct `st_dev` on its own - /// cloned superblock. Pass `false` for consumers that recover a child's - /// identity from its mountinfo root (which a submount reports as "/"). - pub fn new_aggregate(submounts: bool) -> Self { + /// Children are subdirectories of the synthetic root under a single shared + /// superblock; their inode numbers are namespaced per volume to avoid + /// cross-volume `st_ino` collisions. + pub fn new_aggregate() -> Self { Self { inner: Arc::new(VirtioFsInner { // Inode numbers are deduplicated per volume (see `InodeMap`), so // enable the stable-id map and key it by `(volume_id, ino)`. - inodes: RwLock::new(InodeMap::new(true, true)), + inodes: RwLock::new(InodeMap::new(true)), files: RwLock::new(HandleMap::new()), - mode: VirtioFsMode::Aggregate(AggregateState::new(submounts)), + mode: VirtioFsMode::Aggregate(AggregateState::new()), }), } } @@ -621,7 +623,14 @@ impl VirtioFs { } let inode = self.get_inode(request.node_id())?; self.check_writable(&inode)?; - inode.unlink(name, flags) + inode.unlink(name, flags)?; + // On path-keyed (non-stable-id) volumes the path is the inode's + // identity, so detach it now that it is gone; a later create at the + // same path must not alias the removed inode. + if let Some(key) = inode.child_path_dedup_key(name) { + self.inner.inodes.write().evict_dedup_key(&key); + } + Ok(()) } /// Retrieve the inode with the specified node ID. @@ -725,9 +734,11 @@ impl HandleMap { /// globally unique, whereas inode numbers are per-volume. struct InodeMap { inodes_by_node_id: HandleMap>, - // If stable inode numbers are supported, this maps `(volume_id, inode_nr)` to the - // corresponding inode and its FUSE node ID. - inodes_by_inode_nr: Option, u64)>>, + /// Maps a [`DedupKey`] to the registered inode and its FUSE node id, for + /// inodes eligible for deduplication. Stable-id volumes key by inode number + /// ([`DedupKey::Ino`]); volumes that recycle inode numbers (FAT/exFAT) key + /// by path ([`DedupKey::Path`]). + inodes_by_key: HashMap, u64)>, /// When true, node 1 is synthetic and not stored in this map, so node IDs /// are allocated starting at 2 and `clear` does not preserve a real root. aggregate: bool, @@ -735,18 +746,14 @@ struct InodeMap { impl InodeMap { /// Create a new `InodeMap`. - pub fn new(supports_stable_file_id: bool, aggregate: bool) -> Self { + pub fn new(aggregate: bool) -> Self { Self { inodes_by_node_id: if aggregate { HandleMap::starting_at(FUSE_ROOT_ID + 1) } else { HandleMap::new() }, - inodes_by_inode_nr: if supports_stable_file_id { - Some(HashMap::new()) - } else { - None - }, + inodes_by_key: HashMap::new(), aggregate, } } @@ -759,22 +766,18 @@ impl InodeMap { /// Insert an inode into the map, returning its node ID. pub fn insert(&mut self, inode: VirtioFsInode) -> (Arc, u64) { - // Only consult the stable-inode dedup map when the inode's backing - // volume actually has stable file IDs. Volumes without stable IDs - // (e.g. FAT) can reuse inode numbers after rename/deletion, so - // deduplicating by (volume_id, inode_nr) would alias unrelated files. - if let Some(inodes_by_inode_nr) = self - .inodes_by_inode_nr - .as_mut() - .filter(|_| inode.volume.supports_stable_file_id()) - { - match inodes_by_inode_nr.entry((inode.volume_id(), inode.inode_nr())) { + // If this inode has a dedup key, reuse an existing node id for the same + // host file. Stable-id volumes dedup by `(volume_id, inode_nr)`; volumes + // that recycle inode numbers (e.g. FAT) dedup by `(volume_id, path)`, so + // a given path keeps one stable node id across lookups. + if let Some(key) = inode.dedup_key() { + match self.inodes_by_key.entry(key) { Entry::Occupied(entry) => { // Inode found; increment its count and return the existing FUSE node ID. let new_path = inode.clone_path(); - let (inode, node_id) = entry.get(); - inode.lookup(new_path); - return (Arc::clone(inode), *node_id); + let (existing, node_id) = entry.get(); + existing.lookup(new_path); + return (Arc::clone(existing), *node_id); } Entry::Vacant(entry) => { // Inode not found, so insert it into both maps. @@ -786,7 +789,8 @@ impl InodeMap { } } - // No support for stable inode numbers, so just use node ID. + // Inode is not eligible for dedup (e.g. an empty-path volume root); just + // allocate a fresh node id. let inode = Arc::new(inode); let node_id = self.inodes_by_node_id.insert(Arc::clone(&inode)); (inode, node_id) @@ -795,8 +799,28 @@ impl InodeMap { /// Remove an inode with the specified FUSE node ID from the map. pub fn remove(&mut self, node_id: u64) { let inode = self.inodes_by_node_id.remove(node_id).unwrap(); - if let Some(inodes_by_inode_nr) = self.inodes_by_inode_nr.as_mut() { - inodes_by_inode_nr.remove(&(inode.volume_id(), inode.inode_nr())); + if let Some(key) = inode.dedup_key() { + // Only drop the by-key entry if it still points at THIS node. For + // path-keyed volumes a delete+recreate (or an explicit + // `evict_dedup_key`) can repoint the path to a newer inode while + // this (older) one lingers behind a live fd or inotify watch; + // removing it unconditionally would orphan that newer inode. + if let Entry::Occupied(entry) = self.inodes_by_key.entry(key) { + if entry.get().1 == node_id { + entry.remove(); + } + } + } + } + + /// Detach a [`DedupKey::Path`] entry from whatever inode it currently maps + /// to, leaving that inode in `inodes_by_node_id` (a live fd or inotify watch + /// may still reference it) but no longer reachable for dedup. A subsequent + /// create at the same path therefore gets a fresh node id rather than + /// aliasing the removed/renamed file. + pub fn evict_dedup_key(&mut self, key: &DedupKey) { + if matches!(key, DedupKey::Path(..)) { + self.inodes_by_key.remove(key); } } @@ -807,9 +831,7 @@ impl InodeMap { // allocating node IDs after the reserved root id. self.inodes_by_node_id.clear(); self.inodes_by_node_id.next_handle = FUSE_ROOT_ID + 1; - if let Some(inodes_by_inode_nr) = self.inodes_by_inode_nr.as_mut() { - inodes_by_inode_nr.clear(); - } + self.inodes_by_key.clear(); return; } @@ -819,13 +841,10 @@ impl InodeMap { // Re-insert the root inode. assert!(self.inodes_by_node_id.insert(Arc::clone(&root_inode)) == FUSE_ROOT_ID); - // Clear the inode number map if it's supported. - if let Some(inodes_by_inode_nr) = self.inodes_by_inode_nr.as_mut() { - inodes_by_inode_nr.clear(); - inodes_by_inode_nr.insert( - (root_inode.volume_id(), root_inode.inode_nr()), - (root_inode, FUSE_ROOT_ID), - ); + // Rebuild the dedup map with just the root, if it has a dedup key. + self.inodes_by_key.clear(); + if let Some(key) = root_inode.dedup_key() { + self.inodes_by_key.insert(key, (root_inode, FUSE_ROOT_ID)); } } } diff --git a/vm/devices/virtio/virtiofs/src/resolver.rs b/vm/devices/virtio/virtiofs/src/resolver.rs index 64a88cf15e..4084646a6c 100644 --- a/vm/devices/virtio/virtiofs/src/resolver.rs +++ b/vm/devices/virtio/virtiofs/src/resolver.rs @@ -60,7 +60,7 @@ impl ResolveResource for VirtioFsResolver { anyhow::bail!("section fs not supported on this platform") } VirtioFsBackend::Aggregate { children } => { - let fs = VirtioFs::new_aggregate(true); + let fs = VirtioFs::new_aggregate(); for child in children { fs.add_child( &child.name, diff --git a/vm/devices/virtio/virtiofs/src/section.rs b/vm/devices/virtio/virtiofs/src/section.rs index 2c84c4ba99..8999689b44 100644 --- a/vm/devices/virtio/virtiofs/src/section.rs +++ b/vm/devices/virtio/virtiofs/src/section.rs @@ -59,7 +59,7 @@ fn get_attr_with_cur_time() -> fuse::protocol::fuse_attr { gid: 0, rdev: 0, blksize: 0, - flags: 0, + padding: 0, } } diff --git a/vm/devices/virtio/virtiofs/src/util.rs b/vm/devices/virtio/virtiofs/src/util.rs index bf19409986..e5e86596b5 100644 --- a/vm/devices/virtio/virtiofs/src/util.rs +++ b/vm/devices/virtio/virtiofs/src/util.rs @@ -26,7 +26,7 @@ pub fn stat_to_fuse_attr(stat: &lx::Stat) -> fuse_attr { rdev: stat.device_nr_special as u32, // This is `usize` on x64 and `u32` on arm64, avoid a warning. blksize: stat.block_size as _, - flags: 0, + padding: 0, } } From 0a3326b466cf245b2587b1d45f9f030d774fde06 Mon Sep 17 00:00:00 2001 From: Daman Mulye Date: Fri, 17 Jul 2026 09:05:03 -0700 Subject: [PATCH 21/21] feedback + update comments --- vm/devices/virtio/virtiofs/src/aggregate.rs | 24 ++--- vm/devices/virtio/virtiofs/src/inode.rs | 21 ++-- vm/devices/virtio/virtiofs/src/lib.rs | 107 ++++++++------------ 3 files changed, 59 insertions(+), 93 deletions(-) diff --git a/vm/devices/virtio/virtiofs/src/aggregate.rs b/vm/devices/virtio/virtiofs/src/aggregate.rs index 4c751a3d4d..79a5fadbac 100644 --- a/vm/devices/virtio/virtiofs/src/aggregate.rs +++ b/vm/devices/virtio/virtiofs/src/aggregate.rs @@ -27,12 +27,10 @@ use std::path::PathBuf; use std::sync::Arc; use zerocopy::FromZeros; -/// Reserved file handle returned by `open_dir` on the synthetic aggregate root. -/// -/// `read_dir`/`read_dir_plus`/`release_dir` recognize this sentinel and service -/// it from the root registry rather than the (real-file) handle map. `u64::MAX` -/// can never collide with a real handle because `HandleMap` allocates starting -/// at 1 and only increments. +/// Reserved file handle `open_dir` returns for the synthetic aggregate root; +/// `read_dir`/`read_dir_plus`/`release_dir` recognize it and serve the root +/// registry instead of the handle map. `u64::MAX` can't collide with a real +/// handle, since `HandleMap` allocates from 1 upward. pub(crate) const SYNTHETIC_ROOT_FH: u64 = u64::MAX; /// A single host folder exposed as a named child of the synthetic aggregate root. @@ -73,8 +71,7 @@ impl AggregateRegistry { /// State that only exists for an aggregate-mode [`VirtioFs`]. /// /// When present, node 1 is a synthetic directory whose children are the entries -/// in `registry`; when absent (direct mode), node 1 is a real inode at a single -/// volume root (legacy single-share behavior). +/// in `registry`. pub(crate) struct AggregateState { /// Aggregated children and their lifecycle state. registry: RwLock, @@ -146,13 +143,10 @@ impl VirtioFs { Ok(()) } - /// Signal that the owning device host has begun tearing the aggregate - /// device down. After this, [`Self::add_child`] rejects new children with - /// `EAGAIN`, so an in-flight add cannot append a child to a device that is - /// going away. No-op for direct-mode file systems. - /// - /// The running device keeps serving existing inodes until it is fully - /// dropped; this only stops further children from being added. + /// Signal that the aggregate device has begun tearing down, so + /// [`Self::add_child`] rejects further children with `EAGAIN`. Existing + /// inodes keep being served until the device is dropped. No-op in direct + /// mode. pub fn begin_teardown(&self) { if let Some(aggregate) = self.inner.aggregate() { aggregate.registry.write().tearing_down = true; diff --git a/vm/devices/virtio/virtiofs/src/inode.rs b/vm/devices/virtio/virtiofs/src/inode.rs index bf83a2a4ca..bdb35e3ee8 100644 --- a/vm/devices/virtio/virtiofs/src/inode.rs +++ b/vm/devices/virtio/virtiofs/src/inode.rs @@ -400,23 +400,20 @@ impl VirtioFsInode { self.get_path().clone() } - /// The key used to deduplicate this inode in the `InodeMap`, or `None` if it - /// should not be deduplicated. + /// The key used to deduplicate this inode in the `InodeMap`, so that + /// repeated lookups of the same host file return one stable FUSE node id. /// /// Stable-id volumes key by `(volume_id, inode_nr)`. Volumes that recycle /// inode numbers (FAT/exFAT) key by `(volume_id, path)` instead, so a given - /// path maps to a single, stable FUSE node id across lookups. A path-keyed - /// inode with an empty path (a volume root) is not deduplicated and returns - /// `None`. - pub(crate) fn dedup_key(&self) -> Option { + /// path maps to a single, stable FUSE node id across lookups. This includes + /// the empty path of a volume root, so an aggregate child root keeps one + /// stable node id across repeated lookups rather than allocating a fresh one + /// each time. + pub(crate) fn dedup_key(&self) -> DedupKey { if self.volume.supports_stable_file_id() { - return Some(DedupKey::Ino(self.volume_id(), self.inode_nr())); + return DedupKey::Ino(self.volume_id(), self.inode_nr()); } - let path = self.get_path(); - if path.as_os_str().is_empty() { - return None; - } - Some(DedupKey::Path(self.volume_id(), path.clone())) + DedupKey::Path(self.volume_id(), self.get_path().clone()) } /// The [`DedupKey::Path`] that a child named `name` of this inode would use, diff --git a/vm/devices/virtio/virtiofs/src/lib.rs b/vm/devices/virtio/virtiofs/src/lib.rs index 48538cfd89..0b2337c21a 100644 --- a/vm/devices/virtio/virtiofs/src/lib.rs +++ b/vm/devices/virtio/virtiofs/src/lib.rs @@ -56,9 +56,8 @@ struct VirtioFsInner { /// Distinguishes a single-share device from a multi-share aggregate. /// -/// The read-only setting is not tracked here: in direct mode it is baked into -/// the inodes (each carries its volume's setting), and in aggregate mode each -/// child carries its own (see [`AggregateState`]). +/// The read-only setting lives on each volume's inodes, not here, so aggregate +/// children can differ (see [`AggregateState`]). enum VirtioFsMode { /// Single share: node 1 is a real inode at the volume root. Direct, @@ -419,10 +418,9 @@ impl Fuse for VirtioFs { } self.check_writable(&inode)?; inode.rename(name, &new_inode, new_name, flags)?; - // On path-keyed volumes a rename moves data between - // paths without preserving inode identity, so detach both the source - // path (now vacated) and the destination path (its prior occupant, if - // any, was replaced) from any node ids they referenced. + // A rename doesn't preserve inode identity on path-keyed volumes, so + // evict both the vacated source path and the overwritten destination + // path from the dedup map. let mut inodes = self.inner.inodes.write(); if let Some(key) = inode.child_path_dedup_key(name) { inodes.evict_dedup_key(&key); @@ -588,16 +586,12 @@ impl VirtioFs { /// /// Node 1 is a synthetic, read-only directory; use [`Self::add_child`] to /// expose host folders as named children, each with its own read-only - /// setting. - /// - /// Children are subdirectories of the synthetic root under a single shared - /// superblock; their inode numbers are namespaced per volume to avoid - /// cross-volume `st_ino` collisions. + /// setting. Children share one superblock, with inode numbers namespaced + /// per volume to avoid cross-volume `st_ino` collisions. pub fn new_aggregate() -> Self { Self { inner: Arc::new(VirtioFsInner { - // Inode numbers are deduplicated per volume (see `InodeMap`), so - // enable the stable-id map and key it by `(volume_id, ino)`. + // `true` enables aggregate mode: node 1 is synthetic (see `InodeMap`). inodes: RwLock::new(InodeMap::new(true)), files: RwLock::new(HandleMap::new()), mode: VirtioFsMode::Aggregate(AggregateState::new()), @@ -624,9 +618,8 @@ impl VirtioFs { let inode = self.get_inode(request.node_id())?; self.check_writable(&inode)?; inode.unlink(name, flags)?; - // On path-keyed (non-stable-id) volumes the path is the inode's - // identity, so detach it now that it is gone; a later create at the - // same path must not alias the removed inode. + // On path-keyed volumes the path is the inode's identity, so evict it + // now; a later create at the same path must not alias the removed inode. if let Some(key) = inode.child_path_dedup_key(name) { self.inner.inodes.write().evict_dedup_key(&key); } @@ -734,10 +727,8 @@ impl HandleMap { /// globally unique, whereas inode numbers are per-volume. struct InodeMap { inodes_by_node_id: HandleMap>, - /// Maps a [`DedupKey`] to the registered inode and its FUSE node id, for - /// inodes eligible for deduplication. Stable-id volumes key by inode number - /// ([`DedupKey::Ino`]); volumes that recycle inode numbers (FAT/exFAT) key - /// by path ([`DedupKey::Path`]). + /// Maps a [`DedupKey`] to the registered inode and its FUSE node id, so + /// repeated lookups of one host file share a single node id. inodes_by_key: HashMap, u64)>, /// When true, node 1 is synthetic and not stored in this map, so node IDs /// are allocated starting at 2 and `clear` does not preserve a real root. @@ -766,58 +757,43 @@ impl InodeMap { /// Insert an inode into the map, returning its node ID. pub fn insert(&mut self, inode: VirtioFsInode) -> (Arc, u64) { - // If this inode has a dedup key, reuse an existing node id for the same - // host file. Stable-id volumes dedup by `(volume_id, inode_nr)`; volumes - // that recycle inode numbers (e.g. FAT) dedup by `(volume_id, path)`, so - // a given path keeps one stable node id across lookups. - if let Some(key) = inode.dedup_key() { - match self.inodes_by_key.entry(key) { - Entry::Occupied(entry) => { - // Inode found; increment its count and return the existing FUSE node ID. - let new_path = inode.clone_path(); - let (existing, node_id) = entry.get(); - existing.lookup(new_path); - return (Arc::clone(existing), *node_id); - } - Entry::Vacant(entry) => { - // Inode not found, so insert it into both maps. - let inode = Arc::new(inode); - let node_id = self.inodes_by_node_id.insert(Arc::clone(&inode)); - entry.insert((Arc::clone(&inode), node_id)); - return (inode, node_id); - } + // Reuse an existing node id for the same host file; see `DedupKey` + // for how each volume type is keyed. + match self.inodes_by_key.entry(inode.dedup_key()) { + Entry::Occupied(entry) => { + // Inode found; increment its count and return the existing FUSE node ID. + let new_path = inode.clone_path(); + let (existing, node_id) = entry.get(); + existing.lookup(new_path); + (Arc::clone(existing), *node_id) + } + Entry::Vacant(entry) => { + // Inode not found, so insert it into both maps. + let inode = Arc::new(inode); + let node_id = self.inodes_by_node_id.insert(Arc::clone(&inode)); + entry.insert((Arc::clone(&inode), node_id)); + (inode, node_id) } } - - // Inode is not eligible for dedup (e.g. an empty-path volume root); just - // allocate a fresh node id. - let inode = Arc::new(inode); - let node_id = self.inodes_by_node_id.insert(Arc::clone(&inode)); - (inode, node_id) } /// Remove an inode with the specified FUSE node ID from the map. pub fn remove(&mut self, node_id: u64) { let inode = self.inodes_by_node_id.remove(node_id).unwrap(); - if let Some(key) = inode.dedup_key() { - // Only drop the by-key entry if it still points at THIS node. For - // path-keyed volumes a delete+recreate (or an explicit - // `evict_dedup_key`) can repoint the path to a newer inode while - // this (older) one lingers behind a live fd or inotify watch; - // removing it unconditionally would orphan that newer inode. - if let Entry::Occupied(entry) = self.inodes_by_key.entry(key) { - if entry.get().1 == node_id { - entry.remove(); - } + // Only drop the by-key entry if it still points at THIS node: on + // path-keyed volumes the path may have been repointed to a newer inode + // (via delete+recreate or `evict_dedup_key`), which must not be lost. + if let Entry::Occupied(entry) = self.inodes_by_key.entry(inode.dedup_key()) { + if entry.get().1 == node_id { + entry.remove(); } } } - /// Detach a [`DedupKey::Path`] entry from whatever inode it currently maps - /// to, leaving that inode in `inodes_by_node_id` (a live fd or inotify watch - /// may still reference it) but no longer reachable for dedup. A subsequent - /// create at the same path therefore gets a fresh node id rather than - /// aliasing the removed/renamed file. + /// Detach a [`DedupKey::Path`] entry from its current inode so a later + /// create at that path gets a fresh node id instead of aliasing the + /// removed/renamed file. The inode stays in `inodes_by_node_id` for any + /// live fd or watch. pub fn evict_dedup_key(&mut self, key: &DedupKey) { if matches!(key, DedupKey::Path(..)) { self.inodes_by_key.remove(key); @@ -841,10 +817,9 @@ impl InodeMap { // Re-insert the root inode. assert!(self.inodes_by_node_id.insert(Arc::clone(&root_inode)) == FUSE_ROOT_ID); - // Rebuild the dedup map with just the root, if it has a dedup key. + // Rebuild the dedup map with just the root. self.inodes_by_key.clear(); - if let Some(key) = root_inode.dedup_key() { - self.inodes_by_key.insert(key, (root_inode, FUSE_ROOT_ID)); - } + let key = root_inode.dedup_key(); + self.inodes_by_key.insert(key, (root_inode, FUSE_ROOT_ID)); } }