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

Undefined (and potentially incorrect) behavior when pids limit is set to 0 #4014

Open
haircommander opened this issue Sep 12, 2023 · 3 comments · May be fixed by #4015 or #4023
Open

Undefined (and potentially incorrect) behavior when pids limit is set to 0 #4014

haircommander opened this issue Sep 12, 2023 · 3 comments · May be fixed by #4015 or #4023
Labels
kind/bug

Comments

@haircommander
Copy link
Contributor

Description

CRI-O sets its internal pids limit to 0, attempting to "unset" it. I can't find concrete language in the current spec, but seeing opencontainers/runtime-spec#234 implies this should set the pids.max to "max".

However, since the pids limit has been supported in runc, it ignores when PidsLimit is set to 0, which causes values to populate that are unexpected, and potentially are incorrect.

For reference, crun sets the value to max when PidsLimit is set to 0

Steps to reproduce the issue

  1. create a container with pids limit 0
  2. check its pids.max

Describe the results you received and expected

the value seems to not be deterministic, and is not max

What version of runc are you using?

1.1.7, but poking through the code it seems to always have been the case

Host OS information

No response

Host kernel information

No response
@haircommander haircommander linked a pull request Sep 12, 2023 that will close this issue
Open
kolyshkin added a commit to kolyshkin/runc that referenced this issue Sep 22, 2023
@kolyshkin
[debug] test case for issue opencontainers#4014
3bf5f9c 
For opencontainers#4014.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This was referenced Sep 22, 2023
Closed
Open
@kolyshkin
Copy link
Contributor

OK, this only happens when systemd cgroup manager is used; when TasksMax is not set explicitly, the value is derived from the parent.

@kolyshkin
Copy link
Contributor

OK, I think the spec is not really written well in this part.

The current doc (since opencontainers/runtime-spec@488f174) says is "default is no limit". Here "default" means the default Go value (of 0), and "no limit" can be interpreted as "do not set the limit" (i.e. ignore) or "set the limit to unlimited".

Older version of doc (since opencontainers/runtime-spec#234), which @haircommander refers to, says "A value <= 0 indicates no limit".

There were some other changes in the runtime spec, in particular moving from int64 to *int64 and back.

Overall, this resulted in spec being vague, as a result, runc actually treats 0 as "do not change anything" and crun treats 0 as "no limit".

I think what we should do is

  • change the spec to be more clear;
  • fix runtimes according to the spec.

@cyphar
Copy link
Member

cyphar commented Jan 12, 2024 •
edited

0 in runc.spec should be a limit of 0, which the kernel treats like a limit of 1 in most cases. Unset in config.json should mean leave. I looked at this very closely recently, I think the actual spec text is not very unclear but there are issues that make the spec unclear and we need to fix it

IMHO libcontainer API changes are not a big deal. We do not technically support that anyway.

I have a spec PR, I will push when I get back from teaching students.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
kind/bug
Projects
None yet
Milestone
No milestone
4 participants
@lifubang @cyphar @kolyshkin @haircommander