Skip to content

Commit

Permalink
mount: Order call_helper_process calls
Browse files Browse the repository at this point in the history
When we do clone threads in a later stage of restore procedure
it may race with helpers which do call clone_noasan by self.

Thus we need to walk over each clone_noasan call and figure
out if calling it without last_pid lock is safe.

 - open_mountpoint: called by fusectl_dump, dump_empty_fs,
   binfmt_misc_dump, tmpfs_dump -- they all are processing
   dump stage, thus safe

 - call_helper_process: try_remount_writable -- called from
   various places in reg-files.c, in particular open_reg_by_id
   called in parallel with other threads, needs a lock
   remount_readonly_mounts -- called from sigreturn_restore,
   so in parallel, needs a lock

 - call_in_child_process: prepare_net_namespaces -- called
   from prepare_namespace which runs before we start forking,
   no need for lock

Thus call_helper_process should use lock_last_pid and
unlock_last_pid helpers and wait for subprocess to finish.

Same time put a warning text into clone_noasan comment
so next time we need to use it we would recall the pitfalls.

v2:
 - fix unitialized ret variable
v3:
 - use exit_code instead of ret

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
  • Loading branch information
cyrillos authored and avagin committed Feb 4, 2020
1 parent 2237666 commit 1d23dc4
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 5 deletions.
9 changes: 9 additions & 0 deletions criu/clone-noasan.c
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,15 @@
* https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69863
*
* So the only way is to put this wrapper in separate non-instrumented file
*
* WARNING: When calling clone_noasan make sure your not sitting in a later
* __restore__ phase where other tasks might be creating threads, otherwise
* all calls to clone_noasan should be guarder with
*
* lock_last_pid
* clone_noasan
* ... wait for process to finish ...
* unlock_last_pid
*/
int clone_noasan(int (*fn)(void *), int flags, void *arg)
{
Expand Down
21 changes: 16 additions & 5 deletions criu/mount.c
Original file line number Diff line number Diff line change
Expand Up @@ -3738,27 +3738,38 @@ struct ns_desc mnt_ns_desc = NS_DESC_ENTRY(CLONE_NEWNS, "mnt");

static int call_helper_process(int (*call)(void *), void *arg)
{
int pid, status;
int pid, status, exit_code = -1;

/*
* Running new helper process on the restore must be
* done under last_pid mutex: other tasks may be restoring
* threads and the PID we need there might be occupied by
* this clone() call.
*/
lock_last_pid();

pid = clone_noasan(call, CLONE_VFORK | CLONE_VM | CLONE_FILES |
CLONE_IO | CLONE_SIGHAND | CLONE_SYSVSEM, arg);
if (pid == -1) {
pr_perror("Can't clone helper process");
return -1;
goto out;
}

errno = 0;
if (waitpid(pid, &status, __WALL) != pid) {
pr_perror("Unable to wait %d", pid);
return -1;
goto out;
}

if (status) {
pr_err("Bad child exit status: %d\n", status);
return -1;
goto out;
}

return 0;
exit_code = 0;
out:
unlock_last_pid();
return exit_code;
}

static int ns_remount_writable(void *arg)
Expand Down

0 comments on commit 1d23dc4

Please sign in to comment.