[Aries-441]

Summary

Fixes #1991.

Use the existing O_PATH descriptor through /proc/self/fd in getxattr(),
avoiding an extra open/close that can undo O_TRUNC on s3fs.

Give each virtio-fs worker a separate filesystem context with
unshare(CLONE_FS) to prevent concurrent CWD changes from interfering
with proc-fd operations. Add regression tests.

Validation

• virtiofsd unit tests: 51 passed.
• cargo check and formatting checks passed with the pinned toolchain.
• S3 integration: 82/82 passed with the fixed shim.
• Original-shim control: 21 checks failed, confirming regression detection.

These checks were performed before rebasing onto the latest master;
tests have not been rerun after the rebase.

Follow-up

The same reopen pattern was independently reproduced in setxattr(),
listxattr(), and removexattr(). Those operations will be tracked and
fixed separately.

Assisted-by: Codex:GPT-6.1 Sol

[Aries-441]

While investigating this issue, I reproduced the same problem in setxattr(), listxattr(), and removexattr(): each performs an extra open/close that can undo O_TRUNC on s3fs, even when the xattr call returns EOPNOTSUPP.

The current PR focuses on getxattr(). I’ll open a separate issue to track the remaining three operations and follow up with a fix.

[cubesandboxbot]

Each virtio-fs queue thread now unshare(CLONE_FS)s unconditionally, and a failure is fatal: run() returns Err, spawn_virtio_thread signals exit_evt (VM shutdown) and only logs the generic Error running worker. Hosts/containers whose seccomp policy rejects unshare are a documented case here (see the startup probe and comment in hypervisor/virtiofsd/src/main.rs:157-166), so this converts an environment quirk into "virtio-fs cannot be used at all" — including configurations with xattr = false, where the proc-fd CWD dance is never executed (the set_working_directory paths are only reachable when cfg.xattr, cfg.security_label, or cfg.xattr_security_capability is set).

Consider gating the unshare on those settings and/or probing unshare(CLONE_FS) once in Fs::activate(), returning a descriptive ActivateError instead of failing asynchronously in every worker thread.

[cubesandboxbot]

Fixture::new assumes unshare(CLONE_FS) always succeeds, so all four tests panic (rather than skip) in sandboxes where that syscall is filtered — the exact Docker/Moby seccomp situation documented at hypervisor/virtiofsd/src/main.rs:157-166. These tests are about getxattr semantics, not sandbox policy; consider skipping with a clear message when unshare is unavailable so the suite stays usable in restricted CI environments.

[cubesandboxbot]

Automated review — PR #2002

🤖 This review was generated by an AI agent from the PR diff and the base-branch tree. It is not a human approval.

What the change does

• passthrough::getxattr no longer reopens regular files/directories with open_inode(O_RDONLY|O_NONBLOCK); it now always uses the stored O_PATH descriptor through /proc/self/fd (the route that was previously only used for non-is_safe_inode nodes). This avoids the extra open/close that can undo O_TRUNC on s3fs ([Bug Report] Overwriting a file on an S3 volume keeps its old size (virtiofsd getxattr undoes O_TRUNC) #1991).
• virtio-devices/src/fs.rs now calls unshare(CLONE_FS) at the start of every virtio-fs worker thread when xattr, security_label, or xattrmap is configured, so the fchdir-based proc-fd path has a per-thread CWD/umask instead of mutating the whole process.
• Adds virtiofsd/src/passthrough/tests.rs covering count/value replies, ERANGE, ENODATA, unlinked files, ENOSYS when xattr is disabled, and an inotify-based check that getxattr no longer opens the data file.

Verification I performed

• The diff applies cleanly to the current master; the hunk contexts in fs.rs and passthrough/mod.rs still match.
• SYS_unshare is already allowed by the Thread::VirtioFs seccomp rule set (hypervisor/virtio-devices/src/seccomp_filters.rs:374), so no filter change is needed.
• Every set_working_directory() / proc-fd call site is reachable only under one of the three flags in needs_cwd_isolation: getxattr/setxattr/listxattr/removexattr (gated by cfg.xattr), do_mknod_mkdir_symlink_secctx (gated by security_label), and clear_file_capabilities(..., o_path = true) (reachable when xattrmap remaps security.capability). The predicate therefore covers the reachable paths.
• data.get_file() returns the stored O_PATH fd (InodeStore/InodeData::get_file), and /proc/self/fd/N plus fchdir is the same mechanism already used for special files, so the new unconditional path is consistent with the existing code.

Findings

1. unshare(CLONE_FS) failure is fatal to the whole VM, and this applies by default. BackendFsConfig::xattr defaults to true, and a failing worker writes the VMM-wide exit_evt (thread_helper.rs:53-57). A host/container seccomp policy that rejects unshare without CAP_SYS_ADMIN (the exact case documented in virtiofsd/src/main.rs:157-166) therefore turns a previously working deployment into a VM that shuts down. See the inline comment on fs.rs:285 for the suggestion to probe once in activate() or fall back with a warning.
2. The production isolation mechanism has no test coverage. The new tests deliberately hold a process-wide mutex instead of unsharing, so nothing exercises concurrent workers (the race this PR fixes). See the inline comment on tests.rs:15.
3. Minor: posix_acl also mutates shared per-fs state (umask via ScopedUmask in do_create/mkdir/mknod) but is not part of needs_cwd_isolation. In practice posix_acl requires xattr (the xattr entry points return ENOSYS otherwise), and xattr is on by default, so this is likely moot — flagged only in case a posix_acl = true, xattr = false configuration is meant to be supported.
4. Minor: the tests assume user.* xattrs are supported on std::env::temp_dir(). That holds for ext4/tmpfs, but the unit-test container runs on overlayfs; if a CI/filesystem combination lacks user.* support the tests fail with EOPNOTSUPP rather than skipping. Not a blocker, just a portability note.
5. Informational: for handle-backed inodes (inode_file_handles != Never), data.get_file() still performs an open_by_handle_at(O_PATH); the "no open" property is guaranteed only for the default O_PATH-fd-backed mode, which is the only mode the new test exercises. Not relevant to s3fs (no file-handle support), but worth knowing if the guarantee is expected in handle mode too.

Notes

• The PR body states the validation was done before rebasing and not rerun afterwards. The code still applies cleanly, but rerunning the virtiofsd unit tests and the S3 integration lane on the rebased head would be prudent before merge.
• The follow-up acknowledged in the PR description (the same reopen pattern in setxattr/listxattr/removexattr) is accurate against the base tree; the s3fs symptom can still be reproduced through those operations until they are fixed.

Verdict

No correctness defect found in the new getxattr path, and the config gating for needs_cwd_isolation is sound. The main open question is the intentionally-fatal unshare failure semantics (finding 1); the rest are test/coverage/portability suggestions.

[cubesandboxbot]

This makes unshare(CLONE_FS) a hard requirement for the whole VM whenever xattr is enabled — and BackendFsConfig::xattr defaults to true (default_fsconfig_xattr), so this is the default configuration.

On failure run() returns Err, and spawn_virtio_thread reacts by writing to exit_evt (hypervisor/virtio-devices/src/thread_helper.rs:53-57), which is the VMM-wide shutdown eventfd. So a seccomp/LSM policy that rejects unshare no longer degrades to the previous racy-but-working behaviour; it tears down the entire VM. The standalone daemon guards against exactly this by probing unshare(CLONE_FS) once at startup and returning a dedicated UnshareCloneFs error, citing Docker/Moby seccomp profiles that reject it without CAP_SYS_ADMIN (hypervisor/virtiofsd/src/main.rs:157-166).

Would it be better to do the same one-shot probe in activate() (returning an ActivateError so the failure is a clean device-activation error), or to fall back to the previous reopen path with a warning when the syscall is unavailable? At minimum, the current failure semantics seem worth calling out in the PR description.

[cubesandboxbot]

The tests intentionally serialize with this mutex instead of calling unshare(CLONE_FS), so the production mechanism that actually makes the proc-fd route safe — per-worker unshare(CLONE_FS) in virtio-devices/src/fs.rs — has no coverage. Nothing here would catch a regression where two workers concurrently clobber each other's CWD (the exact bug this PR is fixing), and all the assertions run single-threaded.

Could you add one test that spawns two threads performing proc-fd xattr calls concurrently after the same unshare(CLONE_FS) setup the daemon uses (early-return if unshare is unavailable in the test container), and asserts both calls return the expected values? That would make the isolation claim testable rather than implicit.

[Aries-441]

Thanks. Confirmed: CI failed in the test fixture before the xattr assertions, while the same seccomp restriction could also make worker startup fail in production.

I've added an activation-time unshare probe that returns a clear ActivateError, gated it on xattr/security_label/xattrmap, and kept the xattr tests portable with serialized fixtures plus a concurrent CWD-isolation test.

I also verified the suite in a container profile that denies unshare. I'll push after tightening the worker-level skip handling.

[lisongqian]

LGTM. I have run the full hypervisor CI and all of these passed.