From 9d77a4cde935564c66d642c1f51d1cd23b7df80e Mon Sep 17 00:00:00 2001 From: Stephen Akinyemi Date: Sun, 27 Jul 2025 22:32:37 +0200 Subject: [PATCH 1/2] feat(fs): implement file-backed symlinks with virtualized stats for Linux This commit implements a dual symlink representation system for Linux filesystems (overlayfs and passthrough) to support stat virtualization on symlinks. ## Overview On Linux, many filesystems don't support extended attributes on symlinks, which prevents us from virtualizing uid/gid/mode. This implementation provides two symlink representations: 1. **Regular symlinks** - Traditional symlinks (when stat override not needed) 2. **File-backed symlinks** - Regular files storing the link target as content, with the actual file type (S_IFLNK) stored in the override xattr ## Key Changes ### Implementation (overlayfs & passthrough) - Modified symlink creation to always use file-backed representation on Linux - Symlink target is stored as file content (up to PATH_MAX bytes) - File type S_IFLNK is preserved in the override_stat xattr - Added proper readlink support for file-backed symlinks - Fixed readlink to use /proc/self/fd/ for O_PATH file descriptors ### Platform Differences - **Linux**: Uses file-backed symlinks to support xattr-based stat virtualization - **macOS**: Continues using regular symlinks with xattr support ### Test Updates - Comprehensive test updates with platform-specific verification - Added detailed checks for xattr content and file representation - Enhanced tests to verify copy-up behavior, nested symlinks, and edge cases - All tests properly handle both symlink representations ## Technical Details File-backed symlinks: - Created as regular files with mode 0600 - Content: symlink target path - xattr format: "uid:gid:mode" where mode includes S_IFLNK - Transparent to VFS layer - appears as regular symlinks to applications This approach ensures consistent behavior across different filesystem types while maintaining full stat virtualization capabilities. Fixes symlink stat virtualization on Linux filesystems that don't support xattrs on symlinks. --- src/devices/src/virtio/fs/linux/overlayfs.rs | 169 +++++++-- .../src/virtio/fs/linux/passthrough.rs | 134 ++++++- .../src/virtio/fs/tests/overlayfs/create.rs | 343 +++++++++++++++--- .../src/virtio/fs/tests/overlayfs/metadata.rs | 65 ++++ .../src/virtio/fs/tests/passthrough/create.rs | 129 ++++++- 5 files changed, 713 insertions(+), 127 deletions(-) diff --git a/src/devices/src/virtio/fs/linux/overlayfs.rs b/src/devices/src/virtio/fs/linux/overlayfs.rs index dbb8d7dd8..cc6e45e9a 100644 --- a/src/devices/src/virtio/fs/linux/overlayfs.rs +++ b/src/devices/src/virtio/fs/linux/overlayfs.rs @@ -812,12 +812,10 @@ impl OverlayFs { mode: Option, rdev: Option, ) -> io::Result<()> { - // Check file type and skip xattr operations for types that don't support them + // Step 1: Check if this is a real symlink - we can't set xattr on those let file_type = st.st_mode & libc::S_IFMT; - let is_symlink = file_type == libc::S_IFLNK; - - // Handle symlinks specially - if is_symlink { + if file_type == libc::S_IFLNK { + // Real symlink - Linux doesn't support xattr on symlinks // WARNING: While Linux allows changing symlink ownership via lchown(), we cannot // virtualize this because most filesystems don't support extended attributes on // symlinks. Symlink ownership matters for security: @@ -834,14 +832,25 @@ impl OverlayFs { return Ok(()); } - // Get the current values to use as defaults + // Step 2: For all other files (including file-backed symlinks), get the current values let (uid, gid) = if let Some((uid, gid)) = owner { (uid, gid) } else { (st.st_uid, st.st_gid) }; - let mode = mode.unwrap_or(st.st_mode); + // Step 3: Determine the mode to use + let mode = if let Some(m) = mode { + m // Use provided mode + } else { + // No mode provided - preserve existing virtualized mode if it exists + // This is crucial for file-backed symlinks to preserve their S_IFLNK type + if let Ok(Some((_, _, existing_mode, _))) = self.get_override_xattr(fd, st) { + existing_mode // Preserve virtualized mode + } else { + st.st_mode // Fall back to actual file mode + } + }; // Format the xattr value - include full mode with file type bits for special files // that we store as regular files (FIFOs, sockets, device nodes) @@ -2541,7 +2550,7 @@ impl OverlayFs { fn do_symlink( &self, - _ctx: Context, + ctx: Context, linkname: &CStr, parent: Inode, name: &CStr, @@ -2572,48 +2581,136 @@ impl OverlayFs { // Get the parent file descriptor let parent_fd = parent_data.file.as_raw_fd(); - // Create the node device - let res = unsafe { libc::symlinkat(linkname.as_ptr(), parent_fd, name.as_ptr()) }; + // NOTE: We create symlinks as regular files on Linux to support setting + // xattr on them. Most filesystems don't support extended attributes on symlinks. + // The link target is stored as file content and the actual file type (S_IFLNK) + // is stored in the override xattr. + + // First check link target length + let linkname_bytes = linkname.to_bytes(); + if linkname_bytes.len() > libc::PATH_MAX as usize { + return Err(io::Error::from_raw_os_error(libc::ENAMETOOLONG)); + } - if res == 0 { - let file = self.open_file_at(parent_fd, name, libc::O_PATH)?.0; + // Create a regular file to back the symlink + let fd = unsafe { + libc::openat( + parent_fd, + name.as_ptr(), + libc::O_CREAT | libc::O_EXCL | libc::O_WRONLY | libc::O_CLOEXEC, + 0o600, + ) + }; - // NOTE: On Linux, we cannot virtualize symlink ownership because most filesystems - // don't support extended attributes on symlinks. The symlink will have the - // ownership of the process that created it. This is a known limitation. - // We don't call set_override_xattr here because it would return EOPNOTSUPP. + if fd < 0 { + return Err(io::Error::last_os_error()); + } - // Get stat (without virtualized ownership) - let (updated_stat, mnt_id) = Self::unpatched_statx(file.as_raw_fd(), None)?; + // Write the link target as file content + let res = unsafe { + libc::write( + fd, + linkname_bytes.as_ptr() as *const libc::c_void, + linkname_bytes.len(), + ) + }; - let mut path = parent_data.path.clone(); - path.push(self.intern_name(name)?); + if res < 0 || res as usize != linkname_bytes.len() { + unsafe { libc::close(fd) }; + // Clean up the file on error + unsafe { libc::unlinkat(parent_fd, name.as_ptr(), 0) }; + return Err(io::Error::last_os_error()); + } - // Create the inode for the newly created directory - let (inode, _) = self.create_inode( - file, - updated_stat.st_ino, - updated_stat.st_dev, - mnt_id, - path, - parent_data.layer_idx, - ); + // Get initial stat. + let (stat, _) = Self::unpatched_statx(fd, None)?; - // Create the entry for the newly created directory - let entry = self.create_entry(inode, updated_stat); + // Set ownership and permissions with S_IFLNK mode + self.set_override_xattr( + fd, + &stat, + Some((ctx.uid, ctx.gid)), + Some(libc::S_IFLNK | 0o777), + None, + )?; - return Ok(entry); - } + // Close the fd + unsafe { libc::close(fd) }; - // Return the error - Err(io::Error::last_os_error()) + // Re-open with O_PATH for inode management + let file = self.open_file_at(parent_fd, name, libc::O_PATH)?.0; + + // Get updated stat with override applied + let (updated_stat, mnt_id) = self.patched_statx(file.as_raw_fd(), None)?; + + let mut path = parent_data.path.clone(); + path.push(self.intern_name(name)?); + + // Create the inode for the newly created symlink + let (inode, _) = self.create_inode( + file, + updated_stat.st_ino, + updated_stat.st_dev, + mnt_id, + path, + parent_data.layer_idx, + ); + + // Create the entry for the newly created symlink + let entry = self.create_entry(inode, updated_stat); + + Ok(entry) } fn do_readlink(&self, inode: Inode) -> io::Result> { // Get the path for this inode let inode_data = self.get_inode_data(inode)?; - // Allocate a buffer for the link target + // First check if this is a file-backed symlink by reading override xattr + let (stat, _) = Self::unpatched_statx(inode_data.file.as_raw_fd(), None)?; + + // Check if we have override xattr + if let Ok(Some((_, _, mode, _))) = self.get_override_xattr(inode_data.file.as_raw_fd(), &stat) { + // Check if the override mode indicates this is a symlink + if (mode & libc::S_IFMT) == libc::S_IFLNK { + // This is a file-backed symlink, read the file content + let mut buf = vec![0; libc::PATH_MAX as usize]; + + // Since the file is opened with O_PATH, we need to open it through /proc/self/fd + let proc_path = format!("{}\0", inode_data.file.as_raw_fd()); + let fd = unsafe { + libc::openat( + self.proc_self_fd.as_raw_fd(), + proc_path.as_ptr() as *const libc::c_char, + libc::O_RDONLY | libc::O_CLOEXEC, + ) + }; + + if fd < 0 { + return Err(io::Error::last_os_error()); + } + + // Read the link target from file content + let res = unsafe { + libc::read( + fd, + buf.as_mut_ptr() as *mut libc::c_void, + buf.len(), + ) + }; + + unsafe { libc::close(fd) }; + + if res < 0 { + return Err(io::Error::last_os_error()); + } + + buf.resize(res as usize, 0); + return Ok(buf); + } + } + + // Fall back to regular readlinkat for real symlinks let mut buf = vec![0; libc::PATH_MAX as usize]; // Safe because this will only modify the contents of `buf` and we check the return value. diff --git a/src/devices/src/virtio/fs/linux/passthrough.rs b/src/devices/src/virtio/fs/linux/passthrough.rs index 772033909..697538dcb 100644 --- a/src/devices/src/virtio/fs/linux/passthrough.rs +++ b/src/devices/src/virtio/fs/linux/passthrough.rs @@ -920,12 +920,10 @@ impl PassthroughFs { mode: Option, rdev: Option, ) -> io::Result<()> { - // Check file type and skip xattr operations for types that don't support them + // Step 1: Check if this is a real symlink - we can't set xattr on those let file_type = st.st_mode & libc::S_IFMT; - let is_symlink = file_type == libc::S_IFLNK; - - // Handle symlinks specially - if is_symlink { + if file_type == libc::S_IFLNK { + // Real symlink - Linux doesn't support xattr on symlinks // WARNING: While Linux allows changing symlink ownership via lchown(), we cannot // virtualize this because most filesystems don't support extended attributes on // symlinks. Symlink ownership matters for security: @@ -942,14 +940,25 @@ impl PassthroughFs { return Ok(()); } - // Get the current values to use as defaults + // Step 2: For all other files (including file-backed symlinks), get the current values let (uid, gid) = if let Some((uid, gid)) = owner { (uid, gid) } else { (st.st_uid, st.st_gid) }; - let mode = mode.unwrap_or(st.st_mode); + // Step 3: Determine the mode to use + let mode = if let Some(m) = mode { + m // Use provided mode + } else { + // No mode provided - preserve existing virtualized mode if it exists + // This is crucial for file-backed symlinks to preserve their S_IFLNK type + if let Ok(Some((_, _, existing_mode, _))) = self.get_override_xattr(f, st) { + existing_mode // Preserve virtualized mode + } else { + st.st_mode // Fall back to actual file mode + } + }; // Format the xattr value - include full mode with file type bits for special files // that we store as regular files (FIFOs, sockets, device nodes) @@ -1867,7 +1876,7 @@ impl FileSystem for PassthroughFs { fn symlink( &self, - _ctx: Context, + ctx: Context, linkname: &CStr, parent: Inode, name: &CStr, @@ -1888,18 +1897,60 @@ impl FileSystem for PassthroughFs { .cloned() .ok_or_else(ebadf)?; - // Safe because this doesn't modify any memory and we check the return value. - let res = - unsafe { libc::symlinkat(linkname.as_ptr(), data.file.as_raw_fd(), name.as_ptr()) }; - if res == 0 { - // NOTE: We cannot set ownership xattr on symlinks because most filesystems - // don't support extended attributes on symlinks. This means symlink ownership - // will always reflect the host values, not virtualized container values. - // Skip set_override_xattr_before_lookup for symlinks. - self.do_lookup(parent, name) - } else { - Err(io::Error::last_os_error()) + // NOTE: We create symlinks as regular files on Linux to support setting + // xattr on them. Most filesystems don't support extended attributes on symlinks. + // The link target is stored as file content and the actual file type (S_IFLNK) + // is stored in the override xattr. + + // First check link target length + let linkname_bytes = linkname.to_bytes(); + if linkname_bytes.len() > libc::PATH_MAX as usize { + return Err(io::Error::from_raw_os_error(libc::ENAMETOOLONG)); + } + + // Create a regular file to back the symlink + let fd = unsafe { + libc::openat( + data.file.as_raw_fd(), + name.as_ptr(), + libc::O_CREAT | libc::O_EXCL | libc::O_WRONLY | libc::O_CLOEXEC, + 0o600, + ) + }; + + if fd < 0 { + return Err(io::Error::last_os_error()); } + + // Write the link target as file content + let res = unsafe { + libc::write( + fd, + linkname_bytes.as_ptr() as *const libc::c_void, + linkname_bytes.len(), + ) + }; + + if res < 0 || res as usize != linkname_bytes.len() { + unsafe { libc::close(fd) }; + // Clean up the file on error + unsafe { libc::unlinkat(data.file.as_raw_fd(), name.as_ptr(), 0) }; + return Err(io::Error::last_os_error()); + } + + // Close the fd + unsafe { libc::close(fd) }; + + // Set override xattr with S_IFLNK mode + self.set_override_xattr_before_lookup( + &data.file, + name, + &ctx, + Some(libc::S_IFLNK | 0o777), + None, + )?; + + self.do_lookup(parent, name) } fn readlink(&self, _ctx: Context, inode: Inode) -> io::Result> { @@ -1911,6 +1962,51 @@ impl FileSystem for PassthroughFs { .cloned() .ok_or_else(ebadf)?; + // First check if this is a file-backed symlink by reading override xattr + let (stat, _) = unpatched_statx(&data.file)?; + + // Check if we have override xattr + if let Ok(Some((_, _, mode, _))) = self.get_override_xattr(&data.file, &stat) { + // Check if the override mode indicates this is a symlink + if (mode & libc::S_IFMT) == libc::S_IFLNK { + // This is a file-backed symlink, read the file content + let mut buf = vec![0; libc::PATH_MAX as usize]; + + // Since the file is opened with O_PATH, we need to open it through /proc/self/fd + let proc_path = format!("{}\0", data.file.as_raw_fd()); + let fd = unsafe { + libc::openat( + self.proc_self_fd.as_raw_fd(), + proc_path.as_ptr() as *const libc::c_char, + libc::O_RDONLY | libc::O_CLOEXEC, + ) + }; + + if fd < 0 { + return Err(io::Error::last_os_error()); + } + + // Read the link target from file content + let res = unsafe { + libc::read( + fd, + buf.as_mut_ptr() as *mut libc::c_void, + buf.len(), + ) + }; + + unsafe { libc::close(fd) }; + + if res < 0 { + return Err(io::Error::last_os_error()); + } + + buf.resize(res as usize, 0); + return Ok(buf); + } + } + + // Fall back to regular readlinkat for real symlinks let mut buf = vec![0; libc::PATH_MAX as usize]; // Safe because this is a constant value and a valid C string. diff --git a/src/devices/src/virtio/fs/tests/overlayfs/create.rs b/src/devices/src/virtio/fs/tests/overlayfs/create.rs index 7a91067c1..46e10a04e 100644 --- a/src/devices/src/virtio/fs/tests/overlayfs/create.rs +++ b/src/devices/src/virtio/fs/tests/overlayfs/create.rs @@ -610,8 +610,15 @@ fn test_mkdir_multiple_layers() -> io::Result<()> { #[test] fn test_symlink_basic() -> io::Result<()> { - // Create test layers: - // Single layer with a file + // Test basic symlink creation in overlayfs + // This test verifies: + // 1. Creating a symlink through the filesystem API + // 2. The symlink has correct mode and permissions + // 3. The symlink can be looked up correctly + // 4. The physical representation on disk matches the platform behavior + // 5. The symlink target can be read correctly through the filesystem API + + // Create test layers with a single file that will be the symlink target let layers = vec![vec![("target_file", false, 0o644)]]; let (fs, temp_dirs) = helper::create_overlayfs(layers)?; @@ -620,46 +627,102 @@ fn test_symlink_basic() -> io::Result<()> { // Initialize filesystem fs.init(FsOptions::empty())?; - // Create a new symlink + // Create a new symlink pointing to target_file let link_name = CString::new("link").unwrap(); let target_name = CString::new("target_file").unwrap(); let ctx = Context::default(); let entry = fs.symlink(ctx, &target_name, 1, &link_name, Extensions::default())?; - // Verify the symlink was created with correct mode - assert_eq!(entry.attr.st_mode & libc::S_IFMT, libc::S_IFLNK); - assert_eq!(entry.attr.st_mode & 0o777, 0o777); // Symlinks are typically 0777 + // Verify the symlink was created with correct mode through the filesystem API + assert_eq!(entry.attr.st_mode & libc::S_IFMT, libc::S_IFLNK, + "Created entry should have S_IFLNK file type"); + assert_eq!(entry.attr.st_mode & 0o777, 0o777, + "Symlinks should have 0777 permissions"); - // Verify we can look it up + // Verify we can look it up and it still appears as a symlink let lookup_entry = fs.lookup(ctx, 1, &link_name)?; - assert_eq!(lookup_entry.attr.st_mode & libc::S_IFMT, libc::S_IFLNK); + assert_eq!(lookup_entry.attr.st_mode & libc::S_IFMT, libc::S_IFLNK, + "Looked up entry should have S_IFLNK file type"); + assert_eq!(lookup_entry.inode, entry.inode, + "Lookup should return same inode as creation"); - // Verify the symlink exists on disk in the top layer + // Verify the physical representation on disk let link_path = temp_dirs.last().unwrap().path().join("link"); - assert!(link_path.exists()); - assert!(link_path.is_symlink()); + assert!(link_path.exists(), "Symlink should exist on disk"); + + // Platform-specific verification of physical representation + #[cfg(target_os = "linux")] + { + // On Linux, symlinks are implemented as regular files with xattr metadata + // This is done to support extended attributes on filesystems that don't + // support xattrs on symlinks + let metadata = fs::metadata(&link_path)?; + assert!(metadata.is_file(), + "On Linux, symlinks should be represented as regular files"); + + // Verify the override xattr is set correctly + let xattr_value = helper::get_xattr(&link_path, "user.containers.override_stat")?; + assert!(xattr_value.is_some(), + "File-backed symlink should have override_stat xattr"); + + if let Some(xattr_str) = xattr_value { + let parts: Vec<&str> = xattr_str.split(':').collect(); + assert!(parts.len() >= 3, "xattr should have at least uid:gid:mode"); + + // Verify the mode in xattr indicates this is a symlink + let mode = u32::from_str_radix(parts[2], 8).expect("mode should be valid octal"); + assert_eq!(mode & libc::S_IFMT, libc::S_IFLNK, + "xattr mode should indicate S_IFLNK file type"); + } + + // Verify the file content contains the link target + let file_content = fs::read(&link_path)?; + assert_eq!(file_content, target_name.to_bytes(), + "File content should contain the symlink target"); + } + + #[cfg(target_os = "macos")] + { + // On macOS, symlinks are regular symlinks + let metadata = fs::symlink_metadata(&link_path)?; + assert!(metadata.file_type().is_symlink(), + "On macOS, symlinks should be regular symlinks"); + + // Verify the symlink target through filesystem + let target = fs::read_link(&link_path)?; + assert_eq!(target.to_str().unwrap(), "target_file", + "Symlink should point to correct target"); + } - // Verify the symlink points to the correct target + // Verify the symlink target can be read through the filesystem API let target = fs.readlink(ctx, lookup_entry.inode)?; - assert_eq!(target, target_name.to_bytes()); + assert_eq!(target, target_name.to_bytes(), + "readlink should return the correct target"); + + // Additional verification: ensure the symlink behaves correctly + // Try to lookup the target through the symlink (should fail since we're not following) + match fs.lookup(ctx, lookup_entry.inode, &CString::new("anything").unwrap()) { + Err(e) => assert_eq!(e.raw_os_error(), Some(libc::ENOTDIR), + "Looking up through a symlink should fail with ENOTDIR"), + Ok(_) => panic!("Lookup through symlink should fail"), + } Ok(()) } #[test] fn test_symlink_nested() -> io::Result<()> { + // Test symlink creation in nested directory structures across multiple layers + // This test verifies: + // 1. Creating symlinks in directories from different layers + // 2. Copy-up behavior when creating symlinks in lower layer directories + // 3. Symlinks work correctly in nested directory structures + // 4. Each symlink can be read correctly regardless of which layer its parent came from + // Create test layers with complex structure: - // Layer 0 (bottom): - // - dir1/ - // - dir1/file1 - // - dir1/subdir/ - // - dir1/subdir/bottom_file - // Layer 1 (middle): - // - dir2/ - // - dir2/file2 - // Layer 2 (top): - // - dir3/ - // - dir3/top_file + // Layer 0 (bottom): dir1 with files and subdirectories + // Layer 1 (middle): dir2 with a file + // Layer 2 (top): dir3 with a file let layers = vec![ vec![ ("dir1", true, 0o755), @@ -679,9 +742,13 @@ fn test_symlink_nested() -> io::Result<()> { let ctx = Context::default(); - // Test 1: Create symlink in dir1 (should trigger copy-up) + // Test 1: Create symlink in dir1 (from bottom layer - should trigger copy-up) let dir1_name = CString::new("dir1").unwrap(); let dir1_entry = fs.lookup(ctx, 1, &dir1_name)?; + + // Verify dir1 is from bottom layer initially + assert_eq!(dir1_entry.attr.st_mode & libc::S_IFMT, libc::S_IFDIR); + let link_name = CString::new("link_to_file1").unwrap(); let target_name = CString::new("file1").unwrap(); let link_entry = fs.symlink( @@ -693,7 +760,11 @@ fn test_symlink_nested() -> io::Result<()> { )?; assert_eq!(link_entry.attr.st_mode & libc::S_IFMT, libc::S_IFLNK); - // Test 2: Create symlink in dir2 (middle layer, should trigger copy-up) + // Verify dir1 was copied up to top layer + assert!(temp_dirs.last().unwrap().path().join("dir1").exists(), + "dir1 should be copied up to top layer"); + + // Test 2: Create symlink in dir2 (middle layer - should trigger copy-up) let dir2_name = CString::new("dir2").unwrap(); let dir2_entry = fs.lookup(ctx, 1, &dir2_name)?; let middle_link_name = CString::new("link_to_file2").unwrap(); @@ -707,7 +778,11 @@ fn test_symlink_nested() -> io::Result<()> { )?; assert_eq!(middle_link_entry.attr.st_mode & libc::S_IFMT, libc::S_IFLNK); - // Test 3: Create symlink in dir3 (top layer, no copy-up needed) + // Verify dir2 was copied up to top layer + assert!(temp_dirs.last().unwrap().path().join("dir2").exists(), + "dir2 should be copied up to top layer"); + + // Test 3: Create symlink in dir3 (already in top layer - no copy-up needed) let dir3_name = CString::new("dir3").unwrap(); let dir3_entry = fs.lookup(ctx, 1, &dir3_name)?; let top_link_name = CString::new("link_to_top_file").unwrap(); @@ -721,21 +796,83 @@ fn test_symlink_nested() -> io::Result<()> { )?; assert_eq!(top_link_entry.attr.st_mode & libc::S_IFMT, libc::S_IFLNK); - // Verify all symlinks exist in appropriate layers + // Verify all symlinks exist in the top layer (due to copy-up) let top_layer = temp_dirs.last().unwrap().path(); - assert!(fs::symlink_metadata(top_layer.join("dir1/link_to_file1")).is_ok()); - assert!(fs::symlink_metadata(top_layer.join("dir2/link_to_file2")).is_ok()); - assert!(fs::symlink_metadata(top_layer.join("dir3/link_to_top_file")).is_ok()); + + #[cfg(target_os = "linux")] + { + // On Linux, verify file-backed symlinks + for (dir, link) in &[("dir1", "link_to_file1"), ("dir2", "link_to_file2"), ("dir3", "link_to_top_file")] { + let link_path = top_layer.join(dir).join(link); + assert!(link_path.exists(), "{}/{} should exist", dir, link); + + let metadata = fs::metadata(&link_path)?; + assert!(metadata.is_file(), + "{}/{} should be a regular file on Linux", dir, link); + + // Verify xattr + let xattr = helper::get_xattr(&link_path, "user.containers.override_stat")?; + assert!(xattr.is_some(), "{}/{} should have override_stat xattr", dir, link); + } + } + + #[cfg(target_os = "macos")] + { + // On macOS, verify regular symlinks + for (dir, link) in &[("dir1", "link_to_file1"), ("dir2", "link_to_file2"), ("dir3", "link_to_top_file")] { + let link_path = top_layer.join(dir).join(link); + assert!(link_path.exists(), "{}/{} should exist", dir, link); + + let metadata = fs::symlink_metadata(&link_path)?; + assert!(metadata.file_type().is_symlink(), + "{}/{} should be a symlink on macOS", dir, link); + } + } - // Verify symlink targets + // Verify symlink targets through filesystem API let link1_target = fs.readlink(ctx, link_entry.inode)?; - assert_eq!(link1_target, target_name.to_bytes()); + assert_eq!(link1_target, target_name.to_bytes(), + "First symlink should point to file1"); let link2_target = fs.readlink(ctx, middle_link_entry.inode)?; - assert_eq!(link2_target, middle_target.to_bytes()); + assert_eq!(link2_target, middle_target.to_bytes(), + "Second symlink should point to file2"); let link3_target = fs.readlink(ctx, top_link_entry.inode)?; - assert_eq!(link3_target, top_target.to_bytes()); + assert_eq!(link3_target, top_target.to_bytes(), + "Third symlink should point to top_file"); + + // Additional test: Create symlink with absolute path + let abs_link_name = CString::new("abs_link").unwrap(); + let abs_target = CString::new("/absolute/path/to/target").unwrap(); + let abs_link_entry = fs.symlink( + ctx, + &abs_target, + dir1_entry.inode, + &abs_link_name, + Extensions::default(), + )?; + + let abs_target_read = fs.readlink(ctx, abs_link_entry.inode)?; + assert_eq!(abs_target_read, abs_target.to_bytes(), + "Absolute path symlinks should be preserved"); + + // Test symlink in subdirectory + let subdir_name = CString::new("subdir").unwrap(); + let subdir_entry = fs.lookup(ctx, dir1_entry.inode, &subdir_name)?; + let subdir_link_name = CString::new("link_to_bottom").unwrap(); + let subdir_target = CString::new("bottom_file").unwrap(); + let subdir_link_entry = fs.symlink( + ctx, + &subdir_target, + subdir_entry.inode, + &subdir_link_name, + Extensions::default(), + )?; + + let subdir_target_read = fs.readlink(ctx, subdir_link_entry.inode)?; + assert_eq!(subdir_target_read, subdir_target.to_bytes(), + "Symlinks in subdirectories should work correctly"); Ok(()) } @@ -769,20 +906,33 @@ fn test_symlink_existing_name() -> io::Result<()> { #[test] fn test_symlink_multiple_layers() -> io::Result<()> { - // Create test layers: - // Layer 0 (bottom): base files - // Layer 1 (middle): some files - // Layer 2 (top): more files + // Test creating symlinks that point to targets in different layers + // This test verifies: + // 1. Symlinks can point to targets in any layer + // 2. Symlinks are always created in the top layer + // 3. Both relative and cross-directory symlinks work correctly + // 4. The physical representation matches platform expectations + + // Create test layers with directories and files in each layer let layers = vec![ vec![ ("bottom_dir", true, 0o755), ("bottom_dir/target1", false, 0o644), + ("shared_dir", true, 0o755), + ("shared_dir/bottom_file", false, 0o644), ], vec![ ("middle_dir", true, 0o755), ("middle_dir/target2", false, 0o644), + ("shared_dir", true, 0o755), + ("shared_dir/middle_file", false, 0o644), + ], + vec![ + ("top_dir", true, 0o755), + ("top_dir/target3", false, 0o644), + ("shared_dir", true, 0o755), + ("shared_dir/top_file", false, 0o644), ], - vec![("top_dir", true, 0o755), ("top_dir/target3", false, 0o644)], ]; let (fs, temp_dirs) = helper::create_overlayfs(layers)?; @@ -793,29 +943,114 @@ fn test_symlink_multiple_layers() -> io::Result<()> { let ctx = Context::default(); - // Create symlinks to files in different layers + // Test 1: Create symlinks to files in different layers let test_cases = vec![ - ("link_to_bottom", "bottom_dir/target1"), - ("link_to_middle", "middle_dir/target2"), - ("link_to_top", "top_dir/target3"), + ("link_to_bottom", "bottom_dir/target1", "Points to file in bottom layer"), + ("link_to_middle", "middle_dir/target2", "Points to file in middle layer"), + ("link_to_top", "top_dir/target3", "Points to file in top layer"), + ("link_relative", "../bottom_dir/target1", "Relative path symlink"), + ("link_dot_relative", "./top_dir/target3", "Dot-relative path symlink"), ]; - for (link, target) in test_cases.clone() { - let link_name = CString::new(link).unwrap(); - let target_name = CString::new(target).unwrap(); + let mut created_entries = Vec::new(); + + for (link, target, description) in &test_cases { + let link_name = CString::new(*link).unwrap(); + let target_name = CString::new(*target).unwrap(); let entry = fs.symlink(ctx, &target_name, 1, &link_name, Extensions::default())?; - assert_eq!(entry.attr.st_mode & libc::S_IFMT, libc::S_IFLNK); - - // Verify symlink target - let target_bytes = fs.readlink(ctx, entry.inode)?; - assert_eq!(target_bytes, target_name.to_bytes()); + assert_eq!(entry.attr.st_mode & libc::S_IFMT, libc::S_IFLNK, + "{}: Should create symlink", description); + + created_entries.push((entry.inode, target_name.clone(), description)); } // Verify all symlinks exist in the top layer let top_layer = temp_dirs.last().unwrap().path(); - for (link, _) in test_cases { - assert!(fs::symlink_metadata(top_layer.join(link)).is_ok()); + + #[cfg(target_os = "linux")] + { + for (link, _, description) in &test_cases { + let link_path = top_layer.join(link); + assert!(link_path.exists(), + "{}: Symlink should exist in top layer", description); + + // Verify it's a file-backed symlink + let metadata = fs::metadata(&link_path)?; + assert!(metadata.is_file(), + "{}: Should be a regular file on Linux", description); + + // Verify xattr exists + let xattr = helper::get_xattr(&link_path, "user.containers.override_stat")?; + assert!(xattr.is_some(), + "{}: Should have override_stat xattr", description); + + // Read file content to verify target + let content = fs::read(&link_path)?; + let (_, target, _) = test_cases.iter() + .find(|(l, _, _)| l == link) + .unwrap(); + assert_eq!(content, target.as_bytes(), + "{}: File content should match symlink target", description); + } + } + + #[cfg(target_os = "macos")] + { + for (link, target, description) in &test_cases { + let link_path = top_layer.join(link); + assert!(link_path.exists(), + "{}: Symlink should exist in top layer", description); + + // Verify it's a regular symlink + let metadata = fs::symlink_metadata(&link_path)?; + assert!(metadata.file_type().is_symlink(), + "{}: Should be a symlink on macOS", description); + + // Verify target through filesystem + let fs_target = fs::read_link(&link_path)?; + assert_eq!(fs_target.to_str().unwrap(), *target, + "{}: Filesystem symlink should point to correct target", description); + } + } + + // Verify symlink targets through the VFS API + for (inode, expected_target, description) in created_entries { + let target_bytes = fs.readlink(ctx, inode)?; + assert_eq!(target_bytes, expected_target.to_bytes(), + "{}: readlink should return correct target", description); + } + + // Test 2: Create symlink in shared_dir (which exists in all layers) + let shared_dir_name = CString::new("shared_dir").unwrap(); + let shared_dir_entry = fs.lookup(ctx, 1, &shared_dir_name)?; + + let shared_link_name = CString::new("shared_link").unwrap(); + let shared_target = CString::new("bottom_file").unwrap(); + let shared_link_entry = fs.symlink( + ctx, + &shared_target, + shared_dir_entry.inode, + &shared_link_name, + Extensions::default(), + )?; + + // Verify the symlink was created in the top layer's shared_dir + let shared_link_path = top_layer.join("shared_dir/shared_link"); + assert!(shared_link_path.exists(), + "Symlink in shared directory should exist in top layer"); + + let shared_target_read = fs.readlink(ctx, shared_link_entry.inode)?; + assert_eq!(shared_target_read, shared_target.to_bytes(), + "Symlink in shared directory should have correct target"); + + // Test 3: Verify that symlinks don't affect the visibility of their targets + // The targets should still be accessible from their original locations + for dir in ["bottom_dir", "middle_dir", "top_dir"] { + let dir_cstr = CString::new(dir).unwrap(); + let dir_entry = fs.lookup(ctx, 1, &dir_cstr)?; + assert_eq!(dir_entry.attr.st_mode & libc::S_IFMT, libc::S_IFDIR, + "{} should still be accessible as a directory", dir); } Ok(()) diff --git a/src/devices/src/virtio/fs/tests/overlayfs/metadata.rs b/src/devices/src/virtio/fs/tests/overlayfs/metadata.rs index dde706720..536a103cc 100644 --- a/src/devices/src/virtio/fs/tests/overlayfs/metadata.rs +++ b/src/devices/src/virtio/fs/tests/overlayfs/metadata.rs @@ -1164,3 +1164,68 @@ fn test_special_files_metadata() -> io::Result<()> { Ok(()) } + +#[test] +fn test_setattr_symlink() -> io::Result<()> { + // Create test layers with a target file + let layers = vec![vec![("target", false, 0o644)]]; + + let (fs, temp_dirs) = helper::create_overlayfs(layers)?; + helper::debug_print_layers(&temp_dirs, false)?; + + // Initialize filesystem + fs.init(FsOptions::empty())?; + + // Create a symlink + let link_name = CString::new("link").unwrap(); + let target_name = CString::new("target").unwrap(); + let ctx = Context { uid: 1000, gid: 1000, pid: 1234 }; + + let link_entry = fs.symlink(ctx, &target_name, 1, &link_name, Extensions::default())?; + + // Verify symlink was created + assert_eq!(link_entry.attr.st_mode & libc::S_IFMT, libc::S_IFLNK); + + // Try to change ownership on symlink + let mut attr = link_entry.attr; + attr.st_uid = 4000; + attr.st_gid = 4000; + let valid = SetattrValid::UID | SetattrValid::GID; + + // On Linux, this should fail with EOPNOTSUPP because we can't virtualize symlink ownership + #[cfg(target_os = "linux")] + { + let result = fs.setattr(ctx, link_entry.inode, attr, None, valid); + assert!(result.is_err()); + assert_eq!(result.unwrap_err().raw_os_error(), Some(libc::EOPNOTSUPP)); + println!("Linux correctly rejects symlink ownership changes"); + } + + // On macOS, this should succeed because macOS supports xattr on symlinks + #[cfg(target_os = "macos")] + { + let (new_attr, _) = fs.setattr(ctx, link_entry.inode, attr, None, valid)?; + + // Verify the virtualized uid/gid + assert_eq!(new_attr.st_uid, 4000); + assert_eq!(new_attr.st_gid, 4000); + + // Verify xattr was set on the symlink itself + let link_path = temp_dirs.last().unwrap().path().join("link"); + let xattr_value = helper::get_xattr(&link_path, "user.containers.override_stat")?; + assert!(xattr_value.is_some(), "macOS should support xattr on symlinks"); + + let xattr_str = xattr_value.unwrap(); + let parts: Vec<&str> = xattr_str.split(':').collect(); + assert_eq!(parts.len(), 3); + assert_eq!(parts[0], "4000"); // uid + assert_eq!(parts[1], "4000"); // gid + // Verify the mode includes symlink file type + let mode = u32::from_str_radix(parts[2], 8).unwrap(); + assert_eq!(mode & mode_cast!(libc::S_IFMT), mode_cast!(libc::S_IFLNK)); // Should be a symlink + + println!("macOS successfully virtualizes symlink ownership"); + } + + Ok(()) +} diff --git a/src/devices/src/virtio/fs/tests/passthrough/create.rs b/src/devices/src/virtio/fs/tests/passthrough/create.rs index e069484b0..53b22aad4 100644 --- a/src/devices/src/virtio/fs/tests/passthrough/create.rs +++ b/src/devices/src/virtio/fs/tests/passthrough/create.rs @@ -265,6 +265,14 @@ fn test_mknod_basic() -> io::Result<()> { #[test] fn test_symlink_basic() -> io::Result<()> { + // Test basic symlink creation in passthrough filesystem + // This test verifies: + // 1. Creating a symlink through the filesystem API + // 2. The symlink has correct mode (S_IFLNK) + // 3. The physical representation on disk matches platform behavior + // 4. The symlink can be read correctly through the VFS API + // 5. Extended attributes are properly set based on context + // Create test directory with a target file let files = vec![("target_file", false, 0o644)]; @@ -274,7 +282,7 @@ fn test_symlink_basic() -> io::Result<()> { // Initialize filesystem fs.init(FsOptions::empty())?; - // Create a symlink + // Create a symlink with specific context let link_name = CString::new("symlink").unwrap(); let target_name = CString::new("target_file").unwrap(); let ctx = Context { @@ -284,27 +292,112 @@ fn test_symlink_basic() -> io::Result<()> { }; let entry = fs.symlink(ctx, &target_name, 1, &link_name, Extensions::default())?; - // Verify the symlink was created - assert_eq!(entry.attr.st_mode & libc::S_IFMT, libc::S_IFLNK); + // Verify the symlink was created with correct attributes through VFS + assert_eq!(entry.attr.st_mode & libc::S_IFMT, libc::S_IFLNK, + "Created entry should have S_IFLNK file type"); + assert_eq!(entry.attr.st_uid, 1000, "Should have context uid"); + assert_eq!(entry.attr.st_gid, 1000, "Should have context gid"); + assert_eq!(entry.attr.st_mode & 0o777, 0o777, + "Symlinks should have 0777 permissions"); // Verify the symlink exists on disk let link_path = temp_dir.path().join("symlink"); - assert!(link_path.exists()); - let metadata = fs::symlink_metadata(&link_path)?; - assert!(metadata.file_type().is_symlink()); - - // Verify the symlink points to the correct target - let target = fs::read_link(&link_path)?; - assert_eq!(target.to_str().unwrap(), "target_file"); - - // Check if xattr was set (some filesystems don't support xattrs on symlinks) - let xattr_value = helper::get_xattr(&link_path, "user.containers.override_stat")?; - if xattr_value.is_some() { - let xattr_str = xattr_value.unwrap(); - let parts: Vec<&str> = xattr_str.split(':').collect(); - assert_eq!(parts[0], "1000"); // Context default uid - assert_eq!(parts[1], "1000"); // Context default gid + assert!(link_path.exists(), "Symlink should exist on disk"); + + // Platform-specific verification + #[cfg(target_os = "linux")] + { + // On Linux, passthrough creates file-backed symlinks to support xattrs + let metadata = fs::metadata(&link_path)?; + assert!(metadata.is_file(), + "On Linux passthrough, symlinks should be file-backed"); + + // Verify the override xattr is set correctly + let xattr_value = helper::get_xattr(&link_path, "user.containers.override_stat")?; + assert!(xattr_value.is_some(), + "File-backed symlink should have override_stat xattr"); + + if let Some(xattr_str) = xattr_value { + let parts: Vec<&str> = xattr_str.split(':').collect(); + assert!(parts.len() >= 3, "xattr should have at least uid:gid:mode"); + assert_eq!(parts[0], "1000", "xattr should store context uid"); + assert_eq!(parts[1], "1000", "xattr should store context gid"); + + // Verify the mode in xattr indicates this is a symlink + let mode = u32::from_str_radix(parts[2], 8).expect("mode should be valid octal"); + assert_eq!(mode & libc::S_IFMT, libc::S_IFLNK, + "xattr mode should indicate S_IFLNK file type"); + assert_eq!(mode & 0o777, 0o777, + "xattr should preserve symlink permissions"); + } + + // Verify the file content contains the link target + let file_content = fs::read(&link_path)?; + assert_eq!(file_content, target_name.to_bytes(), + "File content should contain the symlink target"); + } + + #[cfg(target_os = "macos")] + { + // On macOS, verify it's a regular symlink + let metadata = fs::symlink_metadata(&link_path)?; + assert!(metadata.file_type().is_symlink(), + "On macOS, should be a regular symlink"); + + // Verify the symlink points to the correct target + let target = fs::read_link(&link_path)?; + assert_eq!(target.to_str().unwrap(), "target_file", + "Symlink should point to correct target"); + + // Check if xattr was set (macOS supports xattrs on symlinks) + let xattr_value = helper::get_xattr(&link_path, "user.containers.override_stat")?; + if let Some(xattr_str) = xattr_value { + let parts: Vec<&str> = xattr_str.split(':').collect(); + assert!(parts.len() >= 3, "xattr should have at least uid:gid:mode"); + assert_eq!(parts[0], "1000", "xattr should store context uid"); + assert_eq!(parts[1], "1000", "xattr should store context gid"); + } + } + + // Test VFS operations on the symlink + + // 1. Verify we can look up the symlink + let lookup_entry = fs.lookup(ctx, 1, &link_name)?; + assert_eq!(lookup_entry.inode, entry.inode, + "Lookup should return same inode"); + assert_eq!(lookup_entry.attr.st_mode & libc::S_IFMT, libc::S_IFLNK, + "Looked up entry should be a symlink"); + + // 2. Verify readlink works correctly + let target_read = fs.readlink(ctx, entry.inode)?; + assert_eq!(target_read, target_name.to_bytes(), + "readlink should return correct target"); + + // 3. Verify that operations through the symlink fail appropriately + match fs.lookup(ctx, entry.inode, &CString::new("anything").unwrap()) { + Err(e) => assert_eq!(e.raw_os_error(), Some(libc::ENOTDIR), + "Lookup through symlink should fail with ENOTDIR"), + Ok(_) => panic!("Lookup through symlink should fail"), } + + // 4. Test creating another symlink with different permissions + let link2_name = CString::new("symlink2").unwrap(); + let abs_target = CString::new("/absolute/path").unwrap(); + let ctx2 = Context { + uid: 2000, + gid: 2000, + pid: 5678, + }; + let entry2 = fs.symlink(ctx2, &abs_target, 1, &link2_name, Extensions::default())?; + + // Verify the second symlink has different ownership + assert_eq!(entry2.attr.st_uid, 2000, "Should have second context uid"); + assert_eq!(entry2.attr.st_gid, 2000, "Should have second context gid"); + + // Verify absolute path is preserved + let target2_read = fs.readlink(ctx2, entry2.inode)?; + assert_eq!(target2_read, abs_target.to_bytes(), + "Absolute paths should be preserved in symlinks"); Ok(()) } From 1d942a87db3746fe2ccdb5aa1ce0202f5ca5c695 Mon Sep 17 00:00:00 2001 From: Stephen Akinyemi Date: Sun, 27 Jul 2025 22:10:15 +0100 Subject: [PATCH 2/2] fix(fs): fix macOS symlink tests in overlayfs - Use symlink_metadata() instead of exists() to check symlink existence since exists() follows symlinks and returns false for broken symlinks - Add XATTR_NOFOLLOW flag when reading xattrs from symlinks on macOS to read from the symlink itself rather than its target These changes ensure the tests properly validate symlink behavior on macOS where xattrs can be set directly on symlinks. --- src/devices/src/virtio/fs/tests/overlayfs/create.rs | 8 +++++--- src/devices/src/virtio/fs/tests/overlayfs/mod.rs | 6 +++++- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/src/devices/src/virtio/fs/tests/overlayfs/create.rs b/src/devices/src/virtio/fs/tests/overlayfs/create.rs index 46e10a04e..208c7356d 100644 --- a/src/devices/src/virtio/fs/tests/overlayfs/create.rs +++ b/src/devices/src/virtio/fs/tests/overlayfs/create.rs @@ -821,7 +821,7 @@ fn test_symlink_nested() -> io::Result<()> { // On macOS, verify regular symlinks for (dir, link) in &[("dir1", "link_to_file1"), ("dir2", "link_to_file2"), ("dir3", "link_to_top_file")] { let link_path = top_layer.join(dir).join(link); - assert!(link_path.exists(), "{}/{} should exist", dir, link); + assert!(link_path.symlink_metadata().is_ok(), "{}/{} should exist", dir, link); let metadata = fs::symlink_metadata(&link_path)?; assert!(metadata.file_type().is_symlink(), @@ -999,7 +999,9 @@ fn test_symlink_multiple_layers() -> io::Result<()> { { for (link, target, description) in &test_cases { let link_path = top_layer.join(link); - assert!(link_path.exists(), + // Use symlink_metadata to check if the symlink itself exists + // (not whether its target exists) + assert!(link_path.symlink_metadata().is_ok(), "{}: Symlink should exist in top layer", description); // Verify it's a regular symlink @@ -1037,7 +1039,7 @@ fn test_symlink_multiple_layers() -> io::Result<()> { // Verify the symlink was created in the top layer's shared_dir let shared_link_path = top_layer.join("shared_dir/shared_link"); - assert!(shared_link_path.exists(), + assert!(shared_link_path.symlink_metadata().is_ok(), "Symlink in shared directory should exist in top layer"); let shared_target_read = fs.readlink(ctx, shared_link_entry.inode)?; diff --git a/src/devices/src/virtio/fs/tests/overlayfs/mod.rs b/src/devices/src/virtio/fs/tests/overlayfs/mod.rs index c8fcffb8d..d563dc945 100644 --- a/src/devices/src/virtio/fs/tests/overlayfs/mod.rs +++ b/src/devices/src/virtio/fs/tests/overlayfs/mod.rs @@ -244,6 +244,10 @@ mod helper { let key_cstr = CString::new(key)?; let mut buf = vec![0u8; 256]; + + // Check if path is a symlink to determine if we need XATTR_NOFOLLOW + let metadata = fs::symlink_metadata(path)?; + let is_symlink = metadata.file_type().is_symlink(); #[cfg(target_os = "macos")] let res = unsafe { @@ -253,7 +257,7 @@ mod helper { buf.as_mut_ptr() as *mut libc::c_void, buf.len(), 0, - 0, + if is_symlink { libc::XATTR_NOFOLLOW } else { 0 }, ) };