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.

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

Env ulimit/prlimit not inherited by Gramine process #1576

Closed
thomasknauth opened this issue Sep 29, 2023 · 8 comments · Fixed by #1973
Closed

Env ulimit/prlimit not inherited by Gramine process #1576

thomasknauth opened this issue Sep 29, 2023 · 8 comments · Fixed by #1973
Labels
enhancement New feature or request

Comments

@thomasknauth
Copy link

Description of the problem

Gramine has some support for get/setrlimit(). Usually, the limits are inherited by a child process. Changing the limit, e.g., a common one is increasing the maximum allowed open files, in a shell session and subsequently invoking Gramine will not have the desired effect.

Steps to reproduce

Using the bash example from CI-Examples, invoke ulimit -n 4096 ; gramine-direct ./bash -c 'ulimit -n'.

Expected results

Gramine should pick up the limits from the env. In the above example, it should report 4096.

Actual results

This will report 900 (DEFAULT_MAX_FDS).

Gramine commit hash

gramine/jammy,now 1.5 amd64

@dimakuv
Copy link

dimakuv commented Sep 29, 2023

The fact that Gramine just uses some built-in default is definitely a bug.

However, I'm not sure that Gramine must get the limits from the untrusted environment. I think a better solution would be to introduce new manifest options, which the app/manifest developer can set to known-to-be-good values, or use a passthrough if the developer wants to pick up the limits from the env.

I think we even discussed this with @mkow at some point.

The proposed manifest syntax may look like this:

loader.rlimit.[NAME] = "[VALUE]"
or
loader.rlimit.[NAME] = { value = "[VALUE]" }
or
loader.rlimit.[NAME] = { passthrough = true }

For example, in @thomasknauth case, the manifest will specify:

loader.rlimit.RLIMIT_NOFILE = { passthrough = true }

This is the same to how Gramine specifies environment variables.

References:

@mkow
Copy link
Member

mkow commented Sep 30, 2023

Gramine should pick up the limits from the env.

What do you mean? Env variables? That's not how it works on Linux?

The proposed manifest syntax may look like this:

loader.rlimit.[NAME] = "[VALUE]"
or
loader.rlimit.[NAME] = { value = "[VALUE]" }
or
loader.rlimit.[NAME] = { passthrough = true }

We could do that, but why is this needed?

Also, the original issue here mixes two independent problems:

  • Whether limits are inherited between in-Gramine processes.
  • What are the default/initial values for them.

@dimakuv
Copy link

dimakuv commented Oct 2, 2023

Whether limits are inherited between in-Gramine processes.

The limits are inherited between in-Gramine processes, notice the attribute_migratable keyword:

static struct __kernel_rlimit64 __rlim[RLIM_NLIMITS] __attribute_migratable = {

Also, I don't think this was an original question.

What are the default/initial values for them.

I think this was the original question.

Gramine should pick up the limits from the env.
What do you mean? Env variables? That's not how it works on Linux?

No, I meant "env" = "the host environment". In case of rlimits, the host information should be queried via the getrlimit() host syscall.

We could do that, but why is this needed?

Sometimes you need to adjust your rlimits on your system, for example, for web servers that have a large number of clients (i.e., large number of opened FDs). On a normal system, you won't rely on the application to do that, instead you as a sysadmin increase the rlimit via e.g. ulimit -n 4096.

Now run this application under Gramine. It will fail because Gramine will refuse to open an FD, since the in-Gramine rlimit is reached. And the application doesn't know/understand that it needs to increase this limit itself, because typically the apps just rely on the sysadmin to do the job.

In Gramine case, we currently don't have this "sysadmin will do this job" (i.e., via passthrough or via value = 4096 in the manifest). That's the current problem.

@thomasknauth
Copy link
Author

thomasknauth commented Oct 25, 2023

Even setting the limit from within the application is not guaranteed to work. By the time the app's main() function is reached and a new limit may be set, the app may have already cloned/forked a bunch of processes/threads which will still have the default hard-coded Gramine limit.

@dimakuv
Copy link

dimakuv commented Oct 26, 2023

By the time the app's main() function is reached and a new limit may be set, the app may have already cloned/forked a bunch of processes/threads which will still have the default hard-coded Gramine limit.

@thomasknauth I can't understand how this flow is possible.

Why would the app clone/fork before the main() function? C/C++/Rust/Go applications do not do that. Are you using some language runtime/framework that does this? Or maybe you use some LD_PRELOAD trick with a constructor function, that is invoked before main()?

@dimakuv dimakuv added the enhancement New feature or request label Oct 26, 2023
@thomasknauth
Copy link
Author

Pretty much. In our case, it is Rust code using the tokio async runtime.

@dimakuv
Copy link

dimakuv commented Jan 11, 2024

For other rlimits, see these issues:

We should also move sys.stack.size and sys.brk_size (and maybe other manifest options) under this rlimits syntax.

@thomasknauth
Copy link
Author

Since it came up again recently, a work around is to set limits and then exec into the actual binary. If this binary now forks/spawns threads before its main function, the limits will already be set to the desired value (ht @haraldh).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants