Utility function to remove memlock rlimit

[folbricht]

This should implement the suggestion in #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

If a program sets RLIM_INFINITY, loads eBPF resources (then usually pins it to a path) and exits - this is fine I think. But there are also use cases where programs load eBPF resources and continue processing. In these cases keeping RLIM_INFINITY is a problem I think. Here RLimit should be increased only temporary.

A pattern I'm used to for such cases is something like:

// IncreaseRlimit updates the memlock resource limit to RLIM_INFINITY.
// It returns a function to reset the resource limit to its original value or an error.
func IncreaseRlimit() (func() error , error) {..}

[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. 🙂

[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+ (?)"

[lmb]

Please import github.com/cilium/ebpf/internal/unix instead, that contains dummy definitions so that working on the code base works on non-linux OS.

[folbricht]

Good call, I had no idea there was one. Done.

[lmb]

Thanks for checking, that is useful context to have!

[ti-mo]

Thanks! Good to go when internal/unix is imported, the necessary symbols are already there.

[ti-mo]

So sorry, thought of one more quick change after I clicked submit: could we also point the rlimit code in internal/testutils/rlimit.go:init() to this function instead? That way, we get test coverage for free.

Will require moving this function to internal/unix and adding a small wrapper or alias in package ebpf.

[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 squashed the commits. Also found a typo, removed unix.Setrlimit and moved the wrapper to syscalls.go, didn't really warrant its own file.

[ti-mo]

I've also found prlimit(2) 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.