runc icon indicating copy to clipboard operation
runc copied to clipboard

nsexec: moving as much as we can to Go

Open cyphar opened this issue 2 years ago • 2 comments

Here is a list of things that nsexec does today, and a description of possible ideas and challenges to moving them to Go:

  • [ ] Basic namespace creation can be done with syscall.SysProcAttr today. There is code in nsexec.c that fixes very old kernel bugs (such as userns ownership being broken on old RHEL 6 kernels) which I'm sure Go stdlib doesn't care about (and maybe we shouldn't either). However there are some things that I suspect are not ideal:
    • [ ] For rootless containers, are newuidmap fallbacks supported by Go stdlib? I suspect not. Though obviously we could write PRs to add them to stdlib...
    • [ ] We currently do CLONE_NEWCGROUP far later during setup than the other namespaces, but this is actually a vestigial implementation detail of the original cgroupns code (which incorrectly added a synchronisation step to "ensure" runc init was in the correct cgroup). The logic probably should've been removed in 5110bd2fc026c0a9849e35322d5b81a3f28c716d.
  • [ ] We might want to use CLONE_INTO_CGROUP (SysProcAttr.CgroupFD, which should make all container nsenter-related operations faster because it avoids taking a bunch of locks in cgroup-core), but with #3931 we need the memfd_create() and /proc/self/exe copy to be executed outside of the container cgroup. This is easy to fix by just doing the clone in Go code (we could even make it part of the runc init fork+exec -- to remove one extra exec).
  • [x] Bind mount sources (#2576) are currently handled in a way that requires C code because we need to temporarily join the container mount namespace to open the mount sources (the kernel blocks bind-mount sources from a different mount namespace). However, open_tree(OPEN_TREE_CLONE) allows us to get around this, meaning that if we bump the minimum kernel version for this optional runc feature, we can do OPEN_TREE_CLONE on the host runc side and just pass the detached-mountfds to the rootfs setup code without any C code.
    • [x] IDMAP_SOURCES_ATTR is implemented in an analogous way to the bind-mount sources code, but because it requires mount_setattr(2), the OPEN_TREE_CLONE suggestion above is actually even more applicable. We can even implement #3943 entirely in Go and apply the mount attribute in the host side of runc.
  • [ ] The session handling by SysProcAttr is probably fine to just use "normally", but I wonder if we need to take anything in particular into account.
  • [ ] As far as I can see, there is no way to join existing namespaces with SysProcAttr -- and since setns(CLONE_NEWUSER) requires us to be single-threaded this would mean we would need to keep nsexec.c. This would require a patch to stdlib, but there is an additional issue to consider:
    • [ ] setns() supports joining a subset of namespaces of a given pidfd since Linux 5.8. This is something we probably want to use, but if we use stdlib there isn't a nice way to handle the fallback (join each namespace separately). It is trivial to detect whether it is supported (pass a pidfd to setns) but due to the API of SysProcAttr, I suspect we would need to detect the support from Go (we can do this safely with CLONE_NEWUSER because it will always fail but this will fail with -EINVAL so we can't use it to detect anything -- maybe we will need to do CLONE_NEWPID in a os-thread-locked goroutine and switch back because that doesn't affect the running process...) and then tune what we pass to SysProcAttr separately.

Agree with @cyphar -- if we can do it in Go, we should do it in Go.

Overall I very much hope we'll eventually be able to do all of it in Go. For example, with cgroupfd support in the kernel (since v5.7) and golang stdlib (since 1.20), we can enter cgroups way easier.

Originally posted by @kolyshkin in https://github.com/opencontainers/runc/issues/3943#issuecomment-1659392866

cyphar avatar Aug 01 '23 04:08 cyphar

@kolyshkin Never say never, and I would love to remove all the C code from our codebase, but I'm not sure if even on newer kernels and with the newest stdlib we will be able to do that (at the very least I don't think Go has handling for newuidmap -- though this could of course be added). I can come up with a list of things to do in a separate issue if you want to have a chat about the problem. For one thing, I think that (for performance and security reasons) we almost certainly want to implement the runc userns creation for mount_setattr(2) in CGO as a (slightly unsafe) fork. CLONE_INTO_CGROUP is something we might want but as I mentioned in #3931, cgroupv2 doesn't migrate memory usage when moving cgroups, so if we use CLONE_INTO_CGROUP we will need to also move the ensure_cloned_binary() logic out of runc init -- though we can always implement in Go so this is probably not that big of a deal.

Originally posted by @cyphar in https://github.com/opencontainers/runc/issues/3943#issuecomment-1659409419

cyphar avatar Aug 01 '23 04:08 cyphar