Skip to content

Fix: update drvfs gid uid after default uid is set in oobe - #41521

Open
Feng Wang (chemwolf6922) wants to merge 8 commits into
masterfrom
user/chemwolf6922/fix-update-drvfs-gid-uid-after-oobe
Open

Feng Wang (chemwolf6922) wants to merge 8 commits into
masterfrom
user/chemwolf6922/fix-update-drvfs-gid-uid-after-oobe

Conversation

@chemwolf6922

Copy link
Copy Markdown
Contributor

Summary of the Pull Request

For a newly installed distro, the drvfs are initially mount as 0:0. And is not updated after oobe sets the default uid. This could cause access issues before the distro is restarted.

This PR updates the auto mounted drvfs gid uid before completing the oobe. The non-auto mounted shares are not remounted to avoid breaking user set gid uid values.

PR Checklist

  • Closes: DrvFs without metadata returns  EPERM  from  fchmod() #41519
  • Communication: I've discussed this with core contributors already. If work hasn't been agreed, this work might be rejected
  • Tests: Added/updated if needed and all pass
  • Localization: All end user facing strings can be localized
  • Dev docs: Added/updated if needed
  • Documentation updated: If checked, please file a pull request on our docs repo and link it here: #xxx

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Update tests:
UnitTests::UnitTests::ModernOOBE

Copilot AI lite review requested due to automatic review settings September 4, 2026 07:10
@chemwolf6922
Feng Wang (chemwolf6922) requested a review from a team as a code owner September 4, 2026 07:10

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The added unit test uses a likely non-writable C:\ root path (risking flakiness) and the new refresh logic needs the proposed correctness tweaks applied before it’s safe to merge.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR addresses an OOBE-time ownership mismatch where DrvFs mounts initially come up as 0:0 and are not updated after OOBE sets the distro’s default UID, which can cause access/permission issues until restart.

Changes:

  • Refreshes (remounts) auto-mounted DrvFs volumes in both elevated and non-elevated mount namespaces after successful OOBE when oobe.defaultUid is present.
  • Tracks which DrvFs volumes were auto-mounted so only those are refreshed (avoids touching user-mounted shares).
  • Adds/extends the ModernOOBE unit test to validate DrvFs ownership refresh and chmod behavior post-OOBE.
File summaries
File Description
test/windows/UnitTests.cpp Adds a ModernOOBE validation to ensure DrvFs mount ownership updates after default UID is set.
src/linux/init/init.cpp Invokes DrvFs owner refresh after successful OOBE in utility VM mode.
src/linux/init/config.h Declares ConfigRefreshDrvFsOwner(...).
src/linux/init/config.cpp Tracks automounted DrvFs volumes and implements refresh logic by remounting only those volumes.
Review details

Suppressed comments (1)

src/linux/init/config.cpp:2606

  • This loop also iterates over all 32 bits and formats targets using 'a' + Index, which can produce invalid mountpoints for indices beyond Z if those bits are ever set. Limiting the loop to 26 drive letters avoids surprising behavior and matches Windows semantics.
    for (size_t Index = 0; Index < SourceAutomountedVolumes.size(); Index += 1)
    {
        const auto Target = std::format("{}{:c}", Config.DrvFsPrefix, 'a' + Index);
        if (SourceAutomountedVolumes[Index] && MountedVolumes.contains(std::make_pair(static_cast<unsigned int>(Index), Target)))
        {
            DestinationAutomountedVolumes.set(Index);
  • Files reviewed: 4/4 changed files
  • Comments generated: 3
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/linux/init/config.cpp Outdated
Comment thread src/linux/init/init.cpp
Comment thread test/windows/UnitTests.cpp Outdated
Copilot AI review requested due to automatic review settings September 4, 2026 08:06

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

It changes low-level mount namespace and DrvFs remount behavior during OOBE, which warrants final human validation beyond automated review.

Review details
  • Files reviewed: 4/4 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread test/windows/UnitTests.cpp
Copilot AI review requested due to automatic review settings September 4, 2026 09:23

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

It changes low-level mount namespace and remount behavior in init, which is correctness-critical and should receive final human review despite the added test coverage.

Review details
  • Files reviewed: 4/4 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One failure-path concern: the existing automount is detached before the replacement mount is attempted, and a failed ConfigMountDrvFsVolumes() leaves that drive untracked and unmounted while OOBE only logs the refresh failure and continues. Could a remount failure restore the previous mount/options, or keep the volume tracked for retry, so a transient error does not make the drive disappear until restart?

Copilot AI review requested due to automatic review settings September 7, 2026 04:25
@chemwolf6922

Copy link
Copy Markdown
Contributor Author

Hi Sylvester Kaczmarek (@sylvesterkaczmarek) .
Thanks for the feedback. The concern is valid. However, the rollback is basically the same operation as the remount. So that likely won't help if the remount fails. I made changes to the warning logic, so warnings collected during oobe gets presented to the user. Including any drvfs mount failures.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The new post-OOBE refresh path validates defaultUid >= 0, but negative oobe.defaultUid values are still accepted by config parsing and can later be used, potentially wrapping to an unintended UID unless explicitly treated as invalid.

Review details

Suppressed comments (1)

src/linux/init/init.cpp:665

  • defaultUid is parsed as a signed int (and config parsing accepts negative values), but if it’s negative we still proceed with the OOBE result flow and later ConfigureUid() uses it. This can wrap when converted to uint32_t inside ConfigureUid, producing an unintended UID. Consider explicitly treating negative oobe.defaultUid as invalid (marking it absent) before any further use.
        if ((OobeResult == 0) && (defaultUidPresent == ConfigKeyPresence::Present) && (defaultUid >= 0) && UtilIsUtilityVm())
        {
            for (const auto Admin : {false, true})
            {
                if (ConfigRefreshDrvFsOwner(defaultUid, Admin, Config) < 0)
  • Files reviewed: 4/4 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

@chemwolf6922

Copy link
Copy Markdown
Contributor Author

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

Copilot AI review requested due to automatic review settings September 7, 2026 08:05

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The remount logic is scoped to auto-mounted DrvFs volumes, is gated to successful OOBE default-UID updates in WSL2, and is covered by an updated unit test.

Review details
  • Files reviewed: 4/4 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

@sylvesterkaczmarek

Copy link
Copy Markdown

Thanks, surfacing the OOBE warning is useful, and I agree that a literal rollback can hit the same failing mount operation. The remaining reliability point is narrower: ConfigRefreshDrvFsOwner() detaches the existing mount, clears its automount bit, and a failed replacement is not restored, so the warning makes the disappearance visible but does not preserve availability or a retryable desired state. If that transient-drive-loss behavior is an accepted OOBE tradeoff, documenting it would resolve my concern; otherwise the refresh needs a path that does not permanently lose the previous mount on replacement failure.

Copilot AI review requested due to automatic review settings September 9, 2026 03:17

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

oobe.defaultUid can be negative (signed parse) and may wrap when used as a uint32_t uid, so it should be validated/normalized before being applied.

Review details

Suppressed comments (1)

src/linux/init/init.cpp:665

  • oobe.defaultUid is parsed as a signed int (the config parser accepts negative values), but it’s later passed to ConfigureUid(uint32_t) and also sent back to the service as result.DefaultUid. If the config contains a negative value (e.g. defaultUid = -1), it will wrap when converted to uint32_t, potentially selecting an unintended uid and producing inconsistent behavior.

Consider treating negative defaultUid as invalid (mark the key as absent) before using it for the DrvFs refresh, returning it to the service, or calling ConfigureUid().

        if ((OobeResult == 0) && (defaultUidPresent == ConfigKeyPresence::Present) && (defaultUid >= 0) && UtilIsUtilityVm())
        {
            for (const auto Admin : {false, true})
            {
                if (ConfigRefreshDrvFsOwner(defaultUid, Admin, Config) < 0)
  • Files reviewed: 4/4 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Comment thread src/linux/init/config.cpp
Comment thread src/linux/init/init.cpp
Copilot AI review requested due to automatic review settings September 14, 2026 09:20

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Refresh failures can leave mounts unavailable and OOBE falsely successful.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

src/linux/init/init.cpp:668

  • A refresh failure is only logged here, while OobeResult remains 0. The service treats that result as successful OOBE, clears RunOOBE, and persists the UID, so a failed refresh is not retried and the original stale-owner problem can remain. Propagate the refresh failure into the OOBE result (or otherwise retain a retryable state) before sending the result.
                if (ConfigRefreshDrvFsOwner(defaultUid, Admin, Config) < 0)
                {
                    LOG_ERROR("Failed to refresh the {} DrvFs mount namespace after OOBE", Admin ? "elevated" : "non-elevated");
                }
  • Files reviewed: 4/4 changed files
  • Comments generated: 2
  • Review effort level: Lite

Comment thread src/linux/init/config.cpp
Comment thread test/windows/UnitTests.cpp
Copilot AI review requested due to automatic review settings September 14, 2026 09:43

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

An unresolved moderate issue can leave DrvFs ownership tracking stale across the forked process path.

Review details

Suppressed comments (1)

src/linux/init/init.cpp:665

  • CreateProcessCommon invokes this path from the forked relay child, so mutations to g_*AutomountedDrvFsVolumes made by ConfigRefreshDrvFsOwner are not visible in the long-lived init process. That process still records the pre-OOBE owner and later ConfigRemountDrvFsImpl copies the stale UID/GID into the other namespace's tracking map, so a subsequent refresh can compare against incorrect ownership. Please synchronize the refreshed state back to the parent or rebuild the tracking state from the actual mount options.
                if (ConfigRefreshDrvFsOwner(defaultUid, Admin, Config) < 0)
  • Files reviewed: 4/4 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants