Consider revert #3931 nsexec: cloned_binary: remove bindfd logic entirely

[lifubang]

Description

With runc built from the main branch:
If I create 450 containers in a 8G memory node, all the memory will be eat by runc.
The node will be in stuck state.

With runc 1.1.8, it will success.

I think when using memfd_create, runc binary will be in memory, so if we batch create containers, it will eat node's memory before the container started.
In my head, maybe containerd uses runc create for the first step when run a container.

Steps to reproduce the issue

1. make runc binary with main branch
2. use a bash script to start 450 containers

the bash script:

#!/bin/bash
for((i=1; i<=450; i ++))
do
        /opt/runc create test"$i" &
done

Describe the results you received and expected

received:
The node is in stuck state, because there is no enough memory left in the node.

expected:
All containers should be created normally.

What version of runc are you using?

runc version 1.1.0+dev
commit: v1.1.0-688-g74c125d8
spec: 1.1.0
go: go1.18.4
libseccomp: 2.5.1

Host OS information

PRETTY_NAME="Ubuntu 22.04.2 LTS"
NAME="Ubuntu"
VERSION_ID="22.04"
VERSION="22.04.2 LTS (Jammy Jellyfish)"
VERSION_CODENAME=jammy
ID=ubuntu
ID_LIKE=debian
HOME_URL="https://www.ubuntu.com/"
SUPPORT_URL="https://help.ubuntu.com/"
BUG_REPORT_URL="https://bugs.launchpad.net/ubuntu/"
PRIVACY_POLICY_URL="https://www.ubuntu.com/legal/terms-and-policies/privacy-policy"
UBUNTU_CODENAME=jammy

Host kernel information

Linux iZ2ze4a8qvjqt6lt7ommf8Z 5.15.0-73-generic #80-Ubuntu SMP Mon May 15 15:18:26 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux

[lifubang]

@cyphar PTAL, thanks.

[rata]

@lifubang does reverting those commit fixes the issue?

[lifubang]

does reverting those commit fixes the issue?

Yes, it works very well after reverted #3931 .

[cyphar]

I can add the contrib program to bind-mount the memfd which should eliminate this issue (see the discussion in the PR). I didn't add it because the primary concern last time was container memory usage, and memfd_create(2) no longer causes issues. The only downside is you need to have a small daemon running on the system.

I really do not want to have to keep bindfd...

[113xiaoji]

I encountered a similar issue. When using the main branch, my test case failed. However, when I solely used the 'remove bindfd' commit, my test case passed. I suspect that this PR might be conflicting with other PRs on the main branch, leading to a new performance issue. I'd recommend not using the main branch and instead, trying only @cyphar's PR, @lifubang.

[kolyshkin]

Oh my, this is tough. Let's discuss further in #3931

[lifubang]

However, when I solely used the 'remove bindfd' commit, my test case passed.

With your suggestion, it can't fix my issue.

I suspect that this PR might be conflicting with other PRs on the main branch

I think maybe not this problem. I think obviously it's a design problem, but not a code bug, cyphar has confirmed it, and he has had a solution to resolve this issue.

So, maybe your issue is not the same as mine. You can discuss it further in #3931 or open an new issue once you know how to reproduce it clearly.

[113xiaoji]

However, when I solely used the 'remove bindfd' commit, my test case passed.

With your suggestion, it can't fix my issue.

I suspect that this PR might be conflicting with other PRs on the main branch

I think maybe not this problem. I think obviously it's a design problem, but not a code bug, cyphar has confirmed it, and he has had a solution to resolve this issue.

So, maybe your issue is not the same as mine. You can discuss it further in #3931 or open an new issue once you know how to reproduce it clearly.

The node is in stuck state. Are there any logs?

[113xiaoji]

However, when I solely used the 'remove bindfd' commit, my test case passed.

With your suggestion, it can't fix my issue.

I suspect that this PR might be conflicting with other PRs on the main branch

I think maybe not this problem. I think obviously it's a design problem, but not a code bug, cyphar has confirmed it, and he has had a solution to resolve this issue.

So, maybe your issue is not the same as mine. You can discuss it further in #3931 or open an new issue once you know how to reproduce it clearly.

Maybe you could test with @cyphar's suggestion. I show you memfd code afternoon.

[cyphar]

I will post a PR, but the code in question is #3931 (comment). If you use it to mount on top of the runc binary, you should be able to run as many runc processes as you like without extra memory overhead. The downside is that the program needs to keep running as a daemon -- if the process dies, you cannot exec the mounted runc path (and package updates are more annoying -- you need to stop the daemon, upgrade the runc package, then restart the daemon).

[113xiaoji]

memfd code is

package main

import (
   "errors"
   "fmt"
   "io"
   "os"
   "os/signal"
   "runtime"
   "strings"
   "time"

   "github.com/urfave/cli"
   "golang.org/x/sys/unix"
)

// version will be populated by the Makefile, read from
// VERSION file of the source code.
var version = ""

// gitCommit will be the hash that the binary was built from
// and will be populated by the Makefile
var gitCommit = ""

const (
   usage = `Open Container Initiative contrib/cmd/memfd-bind

In order to protect against certain container attacks, every runc invocation
that involves creating or joining a container will cause runc to make a copy of
the runc binary in memory (usually to a memfd). While "runc init" is very
short-lived, this extra memory usage can cause problems for containers with
very small memory limits (or containers that have many "runc exec" invocations
applied to them at the same time).

memfd-bind is a tool to create a persistent memfd-sealed-copy of the runc binary,
which will cause runc to not make its own copy. This means you can get the
benefits of using a sealed memfd as runc's binary (even in a container breakout
attack to get write access to the runc binary, neither the underlying binary
nor the memfd copy can be changed).

To use memfd-bind, just specify which path you want to create a socket path at
which you want to receive terminals:

    $ sudo memfd-bind /usr/bin/runc

Note that (due to kernel restrictions on bind-mounts), this program must remain
running on the host in order for the binary to be readable (it is recommended
you use a systemd unit to keep this process around).

If this program dies, there will be a leftover mountpoint that always returns
-EINVAL when attempting to access it. You need to use memfd-bind --cleanup on the
path in order to unmount the path (regular umount(8) will not work):

    $ sudo memfd-bind --cleanup /usr/bin/runc

Note that (due to restrictions on /proc/$pid/fd/$fd magic-link resolution),
only privileged users (specifically, those that have ptrace privileges over the
memfd-bind daemon) can access the memfd bind-mount. This means that using this
tool to harden your /usr/bin/runc binary would result in unprivileged users
being unable to execute the binary. If this is an issue, you could make all
privileged process use a different copy of runc (by making a copy in somewhere
like /usr/sbin/runc) and only using memfd-bind for the version used by
privileged users.
`
)

func cleanup(path string) error {
   file, err := os.OpenFile(path, unix.O_PATH|unix.O_NOFOLLOW|unix.O_CLOEXEC, 0)
   if err != nil {
      return fmt.Errorf("cleanup: failed to open runc binary path: %w", err)
   }
   defer file.Close()
   fdPath := fmt.Sprintf("/proc/self/fd/%d", file.Fd())

   // Keep umounting until we hit a umount error.
   for unix.Unmount(fdPath, unix.MNT_DETACH) == nil {
      // loop...
   }
   fmt.Fprintf(os.Stdout, "memfd-bind: path %q has been cleared of all old bind-mounts\n", path)
   return nil
}

func memfdClone(path string) (*os.File, error) {
   binFile, err := os.Open(path)
   if err != nil {
      return nil, fmt.Errorf("memfd clone: failed to open runc binary path: %w", err)
   }
   defer binFile.Close()

   const memfdComment = "runc_cloned:/proc/self/exe"
   memfd, err := unix.MemfdCreate(memfdComment, unix.MFD_CLOEXEC|unix.MFD_EXEC|unix.MFD_ALLOW_SEALING)
   if errors.Is(err, unix.EINVAL) {
      // old kernel with no MFD_EXEC support
      memfd, err = unix.MemfdCreate(memfdComment, unix.MFD_CLOEXEC|unix.MFD_ALLOW_SEALING)
   }
   if err != nil {
      return nil, fmt.Errorf("memfd clone: failed to create memfd: %w", err)
   }
   memfdFile := os.NewFile(uintptr(memfd), memfdComment)
   // we *do not* defer memfdFile.Close() here...

   // Copy the contents...
   for {
      _, err = io.Copy(memfdFile, binFile)
      if !errors.Is(err, unix.EINTR) {
         break
      }
   }
   if err != nil {
      return nil, fmt.Errorf("memfd clone: failed to copy binary text to memfd: %w", err)
   }

   // Seal the memfd. We ignore errors for the newer seals, but for the
   // RUNC_MEMFD_MIN_SEALS set we will return an error. This matches the
   // behaviour of libcontainer/nsenter/cloned_binary.c.
   const RUNC_MEMFD_MIN_SEALS = unix.F_SEAL_SEAL | unix.F_SEAL_SHRINK | unix.F_SEAL_GROW | unix.F_SEAL_WRITE
   const F_SEAL_EXEC = 0x20 // from <linux/fcntl.h>
   _, _ = unix.FcntlInt(uintptr(memfd), unix.F_ADD_SEALS, unix.F_SEAL_FUTURE_WRITE)
   _, _ = unix.FcntlInt(uintptr(memfd), unix.F_ADD_SEALS, F_SEAL_EXEC)
   _, err = unix.FcntlInt(uintptr(memfd), unix.F_ADD_SEALS, RUNC_MEMFD_MIN_SEALS)
   if err != nil {
      return nil, fmt.Errorf("memfd clone: failed to add required seals to memfd: %w", err)
   }
   fmt.Fprintf(os.Stdout, "memfd-bind: created sealed memfd with contents of %q\n", path)
   return memfdFile, nil
}

func mount(path string) error {
   memfdFile, err := memfdClone(path)
   if err != nil {
      return err
   }
   defer memfdFile.Close()
   memfdPath := fmt.Sprintf("/proc/self/fd/%d", memfdFile.Fd())

   // We have to open an O_NOFOLLOW|O_PATH to the memfd magic-link because we
   // cannot bind-mount the memfd itself (it's in the internal kernel mount
   // namespace and cross-mount-namespace bind-mounts are not allowed). This
   // also requires that this program stay alive continuously for the
   // magic-link to stay alive...
   memfdLink, err := os.OpenFile(memfdPath, unix.O_PATH|unix.O_NOFOLLOW|unix.O_CLOEXEC, 0)
   if err != nil {
      return fmt.Errorf("mount: failed to /proc/self/fd magic-link for memfd: %w", err)
   }
   defer memfdLink.Close()
   memfdLinkFdPath := fmt.Sprintf("/proc/self/fd/%d", memfdLink.Fd())

   exeFile, err := os.OpenFile(path, unix.O_PATH|unix.O_NOFOLLOW|unix.O_CLOEXEC, 0)
   if err != nil {
      return fmt.Errorf("mount: failed to open target runc binary path: %w", err)
   }
   defer exeFile.Close()
   exeFdPath := fmt.Sprintf("/proc/self/fd/%d", exeFile.Fd())

   err = unix.Mount(memfdLinkFdPath, exeFdPath, "", unix.MS_BIND, "")
   if err != nil {
      return fmt.Errorf("mount: failed to mount memfd on top of runc binary path target: %w", err)
   }

   // If there is a signal we want to do cleanup.
   sigCh := make(chan os.Signal, 1)
   signal.Notify(sigCh, os.Interrupt, unix.SIGTERM, unix.SIGINT)
   go func() {
      <-sigCh
      fmt.Fprintf(os.Stdout, "memfd-bind: exit signal caught! cleaning up the bind-mount on %q...\n", path)
      _ = cleanup(path)
      os.Exit(0)
   }()

   // Clean up things we don't need...
   _ = exeFile.Close()
   _ = memfdLink.Close()

   // We now have to stay alive to keep the magic-link alive...
   fmt.Fprintf(os.Stdout, "memfd-bind: bind-mount of memfd over %q created -- looping forever!\n", path)
   for {
      // loop forever...
      time.Sleep(time.Duration(1<<63 - 1))
      // make sure the memfd isn't gc'd
      runtime.KeepAlive(memfdFile)
   }
}

func main() {
   app := cli.NewApp()
   app.Name = "memfd-bind"
   app.Usage = usage

   // Set version to be the same as runC.
   var v []string
   if version != "" {
      v = append(v, version)
   }
   if gitCommit != "" {
      v = append(v, "commit: "+gitCommit)
   }
   app.Version = strings.Join(v, "\n")

   // Set the flags.
   app.Flags = []cli.Flag{
      cli.BoolFlag{
         Name:  "cleanup",
         Usage: "Do not create a new memfd-sealed file, only clean up an existing one at <path>.",
      },
   }

   app.Action = func(ctx *cli.Context) error {
      args := ctx.Args()
      if len(args) != 1 {
         return errors.New("need to specify a single path to the runc binary")
      }
      path := ctx.Args()[0]

      err := cleanup(path)
      // We only care about cleanup errors when doing --cleanup.
      if ctx.Bool("cleanup") {
         return err
      }
      return mount(path)
   }
   if err := app.Run(os.Args); err != nil {
      fmt.Fprintf(os.Stderr, "memfd-bind: %v\n", err)
      os.Exit(1)
   }
}

@lifubang

[lifubang]

I have tried it, it works very well.
My question is that this contrib program is for personal use purpose? I don't know whether other upstream projects, such as containerd, docker, and k8s will change their install script to use it or not? If yes, there will be lots of work to do for these projects.

I think this issue may impact some k8s/dockerswarm cluster when the node is in case of insufficient memory state. So I suggust to revert it before we have an new solution. But if we confirm that it will not impact upstream project, I think we can consider add a contrib program for anyone who needs it.

[cyphar]

The problem is that we cannot just revert it:

• The version post-nsexec: cloned_binary: remove bindfd logic entirely #3931 has this one issue that can be completely fixed for systems that require you to have so many runc processes running by having a daemon on the system. In an absolute worst case, we can look at getting Docker/containerd/cri-o to do the memfd bind-mount in their daemons in order to make the setup be automatic.
• The version pre-nsexec: cloned_binary: remove bindfd logic entirely #3931 had performance issues because libct/nsenter: namespace the bindfd shuffle #3599 made the bindfd happen in a temporary mount namespace which caused serious contention on mount_lock on systems with loads of runc programs running.
• The version pre-libct/nsenter: namespace the bindfd shuffle #3599 had other issues with hanging because systemd parses mountinfo and having all of these temporary ro bind-mounts causes the systemd process to stall which causes container creation to also stall.
• Not having this code leaves runc vulnerable to the /proc/self/exe CVE, at least not until I get the magic-link hardening work I'm working on merged into the kernel.

From where I'm standing, it seems to me that #3931 is the least bad option. Every option has some kind of cost, but at least the memfd one can be mitigated by users -- the mount-based ones affect you and you cannot disable them.

I think this issue may impact some k8s/dockerswarm cluster when the node is in case of insufficient memory state.

If a node cannot have 10MB in memory per container during setup, I don't think the node has enough memory to actually run the container process. Also I don't think Docker, containerd, cri-o or Kubernetes keep processes in the runc create state for long periods of time. The "created" state of a container in (for instance) Docker is completely different to runc create -- there is no live process created with docker create. Most engines using runc only let runc create live for a very short time AFAIK. I might be wrong though.

[rata]

IMHO, either current main or reverting to the state in runc 1.1 is what seems reasonable.

Using cgroups v2 and >= 5.14 kernel is not a valid workaround for these issues in runc 1.1? (Asking due to this comment). If that is the case, maybe it will be nice to have some knob to switch the behavior to what is done in 1.1, if the extra mem is a problem, can be a safe path to have.

Also, just curious, why not hiding /proc/self/exe on newer kernels is the way to go for that CVE? That prctl is merged upstream IIRC.