Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

stage-2-init: fix false positives for RO Nix store mounts #375257

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

numinit
Copy link
Contributor

@numinit numinit commented Jan 20, 2025

Mounting an ext4 Nix store with the option errors=remount-ro results in the store not being remounted as RO after stage2 runs. In fact, many mount options involving "ro" at the end of the option are susceptible to this.

We need to take the "top" mount instead of any mount, which is the last line printed by findmnt. Additionally, make the regex more strict, so we don't select mount options ending in ro (like errors=remount-ro from
ext4, or overlay paths ending in 'ro') and accidentally leave the Nix store RW after boot.

The tests for this look a little tortured, because of how the tests overlay mount the Nix store. It is much easier to hit this on real hardware, with a single mountpoint (though, incidentally, I first hit this with an overlay).

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 25.05 Release Notes (or backporting 24.11 and 25.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Jan 20, 2025
@numinit numinit force-pushed the stage2/fix-store-remount branch from 0098d16 to dd92244 Compare January 20, 2025 07:07
@numinit numinit force-pushed the stage2/fix-store-remount branch 2 times, most recently from 12347d3 to 734b798 Compare January 20, 2025 07:39
- Test RO store mount even given the presence of filesystems with
  options ending in "ro"
- Test postBootCommands in stage2
@numinit numinit force-pushed the stage2/fix-store-remount branch from 734b798 to 381e1f8 Compare January 20, 2025 07:53
@numinit numinit requested review from Mic92, shlevy and edolstra January 20, 2025 08:00
@numinit
Copy link
Contributor Author

numinit commented Jan 20, 2025

@Mic92 @shlevy @edolstra You three touched this code last :-)

If I didn't think of any edgecases here from taking the last thing mounted over the store, let me know, but I'm pretty sure the regex is correct. (Of note, bash regexes match ^$ at the beginning and end of the string, not the line).

We need to take the "top" mount instead of any mount, which is the last
line printed by findmnt. Additionally, make the regex more strict, so we
don't select mount options ending in ro (like `errors=remount-ro` from
ext4, or overlay paths ending in 'ro') and accidentally leave the Nix
store RW after boot.
@numinit numinit force-pushed the stage2/fix-store-remount branch from 381e1f8 to 2f3a80c Compare January 20, 2025 08:27
@numinit numinit requested a review from Mic92 January 20, 2025 08:33
@numinit
Copy link
Contributor Author

numinit commented Jan 20, 2025

One edgecase I considered is that if the store is under a readonly mount (as opposed to one itself) we will still perform the RO bind mount, but I'm not sure this is actually worth changing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants