[v2,09/10] pstree: leaders wait group members temporary setpgid to their pid

Submitted by Pavel Tikhomirov on July 7, 2017, 7:35 a.m.

Details

Message ID 20170707073555.16340-1-ptikhomirov@virtuozzo.com
State New
Series "rework pgid restore for pidnses"
Headers show

Commit Message

Pavel Tikhomirov July 7, 2017, 7:35 a.m.
That is becaues group leader can create group, give it to members
and then change own pgid to some other group, becoming non-leader.

v2: fix comment, fix setting pgrp_member_cnt in prepare_pstree_leaders
Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
---
 criu/cr-restore.c       | 80 ++++++++++++++++++++++++-------------------------
 criu/include/rst_info.h |  7 ++---
 criu/pstree.c           | 45 +++-------------------------
 3 files changed, 46 insertions(+), 86 deletions(-)

Patch hide | download patch | download mbox

diff --git a/criu/cr-restore.c b/criu/cr-restore.c
index 1179606..4a471df 100644
--- a/criu/cr-restore.c
+++ b/criu/cr-restore.c
@@ -1594,54 +1594,53 @@  static void restore_sid(void)
 	BUG_ON(rsti(current)->curr_sid != vsid(current));
 }
 
-static void restore_pgid(void)
-{
-	/*
-	 * Unlike sessions, process groups (a.k.a. pgids) can be joined
-	 * by any task, provided the task with pid == pgid (group leader)
-	 * exists. Thus, in order to restore pgid we must make sure that
-	 * group leader was born and created the group, then join one.
-	 *
-	 * We do this _before_ finishing the forking stage to make sure
-	 * helpers are still with us.
-	 */
-
-	pid_t pgid, my_pgid = last_level_pid(current->pgid);
-
-	if (!list_empty(&top_pid_ns->children))
-		return;
-
-	pr_info("Restoring %d to %d pgid\n", vpid(current), my_pgid);
+/*
+ * Leader of process group should be alive to setpgrp to it's pgrp
+ * so we do restore_pgid before helpers die stage and wait for the
+ * group leader to set pgrp_set futex, thus it already became alive,
+ * and at the same time we satisfied the demand to have the right pgid.
+ *
+ * FIXME
+ * We have a simple realization of pgid restore which works only
+ * if all processes which belong to a process group can "see" pgid
+ * leader. Cases where process with it's parent is in pidns from
+ * which one can't "see" the group leader and can only inherit pgid
+ * same as sid are not considered as for one process to get pgid in
+ * worst case we need to lock states of 3 more processes(e.g.: parent,
+ * grand parent and group leader should be in same session) that can
+ * lead to deadlocks.
+ */
 
-	pgid = getpgrp();
-	if (my_pgid == pgid)
-		return;
+static int restore_pgid(void)
+{
+	struct pstree_item *leader;
 
-	if (my_pgid != last_level_pid(current->pid)) {
-		struct pstree_item *leader;
+	/* Setup the leader to be a leader */
+	if (futex_get(&rsti(current)->pgrp_member_cnt)) {
+		if (rsti(current)->curr_pgid != vpid(current))
+			if (set_pgid(current, vpid(current)))
+				return 1;
+		futex_set_and_wake(&rsti(current)->pgrp_set, 1);
+		futex_wait_until(&rsti(current)->pgrp_member_cnt, 0);
+	}
 
-		/*
-		 * Wait for leader to become such.
-		 * Missing leader means we're going to crtools
-		 * group (-j option).
-		 */
+	leader = pstree_item_by_virt(vpgid(current));
+	BUG_ON(!leader || leader->pid->state == TASK_UNDEF);
 
-		leader = rsti(current)->pgrp_leader;
-		if (leader) {
-			BUG_ON(vpgid(current) != vpid(leader));
+	/* Set the right pgid */
+	if (rsti(current)->curr_pgid != vpgid(current)) {
+		if (current != leader)
 			futex_wait_until(&rsti(leader)->pgrp_set, 1);
-		}
+		if (set_pgid(current, vpgid(current)))
+			return 1;
 	}
 
-	pr_info("\twill call setpgid, mine pgid is %d\n", pgid);
-	if (set_pgid(current, vpgid(current))) {
-		pr_perror("Can't restore pgid (%d/%d->%d)", vpid(current), pgid, vpgid(current));
-		exit(1);
+	if (current != leader && !is_session_leader(leader)) {
+		futex_dec_and_wake(&rsti(leader)->pgrp_member_cnt);
+		BUG_ON(futex_get(&rsti(leader)->pgrp_member_cnt) < 0);
 	}
-	rsti(current)->curr_pgid = vpid(current);
 
-	if (my_pgid == last_level_pid(current->pid))
-		futex_set_and_wake(&rsti(current)->pgrp_set, 1);
+	return 0;
 }
 
 static int mount_proc(void)
@@ -1862,7 +1861,8 @@  static int restore_task_with_children(void *_arg)
 	if (unmap_guard_pages(current))
 		goto err;
 
-	restore_pgid();
+	if (restore_pgid())
+		goto err;
 
 	if (current->parent == NULL) {
 		/*
diff --git a/criu/include/rst_info.h b/criu/include/rst_info.h
index de68457..1aaec64 100644
--- a/criu/include/rst_info.h
+++ b/criu/include/rst_info.h
@@ -45,11 +45,6 @@  struct rst_info {
 
 	u32			cg_set;
 
-	union {
-		struct pstree_item	*pgrp_leader;
-		futex_t			pgrp_set;
-	};
-
 	struct file_desc	*cwd;
 	struct file_desc	*root;
 	bool			has_umask;
@@ -69,6 +64,8 @@  struct rst_info {
 	int			curr_sid;
 	int			curr_pgid;
 	int			forked;
+	futex_t			pgrp_set;
+	futex_t			pgrp_member_cnt;
 };
 
 extern struct task_entries *task_entries;
diff --git a/criu/pstree.c b/criu/pstree.c
index ddce59f..4ca55a2 100644
--- a/criu/pstree.c
+++ b/criu/pstree.c
@@ -1066,6 +1066,9 @@  static void prepare_pstree_leaders(void) {
 		group_leader = pstree_item_by_virt(vpgid(item));
 		BUG_ON(group_leader == NULL);
 
+		if (!is_session_leader(group_leader))
+			futex_inc(&rsti(group_leader)->pgrp_member_cnt);
+
 		/* Only the item with full pgid has full info of leader's pidns */
 		if (!equal_pid(item->pgid, group_leader->pid))
 			continue;
@@ -1086,6 +1089,7 @@  static void prepare_pstree_leaders(void) {
 				BUG_ON(!init);
 			} else {
 				init = root_item;
+				futex_set(&rsti(group_leader)->pgrp_set, 1);
 			}
 
 			/*
@@ -1442,47 +1446,6 @@  static int prepare_pstree_ids(void)
 		}
 	}
 
-	/*
-	 * FIXME
-	 * Skip process group restore preparation in case of nested pidns.
-	 * As pgid restore is not yet reworked, will do it in a next seriess,
-	 * see corresponding if-check in restore_pgid.
-	 *
-	 * Problem here is that to do setpgid(pid, pgid) a) one should be
-	 * in a same thread group with pid or in a same thread group with
-	 * pid's parent; b) one should be in same pid ns (or it's predecessor)
-	 * with pgid. So if we entered pidns, forked again to have parent in
-	 * same pidns and the grouop leader is outside, we can only get the
-	 * right pgid by inheriting it, so same thing as we do with sessions
-	 * should be done.
-	 */
-	if (!list_empty(&top_pid_ns->children))
-		return 0;
-
-	/* Setup pgrp_leader to wait for it to became a real leader */
-	for_each_pstree_item(item) {
-		struct pid *pid;
-
-		if (is_group_leader(item))
-			continue;
-
-		/*
-		 * If the PGID is eq to current one -- this
-		 * means we're inheriting group from the current
-		 * or setpgid group to some already created group,
-		 * so we do not need to wait leaders's creation.
-		 */
-		if (opts.shell_job && !is_session_leader(root_item)
-				&& vpgid(root_item) == vpgid(item))
-			continue;
-
-		pid = pstree_pid_by_virt(vpgid(item));
-		BUG_ON(pid == NULL || pid->state == TASK_UNDEF);
-		BUG_ON(pid->state == TASK_THREAD);
-		rsti(item)->pgrp_leader = pid->item;
-		continue;
-	}
-
 	return 0;
 }