Conversation
|
Being tested in #2252 |
|
My bad; clicked the wrong button 😃 The test in #2252 passed, so marking this as ready for review. |
|
With this PR, a restore without Gory DetailsCRIU checkpoints and restores `pdeath_sig` (`criu/cr-dump.c` and `criu/pie/restorer.c:restore_pdeath_sig`). With `criu_restore_child()`, which sets `rst_sibling`, CRIU creates the root task with `CLONE_PARENT`, so its parent is whoever called into libcriu. The comment in `criu/cr-restore.c` (`maybe_clone_parent`) says this is deliberate: "This is the only way to correctly restore the pdeath_sig of the root task".Before this PR, the caller was crun itself, which stays alive for as long as list_for_each_entry(p, &father->children, sibling) {
for_each_thread(p, t) {
RCU_INIT_POINTER(t->real_parent, reaper);
...
if (t->pdeath_signal)
group_send_sig_info(t->pdeath_signal,
SEND_SIG_NOINFO, t,
PIDTYPE_TGID);Being a subreaper does not help: the signal is sent no matter who the new
I tested this with an addition to the test added by this PR: a test init that
As far as I can see, a fix has to keep the process which calls |
| ret = criu_restore_child_in_cgroup (status->cgroup_path, &criu_ret, err); | ||
| if (UNLIKELY (ret < 0)) |
There was a problem hiding this comment.
So once the restore is done we should stop being a subreaper (as we only wait for init). I would add this:
ret = criu_restore_child_in_cgroup (status->cgroup_path, &criu_ret, err);
+ if (! cr_options->detach && UNLIKELY (prctl (PR_SET_CHILD_SUBREAPER, 0, 0, 0, 0) < 0))
+ libcrun_warning ("cannot stop being a child subreaper: %s", strerror (errno));
if (UNLIKELY (ret < 0))I have a test case for this but it's way too big and probably won't worth adding here. You can see it kolyshkin@ed0a165.
My thinking is it's a niche case but the fix is easy so why not add it.
thanks for trying that. This is indeed an issue, and if we don't want to just error out, it is better to leave the code as it is. For users using libcrun (if any...), I've added a new function to introduce the behavior I've suggested earlier |
5869e6c to
07dcbe3
Compare
kolyshkin
left a comment
kolyshkin
left a comment
There was a problem hiding this comment.
This could probably be a crun restore option, too.
Yet better, check (from a crun dump) if the dumped init has a parent death signal set. If it's not, we can use PREFORK, otherwise we can't.
kolyshkin
left a comment
kolyshkin
left a comment
There was a problem hiding this comment.
One more thing -- when detach == true, we can (and should) use PREFORK, as the issue is only with foreground restore.
This way we'll also have some CI coverage for this code (which is currently absent).
| SIGCHLD. Its result is still visible, so use it. */ | ||
| if (errno != ECHILD) | ||
| return crun_make_error (err, errno, "waitpid for the CRIU restore process"); | ||
| return 0; |
There was a problem hiding this comment.
Seems we can lose real err here.
| return 0; | |
| /* The child might have failed before calling CRIU, and left an error. */ | |
| return *err != NULL ? -1 : 0; |
|
One more thing: Subject: [PATCH] criu: block signals across the vfork for restore
The throw-away process used with LIBCRUN_RESTORE_OPTIONS_PREFORK shares
the memory and the stack with the caller, and it runs for the whole
restore: libcriu forks CRIU, talks to it over RPC and waits for it. If
a signal is delivered to it in the meantime, a handler installed by the
program embedding libcrun runs there, and can corrupt the state of the
suspended caller.
Do what posix_spawn does: block all signals before the vfork, reset the
handlers to SIG_DFL in the child (keeping the ignored signals ignored,
as CRIU would inherit them otherwise too), and restore the original
signal mask both in the child, before calling libcriu, and in the
parent, once the child is gone.
Signed-off-by: Kir Kolyshkin <[email protected]>
---
src/libcrun/criu.c | 42 ++++++++++++++++++++++++++++++++++++++----
1 file changed, 38 insertions(+), 4 deletions(-)
diff --git a/src/libcrun/criu.c b/src/libcrun/criu.c
index 52307f88..2427011a 100644
--- a/src/libcrun/criu.c
+++ b/src/libcrun/criu.c
@@ -25,6 +25,7 @@
# include <sys/types.h>
# include <criu/criu.h>
# include <sched.h>
+# include <signal.h>
# include <sys/stat.h>
# include <sys/mount.h>
# include <fcntl.h>
@@ -992,26 +993,56 @@ move_back_to_cgroups (const char *cgroups)
process tree is a sibling of CRIU, hence a child of the process calling
criu_restore_child().
- The child is kept as small as possible, as it shares the memory with the
- caller. This is a separate function so that no variable of the caller is
- live across the vfork. */
+ As the child shares the memory and the stack with the caller, all signals
+ are blocked across the vfork, and the child resets the signal handlers
+ before unblocking them, so that no handler of the caller can run there.
+ This is a separate function so that no variable of the caller is live
+ across the vfork. */
static int
criu_restore_child_in_cgroup (const char *cgroup_path, int *criu_ret, libcrun_error_t *err)
{
+ sigset_t all_signals, old_mask;
int wait_status = 0;
pid_t pid;
int ret;
*criu_ret = -1;
+ sigfillset (&all_signals);
+ ret = sigprocmask (SIG_BLOCK, &all_signals, &old_mask);
+ if (UNLIKELY (ret < 0))
+ return crun_make_error (err, errno, "sigprocmask");
+
/* Must be vfork: the child shares our memory space, so both *criu_ret and
the error it creates are visible here once it is gone. */
pid = vfork ();
if (UNLIKELY (pid < 0))
- return crun_make_error (err, errno, "vfork");
+ {
+ int saved_errno = errno;
+
+ sigprocmask (SIG_SETMASK, &old_mask, NULL);
+ return crun_make_error (err, saved_errno, "vfork");
+ }
if (pid == 0)
{
+ struct sigaction act;
+ int i;
+
+ /* The signal dispositions are not shared with the parent, so resetting
+ them here does not affect it. Keep the ignored signals ignored, as
+ CRIU would inherit them if it was run directly by the caller. */
+ for (i = 1; i < NSIG; i++)
+ {
+ if (sigaction (i, NULL, &act) < 0 || act.sa_handler == SIG_IGN || act.sa_handler == SIG_DFL)
+ continue;
+
+ memset (&act, 0, sizeof (act));
+ act.sa_handler = SIG_DFL;
+ sigaction (i, &act, NULL);
+ }
+ sigprocmask (SIG_SETMASK, &old_mask, NULL);
+
if (! is_empty_string (cgroup_path))
{
ret = libcrun_move_process_to_cgroup (0, 0, cgroup_path, false, err);
@@ -1023,6 +1054,9 @@ criu_restore_child_in_cgroup (const char *cgroup_path, int *criu_ret, libcrun_er
_safe_exit (EXIT_SUCCESS);
}
+ /* The child is gone by now, it is safe to handle signals again. */
+ sigprocmask (SIG_SETMASK, &old_mask, NULL);
+
ret = waitpid_ignore_stopped (pid, &wait_status, 0);
if (UNLIKELY (ret < 0))
{
--
2.55.0
|
Answering to myself -- option is probably a bad idea, no one knows how/when to use it. Checking the dump is probably somewhat complicated (haven't tried), so we can leave it for later. Using the PREFORK when |
CRIU restores the container as a child of the process driving it, which must also join the container cgroup for the time of the restore, and move back out of it afterwards. With `--detach` there is no reason for that process to be crun itself: it does not wait for the container, so being its parent is useless, and the container is reparented as soon as crun exits. It is worse for a program embedding libcrun, which gets moved in and out of the container cgroup and is left with the container init as a child it has to reap. Use a throw-away process instead, the way libcrun_container_run already forks when detaching. A foreground restore is unchanged: the container must stay a child of crun for crun to wait for it, and a throw-away process cannot be used there, as the restored init gets its parent death signal as soon as the process which drove the restore is gone. Signed-off-by: Giuseppe Scrivano <[email protected]>
The throw-away process used by a detached restore shares the memory and the stack with the caller, and it runs for the whole restore: libcriu forks CRIU, talks to it over RPC and waits for it. If a signal is delivered to it in the meantime, a handler installed by the program embedding libcrun runs there, and can corrupt the state of the suspended caller. Do what posix_spawn does: block all signals before the vfork, reset the handlers to SIG_DFL in the child (keeping the ignored signals ignored, as CRIU would inherit them otherwise too), and restore the original signal mask both in the child, before calling libcriu, and in the parent, once the child is gone. Signed-off-by: Kir Kolyshkin <[email protected]>
07dcbe3 to
cbd6ca4
Compare
|
we don't really need the API change if we ``vfork` with detach, an API user can just use detach. So I've dropped that. I've adjusted your commit message to not refer to the proposed API and amended the following snippet, let me know if it is fine for you: |
It looks like |
2 participants
Move the call into a throw-away child process instead: it joins the container cgroup, asks CRIU to restore, and exits. Nothing has to be undone afterwards and does not affect the current process.
A followup to #2241.