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

Add support for seccomp thread sync feature #58

Merged
merged 1 commit into from Sep 12, 2023
Merged

Add support for seccomp thread sync feature #58

merged 1 commit into from Sep 12, 2023

Conversation

boustrophedon
Copy link
Contributor

@boustrophedon boustrophedon commented Sep 6, 2023 •
edited

Summary of the PR

  • Adds public function seccompiler::apply_filter_all_threads
  • Moves the body of apply_filter into apply_filter_with_flags
  • Use seccomp syscall instead of prctl to apply seccomp filters

Resolves #57

Requirements

Before submitting your PR, please make sure you addressed the following
requirements:

  • All commits in this PR are signed (with git commit -s), and the commit
    message has max 60 characters for the summary and max 75 characters for each
    description line.
  • All added/changed functionality has a corresponding unit/integration
    test.
  • All added/changed public-facing functionality has entries in the "Upcoming
    Release" section of CHANGELOG.md (if no such section exists, please create one).
  • Any newly added unsafe code is properly documented.

boustrophedon
boustrophedon commented Sep 6, 2023
View reviewed changes
src/backend/flags.rs Outdated Show resolved Hide resolved
boustrophedon
boustrophedon commented Sep 6, 2023
View reviewed changes
tests/multi_thread.rs
@@ -0,0 +1,80 @@
#![allow(clippy::undocumented_unsafe_blocks)]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test was adapted from my tsync test in extrasafe. https://github.com/boustrophedon/extrasafe/blob/master/tests/thread_multi.rs

@boustrophedon
Copy link
Contributor Author

boustrophedon commented Sep 6, 2023 •
edited

Looks like musl doesn't have the linux/seccomp.h SECCOMP_SET_MODE_FILTER constant defined in libc? Strange though because I've build libseccomp itself with musl before.

There's also lint and formatting errors that I need to fix.

@boustrophedon
Copy link
Contributor Author

It looks like everything except coverage and musl builds are passing now. I'm not sure what the best resolution is for the missing constant in musl libc.

@rbradford
Copy link

It looks like everything except coverage and musl builds are passing now. I'm not sure what the best resolution is for the missing constant in musl libc.

Open a PR against rust-lang/libc since this looks like it wasn't included when then gnu/musl split happened.

@boustrophedon
Copy link
Contributor Author

boustrophedon commented Sep 6, 2023 •
edited

rust-lang/libc#3342

Did some digging, seems like it was just missed at some point.

In the meantime, I'll update this diff to use libc::SECCOMP_FILTER_FLAG_TSYNC inside flags.rs which I had missed before but does anyone have an issue with landing this just using a locally-defined const SECCOMP_SET_MODE_FILTER: libc::c_int = 1 inside seccompiler until the libc PR gets merged? It seems like that's happening a lot in bpf.rs already so maybe I can do a follow up PR after the libc PR gets merged to clean up all the constants.

@alindima
Copy link
Collaborator

alindima commented Sep 7, 2023 •
edited

rust-lang/libc#3342

Did some digging, seems like it was just missed at some point.

In the meantime, I'll update this diff to use libc::SECCOMP_FILTER_FLAG_TSYNC inside flags.rs which I had missed before but does anyone have an issue with landing this just using a locally-defined const SECCOMP_SET_MODE_FILTER: libc::c_int = 1 inside seccompiler until the libc PR gets merged? It seems like that's happening a lot in bpf.rs already so maybe I can do a follow up PR after the libc PR gets merged to clean up all the constants.

Yes, this is fine until the PR is merged in rust/libc 👍🏻 Have you checked that it's the same value on x86 and arm as well?

alindima
alindima reviewed Sep 7, 2023
View reviewed changes
src/backend/flags.rs Outdated Show resolved Hide resolved
alindima
alindima reviewed Sep 7, 2023
View reviewed changes
src/lib.rs Outdated Show resolved Hide resolved
alindima
alindima reviewed Sep 7, 2023
View reviewed changes
CHANGELOG.md Show resolved Hide resolved
alindima
alindima reviewed Sep 7, 2023
View reviewed changes
src/lib.rs Outdated Show resolved Hide resolved
alindima
alindima reviewed Sep 7, 2023
View reviewed changes
src/lib.rs Show resolved Hide resolved
@boustrophedon
Copy link
Contributor Author

Thanks for the review! I just pushed my PR to libc and am about to go to bed but I'll start working on the fixes tomorrow!

alindima
alindima reviewed Sep 7, 2023
View reviewed changes
src/lib.rs Outdated Show resolved Hide resolved
alindima
alindima reviewed Sep 7, 2023
View reviewed changes
Copy link
Collaborator

@alindima alindima left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! Glad to hear you're using seccompiler 😄

tests/multi_thread.rs Show resolved Hide resolved
@boustrophedon
Copy link
Contributor Author

boustrophedon commented Sep 7, 2023 •
edited

I've made all the requested changes I think.

Regarding the value of SECCOMP_SET_MODE_FILTER I'm pretty sure it's the same on all platforms. It's only defined once in include/uapi/linux/seccomp.h in the linux repo as far as I can see.

One thing I just noticed is that the manpage for seccomp says that the flags parameter is an unsigned int, but the definition for the flags in <linux/seccomp.h> is (1UL << 0) i.e. an unsigned long. I've changed the parameter to libc::c_ulong since the TSYNC flag constant is a ulong and my PR also uses ulong.

The only thing I'm not sure about is the source impl for ThreadSync - currently I left it as None since i64 doesn't impl Error but maybe there's something better I could do?

alindima
alindima reviewed Sep 8, 2023
View reviewed changes
src/lib.rs Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
petreeftime
petreeftime reviewed Sep 8, 2023
View reviewed changes
src/lib.rs
/// * `flags` - A u64 representing a bitset of seccomp's flags parameter.
///
/// [`BpfProgram`]: type.BpfProgram.html
fn apply_filter_with_flags(bpf_filter: BpfProgramRef, flags: libc::c_ulong) -> Result<()> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ideally flags should be similar to something like OFlags in nix: https://docs.rs/nix/latest/nix/fcntl/struct.OFlag.html, it's slightly more robust, but if apply_filter_with_flags remain just for internal purposes (ie. flags is not part of the public API), then leaving it like this for now is fine.

Copy link
Collaborator

@alindima alindima Sep 8, 2023 •
edited

Choose a reason for hiding this comment

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

This was the initial approach in the PR but since different flags for the seccomp syscall results in different return types, I suggested to keep this function for internal use only, to keep things simple and hard to misuse

@boustrophedon
Copy link
Contributor Author

Thanks for catching everything that I missed!

Regarding the clippy failure, I can make the change if you want but I personally think clippy's recommendation is not nearly as readable as just writing "rc > 0". If I'm looking at a single arm, "Ordering::Greater" doesn't tell me which is greater - I have to refer back to the top of the match statement. x < y is something everyone learned how to read in elementary school.

alindima
alindima previously approved these changes Sep 11, 2023
View reviewed changes
Copy link
Collaborator

@alindima alindima left a comment

Choose a reason for hiding this comment

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

Thanks

tests/multi_thread.rs Show resolved Hide resolved
@alindima
Copy link
Collaborator

I agree that clippy's suggestion is less readable.
You can use this line to disable it on the function definition: #[allow(clippy::comparison_chain)]

@boustrophedon
Add support for seccomp thread sync feature
5a2a6d5 
- Adds public functions `seccompiler::apply_filter_all_threads` and
  private `apply_filter_with_flags`
- Moves the body of apply_filter into apply_filter_with_flags
- Uses seccomp call directly in apply_filter, so new Error variant is
  added.
- Error variant also added for TSYNC failures

Resolves rust-vmm#57

Signed-off-by: Harry Stern <harry@harrystern.net>
@boustrophedon boustrophedon mentioned this pull request Sep 11, 2023
Closed
@boustrophedon
Copy link
Contributor Author

Looks like everything is passing now, thanks everyone for the reviews!

alindima
alindima approved these changes Sep 12, 2023
View reviewed changes
Copy link
Collaborator

@alindima alindima left a comment

Choose a reason for hiding this comment

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

LGTM

@petreeftime petreeftime merged commit 184f2d6 into rust-vmm:main Sep 12, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alindima alindima alindima approved these changes

@petreeftime petreeftime petreeftime approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support for SECCOMP_FILTER_FLAG_TSYNC
4 participants
@boustrophedon @rbradford @alindima @petreeftime