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..208c7356d 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.symlink_metadata().is_ok(), "{}/{} 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,116 @@ 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); + // 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 + 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.symlink_metadata().is_ok(), + "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/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 }, ) }; 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(()) }