clean cached rlimit nofile in go runtime #4237

ls-ggg · 2024-03-29T10:24:29Z

fix:#4195
In a relatively new runc version, in other words a new go version (in my environment the go version is go1.20.13),
the runtime will cache the value of rlimit nofile, which will cause the container's rlimit nofile configuration to not take effect. This problem must occur when the customer configures the CAP_SYS_RESOURCE permission.

refer to:[commit](// golang/go@f5eef58#diff-ec665e9789f8cf5cd1828ad7fa9f0ff4ebc1f5b5dd0fc82a296da5c07da7ece6)

go runtime caches rlimit-nofile during startup phase

and before exec,runtime will reset rlimit-nofile.

kolyshkin · 2024-04-02T00:07:41Z

@ls-ggg can you do a rebase instead of a merge? I.e. do not add merge commits.

Also, can we have a test case?

ls-ggg · 2024-04-02T05:24:08Z

@ls-ggg can you do a rebase instead of a merge? I.e. do not add merge commits.

Also, can we have a test case?

@kolyshkin I've done the rebase and added test cases.

kolyshkin · 2024-04-02T17:54:39Z

OK, I took a deeper look and in fact this is already fixed in the main branch via #3813, which brings in this change:
https://go-review.googlesource.com/c/sys/+/476695

So, to my best understanding, this PR is not needed.

For release-1.1 branch to fix this, we need something like #3813.

kolyshkin · 2024-04-02T20:57:43Z

For release-1.1 branch to fix this, we need something like #3813.

Opened #4239; PTAL

kolyshkin · 2024-04-02T22:15:19Z

OK, I took a deeper look and in fact this is already fixed in the main branch via #3813, which brings in this change: https://go-review.googlesource.com/c/sys/+/476695

So, to my best understanding, this PR is not needed.

Closing (not needed in main branch, opened #4239 for release-1.1 branch).

kolyshkin · 2024-04-03T01:20:22Z

Interestingly, runc-1.1.12 (as used in the original bug report, #4195) is compiled with Go 1.20.13:

$ ./runc-1.1.12 --version
runc version 1.1.12
commit: v1.1.12-0-g51d5e946
spec: 1.0.2-dev
go: go1.20.13
libseccomp: 2.5.4

and I was able to reproduce the issue with this version.

Found out that Go 1.20 has CL 476097 backported (as CL 478659), so Go 1.20 is also affected (since Go1.20.4).

kolyshkin · 2024-04-03T04:40:31Z

Found out that Go 1.20 has CL 476097 backported (as CL 478659), so Go 1.20 is also affected (since Go 1.20.4).

As well as Go 1.19.+ (CL 478660).

kolyshkin

Maybe we should drop the test case entirely, as it takes up to 100000 execs (in my setup) to catch the issue, and we can't afford it in CI.

tests/integration/resources.bats

libcontainer/utils/utils.go

kolyshkin · 2024-04-03T05:25:13Z

For some reason, my alternative approach (#4239) is not working.

Testing this PR, I got this:

$ RUNC=~/git/runc/runc-4237-go1.21; i=0; $RUNC --version && while [ $i -lt 100000 ]; do LIM=$(sudo $RUNC exec bionic sh -c "ulimit -n"); if [ $LIM -ne 65536 ]; then echo "WHOOPSIE (iter $i) got numfile $LIM"; break; fi; ((i%100==0)) && printf "[%s] %10d\n" "$(date)" "$i"; let i++; done
runc version 1.1.0+dev
commit: v1.1.0-994-g4641f17e-dirty
spec: 1.2.0
go: go1.21.7
libseccomp: 2.5.1
[Tue 02 Apr 2024 10:10:36 PM PDT]          0
[Tue 02 Apr 2024 10:10:38 PM PDT]        100
...
[Tue 02 Apr 2024 10:23:23 PM PDT]      42300
WHOOPSIE (iter 42333) got numfile 1048576

So there is still some race going on :(

kolyshkin · 2024-04-03T05:26:14Z

WHOOPSIE (iter 42333) got numfile 1048576
So there is still some race going on :(

Note the number is now as go runtime sets it for itself.

cyphar

With these changes, I would be okay with taking something like this as a hotfix but IMHO this is clearly a Go stdlib bug. We use prlimit(2) to set another process's rlimits (to avoid permission issues) and it's not reasonable for Go to assume that this won't happen.

libcontainer/utils/utils.go

kolyshkin · 2024-04-05T00:00:19Z

Most importantly, this fix does not work :-\

ls-ggg · 2024-04-12T08:40:22Z

OK, I took a deeper look and in fact this is already fixed in the main branch via #3813, which brings in this change: https://go-review.googlesource.com/c/sys/+/476695

So, to my best understanding, this PR is not needed.

For release-1.1 branch to fix this, we need something like #3813.

@kolyshkin
The #3813 cannot fix it. the correct way is to clear the cache in the runtime.

cyphar · 2024-04-17T23:35:51Z

libcontainer/setns_init_linux.go

+	rlimit := syscall.Rlimit{}
+	if err := syscall.Getrlimit(syscall.RLIMIT_NOFILE, &rlimit); err != nil {
+		return err
+	}
+	if err := syscall.Setrlimit(syscall.RLIMIT_NOFILE, &rlimit); err != nil {
+		return err
+	}


As I suggested in the earlier review, for correctness reasons, I think

Suggested change

rlimit := syscall.Rlimit{}

if err := syscall.Getrlimit(syscall.RLIMIT_NOFILE, &rlimit); err != nil {

return err

}

if err := syscall.Setrlimit(syscall.RLIMIT_NOFILE, &rlimit); err != nil {

return err

}

for _, rlimit := range l.config.Rlimits {

if rlimit.Type == unix.RLIMIT_NOFILE {

if err := unix.Setrlimit(rlimit.Type, &unix.Rlimit{

Cur: rlimit.Soft,

Max: rlimit.Hard,

}); err != nil {

return fmt.Errorf("failed to re-apply nofile rlimit: %w", err)

}

}

}

makes more sense.

Yes, the proposed implementation is better

But running the unit test failed and I encountered an error:
=== RUN TestInitJoinNetworkAndUser
exec_test.go:1543: unexpected error: unable to start container process: error during container init: failed to re-apply nofile rlimit: operation not permitte

My code just clears the cache in go runtime and has no meaning to the operating system.

Just curious about 2 things:

I think the code provided by you and cyphar have the same effect, but I don't know why cyphar's code will be fail, could you please give some explainations? If yes, it will be appreciated.

If I understand right, there is no similar bug for runc run, this bug is only in runc exec? If yes, in setns_init_linux, I think we can use procReady msg to tell the parent process do Setrlimit, like what is doing in standard_init_linux.

2. like what is doing in standard_init_linux.

runc/libcontainer/process_linux.go

Lines 775 to 781 in f9f8abf

case procReady:

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)

}

As reported in issue opencontainers#4195, the new version of go runtime will cache rlimit-nofile. before executing exec, the rlimit-nofile of the process will be updated with the cache. in runc, this will cause the rlimit-nofile set by the parent process for the container to become invalid. this can be solved by clearing the cache. Signed-off-by: ls-ggg <335814617@qq.com>

ls-ggg · 2024-04-28T07:42:57Z

Can this mr be approved?

kolyshkin · 2024-04-29T21:14:44Z

Can this mr be approved?

As I said a month ago, this fix does not work.

ls-ggg force-pushed the fix-rlimit-nofile-wrong branch from 8e3496c to 63ebcf7 Compare March 29, 2024 10:34

AkihiroSuda approved these changes Apr 1, 2024

View reviewed changes

AkihiroSuda added the backport/todo/1.1 A PR in main branch which needs to be backported to release-1.1 label Apr 1, 2024

ls-ggg force-pushed the fix-rlimit-nofile-wrong branch 4 times, most recently from b275b24 to 83eebdf Compare April 2, 2024 05:09

ls-ggg force-pushed the fix-rlimit-nofile-wrong branch from 83eebdf to d0dbf90 Compare April 2, 2024 05:33

kolyshkin closed this Apr 2, 2024

kolyshkin mentioned this pull request Apr 3, 2024

racy RLIMIT_NOFILE setting with Go 1.19+ #4195

Open

kolyshkin reopened this Apr 3, 2024

kolyshkin requested changes Apr 3, 2024

View reviewed changes

cyphar requested changes Apr 3, 2024

View reviewed changes

libcontainer/utils/utils.go Outdated Show resolved Hide resolved

libcontainer/utils/utils.go Outdated Show resolved Hide resolved

ls-ggg force-pushed the fix-rlimit-nofile-wrong branch 2 times, most recently from d2c509f to bbb035c Compare April 12, 2024 16:18

ls-ggg requested review from kolyshkin and cyphar April 12, 2024 16:20

cyphar reviewed Apr 17, 2024

View reviewed changes

ls-ggg force-pushed the fix-rlimit-nofile-wrong branch from bbb035c to 28c5edd Compare April 18, 2024 02:21

ls-ggg force-pushed the fix-rlimit-nofile-wrong branch from 28c5edd to fa72652 Compare April 18, 2024 02:22

ls-ggg requested a review from cyphar April 18, 2024 02:26

ls-ggg force-pushed the fix-rlimit-nofile-wrong branch from fa72652 to 2b33989 Compare April 18, 2024 02:31

ls-ggg force-pushed the fix-rlimit-nofile-wrong branch from 2b33989 to f9f8abf Compare April 18, 2024 03:14

lifubang mentioned this pull request Apr 30, 2024

Fix set rlimit nofile race #4265

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

clean cached rlimit nofile in go runtime #4237

clean cached rlimit nofile in go runtime #4237

ls-ggg commented Mar 29, 2024 •

edited

kolyshkin commented Apr 2, 2024

ls-ggg commented Apr 2, 2024 •

edited

kolyshkin commented Apr 2, 2024

kolyshkin commented Apr 2, 2024

kolyshkin commented Apr 2, 2024

kolyshkin commented Apr 3, 2024

kolyshkin commented Apr 3, 2024

kolyshkin left a comment

kolyshkin commented Apr 3, 2024

kolyshkin commented Apr 3, 2024

cyphar left a comment

kolyshkin commented Apr 5, 2024

ls-ggg commented Apr 12, 2024

cyphar Apr 17, 2024

ls-ggg Apr 18, 2024

ls-ggg Apr 18, 2024

lifubang Apr 28, 2024

lifubang Apr 28, 2024

ls-ggg commented Apr 28, 2024

kolyshkin commented Apr 29, 2024

-	rlimit := syscall.Rlimit{}
-	if err := syscall.Getrlimit(syscall.RLIMIT_NOFILE, &rlimit); err != nil {
-		return err
-	}
-	if err := syscall.Setrlimit(syscall.RLIMIT_NOFILE, &rlimit); err != nil {
-		return err
-	}
+	for _, rlimit := range l.config.Rlimits {
+		if rlimit.Type == unix.RLIMIT_NOFILE {
+			if err := unix.Setrlimit(rlimit.Type, &unix.Rlimit{
+				Cur: rlimit.Soft,
+				Max: rlimit.Hard,
+			}); err != nil {
+				return fmt.Errorf("failed to re-apply nofile rlimit: %w", err)
+			}
+		}
+	}

	case procReady:
	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)
	}

clean cached rlimit nofile in go runtime #4237

Are you sure you want to change the base?

clean cached rlimit nofile in go runtime #4237

Conversation

ls-ggg commented Mar 29, 2024 • edited

kolyshkin commented Apr 2, 2024

ls-ggg commented Apr 2, 2024 • edited

kolyshkin commented Apr 2, 2024

kolyshkin commented Apr 2, 2024

kolyshkin commented Apr 2, 2024

kolyshkin commented Apr 3, 2024

kolyshkin commented Apr 3, 2024

kolyshkin left a comment

Choose a reason for hiding this comment

kolyshkin commented Apr 3, 2024

kolyshkin commented Apr 3, 2024

cyphar left a comment

Choose a reason for hiding this comment

kolyshkin commented Apr 5, 2024

ls-ggg commented Apr 12, 2024

cyphar Apr 17, 2024

Choose a reason for hiding this comment

ls-ggg Apr 18, 2024

Choose a reason for hiding this comment

ls-ggg Apr 18, 2024

Choose a reason for hiding this comment

lifubang Apr 28, 2024

Choose a reason for hiding this comment

lifubang Apr 28, 2024

Choose a reason for hiding this comment

ls-ggg commented Apr 28, 2024

kolyshkin commented Apr 29, 2024

ls-ggg commented Mar 29, 2024 •

edited

ls-ggg commented Apr 2, 2024 •

edited