rlimit: avoid performing the test and setting limit if its infinite

[RonFed]

If the current rlimit is infinity, raising it will be a no-op, so we can skip the init test and the later potential rlimit syscall to raise it to infinity.

[ti-mo]

@RonFed What problem does this solve? This defeats the purpose of the probe. Rlimit being set to infinity doesn't mean the kernel has memcg accounting.

[RonFed]

@RonFed What problem does this solve? This defeats the purpose of the probe. Rlimit being set to infinity doesn't mean the kernel has memcg accounting.

The main motivation is to avoid CAP_SYS_RESOURCE when it's not required. Performing the rlimit raise requires this capability, and if the process already runs with an infinite memlock limit — for example, via LimitMEMLOCK=infinity in a systemd unit file — it's not needed. If the kernel has memcg accounting the behavior is unchanged (no-op either way); if it doesn't, RemoveMemlock is already a no-op since the limit is infinite. This acts as an early detection of that case without requiring unnecessary privileges.

[ti-mo]

Okay, but your proposed fix still disables the probe, which is not what you'd want under any circumstance. There's also no guarantee that code running in between the probe and the first BPF() syscall doesn't lower rlimit. The import side effect of package rlimit doesn't crash or otherwise interfere with the Go program, and its result is memoized until for RemoveMemlock.

Your program should tolerate errors from RemoveMemlock in order to run successfully without CAP_SYS_RESOURCE. It doesn't seem like we've documented the fact that the returned error wraps an ErrNotSupported, though. Maybe that could be changed.

[RonFed]

Okay, but your proposed fix still disables the probe, which is not what you'd want under any circumstance.

Can you please elaborate on that? As I understand it, the memcg probing is an internal detail which is used to determine whether we need to remove the limit or not - "is the kernel using memcg` is not part of the api of this package - so if we reach the same end conclusion (not raising the limit) without requiring extra capabilities, it sounds ok to me.

There's also no guarantee that code running in between the probe and the first BPF() syscall doesn't lower rlimit.

This is always correct, no? regardless of the suggested addition?

Your program should tolerate errors from RemoveMemlock in order to run successfully without CAP_SYS_RESOURCE. It doesn't seem like we've documented the fact that the returned error wraps an ErrNotSupported, though. Maybe that could be changed.

I see your point, and it is doable - but wouldn't it be nice if the library would identify the situation where RemoveMemLock is not required - and handle this internally? similar to have it is done when memcg is used.
BTW, on one of my use cases I got unexpected error detecting memory cgroup accounting: permission denied, which based on the comment in the code is not expected.

[ti-mo]

Okay, but your proposed fix still disables the probe, which is not what you'd want under any circumstance.

Can you please elaborate on that? As I understand it, the memcg probing is an internal detail which is used to determine whether we need to remove the limit or not - "is the kernel using memcg` is not part of the api of this package - so if we reach the same end conclusion (not raising the limit) without requiring extra capabilities, it sounds ok to me.

Internal APIs should also do what they say on the tin, hence my pushback on disabling the probe. It will return a false positive if rlimit is set to infinite already.

There's also no guarantee that code running in between the probe and the first BPF() syscall doesn't lower rlimit.

This is always correct, no? regardless of the suggested addition?

My point is that the user can only be 'sure' of an infinite rlimit as soon as they've called RemoveMemlock() explicitly, since that triggers an action on their behalf. There may be other probes that try to fudge rlimit in a similar manner, or something may leave rlimit in an inconsistent state.

What you're suggesting introduces an extra possibility for a TOCTOU error. Bail out when infinite, and make RemoveMemlock do nothing, even if something could've lowered the rlimit in the meantime. The import side effect is specifically designed in a way to reduce the likelihood of a false positive probe conclusion. Bailing out when infinite doesn't provide the same guarantees.

Your program should tolerate errors from RemoveMemlock in order to run successfully without CAP_SYS_RESOURCE. It doesn't seem like we've documented the fact that the returned error wraps an ErrNotSupported, though. Maybe that could be changed.

I see your point, and it is doable - but wouldn't it be nice if the library would identify the situation where RemoveMemLock is not required - and handle this internally? similar to have it is done when memcg is used. BTW, on one of my use cases I got unexpected error detecting memory cgroup accounting: permission denied, which based on the comment in the code is not expected.

Unexpected means we bubble up to the user since we can't conclude anything. EPERM means the probe can't run, so the user will need to decide what to do with the error. If you run in an environment where setrlimit is not permitted, you'll obviously have to tolerate EPERM as well. The library should not make that assumption.

All in all, I only see one solution if you want to go to greater lengths to hide the error: another getrlimit() at the start of RemoveMemlock, and returning nil if infinite. That would mean get -> set -> bpf() -> set in init() and get -> set in RemoveMemlock(). I think that's even worse. 🙂 Better to ask for forgiveness than permission, since the latter always incurs TOCTOU risk.