Skip to content

[20.10 backport] daemon.WithCommonOptions() fix detection of user-namespaces#43084

Merged
tianon merged 1 commit intomoby:20.10from
AkihiroSuda:cherrypick-42736
Jan 8, 2022
Merged

[20.10 backport] daemon.WithCommonOptions() fix detection of user-namespaces#43084
tianon merged 1 commit intomoby:20.10from
AkihiroSuda:cherrypick-42736

Conversation

@AkihiroSuda
Copy link
Copy Markdown
Member

@AkihiroSuda AkihiroSuda commented Dec 15, 2021

Cherry-pick #42736

Commit dae652e (#41030) added support for non-privileged containers to use ICMP_PROTO (used for ping). This option cannot be set for containers that have user-namespaces enabled.

However, the detection looks to be incorrect; HostConfig.UsernsMode was added in 6993e89 (#20111) / ee21838 (#20913), and the property only has meaning if the daemon is running with user namespaces enabled. In other situations, the property has no meaning.

As a result of the above, the sysctl would only be set for containers running with UsernsMode=host on a daemon running with user-namespaces enabled.

This patch adds a check if the daemon has user-namespaces enabled (RemappedRoot having a non-empty value) to fix the detection.

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

The cherry-pick was almost clean but userns.RunningInUserNS() -> sys.RunningInUserNS().

diff --git a/daemon/oci_linux.go b/daemon/oci_linux.go
index 9b39b8f615..3d34dcb31d 100644
--- a/daemon/oci_linux.go
+++ b/daemon/oci_linux.go
@@ -768,7 +768,7 @@ func WithCommonOptions(daemon *Daemon, c *container.Container) coci.SpecOpts {
                if c.HostConfig.NetworkMode.IsPrivate() {
                        // We cannot set up ping socket support in a user namespace
                        userNS := daemon.configStore.RemappedRoot != "" && c.HostConfig.UsernsMode.IsPrivate()
-                       if !userNS && !userns.RunningInUserNS() && sysctlExists("net.ipv4.ping_group_range") {
+                       if !userNS && !sys.RunningInUserNS() && sysctlExists("net.ipv4.ping_group_range") {
                                // allow unprivileged ICMP echo sockets without CAP_NET_RAW
                                s.Linux.Sysctl["net.ipv4.ping_group_range"] = "0 2147483647"
                        }

Fix docker/buildx#561

@thaJeztah @AkihiroSuda
daemon.WithCommonOptions() fix detection of user-namespaces
660b996 
Commit dae652e added support for non-privileged
containers to use ICMP_PROTO (used for `ping`). This option cannot be set for
containers that have user-namespaces enabled.

However, the detection looks to be incorrect; HostConfig.UsernsMode was added
in 6993e89 / ee21838,
and the property only has meaning if the daemon is running with user namespaces
enabled. In other situations, the property has no meaning.
As a result of the above, the sysctl would only be set for containers running
with UsernsMode=host on a daemon running with user-namespaces enabled.

This patch adds a check if the daemon has user-namespaces enabled (RemappedRoot
having a non-empty value), or if the daemon is running inside a user namespace
(e.g. rootless mode) to fix the detection.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
(cherry picked from commit a826ca3)

---
The cherry-pick was almost clean but `userns.RunningInUserNS()` -> `sys.RunningInUserNS()`.

Fix docker/buildx issue 561
---

Signed-off-by: Akihiro Suda <[email protected]>
@AkihiroSuda AkihiroSuda mentioned this pull request Dec 15, 2021
Closed
@AkihiroSuda AkihiroSuda changed the title daemon.WithCommonOptions() fix detection of user-namespaces [20.10 backport] daemon.WithCommonOptions() fix detection of user-namespaces Dec 15, 2021
@AkihiroSuda AkihiroSuda added this to the 20.10.13 milestone Dec 15, 2021
@AkihiroSuda AkihiroSuda mentioned this pull request Dec 15, 2021
Merged
thaJeztah
thaJeztah approved these changes Dec 15, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@AkihiroSuda AkihiroSuda requested a review from cpuguy83 January 7, 2022 05:58
@tianon tianon merged commit b1fc0c8 into moby:20.10 Jan 8, 2022
@dmarteau dmarteau mentioned this pull request Mar 11, 2022
Closed
@thaJeztah thaJeztah mentioned this pull request May 3, 2022
Open
@thaJeztah thaJeztah mentioned this pull request May 18, 2022
Closed
3 tasks
@thaJeztah thaJeztah mentioned this pull request Mar 15, 2023
Open
@thaJeztah thaJeztah mentioned this pull request Sep 15, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tianon tianon tianon approved these changes

@thaJeztah thaJeztah thaJeztah approved these changes

@tonistiigi tonistiigi Awaiting requested review from tonistiigi

@cpuguy83 cpuguy83 Awaiting requested review from cpuguy83

Assignees

No one assigned

Labels

impact/changelog kind/bugfix PR's that fix bugs status/2-code-review

Projects

None yet

Milestone

20.10.13

Development

Successfully merging this pull request may close these issues.

3 participants

@AkihiroSuda @tianon @thaJeztah