Implemented clone fallback when clone3 returns ENOSYS #2121

yihuaf · 2023-07-04T07:28:43Z

Fix #2030

Apologize for the large change. This PR implements the clone fallback when clone3 returns ENOSYS. clone interface is messier to use compared to the clone3 kernel syscall, so a large chunk of the changes is to deal with that. For example, the container_args now implements Clone so we can clearly pass the ownership of the memory to the new process.

A few other changes to make this happen:

Delay the creation of the syscall struct and only pass around a enum signaling the type of syscall.
Remove the use the executor and use a function pointer instead. Now the executor is much more explicit about the order of how each executor is tried. Instead of a list of Executor, now executors are composable to create new executor.
Delay the creation of cgroup manager until the intermediate process.
Changed some of the cgroup functions to take &Path instead of PathBuf.

codecov-commenter · 2023-07-04T07:46:55Z

Codecov Report

Merging #2121 (c10b27e) into main (71cb81a) will increase coverage by 0.05%.
The diff coverage is 46.84%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2121      +/-   ##
==========================================
+ Coverage   64.82%   64.87%   +0.05%     
==========================================
  Files         129      129              
  Lines       14768    14954     +186     
==========================================
+ Hits         9573     9702     +129     
- Misses       5195     5252      +57

YJDoc2

Hey, haven't gone through the all changes or conceptual changes, just had a couple of specific questions. If any changes are required, I'd suggest to keep them and bundle with any other changes from other review.

YJDoc2 · 2023-07-04T11:40:16Z

crates/libcontainer/src/container/builder.rs

+    pub fn with_executor(mut self, executor: Executor) -> Result<Self, LibcontainerError> {
+        self.executor = executor;


Does this still needs to return result? we are simply setting the executor and returning, so can simply return self instead of a result

No. Good catch.

YJDoc2 · 2023-07-04T11:49:25Z

crates/libcontainer/src/container/builder_impl.rs

+        // namespace. We also need to create to socket before entering into the
+        // user namespace in the case that the path is located in paths only
+        // root can access.
+        let notify_listener =


very minor nitpick, but we can directly name this notify_socket , so we don't have to specify when building ContainerArgs below

YJDoc2 · 2023-07-04T11:54:22Z

crates/libcontainer/src/channel.rs

+#[derive(Clone)]
 pub struct Receiver<T> {
    receiver: RawFd,
    phantom: PhantomData<T>,
 }

+#[derive(Clone)]
 pub struct Sender<T> {
    sender: RawFd,
    phantom: PhantomData<T>,


Given that this contains RawFd, is it fine to allow clone on this? Also, should we impose Clone on T as well (even if we are not storing that) ?

Clone RawFd

This is just a c_int, so this is safe to do. It is like pass the RawFd by value with copy. When across the fork/clone boundary, the file descriptor table is duplicated so existing fds works exactly the same.

T for Clone

I don't think we have to impose Clone to T. In fact, it is possible that T does not have Clone trait. As long as T is serializable, we can use the channel to send and receive. So T can be serializable but not clone-able. The PhantomData does not store T as you pointed out and it is simply there to signal ownership of T.

yihuaf · 2023-07-05T04:47:24Z

I am leaving the commits for easier time to review, but I should probably squash before we merge.

utam0k · 2023-07-05T12:24:28Z

I can't find time for this PR unless it's on a Saturday or Sunday, so wait a minute 🙏

yihuaf · 2023-07-05T23:14:24Z

Also, some of the rust clone can be mitigated using Rc, especially to manage the cost of duplicating structures. I am working on a follow up PR next to address the issue in case there is a concern on this.

implements a clone fallback path when `clone3` is blocked with ENOSYS Removed the use of clone3 crate. Our usecase for the clone3 crate is very specific and do not need to support everything. I can easily replace the clone3 crate with a few lines of code, so I did it in this PR. We usually favors safe syscall wrappers like nix over libc, but I believe the save is worth it here. We use the raw clone syscall instead of glibc wrapper. Details are in the comments. Using raw syscalls for both clone and clone3 actually makes the code well organized. For our usecase, they share the input types. Unit tests using seccomp profile to simulate clone3 being blocked are added. Signed-off-by: yihuaf <yihuaf@unkies.org>

Signed-off-by: yihuaf <yihuaf@unkies.org>

utam0k

This is too big a change to mix in with this change. libcontainer has people using it. We need to contact them. Please leave this out of this PR 🙏

Remove the use the executor and use a function pointer instead. Now the executor is much more explicit about the order of how each executor is tried. Instead of a list of Executor, now executors are composable to create new executor.

utam0k · 2023-07-09T12:55:11Z

crates/libcontainer/src/container/builder.rs

    /// Id of the container
    pub(super) container_id: String,
    /// Root directory for container state
    pub(super) root_path: PathBuf,
    /// Interface to operating system primitives
-    pub(super) syscall: &'a dyn Syscall,
+    pub(super) syscall: SyscallType,


Since this is a breaking change for the users of libcontainer, please create another PR.

I will make these changes into a series of PR so we can these changes out.

yihuaf · 2023-07-10T20:39:29Z

@utam0k maintaining backward compatibility is going to be hard and I don't think we have thought enough about the libcontainer api to support it. I hope there is a way to can make breaking changes to the interface. With that being said, I will break the change into smaller pieces and let see how to work through this. I do believe the importance of making this PR to go in as I have great concerns with us using clone3 syscall without libc directly as the only path.

yihuaf · 2023-07-10T20:41:43Z

This is too big a change to mix in with this change. libcontainer has people using it. We need to contact them. Please leave this out of this PR 🙏

Remove the use the executor and use a function pointer instead. Now the executor is much more explicit about the order of how each executor is tried. Instead of a list of Executor, now executors are composable to create new executor.

Can you give me a sense of who are the big consumers? I can help take a look and gauge the amount of effort to fix.

yihuaf · 2023-07-10T20:42:36Z

Converting this PR to draft. I will break this PR into smaller piece and use this as a reference for the eventual intent.

utam0k · 2023-07-10T21:55:09Z

@yihuaf I don't mean to deny breaking change. If they are necessary, let's include them. However, I do think that breakrng changes that are not related to 1PR will cause confusion among users. Runwasi is probably the biggest user. Thanks for understanding🙇🙇

yihuaf · 2023-07-10T22:27:12Z

@yihuaf I don't mean to deny breaking change. If they are necessary, let's include them. However, I do think that breakrng changes that are not related to 1PR will cause confusion among users. Runwasi is probably the biggest user. Thanks for understanding🙇🙇

Understand. I will parse into multiple PRs and the destructive changes should be in their own PR. I will also fix runwasi when I introduce the executor change.

yihuaf · 2023-07-10T22:27:56Z

Actually, since I will break this PR into multiple PRs, we can close this to avoid confusion.

utam0k · 2023-07-10T22:40:16Z

@yihuaf Thanks a lot🙏

yihuaf force-pushed the yihuaf/clone_syscall branch from 9f6f7ab to 04c4800 Compare July 4, 2023 07:31

yihuaf changed the title ~~Implemented clone syscall~~ Implemented clone fallback when clone3 returns ENOSYS Jul 4, 2023

yihuaf marked this pull request as ready for review July 4, 2023 09:02

yihuaf requested review from utam0k and a team July 4, 2023 09:02

yihuaf self-assigned this Jul 4, 2023

YJDoc2 reviewed Jul 4, 2023

View reviewed changes

YJDoc2 added the kind/feature label Jul 4, 2023

yihuaf force-pushed the yihuaf/clone_syscall branch from 29e0bfc to b220267 Compare July 4, 2023 17:52

yihuaf requested a review from YJDoc2 July 5, 2023 04:46

yihuaf mentioned this pull request Jul 6, 2023

cargo test with musl hangs occasionally #2144

Closed

yihuaf added 15 commits July 7, 2023 22:28

this compiles but doesn't work

599eddb

Signed-off-by: yihuaf <yihuaf@unkies.org>

move notify listener creation to init process

6310876

Signed-off-by: yihuaf <yihuaf@unkies.org>

refactored the creation of cgroup manager

deecf4f

Signed-off-by: yihuaf <yihuaf@unkies.org>

fix syscall reference and lifetime

a628cbd

Signed-off-by: yihuaf <yihuaf@unkies.org>

fixed spec and rootfs not using reference

de914ac

Signed-off-by: yihuaf <yihuaf@unkies.org>

refactored the executor to be clone-able

a32c3ed

Signed-off-by: yihuaf <yihuaf@unkies.org>

remove the rest of reference from container_args

22083d8

Signed-off-by: yihuaf <yihuaf@unkies.org>

put enough clone to make the closure work

46fb4d4

Signed-off-by: yihuaf <yihuaf@unkies.org>

implemented the rest of the clone

2ce4953

Signed-off-by: yihuaf <yihuaf@unkies.org>

fix comments

552bed9

Signed-off-by: yihuaf <yihuaf@unkies.org>

fix doc test

e897907

Signed-off-by: yihuaf <yihuaf@unkies.org>

spellcheck

9428e0e

Signed-off-by: yihuaf <yihuaf@unkies.org>

fix lint

60d7c61

Signed-off-by: yihuaf <yihuaf@unkies.org>

fix feature test

db3c179

Signed-off-by: yihuaf <yihuaf@unkies.org>

yihuaf added 6 commits July 7, 2023 22:28

fix

d07801a

Signed-off-by: yihuaf <yihuaf@unkies.org>

fix notify listener

5be4fc8

Signed-off-by: yihuaf <yihuaf@unkies.org>

rebase

7cb9e47

Signed-off-by: yihuaf <yihuaf@unkies.org>

address review

e4899ad

Signed-off-by: yihuaf <yihuaf@unkies.org>

fix

5b3a931

Signed-off-by: yihuaf <yihuaf@unkies.org>

fix

64c8133

Signed-off-by: yihuaf <yihuaf@unkies.org>

yihuaf force-pushed the yihuaf/clone_syscall branch from c10b27e to 64c8133 Compare July 8, 2023 05:29

utam0k requested changes Jul 8, 2023

View reviewed changes

utam0k reviewed Jul 9, 2023

View reviewed changes

yihuaf marked this pull request as draft July 10, 2023 20:42

yihuaf mentioned this pull request Jul 10, 2023

Use syscall type to delay the creation of syscall struct. #2155

Merged

yihuaf closed this Jul 10, 2023

yihuaf mentioned this pull request Aug 3, 2023

[RFC] Implement a fallback when clone3 returns ENOSYS #2030

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented clone fallback when clone3 returns ENOSYS #2121

Implemented clone fallback when clone3 returns ENOSYS #2121

yihuaf commented Jul 4, 2023 •

edited

Loading

codecov-commenter commented Jul 4, 2023 •

edited

Loading

YJDoc2 left a comment

YJDoc2 Jul 4, 2023

yihuaf Jul 4, 2023

yihuaf Jul 4, 2023

YJDoc2 Jul 4, 2023

yihuaf Jul 4, 2023

yihuaf Jul 4, 2023

YJDoc2 Jul 4, 2023

yihuaf Jul 4, 2023

yihuaf commented Jul 5, 2023

utam0k commented Jul 5, 2023

yihuaf commented Jul 5, 2023

utam0k left a comment

utam0k Jul 9, 2023

yihuaf Jul 10, 2023

yihuaf commented Jul 10, 2023

yihuaf commented Jul 10, 2023

yihuaf commented Jul 10, 2023 •

edited

Loading

utam0k commented Jul 10, 2023 •

edited

Loading

yihuaf commented Jul 10, 2023

yihuaf commented Jul 10, 2023

utam0k commented Jul 10, 2023

		pub fn with_executor(mut self, executor: Executor) -> Result<Self, LibcontainerError> {
		self.executor = executor;

Implemented clone fallback when clone3 returns ENOSYS #2121

Implemented clone fallback when clone3 returns ENOSYS #2121

Conversation

yihuaf commented Jul 4, 2023 • edited Loading

codecov-commenter commented Jul 4, 2023 • edited Loading

Codecov Report

YJDoc2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yihuaf commented Jul 5, 2023

utam0k commented Jul 5, 2023

yihuaf commented Jul 5, 2023

utam0k left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yihuaf commented Jul 10, 2023

yihuaf commented Jul 10, 2023

yihuaf commented Jul 10, 2023 • edited Loading

utam0k commented Jul 10, 2023 • edited Loading

yihuaf commented Jul 10, 2023

yihuaf commented Jul 10, 2023

utam0k commented Jul 10, 2023

yihuaf commented Jul 4, 2023 •

edited

Loading

codecov-commenter commented Jul 4, 2023 •

edited

Loading

yihuaf commented Jul 10, 2023 •

edited

Loading

utam0k commented Jul 10, 2023 •

edited

Loading