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

Set up user namespace in builder, not in parent #8023

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Mar 10, 2023

Motivation

This is a rebase of #2717. The following is @catern's original PR description

This is a minor simplification; there's now slightly less handshaking required between the builder and the parent, and we create one less pipe when starting the builder.

As a niche benefit, this also replaces our access to /proc/builder_pid in the parent with an access to /proc/self in the builder. The former access could fail if /proc was mounted in a different pid namespace from the one we are running in; now we will work even if there is such a misconfiguration.

Context

Rebaser's (@Ericson2314's) notes:

A good questions why it was ever done the old way, since this is simpler and easier to write. The original commit doing this was c68e591, adding user namespace support for the first time, and referencing issue #625. But neither mention this design decision.

In #2717 (comment) @edolstra said:

I don't remember why it's done this way. Maybe the kernel required it at the time?

That seems to me like a likely explanation. User namespaces were famously fiddly for many years.

Checklist for maintainers

Maintainers: tick if completed or explain if not relevant

  • agreed on idea
  • agreed on implementation strategy
  • tests, as appropriate
    • functional tests - tests/**.sh
    • unit tests - src/*/tests
    • integration tests - tests/nixos/*
  • documentation in the manual
  • code and comments are self-explanatory
  • commit message explains why the change was made
  • new feature or incompatible change: updated release notes

Priorities

Add 👍 to pull requests you find important.

@Ericson2314 Ericson2314 marked this pull request as draft March 10, 2023 21:35
@Ericson2314 Ericson2314 requested a review from edolstra March 10, 2023 21:35
@Ericson2314
Copy link
Member Author

CC @yorickvP Since you are currently refactoring this code, and might have some opinions on this sort of thing.

@Ericson2314
Copy link
Member Author

@edolstra Rebased after the macOS fix. Since you said

We should do #8023 as well

I am assigning it to you! :)

@Ericson2314
Copy link
Member Author

error: cannot connect to socket at '/private/tmp/nix-build-nix-2.15.0pre20230316_5e86d92.drv-0/nix-test/tests/gc-non-blocking/var/nix/gc-socket/socket'

Hmm I wonder if we have a different macOS spurious error with the non-blocking GC besides the EOF one.

@thufschmitt
Copy link
Member

This is breaking causing a new error in the installer tests:

$ nix build .\#hydraJobs.installerTests.ubuntu-22-04.x86_64-linux.install-force-daemon -L
...
installer-test-ubuntu> ~~> Setting up the default profile                                                               
installer-test-ubuntu> installing 'nix-2.15.0pre20230320_88265e3'                                                       
installer-test-ubuntu> error:                                                                                           
installer-test-ubuntu>        … while setting up the build environment                                                  
installer-test-ubuntu>        … writing file '/proc/self/uid_map'                                                       
installer-test-ubuntu>        error: writing to file: Operation not permitted

@Ericson2314 Ericson2314 marked this pull request as draft April 3, 2023 03:48
This is a minor simplification; there's now slightly less handshaking
required between the builder and the parent, and we create one less pipe
when starting the builder.

As a niche benefit, this also replaces our access to `/proc/builder_pid`
in the parent with an access to `/proc/self` in the builder. The former
access could fail if `/proc` was mounted in a different pid namespace
from the one we are running in; now we will work even if there is such a
misconfiguration.

----

Rebaser's (@Ericson2314's) notes:

A good questions why it was ever done the old way, since this is simpler
and easier to write. The original commit doing this was
c68e591, adding user namespae support
for the first time, and referencing issue NixOS#625. But neither mention this
design decision.

In NixOS#2717 (comment) Eelco
said:

> I don't remember why it's done this way. Maybe the kernel required it
> at the time?

That seems to me like a likely explanation. User namespaces were
famously fiddly for many years.
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