Merge pull request #4265 from lifubang/fix-set-RLIMIT_NOFILE-race

Fix set nofile rlimit error

libcontainer/init_linux.go

@@ -223,6 +223,12 @@ func containerInit(t initType, config *initConfig, pipe *syncSocket, consoleSock
 		return err
 	}
 
+	// Clean the RLIMIT_NOFILE cache in go runtime.
+	// Issue: https://github.com/opencontainers/runc/issues/4195
+	if containsRlimit(config.Rlimits, unix.RLIMIT_NOFILE) {
+		system.ClearRlimitNofileCache()
+	}
+
 	switch t {
 	case initSetns:
 		i := &linuxSetnsInit{
@@ -649,6 +655,15 @@ func setupRoute(config *configs.Config) error {
 	return nil
 }
 
+func containsRlimit(limits []configs.Rlimit, resource int) bool {
+	for _, rlimit := range limits {
+		if rlimit.Type == resource {
+			return true
+		}
+	}
+	return false
+}
+
 func setupRlimits(limits []configs.Rlimit, pid int) error {
 	for _, rlimit := range limits {
 		if err := unix.Prlimit(pid, rlimit.Type, &unix.Rlimit{Max: rlimit.Hard, Cur: rlimit.Soft}, nil); err != nil {

libcontainer/integration/exec_test.go

@@ -136,11 +136,13 @@ func testRlimit(t *testing.T, userns bool) {
 
 	config := newTemplateConfig(t, &tParam{userns: userns})
 
-	// ensure limit is lower than what the config requests to test that in a user namespace
+	// Ensure limit is lower than what the config requests to test that in a user namespace
 	// the Setrlimit call happens early enough that we still have permissions to raise the limit.
+	// Do not change the Cur value to be equal to the Max value, please see:
+	// https://github.com/opencontainers/runc/pull/4265#discussion_r1589666444
 	ok(t, unix.Setrlimit(unix.RLIMIT_NOFILE, &unix.Rlimit{
 		Max: 1024,
-		Cur: 1024,
+		Cur: 512,
 	}))
 
 	out := runContainerOk(t, config, "/bin/sh", "-c", "ulimit -n")

libcontainer/process_linux.go

@@ -268,20 +268,26 @@ func (p *setnsProcess) start() (retErr error) {
 			}
 		}
 	}
-	// set rlimits, this has to be done here because we lose permissions
-	// to raise the limits once we enter a user-namespace
-	if err := setupRlimits(p.config.Rlimits, p.pid()); err != nil {
-		return fmt.Errorf("error setting rlimits for process: %w", err)
-	}
+
 	if err := utils.WriteJSON(p.comm.initSockParent, p.config); err != nil {
 		return fmt.Errorf("error writing config to pipe: %w", err)
 	}
 
+	var seenProcReady bool
 	ierr := parseSync(p.comm.syncSockParent, func(sync *syncT) error {
 		switch sync.Type {
 		case procReady:
-			// This shouldn't happen.
-			panic("unexpected procReady in setns")
+			seenProcReady = true
+			// Set rlimits, this has to be done here because we lose permissions
+			// to raise the limits once we enter a user-namespace
+			if err := setupRlimits(p.config.Rlimits, p.pid()); err != nil {
+				return fmt.Errorf("error setting rlimits for ready process: %w", err)
+			}
+
+			// Sync with child.
+			if err := writeSync(p.comm.syncSockParent, procRun); err != nil {
+				return err
+			}
 		case procHooks:
 			// This shouldn't happen.
 			panic("unexpected procHooks in setns")
@@ -340,6 +346,9 @@ func (p *setnsProcess) start() (retErr error) {
 	if err := p.comm.syncSockParent.Shutdown(unix.SHUT_WR); err != nil && ierr == nil {
 		return err
 	}
+	if !seenProcReady && ierr == nil {
+		ierr = errors.New("procReady not received")
+	}
 	// Must be done after Shutdown so the child will exit and we can wait for it.
 	if ierr != nil {
 		_, _ = p.wait()
@@ -774,7 +783,7 @@ func (p *initProcess) start() (retErr error) {
 			}
 		case procReady:
 			seenProcReady = true
-			// set rlimits, this has to be done here because we lose permissions
+			// Set rlimits, this has to be done here because we lose permissions
 			// to raise the limits once we enter a user-namespace
 			if err := setupRlimits(p.config.Rlimits, p.pid()); err != nil {
 				return fmt.Errorf("error setting rlimits for ready process: %w", err)

libcontainer/setns_init_linux.go

@@ -49,6 +49,7 @@ func (l *linuxSetnsInit) Init() error {
 			}
 		}
 	}
+
 	if l.config.CreateConsole {
 		if err := setupConsole(l.consoleSocket, l.config, false); err != nil {
 			return err
@@ -77,6 +78,13 @@ func (l *linuxSetnsInit) Init() error {
 		}
 	}
 
+	// Tell our parent that we're ready to exec. This must be done before the
+	// Seccomp rules have been applied, because we need to be able to read and
+	// write to a socket.
+	if err := syncParentReady(l.pipe); err != nil {
+		return fmt.Errorf("sync ready: %w", err)
+	}
+
 	if err := selinux.SetExecLabel(l.config.ProcessLabel); err != nil {
 		return err
 	}

libcontainer/system/linux.go

@@ -8,13 +8,28 @@ import (
 	"io"
 	"os"
 	"strconv"
+	"sync/atomic"
 	"syscall"
 	"unsafe"
 
 	"github.com/sirupsen/logrus"
 	"golang.org/x/sys/unix"
 )
 
+//go:linkname syscallOrigRlimitNofile syscall.origRlimitNofile
+var syscallOrigRlimitNofile atomic.Pointer[syscall.Rlimit]
+
+// As reported in issue #4195, the new version of go runtime(since 1.19)
+// will cache rlimit-nofile. Before executing execve, the rlimit-nofile
+// of the process will be restored with the cache. In runc, this will
+// cause the rlimit-nofile setting by the parent process for the container
+// to become invalid. It can be solved by clearing this cache. But
+// unfortunately, go stdlib doesn't provide such function, so we need to
+// link to the private var `origRlimitNofile` in package syscall to hack.
+func ClearRlimitNofileCache() {
+	syscallOrigRlimitNofile.Store(nil)
+}
+
 type ParentDeathSignal int
 
 func (p ParentDeathSignal) Restore() error {

tests/integration/rlimits.bats

@@ -0,0 +1,88 @@
+#!/usr/bin/env bats
+
+load helpers
+
+function setup() {
+	# Do not change the Cur value to be equal to the Max value
+	# Because in some environments, the soft and hard nofile limit have the same value.
+	[ $EUID -eq 0 ] && prlimit --nofile=1024:65536 -p $$
+	setup_busybox
+}
+
+function teardown() {
+	teardown_bundle
+}
+
+# Set and check rlimit_nofile for runc run. Arguments are:
+#  $1: soft limit;
+#  $2: hard limit.
+function run_check_nofile() {
+	soft="$1"
+	hard="$2"
+	update_config ".process.rlimits = [{\"type\": \"RLIMIT_NOFILE\", \"soft\": ${soft}, \"hard\": ${hard}}]"
+	update_config '.process.args = ["/bin/sh", "-c", "ulimit -n; ulimit -H -n"]'
+
+	runc run test_rlimit
+	[ "$status" -eq 0 ]
+	[[ "${lines[0]}" == "${soft}" ]]
+	[[ "${lines[1]}" == "${hard}" ]]
+}
+
+# Set and check rlimit_nofile for runc exec. Arguments are:
+#  $1: soft limit;
+#  $2: hard limit.
+function exec_check_nofile() {
+	soft="$1"
+	hard="$2"
+	update_config ".process.rlimits = [{\"type\": \"RLIMIT_NOFILE\", \"soft\": ${soft}, \"hard\": ${hard}}]"
+
+	runc run -d --console-socket "$CONSOLE_SOCKET" test_rlimit
+	[ "$status" -eq 0 ]
+
+	runc exec test_rlimit /bin/sh -c "ulimit -n; ulimit -H -n"
+	[ "$status" -eq 0 ]
+	[[ "${lines[0]}" == "${soft}" ]]
+	[[ "${lines[1]}" == "${hard}" ]]
+}
+
+@test "runc run with RLIMIT_NOFILE(The same as system's hard value)" {
+	hard=$(ulimit -n -H)
+	soft="$hard"
+	run_check_nofile "$soft" "$hard"
+}
+
+@test "runc run with RLIMIT_NOFILE(Bigger than system's hard value)" {
+	requires root
+	limit=$(ulimit -n -H)
+	soft=$((limit + 1))
+	hard=$soft
+	run_check_nofile "$soft" "$hard"
+}
+
+@test "runc run with RLIMIT_NOFILE(Smaller than system's hard value)" {
+	limit=$(ulimit -n -H)
+	soft=$((limit - 1))
+	hard=$soft
+	run_check_nofile "$soft" "$hard"
+}
+
+@test "runc exec with RLIMIT_NOFILE(The same as system's hard value)" {
+	hard=$(ulimit -n -H)
+	soft="$hard"
+	exec_check_nofile "$soft" "$hard"
+}
+
+@test "runc exec with RLIMIT_NOFILE(Bigger than system's hard value)" {
+	requires root
+	limit=$(ulimit -n -H)
+	soft=$((limit + 1))
+	hard=$soft
+	exec_check_nofile "$soft" "$hard"
+}
+
+@test "runc exec with RLIMIT_NOFILE(Smaller than system's hard value)" {
+	limit=$(ulimit -n -H)
+	soft=$((limit - 1))
+	hard=$soft
+	exec_check_nofile "$soft" "$hard"
+}