Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

clean cached rlimit nofile in go runtime #4237

Closed
wants to merge 1 commit into from
+49 −5
Closed

clean cached rlimit nofile in go runtime #4237

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 12 additions & 0 deletions libcontainer/setns_init_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"fmt"
"os"
"os/exec"
"syscall"

"github.com/opencontainers/selinux/go-selinux"
"github.com/sirupsen/logrus"
Expand Down Expand Up @@ -49,6 +50,17 @@ func (l *linuxSetnsInit) Init() error {
}
}
}

// Set RLIMIT_NOFILE again to clean the cache in go runtime
// The problem originates from https://github.com/golang/go/commit/f5eef58e4381259cbd84b3f2074c79607fb5c821
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
}
Comment on lines +56 to +62
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the proposed implementation is better

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious about 2 things:

  1. 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.
  2. 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
}


if l.config.CreateConsole {
if err := setupConsole(l.consoleSocket, l.config, false); err != nil {
return err
Expand Down
20 changes: 15 additions & 5 deletions libcontainer/standard_init_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,18 +5,18 @@ import (
"fmt"
"os"
"os/exec"

"github.com/opencontainers/runtime-spec/specs-go"
"github.com/opencontainers/selinux/go-selinux"
"github.com/sirupsen/logrus"
"golang.org/x/sys/unix"
"syscall"

"github.com/opencontainers/runc/libcontainer/apparmor"
"github.com/opencontainers/runc/libcontainer/configs"
"github.com/opencontainers/runc/libcontainer/keys"
"github.com/opencontainers/runc/libcontainer/seccomp"
"github.com/opencontainers/runc/libcontainer/system"
"github.com/opencontainers/runc/libcontainer/utils"
"github.com/opencontainers/runtime-spec/specs-go"
"github.com/opencontainers/selinux/go-selinux"
"github.com/sirupsen/logrus"
"golang.org/x/sys/unix"
)

type linuxStandardInit struct {
Expand Down Expand Up @@ -77,6 +77,16 @@ func (l *linuxStandardInit) Init() error {
}
}

// Set RLIMIT_NOFILE again to clean the cache in go runtime
// The problem originates from https://github.com/golang/go/commit/f5eef58e4381259cbd84b3f2074c79607fb5c821
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
}

if err := setupNetwork(l.config); err != nil {
return err
}
Expand Down
22 changes: 22 additions & 0 deletions tests/integration/resources.bats
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
#!/usr/bin/env bats

load helpers

function setup() {
setup_busybox
}

function teardown() {
teardown_bundle
}

@test "runc run with RLIMIT_NOFILE" {
update_config '.process.args = ["/bin/sh", "-c", "ulimit -n"]'
update_config '.process.capabilities.bounding = ["CAP_SYS_RESOURCE"]'
update_config '.process.rlimits = [{"type": "RLIMIT_NOFILE", "hard": 10000, "soft": 10000}]'

runc run test_hello
[ "$status" -eq 0 ]

[[ "${output}" == "10000" ]]
}