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

nsexec: spring cleaning #3953

Closed
wants to merge 11 commits into from
Closed

nsexec: spring cleaning #3953

wants to merge 11 commits into from

Conversation

cyphar
Copy link
Member

@cyphar cyphar commented Aug 1, 2023

This includes several cleanups to nsexec.c:

  • (WIP) Moving the id-mapping and bindmount source-fd logic from nsexec.c with all of the complicated rewriting and opening magic to just using open_tree() in the Go portion of runc and passing the file descriptors down to rootfs_linux.go to just do a simple move_mount(2). This also allows us to easily implement handling of different id-mappings for mounts and is thus a replacement for Support for ID map mounts without userns #3943.
  • Remove the special-casing of CLONE_NEWCGROUP that was arguably never necessary and definitely isn't necessary now.
  • Remove the repetitive (and error-prone) sane_kill error path handling by using prctl(PR_SET_PDEATHSIG, SIGKILL) to auto-kill our runc C code if the parent runc process dies. We clear pdeathsig once setup is complete.
  • (Depends on nsexec: cloned_binary: remove bindfd logic entirely #3931.) Move the cloned binary logic entirely to Go so that we can save time on an execve by directly executing the copy when we spawn runc init.

This includes several major steps towards the (maybe possible) goal of #3951.

Closes #3943

@cyphar

This comment was marked as outdated.

@cyphar cyphar mentioned this pull request Aug 4, 2023
@adrianreber
Copy link
Contributor

I know that CRIU uses the new mount API if available. Maybe this is related if the kernel is too old, but @Snorch is the expert when it comes to mounts.

@Snorch do you have any ideas regarding @cyphar's question?

@cyphar

This comment was marked as outdated.

Comment on lines +81 to +88
- name: procfs mount
run: |
# Get the list of mounts to help with debugging.
cat /proc/self/mounts
# Create a procfs mount that is not masked, to ensure that container
# procfs mounts will succeed.
sudo mkdir -p /tmp/.procfs-stashed-mount
sudo unshare -pf mount -t proc -o subset=pid proc /tmp/.procfs-stashed-mount
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know why this has become necessary with this PR (the "problem" commit is the /proc/self/exe cloning change) but this solves the issue and this is a CI-weirdness issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

@cyphar Does that mean that runc won't work as-is inside "your usual" GHA, or is it only for the sake of testing?

@cyphar
Copy link
Member Author

cyphar commented Aug 6, 2023

@adrianreber @Snorch I figured out what the underlying issue is, the nested mount is being leaked to the host when using open_tree(2) for all bind-mounts (and is actually an existing issue with the old code, it's just that allowing for remapping by non-userns mounts causes the mount to be leaked). Basically the core issue is that the rootfs mount propagation doesn't apply this early in the runc startup process. Working on a solution...

We also really need a separate integration test for the leaking behaviour. The fact that only this criu test fails for this fairly broken behaviour (and doesn't fail on newer kernels) is quite concerning.

@Snorch
Copy link
Contributor

Snorch commented Aug 7, 2023

Hello, @cyphar

In particular is there a reason to think that (on older kernels) criu would struggle to deal with bind mounts created with the new mount API?

I would say, there is no.

If criu lacks some new mount API (move_mount(..._SET_GROUP) and openat2(RESOLVE_NO_XDEV)) it switches to "old" mount engine which uses only old mount API.

Long story short: We have two mount engines in latest CRIU, "new" and "old" and there is no big difference between them in simple cases. The "new" is though more precise so it can fail in some cases where "old" just silently did something wrong, at the same time "new" supports complex cases related to propagation so in some cases where "old" would fail or do something wrong "new" will succeed =)

But I would not say that there is some general rule that "old" can't restore (or badly restore) mounts created with new mount API. Only thing we can do is investigate each case.

I figured out what the underlying issue is, the nested mount is being leaked to the host when using open_tree(2) for all bind-mounts (and is actually an existing issue with the old code, it's just that allowing for remapping by non-userns mounts causes the mount to be leaked).

Not sure that I fully understand it, but from context it looks like you talk about issue in runc, just detected on CRIU test.

Basically the core issue is that the rootfs mount propagation doesn't apply this early in the runc startup process.

Just in case, "new" mount engine of CRIU does not support if root mount of container is shared (only slave is supported) to mount outside of the container (e.g. from which it is created, i.e. --root criu option), after CRIU c/r root mount of the container would become separate sharing group if it was shared outside before.

libcontainer/container_linux.go Outdated Show resolved Hide resolved
if err := unix.MountSetattr(int(mountFile.Fd()), "", unix.AT_EMPTY_PATH|setattrFlags, &unix.MountAttr{
Propagation: uint64(propFlags &^ unix.MS_REC),
}); err != nil {
return fmt.Errorf("remap mount sources: failed to set mount propagation of %q bind-mount to 0x%x: %w", m.Source, propFlags, err)
Copy link
Member

@rata rata Aug 7, 2023

Choose a reason for hiding this comment

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

It will be nice to say which syscall is failing too.

Copy link
Contributor

@eiffel-fl eiffel-fl 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 for it! It permits to remove a lot of code and your implementation of ID map mounts without user ns is more flexible than mine.
I will take a deeper look later, but for now here are some questions:

libcontainer/container_linux.go Show resolved Hide resolved
libcontainer/container_linux.go Show resolved Hide resolved
// NOTE: when running a container with no PID namespace and the parent process spawning the container is
// PID1 the pdeathsig is being delivered to the container's init process by the kernel for some reason
// even with the parent still running.
// Due to a Go stdlib bug, we need to add c.safeExeFile to the set of
Copy link
Contributor

Choose a reason for hiding this comment

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

I am curious, can you please share more information regarding this bug?

Copy link
Member Author

Choose a reason for hiding this comment

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

libcontainer/userns/usernsfd_linux.go Show resolved Hide resolved
This is needed to make sure that our userns tests work in GitHub Actions
if the host /proc is masked.

Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
Otherwise TESTFLAGS="-run FooBar" will result in TESTFLAGS=-run being
executed in the container.

Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
This includes quite a few cleanups and improvements to the way we do
syncrhonisation. The core behaviour is unchanged, but switching to
embedding json.RawMessage into the synchronisation structure will allow
us to do more complicated synchronisation operations in future patches.

The file descriptor passing through the syncrhonisation system feature
will be used as part of the idmapped-mount and bind-mount-source
features when switching that code to use the new mount API outside of
nsexec.c.

Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
*os.File is correctly tracked by the garbage collector, and there's no
need to use raw file descriptors for this code.

Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
The kernel ignores these arguments, and passing them can lead to
confusing error messages (the old source is irrelevant for MS_REMOUNT),
as well as causing issues for a future patch where we switch to
move_mount(2).

Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
The original implementation of cgroupns had additional synchronisation
to "ensure" that the process is in the correct cgroup before unsharing
the cgroupns. This behaviour was actually never necessary, and after
commit 5110bd2 ("nsenter: remove cgroupns sync mechanism") there is
no synchronisation at all, meaning that CLONE_NEWCGROUP should not get
any special treatment.

Fixes: 5110bd2 ("nsenter: remove cgroupns sync mechanism")
Fixes: df3fa11 ("Add support for cgroup namespace")
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
This allow us to remove the amount of C code in runc quite
substantially, as well as removing a whole execve(2) from the nsexec
path because we no longer spawn "runc init" only to re-exec "runc init"
after doing the clone.

Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
In the runc state JSON we always use snake_case. This is a no-op change,
but it will cause any existing container state files to be incorrectly
parsed. Luckily, commit fbf183c ("Add uid and gid mappings to
mounts") has never been in a runc release so we can change this before a
1.2.z release.

Fixes: fbf183c ("Add uid and gid mappings to mounts")
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
With open_tree(OPEN_TREE_CLONE), it is possible to implement both the
id-mapped mounts and bind-mount source file descriptor logic entirely in
Go without requiring any complicated handling from nsexec.

This allows us to remove the amount of C code we have in nsexec, as well
as simplifying a whole host of places that were made more complicated
with the addition of id-mapped mounts and the bind sourcefd logic. The
one downside of this is that the bind sourcefd feature now depends on
Linux 5.4.

In addition, we can easily add support for id-mappings that don't match
the container's user namespace. The approach taken here is to use Go's
officially supported mechanism for spawning a process in a user
namespace, but (ab)use PTRACE_TRACEME to avoid actually having to exec a
different process. The most efficient way to implement this would be to
do clone() in cgo directly to run a function that just does
kill(getpid(), SIGSTOP) -- we can always switch to that if it turns out
this approach is too slow.

This also includes a partial fix for a bug in the handling of idmap
mounts and mount propagation. In short, the issue is that because we do
OPEN_TREE_CLONE on the host, the RootPropagation flag does not apply
(nor any other mount propagation flags configured in config.json) and
thus recursive bind-mounts can and will be leaked to the host. Because
this patch switches the feature from 9c44407 ("Open bind mount
sources from the host userns") to use OPEN_TREE_CLONE, this resulted in
all bind-mounts having this behaviour. The partial fix here is to try to
emulate the behaviour of RootPropagation for bind-mounts from the host.

It turns out that bind-mounts inside containers were broken when using
the fd-passing feature from 9c44407 ("Open bind mount sources from
the host userns") because the file descriptor opens were done before we
start doing any mounts in rootfs_linux.go. The solution for this is more
involved and is fixed in a separate patch. At the very least, this patch
doesn't worsen the mount propagation situation.

Fixes: fda12ab ("Support idmap mounts on volumes")
Fixes: 9c44407 ("Open bind mount sources from the host userns")
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
With the rework of nsexec.c to handle MOUNT_ATTR_IDMAP in our Go code we
can now handle arbitrary mappings without issue, so remove the primary
artificial limit of mappings (must use the same mapping as the
container's userns) and add some tests.

We still only support idmap mounts for bind-mounts because configuring
mappings for other filesystems would require switching our entire mount
machinery to the new mount API. The current design would easily allow
for this but we would need to convert new mount options entirely to the
fsopen/fsconfig/fsmount API. This can be done in the future.

Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
@kolyshkin
Copy link
Contributor

I spent some time rebasing this PR and almost succeeded, except for idmap.bats changes.

One thing that helped me when applying commit 18ce9de ("iidmap: allow arbitrary idmap mounts regardless of userns configuration") is using git format-patch -1 --diff-algorithm=patience 18ce9de4e3e0c6 which produces much more sensible diff for validate_test.go, which applied fine except for 1 trivial hunk (empty line removal).

// syncParentSeccomp sends the fd associated with the seccomp file descriptor
// to the parent, and wait for the parent to do pidfd_getfd() to grab a copy.
func syncParentSeccomp(pipe *os.File, seccompFd int) error {
if seccompFd >= 0 {

This comment was marked as resolved.

@kolyshkin
Copy link
Contributor

I suggest we split this one into more digestible PRs. Ideally, each medium-sized feature should be a separate PR.

I rebased the more-or-less trivial stuff from this PR to #3982, which I hope we can merge soon.

// syncParentSeccomp sends the fd associated with the seccomp file descriptor
// to the parent, and wait for the parent to do pidfd_getfd() to grab a copy.
func syncParentSeccomp(pipe *os.File, seccompFd int) error {
if seccompFd >= 0 {

This comment was marked as resolved.

Comment on lines +189 to +193
// We have a copy, the child can keep working. We don't need to
// wait for the seccomp notify listener to get the fd before we
// permit the child to continue because the child will happily wait
// for the listener if it hits SCMP_ACT_NOTIFY.
if err := writeSync(p.messageSockPair.parent, procSeccompDone); err != nil {

This comment was marked as resolved.

@cyphar
Copy link
Member Author

cyphar commented Aug 20, 2023

All of this code is now split into separate PRs #3982, #3967, #3985, #3987. Closing in favour of those...

@cyphar cyphar closed this Aug 20, 2023
@cyphar cyphar deleted the nsexec-spring-cleaning branch August 20, 2023 14:02
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.

6 participants