set up user namespace in builder, not in parent #2717
Closed
Conversation
This file contains 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
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.
|
@edolstra ping? |
|
I marked this as stale due to inactivity. → More info |
|
I still care about this change (and if someone approves it, I'll rebase it - it should still apply fine) |
|
I marked this as stale due to inactivity. → More info |
|
Still important to me |
|
Does this potentially allow the child to make additional changes to its own namespace? I'm not super familiar with the origin of this, but I'd like to understand and it looks like it is intentional. |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/2023-01-20-nix-team-meeting-minutes-25/25432/1 |
|
@catern Are you still interested in this? I am assigned to get it shepherded over the finish line if so, or closed otherwise. |
|
I regret to say that I'm no longer interested in this |
Can we ask for an explanation of the intent and intended benefit? As above, I’m not quite sure of what all the consequences would be. |
|
Yeah @catern if it is "just what it says on the tin" that is fine, but if there was a sharable use-case separate from the technical details that it isn't too much effort to remember on your part that would be good to know. I might take a look at rebasing this anyways :). |
|
It's just what it says on the tin. I made this change after writing some container code which made a new pid namespace but didn't remount /proc (which is buggy behavior) and then running Nix inside it. This change made Nix work but also my container code was buggy in the first place. There's no actual use case for this change, except that it slightly simplifies the code. |
|
After like 10 rebases :) I have master...obsidiansystems:userns |
|
@edolstra Do you know why you had the parent not child set the user namespace originally? As @tomberek says, this is a good case of "Chesterton's Fence" --- we should ask that question before undoing this. (Also, In the meeting I recall you saying you thought you had done this since, but the rebase I linked above demonstrates this is not the case.) I had done a
And neither that nor issue #625 seem to shed any light on this implementation detail. |
|
I don't remember why it's done this way. Maybe the kernel required it at the time? |
|
Thanks @edolstra. Yes I too vaguely recall user namespace used to be very finicky. That is also my best guess. |
fricklerhandwerk
added
contributor-experience
Ericson2314
pushed a commit
to Ericson2314/nix
that referenced
this pull request
Mar 10, 2023
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 (@Ericson3214'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.
7 tasks
Ericson2314
pushed a commit
to Ericson2314/nix
that referenced
this pull request
Mar 10, 2023
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 (@Ericson3214'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.
|
Rebased as #8023 |
Ericson2314
pushed a commit
to Ericson2314/nix
that referenced
this pull request
Mar 16, 2023
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 (@Ericson3214'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.
Ericson2314
pushed a commit
to Ericson2314/nix
that referenced
this pull request
Mar 16, 2023
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.
Ericson2314
pushed a commit
to Ericson2314/nix
that referenced
this pull request
Mar 20, 2023
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.
Ericson2314
pushed a commit
to Ericson2314/nix
that referenced
this pull request
Apr 15, 2023
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.
Ericson2314
pushed a commit
to Ericson2314/nix
that referenced
this pull request
May 8, 2023
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.
Ericson2314
pushed a commit
to Ericson2314/nix
that referenced
this pull request
Jun 21, 2023
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
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.
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.