[CRIU] [PATCH v2] mount: Order call_helper_process calls

[Cyrill Gorcunov]

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

Signed-off-by: Cyrill Gorcunov <gorcunov at gmail.com>
---
 criu/clone-noasan.c |  9 +++++++++
 criu/mount.c        | 21 ++++++++++++++++-----
 2 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/criu/clone-noasan.c b/criu/clone-noasan.c
index 627d924d5346..1898ca5a1105 100644
--- a/criu/clone-noasan.c
+++ b/criu/clone-noasan.c
@@ -75,6 +75,15 @@ int clone_noasan_set_mutex(mutex_t *clone_mutex)
  *         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)
 {
diff --git a/criu/mount.c b/criu/mount.c
index 974af6eb2218..0bdbe521cf5e 100644
--- a/criu/mount.c
+++ b/criu/mount.c
@@ -3702,27 +3702,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, ret = -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;
+	ret = 0;
+out:
+	unlock_last_pid();
+	return ret;
 }
 
 static int ns_remount_writable(void *arg)
-- 
2.20.1