Fix virtiofs bind mounts exposing files as root-owned (#40719)#40733
Open
halter73 wants to merge 2 commits into
Open
Fix virtiofs bind mounts exposing files as root-owned (#40719)#40733halter73 wants to merge 2 commits into
halter73 wants to merge 2 commits into
Conversation
Add the 'metadata' option to virtiofs shares created via HcsVirtualMachine::AddShare (the WSLC/Docker container path). Without this option, the virtiofs device host cannot persist per-file uid/gid in NTFS extended attributes, so all files default to uid=0/gid=0 regardless of the creating user. This matches the behavior of the regular distro mount path (WslCoreVm::AddVirtioFsShare) which receives metadata/uid/gid options from the Linux init's ConvertDrvfsMountOptionsToPlan9. Also adds a regression test (WindowsMountsVirtioFsFileOwnership) that verifies file ownership is preserved on virtiofs mounts. Fixes microsoft#40719 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Enable VirtioFS metadata option for Windows folder mounts so Linux file ownership (uid/gid) is persisted, and add a regression test validating ownership preservation.
Changes:
- Always include
metadatain VirtioFS mount options (including read-only mounts). - Add a Windows test that validates
chownpersistence and non-root file creation ownership on a VirtioFS-mounted Windows folder.
Show a summary per file
| File | Description |
|---|---|
| test/windows/WSLCTests.cpp | Adds a regression test validating VirtioFS ownership persistence for chown and non-root file creation. |
| src/windows/service/exe/HcsVirtualMachine.cpp | Ensures VirtioFS mount options include metadata so ownership is preserved. |
Copilot's findings
- Files reviewed: 2/2 changed files
- Comments generated: 3
- Add inline comment explaining why 'metadata' is required - Use unique mount point (/virtiofs-ownership-test) to avoid test interference - Use numeric UID (su '#65534') instead of username to avoid environment dependency Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Collaborator
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add the 'metadata' option to virtiofs shares created via HcsVirtualMachine::AddShare (the WSLC/Docker container path). Without this option, the virtiofs device host cannot persist per-file uid/gid in NTFS extended attributes, so all files default to uid=0/gid=0 regardless of the creating user.
This matches the behavior of the regular distro mount path (WslCoreVm::AddVirtioFsShare) which receives metadata/uid/gid options from the Linux init's ConvertDrvfsMountOptionsToPlan9.
Also adds a regression test (WindowsMountsVirtioFsFileOwnership) that verifies file ownership is preserved on virtiofs mounts.
Fixes #40719
Summary of the Pull Request
Fix virtiofs bind mounts exposing all files as root-owned when used via WSLC containers (e.g., Docker Desktop bind mounts). Add
metadatato the virtiofs share options inHcsVirtualMachine::AddShareso the device host persists per-file uid/gid in NTFS extended attributes.PR Checklist
virtiofs=true: file created by non-root user on Windows bind mount is exposed as root-owned and cannot be reopened for write #40719Detailed Description of the Pull Request / Additional comments
When
virtiofs=trueis set in.wslconfig, Docker bind mounts go throughHcsVirtualMachine::AddSharewhich creates a virtiofs device for the shared Windows folder. Previously, the options passed to the device were just"ro"(read-only) or""(read-write) — missing themetadataoption.Without
metadata, the virtiofs device host (wsldevicehost.dll) cannot store per-file ownership in NTFS extended attributes. All files appear asuid=0 gid=0regardless of which user created them, and non-root users cannot reopen files they just created.The regular distro drvfs mount path (
MountVirtioFs→WslCoreVm::AddVirtioFsShare) already receivesmetadata/uid/gidoptions from the Linux init'sConvertDrvfsMountOptionsToPlan9, which is why standard/mnt/cmounts work correctly with virtiofs. The WSLC container path was simply missing this option.Fix: Change line 583 of
HcsVirtualMachine.cpp:Validation Steps Performed
Reproduced the bug using the exact Docker repro steps from
virtiofs=true: file created by non-root user on Windows bind mount is exposed as root-owned and cannot be reopened for write #40719 on WSL 2.7.3.0 / Docker 29.5.2 / Windows 10.0.26200.8390. Confirmed files showuid=0 gid=0with virtiofs and correct ownership with 9p fallback.Verified the fix by deploying the patched
wslservice.exeand re-running the repro — files now retain the creating user's uid/gid.Added regression test
WSLC_TEST_METHOD(WindowsMountsVirtioFsFileOwnership)inWSLCTests.cppthat:chowns to uid 1000:100, then verifiesstatreports1000 100su nobodyto create a file as uid 65534, then verifies ownership is65534Confirmed test fails on current main (stat reports
0 0without the fix) and passes with the fix.Build verification: Both
wslservice.exeandwsltests.dllcompile successfully.