[CRIU] [PATCH 09/10] pstree: leaders wait group members temporary setpgid to their pid
Pavel Tikhomirov
ptikhomirov at virtuozzo.com
Tue Jul 4 12:08:08 MSK 2017
That is becaues group leader can create group, give it to members
and then change own pgid to some other group, becoming non-leader.
Signed-off-by: Pavel Tikhomirov <ptikhomirov at virtuozzo.com>
---
criu/cr-restore.c | 81 +++++++++++++++++++++++++------------------------
criu/include/rst_info.h | 7 ++---
criu/pstree.c | 45 +++------------------------
3 files changed, 47 insertions(+), 86 deletions(-)
diff --git a/criu/cr-restore.c b/criu/cr-restore.c
index 15c1aab..20b854f 100644
--- a/criu/cr-restore.c
+++ b/criu/cr-restore.c
@@ -1587,54 +1587,54 @@ 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 when all task are already forked. We also
+ * need from our group leader to be setpgid to it's group pgid. As
+ * it is absolutely valid to have process group with it's leader
+ * living in a different group.
+ *
+ * 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)
@@ -1853,7 +1853,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..894a9bd 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 (group_leader->pid->state == TASK_UNDEF || !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;
}
--
2.9.4
More information about the CRIU
mailing list