Repository: cilium/ebpf
Pull request #392 (2021-08-31 05:05:50)

Utility function to remove memlock rlimit
This should implement the suggestion in https://github.com/cilium/ebpf/issues/290 if I understood it correctly.

Wasn't sure what package this would best live in, let me know if there's a better location/name.

Going to open another PR to update the examples once (if) this gets merged.

florianl: Personal opinion, but I think it is not a good idea to set limits to infinity without the option to set them to their original values. Rlimits often need to be increased to load eBPF resources but should then be set again to the original value, I think.
lmb: @florianl I think in practice everyone just bumps RLIMIT_MEMLOCK unconditionally. We could return the original memlock limit from the function, to enable users to reset the limit themselves. But then they can just call the syscall directly, I would think?

What I wonder: should this function be `RemoveMemlockLimitIfNecessary` (needs a better name), and only bump MEMLOCK if on a kernel that doesn't have memcg accounting? Caveat: I don't know how to detect whether a kernel has memcg accounting or not.
florianl: Maybe my context is different, but I think it is good practice to only increase RLimit while loading eBPF resources.

Staying with `RLIM_INFINITY` might lead to resource exhaustion or resource scaling issues.

E.g. cilium/cilium also follows this practice:
https://github.com/cilium/cilium/blob/1695d9c59ac4e78b5a02a96e83a57ec07ddbaa7f/pkg/bpf/bpf_linux.go#L633-L644
lmb: I don't think that is true in general for the cilium agent: https://github.com/cilium/cilium/blob/1695d9c59ac4e78b5a02a96e83a57ec07ddbaa7f/daemon/cmd/daemon.go#L340 I shared your hesitancy re. RLIMIT_MEMLOCK, but it's what we've been using in production for several years without adverse effects. Do know of cases where this caused a problem?
ti-mo: Although still potentially racy, I think returning a function that can be used to reset the rlimit to its value before the infinity bump could make sense here. The caller remains in control, and they're free to ignore the function. This would be rather convenient, and maintains rlimit's footgun removal feature. :slightly_smiling_face: 
florianl: Monitoring `RLIMIT_MEMLOCK` can indicate unintended memory leaks and/or trigger spawning of instances. Depending how the environment is used and configured.
So if a package needs to increase it temporary, I think it would be nice to provide a way to set it to its original values afterwards.
folbricht: Thanks, I updated it to return a function that resets the value as suggested. Any thoughts on a good name for it? Would you prefer `IncreaseRlimit()`?
lmb: > Would you prefer IncreaseRlimit()?

Also nice, both work for me.

> What I wonder: should this function be RemoveMemlockLimitIfNecessary (needs a better name), and only bump MEMLOCK if on a kernel that doesn't have memcg accounting? Caveat: I don't know how to detect whether a kernel has memcg accounting or not.

Do you have thoughts on this?



folbricht: I wasn't able to find a way to detect if memcg is available. Found https://lore.kernel.org/bpf/CAEf4BzYe1SDnWBzAP_U7NtUhu_cWa0EEMLv+d-q3YTFvP+y3og@mail.gmail.com/ which discussed the same issue but also didn't come up with a solution. The best alternative I can think of at the moment would be to simply document this in the doc string of the function, saying something along the way of "not necessary on kernels 5.11+ (?)" 
folbricht: Thank you

- Updated to use `github.com/cilium/ebpf/internal/unix`
- `internal/testutils/rlimit.go:init()` uses it too now

TODO (in a separate PR) is to update what's under `examples/`. There was another PR that requires updating the examples anyway.
lmb: Works for me. @ti-mo?
ti-mo: I've also found [`prlimit(2)`](https://man7.org/linux/man-pages/man2/prlimit.2.html) on Linux that can perform this operation atomically, which would improve the safety of this call. This isn't exposed in the Go stdlib, though, but there's a playground snippet floating around. I'll create an issue.

Patch:
diff --git a/internal/unix/types_linux.go b/internal/unix/types_linux.go
index edd9242..9d204ee 100644
--- a/internal/unix/types_linux.go
+++ b/internal/unix/types_linux.go
@@ -5,6 +5,7 @@ package unix
 
 import (
 	"bytes"
+	"fmt"
 	"syscall"
 
 	linux "golang.org/x/sys/unix"
@@ -71,11 +72,6 @@ type Statfs_t = linux.Statfs_t
 // Rlimit is a wrapper
 type Rlimit = linux.Rlimit
 
-// Setrlimit is a wrapper
-func Setrlimit(resource int, rlim *Rlimit) (err error) {
-	return linux.Setrlimit(resource, rlim)
-}
-
 // Syscall is a wrapper
 func Syscall(trap, a1, a2, a3 uintptr) (r1, r2 uintptr, err syscall.Errno) {
 	return linux.Syscall(trap, a1, a2, a3)
@@ -204,3 +200,24 @@ func KernelRelease() (string, error) {
 	release := string(uname.Release[:end])
 	return release, nil
 }
+
+func RemoveMemlockRlimit() (func() error, error) {
+	oldLimit := new(Rlimit)
+	if err := linux.Getrlimit(RLIMIT_MEMLOCK, oldLimit); err != nil {
+		return nil, fmt.Errorf("failed to get memlock rlimit: %w", err)
+	}
+
+	if err := linux.Setrlimit(RLIMIT_MEMLOCK, &Rlimit{
+		Cur: RLIM_INFINITY,
+		Max: RLIM_INFINITY,
+	}); err != nil {
+		return nil, fmt.Errorf("failed to set memlock rlimit: %w", err)
+	}
+
+	return func() error {
+		if err := linux.Setrlimit(RLIMIT_MEMLOCK, oldLimit); err != nil {
+			return fmt.Errorf("failed to reset memlock rlimit: %w", err)
+		}
+		return nil
+	}, nil
+}
diff --git a/internal/unix/types_other.go b/internal/unix/types_other.go
index ef414e1..4245eb9 100644
--- a/internal/unix/types_other.go
+++ b/internal/unix/types_other.go
@@ -88,11 +88,6 @@ type Rlimit struct {
 	Max uint64
 }
 
-// Setrlimit is a wrapper
-func Setrlimit(resource int, rlim *Rlimit) (err error) {
-	return errNonLinux
-}
-
 // Syscall is a wrapper
 func Syscall(trap, a1, a2, a3 uintptr) (r1, r2 uintptr, err syscall.Errno) {
 	return 0, 0, syscall.Errno(1)
@@ -263,3 +258,7 @@ func Renameat2(olddirfd int, oldpath string, newdirfd int, newpath string, flags
 func KernelRelease() (string, error) {
 	return "", errNonLinux
 }
+
+func RemoveMemlockRlimit() (func() error, error) {
+	return nil, errNonLinux
+}
diff --git a/map.go b/map.go
index bf854e3..546ef1b 100644
--- a/map.go
+++ b/map.go
@@ -197,7 +197,7 @@ func NewMap(spec *MapSpec) (*Map, error) {
 //
 // The caller is responsible for ensuring the process' rlimit is set
 // sufficiently high for locking memory during map creation. This can be done
-// by calling unix.Setrlimit with unix.RLIMIT_MEMLOCK prior to calling NewMapWithOptions.
+// by calling ebpf.RemoveMemlockRlimit() prior to calling NewMapWithOptions.
 //
 // May return an error wrapping ErrMapIncompatible.
 func NewMapWithOptions(spec *MapSpec, opts MapOptions) (*Map, error) {
diff --git a/syscalls.go b/syscalls.go
index f8cb5f0..cb38e88 100644
--- a/syscalls.go
+++ b/syscalls.go
@@ -17,6 +17,16 @@ import (
 // Deprecated: use os.ErrNotExist instead.
 var ErrNotExist = os.ErrNotExist
 
+// RemoveMemlockRlimit removes the limit on the amount of memory the current
+// process can lock into RAM. Returns a function that restores the limit to
+// its previous value.
+//
+// This is not required to load eBPF resources on kernel versions 5.11+
+// due to the introduction of cgroup-based memory accounting.
+func RemoveMemlockRlimit() (func() error, error) {
+	return unix.RemoveMemlockRlimit()
+}
+
 // invalidBPFObjNameChar returns true if char may not appear in
 // a BPF object name.
 func invalidBPFObjNameChar(char rune) bool {


Test patch:
diff --git a/internal/testutils/rlimit.go b/internal/testutils/rlimit.go
index 3c4e754..58ae784 100644
--- a/internal/testutils/rlimit.go
+++ b/internal/testutils/rlimit.go
@@ -10,11 +10,7 @@ import (
 func init() {
 	// Increase the memlock for all tests unconditionally. It's a great source of
 	// weird bugs, since different distros have different default limits.
-	err := unix.Setrlimit(unix.RLIMIT_MEMLOCK, &unix.Rlimit{
-		Cur: unix.RLIM_INFINITY,
-		Max: unix.RLIM_INFINITY,
-	})
-	if err != nil {
+	if _, err := unix.RemoveMemlockRlimit(); err != nil {
 		fmt.Fprintln(os.Stderr, "WARNING: Failed to adjust rlimit, tests may fail")
 	}
 }