Skip to content

Conversation

@trinity-1686a
Copy link
Contributor

fix #16

Copy link
Collaborator

@pchickey pchickey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI shows that flag isn't available across all Linux versions - can you add the appropriate cfg logic to handle that?

@trinity-1686a
Copy link
Contributor Author

trinity-1686a commented Jun 1, 2021

Do you think I should make UffdBuilder::user_mode_only always exists, but do nothing when older kernel is used (with doc comment to clarify), or make the function appear only when building with the right flag? I began with the 2nd option as it seemed clearer, but it makes chaining calls like .close_on_exec(true).non_blocking(true).user_mode_only(true).create a lot messier.

I've gone with 1st option for the time being. If it won't do, do tell.

and add building for Linux 5.11 to CI
@olivierlemasle
Copy link
Contributor

Does that mean the kernel detection should be done by the user of this crate, when deciding if feature linux5_11 should be enabled? How does a dependent crate (e.g. wasmtime-runtime) know if linux5_11 should be enabled?

I know that this crate previously used linux version detection and switched to features; however with no feature enabled, it worked for all kernels (>= 4.11), without the extra features.

@trinity-1686a
Copy link
Contributor Author

I've changed so at compile time there is no feature-flag. If UFFD_USER_MODE_ONLY is not found as part of kernel headers, it's added with the same value as in Linux 5.11, 5.12 and 5.13.
At run-time, the flag is only used when the kernel supports it, so it's always safe to use user_mode_only(true) even with an older kernel, and it won't return EINVAL

.unwrap_or(true)
{
flags |= raw::UFFD_USER_MODE_ONLY as i32;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understood your reply in the PR thread - does adding this flag cause an EINVAL on pre-5.11 kernels?

If so please add a comment above this check so we know why we are performing a runtime check

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just tested with kernel 4.9 and calling userfaultfd with UFFD_USER_MODE_ONLY sets errno to EINVAL. I found a more robust way to check was to always try opening the fd with UFFD_USER_MODE_ONLY but retry without if it gives EINVAL (here linux_version seems to be parsing uname which could theoretically be inaccurate if running on a weirdly named kernel).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does uname -r report on this machine?


[build-dependencies]
bindgen = { version = "0.51", default-features = false }
bindgen = { version = "0.57", default-features = false }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was this upgrade required?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, otherwise there was a conflict on native crate clang-sys, linux-version transitively depends on v0.28.0, but userfaultfd-sys depends on ^1

@pchickey pchickey requested a review from acfoltzer June 3, 2021 21:27
Copy link
Collaborator

@acfoltzer acfoltzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! I have a couple documentation tweaks, and a request to change the default behavior of this new flag.

trinity-1686a and others added 2 commits June 10, 2021 19:37
Co-authored-by: Adam C. Foltzer <acfoltzer@acfoltzer.net>
@olivierlemasle
Copy link
Contributor

@acfoltzer @pchickey Would you be ok to make a release of the crates after this PR is merged (if PR is ok for you)?

@acfoltzer acfoltzer self-assigned this Jun 14, 2021
Copy link
Collaborator

@pchickey pchickey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry we forgot to get back to this one. Github Actions just upgraded the kernel underneath ubuntu-20.04 to 5.11, so we need this fix everywhere downstream to fix CI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support for unprivileged use in Linux 5.11+

5 participants