[1.1] Fix set nofile rlimit error #4277

lifubang · 2024-05-09T11:02:08Z

This is backport of #4268, #4265.

1. Fix a get/set race between `runc exec` and `syscall.rlimit.init()`

As @ls-ggg has given the detailed steps to reproduce the issue #4195 , and has given the core reason is that the go runtime will cache the value of rlimit nofile. [1]
When we are running runc exec with nofile limit, runc set it in the parent process, it may cause a race with go runtime init. The race condition is in the time when runc parent process set the container child process's nofile limit just after go runtime init has fetched the nofile limit. [2]
So, we should set nofile limit after syscall.rlimit.init() completed.

2. Fix an edge case caused by nofile rlimit cache in go stdlib

As @kolyshkin have found an edge case for set nofile rlimit by runc, which is also caused by nofile rlimit cache in go stdlib. Although we have a way to resolve the above get/set race, but if the hard value of nofile rlimit configured in config.json is bigger than this value of runc create/run/exec, it will also be incorrect restored by syscall.Exec in runc init. So we need to clear the nofile rlimit cache before we start the container initial process if we need to set nofile rlimit for the container.

[1] golang/go@f5eef58#diff-ec665e9789f8cf5cd1828ad7fa9f0ff4ebc1f5b5dd0fc82a296da5c07da7ece6
[2] https://github.com/golang/go/blob/f5eef58e4381259cbd84b3f2074c79607fb5c821/src/syscall/rlimit.go#L34-L35

CHANGELOG:
deprecated: libct.system.Execv is not used in runc anymore, it will be removed in v1.2.0.

lifubang · 2024-05-09T11:56:36Z

Note that we don’t support go version less than 1.19 anymore once we merge this PR.

kolyshkin · 2024-05-09T20:28:32Z

Note that we don’t support go version less than 1.19 anymore once we merge this PR.

This is a questionable decision. I think what we should do instead in 1.1 is add two versions of func ClearRlimitNofileCache -- one for Go 1.19+ which does what it does now, and the other is for !go1.19, which does nothing.

lifubang · 2024-05-10T10:49:30Z

I think what we should do instead in 1.1 is add two versions of func ClearRlimitNofileCache -- one for Go 1.19+ which does what it does now, and the other is for !go1.19, which does nothing.

Thanks your suggestion.

kolyshkin · 2024-05-10T18:18:07Z

libcontainer/system/rlimit_stub.go

+// 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.


No need to repeat the comment I guess.

libcontainer/system/rlimit_g119.go

kolyshkin · 2024-05-10T18:20:29Z

@lifubang thanks for the changes! Can you tell why are you still bumping go version?

lifubang · 2024-05-10T23:38:54Z

The complete generic features are supported since go 1.18

The complete generic features are supported since go 1.18, because go 1.19 and above need this change.

kolyshkin · 2024-05-13T22:51:16Z

The complete generic features are supported since go 1.18

The complete generic features are supported since go 1.18, because go 1.19 and above need this change.

I'm sorry, I still don't understand what do you mean :(

This code works with Go 1.17 AFAICS. Can you explain why you changed go version to 1.18 in go.mod in the last commit?

lifubang · 2024-05-14T00:13:12Z

This code works with Go 1.17 AFAICS.

Yes, this works with go 1.17, but if we use go 1.19, because of the ‘go 1.17’ defined in ‘go.mod’, it can’t work.

kolyshkin · 2024-05-16T02:18:58Z

Ah, I see! This is what happens without the commit bumping the go version in go.mod:

[kir@kir-rhat runc]$ make all GO=go1.20
go1.20 build -trimpath "-buildmode=pie"  -tags "seccomp" -ldflags "-X main.gitCommit=v1.1.12-21-ge36b3ae7 -X main.version=1.1.12+dev " -o runc .
# github.com/opencontainers/runc/libcontainer/system
libcontainer/system/rlimit_go119.go:13:43: type instantiation requires go1.18 or later (-lang was set to go1.17; check go.mod)
make: *** [Makefile:61: runc] Error 1

So, despite libcontainer/system/rlimit_go119.go being guarded with //go:build go1.19, go build still complains. To me this looks like a bug, and there's an issue opened about this (golang/go#52880), but it looks like it won't be fixed.

This is why the go version bump is needed.

kolyshkin

LGTM

thaJeztah · 2024-05-16T08:12:28Z

So, despite libcontainer/system/rlimit_go119.go being guarded with //go:build go1.19, go build still complains. To me this looks like a bug, and there's an issue opened about this (golang/go#52880), but it looks like it won't be fixed.

This is why the go version bump is needed.

Wait, so we're using go1.20, but the go version in go.mod causes it to downgrade to language version 1.17, but ... then it does not use the < go1.19 fallback (through //go:build constraints) implementation? So what's the value of go build-constraints now if the only version used is the version specified in go.mod, and consumers of the module MUST be using that version (or above)? Does that mean it's no longer possible to provide fallback implementations?

This stuff is getting more and more confusing.

thaJeztah

Curious why we're back porting this one;

Remove unused system.Execv #4268

Technically that's a breaking change so not something were could include in a patch release. Perhaps we're not using it ourselves (so it would be "dead code" for us), but someone using the module could be using it.

I couldn't find external consumers, but if we have no strong reason, perhaps we should skip it to reduce the diff for path releases?

libcontainer/system/rlimit_go119.go

lifubang · 2024-05-16T10:38:38Z

Curious why we're back porting this one;

Remove unused system.Execv #4268

Technically that's a breaking change so not something were could include in a patch release. Perhaps we're not using it ourselves (so it would be "dead code" for us), but someone using the module could be using it.

I couldn't find external consumers, but if we have no strong reason, perhaps we should skip it to reduce the diff for path releases?

I agree. In fact we have no strong reason to back port this commit, we just only need a comment change from this commit.
I'll remove this backport later.
Thanks.

thaJeztah · 2024-05-16T10:57:04Z

Thanks! Yes, it's probably fine, but if we don't have a strong need, we may as well skip it (just in case).

thaJeztah · 2024-05-16T10:59:04Z

^ I'd be fine adding a Deprecated: comment on it (for the 1.1 branch) if we want to raise visibility for it going away. I think there's some other exported functions that we removed in 1.2 as well, and I guess every bit could help to raise awareness.

lifubang · 2024-05-16T11:23:37Z

Wait, so we're using go1.20, but the go version in go.mod causes it to downgrade to language version 1.17, but ... then it does not use the < go1.19 fallback (through //go:build constraints) implementation?

No, because go version still be go1.20.

There are two explanations related to go directive in go.mod:

The go directive affects use of new language features
This means that if we using go 1.19 to compile runc, we can’t use any new language features provided after 1.17, for example generic type;
Before Go 1.21, the directive was advisory only
This means that we can support go 1.17 like before.

Refer:
https://go.dev/doc/modules/gomod-ref#go

This is not used since commit dac4171. It will be removed in v1.2.0 Signed-off-by: Kir Kolyshkin <[email protected]> (cherry picked from commit dbd0c33) Signed-off-by: lifubang <[email protected]>

Do not refer to the function which was removed. Signed-off-by: Kir Kolyshkin <[email protected]> (cherry picked from commit bac5064) Signed-off-by: lifubang <[email protected]>

Issue: opencontainers#4195 Since https://go-review.googlesource.com/c/go/+/476097, there is a get/set race between runc exec and syscall.rlimit.init, so we need to call setupRlimits after syscall.rlimit.init() completed. Signed-off-by: lifubang <[email protected]> (cherry picked from commit a853a82) Signed-off-by: lifubang <[email protected]>

lifubang · 2024-05-16T14:14:53Z

^ I'd be fine adding a Deprecated: comment on it (for the 1.1 branch) if we want to raise visibility for it going away. I think there's some other exported functions that we removed in 1.2 as well, and I guess every bit could help to raise awareness.

Switched to this suggection, make it deprecated in 1.1 .

kolyshkin · 2024-05-17T00:58:03Z

A single nit: I think libct: clean cached rlimit nofile in go runtime should come AFTER use go 1.18 in go.mod, as otherwise we're breaking git-bisect.

The complete generic features are supported since go 1.18 Signed-off-by: lfbzhm <[email protected]>

As reported in issue opencontainers#4195, the new version(since 1.19) of go runtime 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 set by the parent process for the container to become invalid. It can be solved by clearing the cache. Signed-off-by: ls-ggg <[email protected]> (cherry picked from commit f9f8abf) Signed-off-by: lifubang <[email protected]> (cherry picked from commit da68c8e) Signed-off-by: lifubang <[email protected]>

issues: opencontainers#4195 opencontainers#4265 (comment) Signed-off-by: lifubang <[email protected]> (cherry picked from commit 4ea0bf8) Signed-off-by: lfbzhm <[email protected]>

Signed-off-by: lifubang <[email protected]> (cherry picked from commit a35f7d8) Signed-off-by: lifubang <[email protected]>

lifubang · 2024-05-17T10:42:42Z

A single nit: I think libct: clean cached rlimit nofile in go runtime should come AFTER use go 1.18 in go.mod, as otherwise we're breaking git-bisect.

done

AkihiroSuda · 2024-05-17T17:46:18Z

go.mod

@@ -1,6 +1,6 @@
 module github.com/opencontainers/runc

-go 1.17
+go 1.18


(Ideally we should not bump up Go version in backport releases, but it's not a huge deal, as Go 1.17 has already reached EOL)

This is needed here because we rely on a feature that is only available since Go 1.18, and despite the feature being guarded by go1.20 build tag, this bump is still required (see #4277 (comment) above).

Nevertheless, go 1.17 can still be used just fine to compile this (as you can see in CI).

All that doesn't make much sense to me either, but Go developers seem to disagree (golang/go#52880).

v1.1.13 -- "There is no certainty in the world. This is the only certainty I have." This is the thirteenth patch release in the 1.1.z release branch of runc. It brings in Go 1.12.x compatibility and fixes a few issues, including an occasional wrong nofile rlimit in runc exec, and a race between runc list and runc delete. NOTE that if using Go 1.22.x to build runc, make sure to use 1.22.4 or a later version. For more details, see issue opencontainers#4233. * Support go 1.22.4+. (opencontainers#4313) * runc list: fix race with runc delete. (opencontainers#4231) * Fix set nofile rlimit error. (opencontainers#4277, opencontainers#4299) * libct/cg/fs: fix setting rt_period vs rt_runtime. (opencontainers#4284) * Fix a debug msg for user ns in nsexec. (opencontainers#4315) * script/*: fix gpg usage wrt keyboxd. (opencontainers#4316) * CI fixes and misc backports. (opencontainers#4241) * Fix codespell warnings. (opencontainers#4300) * Silence security false positives from golang/net. (opencontainers#4244) * libcontainer: allow containers to make apps think fips is enabled/disabled for testing. (opencontainers#4257) * allow overriding VERSION value in Makefile. (opencontainers#4270) * Vagrantfile.fedora: bump Fedora to 39. (opencontainers#4261) * ci/cirrus: rm centos stream 8. (opencontainers#4305, opencontainers#4308) Thanks to all of the contributors who made this release possible: * Akhil Mohan <[email protected]> * Akihiro Suda <[email protected]> * Aleksa Sarai <[email protected]> * Kir Kolyshkin <[email protected]> * Sohan Kunkerkar <[email protected]> * TTFISH <[email protected]> * kychen <[email protected]> * lifubang <[email protected]> * ls-ggg <[email protected]> Signed-off-by: Kir Kolyshkin <[email protected]> # -----BEGIN PGP SIGNATURE----- # # iQEzBAABCAAdFiEEwkKM11cg+s3PdrbqF95ey3WhEA4FAmZrFGYACgkQF95ey3Wh # EA7DPwf9HVwO0EO3s7OuJPBCmZBp92L6AMDBmkpnE14Pi1c4DVcWtlrBna2CNnUJ # 4Hu8rgEtT80Y8L3GBf96Wo3C1DHR6lG6dyu6FjHozWu97WfrTtw92I/254dQZnsr # i7m+5C6Tluewr9pH6ageRI0rRYt4QPpyRihMkiZQHl44Z5ogRGJvCCkjk9nIDlxi # ok2U5aPIw4NWPwnMg3wC6CmcviaM81kyuWh2Twc1OPwRilCPQXWblcUgqujg5tOr # C3Z6AwiIedpMt6Nr0jdWZh9Rh0ffuOXBEiUO/K8vYqE/eDvqJd42c8ALi1HOONoU # ZwrNWNU3o2pIQ4qz0Fs4vauK4wSs1A== # =IFN9 # -----END PGP SIGNATURE----- # gpg: Signature made Thu Jun 13 08:46:46 2024 PDT # gpg: using RSA key C2428CD75720FACDCF76B6EA17DE5ECB75A1100E # gpg: Can't check signature: No public key # Conflicts: # CHANGELOG.md # VERSION # go.mod # go.sum # vendor/golang.org/x/sys/unix/mmap_nomremap.go # vendor/golang.org/x/sys/windows/syscall_windows.go # vendor/modules.txt

v1.1.13 -- "There is no certainty in the world. This is the only certainty I have." This is the thirteenth patch release in the 1.1.z release branch of runc. It brings in Go 1.12.x compatibility and fixes a few issues, including an occasional wrong nofile rlimit in runc exec, and a race between runc list and runc delete. NOTE that if using Go 1.22.x to build runc, make sure to use 1.22.4 or a later version. For more details, see issue opencontainers#4233. * Support go 1.22.4+. (opencontainers#4313) * runc list: fix race with runc delete. (opencontainers#4231) * Fix set nofile rlimit error. (opencontainers#4277, opencontainers#4299) * libct/cg/fs: fix setting rt_period vs rt_runtime. (opencontainers#4284) * Fix a debug msg for user ns in nsexec. (opencontainers#4315) * script/*: fix gpg usage wrt keyboxd. (opencontainers#4316) * CI fixes and misc backports. (opencontainers#4241) * Fix codespell warnings. (opencontainers#4300) * Silence security false positives from golang/net. (opencontainers#4244) * libcontainer: allow containers to make apps think fips is enabled/disabled for testing. (opencontainers#4257) * allow overriding VERSION value in Makefile. (opencontainers#4270) * Vagrantfile.fedora: bump Fedora to 39. (opencontainers#4261) * ci/cirrus: rm centos stream 8. (opencontainers#4305, opencontainers#4308) Thanks to all of the contributors who made this release possible: * Akhil Mohan <[email protected]> * Akihiro Suda <[email protected]> * Aleksa Sarai <[email protected]> * Kir Kolyshkin <[email protected]> * Sohan Kunkerkar <[email protected]> * TTFISH <[email protected]> * kychen <[email protected]> * lifubang <[email protected]> * ls-ggg <[email protected]> Signed-off-by: Kir Kolyshkin <[email protected]> # -----BEGIN PGP SIGNATURE----- # # iQEzBAABCAAdFiEEwkKM11cg+s3PdrbqF95ey3WhEA4FAmZrFGYACgkQF95ey3Wh # EA7DPwf9HVwO0EO3s7OuJPBCmZBp92L6AMDBmkpnE14Pi1c4DVcWtlrBna2CNnUJ # 4Hu8rgEtT80Y8L3GBf96Wo3C1DHR6lG6dyu6FjHozWu97WfrTtw92I/254dQZnsr # i7m+5C6Tluewr9pH6ageRI0rRYt4QPpyRihMkiZQHl44Z5ogRGJvCCkjk9nIDlxi # ok2U5aPIw4NWPwnMg3wC6CmcviaM81kyuWh2Twc1OPwRilCPQXWblcUgqujg5tOr # C3Z6AwiIedpMt6Nr0jdWZh9Rh0ffuOXBEiUO/K8vYqE/eDvqJd42c8ALi1HOONoU # ZwrNWNU3o2pIQ4qz0Fs4vauK4wSs1A== # =IFN9 # -----END PGP SIGNATURE----- # gpg: Signature made Thu Jun 13 08:46:46 2024 PDT # gpg: using RSA key C2428CD75720FACDCF76B6EA17DE5ECB75A1100E # gpg: Can't check signature: No public key

lifubang added the backport/1.1-pr A backport PR to release-1.1 label May 9, 2024

kolyshkin added this to the 1.1.13 milestone May 9, 2024

lifubang force-pushed the backport-4265-nofilerlimit branch 4 times, most recently from 4adca11 to f67218a Compare May 10, 2024 10:48

kolyshkin reviewed May 10, 2024

View reviewed changes

libcontainer/system/rlimit_g119.go Outdated Show resolved Hide resolved

lifubang force-pushed the backport-4265-nofilerlimit branch 2 times, most recently from 7d7ef39 to cb01f3b Compare May 10, 2024 23:37

kolyshkin approved these changes May 16, 2024

View reviewed changes

thaJeztah reviewed May 16, 2024

View reviewed changes

libcontainer/system/rlimit_go119.go Outdated Show resolved Hide resolved

kolyshkin and others added 3 commits May 16, 2024 21:47

deprecate libct.system.Execv

debf52a

This is not used since commit dac4171. It will be removed in v1.2.0 Signed-off-by: Kir Kolyshkin <[email protected]> (cherry picked from commit dbd0c33) Signed-off-by: lifubang <[email protected]>

libct: fix a comment

fbddb71

Do not refer to the function which was removed. Signed-off-by: Kir Kolyshkin <[email protected]> (cherry picked from commit bac5064) Signed-off-by: lifubang <[email protected]>

lifubang force-pushed the backport-4265-nofilerlimit branch from cb01f3b to ed13815 Compare May 16, 2024 13:57

lifubang and others added 4 commits May 17, 2024 18:18

use go 1.18 in go.mod

42c2ab2

The complete generic features are supported since go 1.18 Signed-off-by: lfbzhm <[email protected]>

update/add some tests for rlimit

2992049

issues: opencontainers#4195 opencontainers#4265 (comment) Signed-off-by: lifubang <[email protected]> (cherry picked from commit 4ea0bf8) Signed-off-by: lfbzhm <[email protected]>

fix comments for ClearRlimitNofileCache

c918058

Signed-off-by: lifubang <[email protected]> (cherry picked from commit a35f7d8) Signed-off-by: lifubang <[email protected]>

lifubang force-pushed the backport-4265-nofilerlimit branch from ed13815 to c918058 Compare May 17, 2024 10:18

lifubang requested review from cyphar and AkihiroSuda May 17, 2024 10:43

AkihiroSuda reviewed May 17, 2024

View reviewed changes

AkihiroSuda approved these changes May 17, 2024

View reviewed changes

kolyshkin merged commit 9244703 into opencontainers:release-1.1 May 20, 2024
29 checks passed

lifubang mentioned this pull request Jun 10, 2024

Release 1.1.13 #4318

Merged

This was referenced Jun 29, 2024

Calling fakeroot (in an Archlinux container) hangs forever docker/for-mac#7331

Open

chore: remove patch version from go version in go.mod gofrs/flock#88

Closed

thaJeztah mentioned this pull request Aug 15, 2024

ci: fix GHA CI build matrix distribution/distribution#4436

Merged

lifubang deleted the backport-4265-nofilerlimit branch October 15, 2024 05:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[1.1] Fix set nofile rlimit error #4277

[1.1] Fix set nofile rlimit error #4277

lifubang commented May 9, 2024 •

edited

Loading

lifubang commented May 9, 2024

kolyshkin commented May 9, 2024 •

edited

Loading

lifubang commented May 10, 2024

kolyshkin May 10, 2024

kolyshkin commented May 10, 2024

lifubang commented May 10, 2024

kolyshkin commented May 13, 2024

lifubang commented May 14, 2024

kolyshkin commented May 16, 2024

kolyshkin left a comment

thaJeztah commented May 16, 2024

thaJeztah left a comment

lifubang commented May 16, 2024

thaJeztah commented May 16, 2024

thaJeztah commented May 16, 2024

lifubang commented May 16, 2024

lifubang commented May 16, 2024

kolyshkin commented May 17, 2024

lifubang commented May 17, 2024

AkihiroSuda May 17, 2024

kolyshkin May 20, 2024

[1.1] Fix set nofile rlimit error #4277

[1.1] Fix set nofile rlimit error #4277

Conversation

lifubang commented May 9, 2024 • edited Loading

This is backport of #4268, #4265.

1. Fix a get/set race between runc exec and syscall.rlimit.init()

2. Fix an edge case caused by nofile rlimit cache in go stdlib

lifubang commented May 9, 2024

kolyshkin commented May 9, 2024 • edited Loading

lifubang commented May 10, 2024

kolyshkin May 10, 2024

Choose a reason for hiding this comment

kolyshkin commented May 10, 2024

lifubang commented May 10, 2024

kolyshkin commented May 13, 2024

lifubang commented May 14, 2024

kolyshkin commented May 16, 2024

kolyshkin left a comment

Choose a reason for hiding this comment

thaJeztah commented May 16, 2024

thaJeztah left a comment

Choose a reason for hiding this comment

lifubang commented May 16, 2024

thaJeztah commented May 16, 2024

thaJeztah commented May 16, 2024

lifubang commented May 16, 2024

lifubang commented May 16, 2024

kolyshkin commented May 17, 2024

lifubang commented May 17, 2024

AkihiroSuda May 17, 2024

Choose a reason for hiding this comment

kolyshkin May 20, 2024

Choose a reason for hiding this comment

lifubang commented May 9, 2024 •

edited

Loading

1. Fix a get/set race between `runc exec` and `syscall.rlimit.init()`

kolyshkin commented May 9, 2024 •

edited

Loading