c/r: restore tty pgrp properly
PATCH 1/1 c/r: restore tty pgrp properly
When restoring without keeping old PIDs, it is incorrect to set tty pgrp to same numeric value as it was at checkpoint time. This pgrp may not exist at restore time, and do_tiocspgrp() call from restore_signal() returns -ESRCH.
This patch tries to solve this by searching process with pgid equal to tty->pgrp
at checkpoint time, save index of that process in checkpoint, and at use restore-time
pgid of that process to set tty->pgrp at restore.
--- checkpoint/signal.c | 16 +++++++++++++--- include/linux/checkpoint_hdr.h | 2 +- 2 files changed, 14 insertions(+), 4 deletions(-)
checkpoint/signal.c +0 -0
@@ -323,6 +323,7 @@ static int checkpoint_signal(struct ckpt_ctx *ctx,
struct task_struct *t)
cputime_t cputime;
unsigned long flags;
int i, ret = 0;+ pid_t tty_pgrp, prgp_i;
h = ckpt_hdr_get_type(ctx, sizeof(*h), CKPT_HDR_SIGNAL);
if (!h)
@@ -411,7 +412,16 @@ static int checkpoint_signal(struct ckpt_ctx *ctx,
struct task_struct *t)
if (tty) {
/* irq is already disabled */
spin_lock(&tty->ctrl_lock);
- h->tty_pgrp = ckpt_pid_nr(ctx, tty->pgrp);
+ h->tty_pgrp_owner = -1;
+ tty_pgrp = ckpt_pid_nr(ctx, tty->pgrp);
+ for (i = 0; i < ctx->nr_tasks; i++) {+ pgrp_i = task_pgrp_nr_ns(ctx->tasks_arr[i],
+ ctx->root_nsproxy->pid_ns);
+ if (tty_pgrp == pgrp_i) {
+ h->tty_pgrp_owner = i;
+ break;
+ }
+ }
spin_unlock(&tty->ctrl_lock);
tty_kref_put(tty);
}
@@ -537,13 +547,13 @@ static int restore_signal(struct ckpt_ctx *ctx)
if (ret < 0)
goto out;
/* now restore the foreground group (job control) */- if (h->tty_pgrp) {
+ if (h->tty_pgrp_owner != -1) {
/*
* If tty_pgrp == CKPT_PID_NULL, below will
* fail, so no need for explicit test */
ret = do_tiocspgrp(tty, tty_pair_get_tty(tty),
- h->tty_pgrp);
+ ctx->pids_arr[h->tty_pgrp_owner].vpgid);
if (ret < 0)
goto out;
}include/linux/checkpoint_hdr.h +1 -1
@@ -898,7 +898,7 @@ struct ckpt_hdr_signal {__u64 it_prof_incr;/* tty */ __s32 tty_objref; -__s32 tty_pgrp; +__u32 tty_pgrp_owner;__s32 tty_old_pgrp; } __attribute__((aligned(8)));
Signature · 1 line
1.5.6.5
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 5 lines · Leonid Snegirev
When restoring without keeping old PIDs, it is incorrect to set tty pgrp to same numeric value as it was at checkpoint time. This pgrp may not exist at restore time, and
do_tiocspgrp()call fromrestore_signal()returns-ESRCH.
Quoted 7 lines
This patch tries to solve this by searching process with pgid equal to
tty->pgrp
at checkpoint time, save index of that process in checkpoint, and at use restore-time
pgid of that process to settty->pgrpat restore.Signed-off-by Leonid Snegirev <[email protected]>
Seems like a good idea. However, I suspect this is already covered in userspace by rewriting the pids array prior to calling sys_restart(). See ckpt_init_tree() in user-cr's restart.c and let me know if you agree and/or see any problems with that approach.
Do you have a testcase where restart doesn't work because of problems with tty->pgrp restore? Or was this just something that caught your eye during review?
Regardless, thanks for taking the time to put this patch together.
Quoted 38 lines
--- checkpoint/signal.c | 16 +++++++++++++--- include/linux/checkpoint_hdr.h | 2 +- 2 files changed, 14 insertions(+), 4 deletions(-)checkpoint/signal.c +1
-0@@ -323,6 +323,7 @@ static int checkpoint_signal(struct ckpt_ctx *ctx, struct task_struct *t) cputime_t cputime;unsigned long flags;int i, ret =0 ; +pid_t tty_pgrp, prgp_i; h =ckpt_hdr_get_type (ctx,sizeof (*h),CKPT_HDR_SIGNAL );if (!h)checkpoint/signal.c +10
-1@@ -411,7 +412,16 @@ static int checkpoint_signal(struct ckpt_ctx *ctx, struct task_struct *t) if (tty) {/* irq is already disabled */ spin_lock (&tty->ctrl_lock); - h->tty_pgrp =ckpt_pid_nr (ctx, tty->pgrp); + h->tty_pgrp_owner =-1 ; + tty_pgrp =ckpt_pid_nr (ctx, tty->pgrp); +for (i =0 ; i < ctx->nr_tasks; i++) { + pgrp_i =task_pgrp_nr_ns (ctx->tasks_arr[i], + ctx->root_nsproxy->pid_ns); +if (tty_pgrp == pgrp_i) { + h->tty_pgrp_owner = i; +break ; + } + }spin_unlock (&tty->ctrl_lock);tty_kref_put (tty);}
checkpoint/signal.c static int restore_signal(struct ckpt_ctx *ctx) · line 550
Quoted 5 lines
checkpoint/signal.c +1
-1@@ -537,13 +547,13 @@ static int restore_signal(struct ckpt_ctx *ctx)if (ret <0 ) goto out;/* now restore the foreground group (job control) */ -if (h->tty_pgrp) { +if (h->tty_pgrp_owner !=-1 ) {
Userspace can change h->tty_pgrp_owner to any value (bug, cosmic ray, exploit attempt) before passing the checkpoint image to restart. We don't want malicious programs to be able to cause an Oops or worse so we need to make sure the index is less than the number of entries in the pids_arr. Since the value was a prgrp id prior to this patch I doubt that check already exists.
checkpoint/signal.c static int restore_signal(struct ckpt_ctx *ctx) · line 559
Quoted 10 lines
checkpoint/signal.c +1
-1/* * If tty_pgrp == CKPT_PID_NULL, below will * fail, so no need for explicit test */ ret =do_tiocspgrp (tty,tty_pair_get_tty (tty), - h->tty_pgrp); + ctx->pids_arr[h->tty_pgrp_owner].vpgid);if (ret <0 ) goto out; }
<snip>
Cheers,
-Matt Helsley
A message that is not in the archive (sent off-list or not yet received): 20100408210008.GF3345-52DBMbEzqgQ/wnmkkaCWp/[email protected]
Quoted 12 lines · Matt Helsley
Quoted 11 lines · Leonid Snegirev
When restoring without keeping old PIDs, it is incorrect to set tty pgrp to same numeric value as it was at checkpoint time. This pgrp may not exist at restore time, and
do_tiocspgrp()call fromrestore_signal()returns-ESRCH.This patch tries to solve this by searching process with pgid equal to
tty->pgrp
at checkpoint time, save index of that process in checkpoint, and at use restore-time
pgid of that process to settty->pgrpat restore.
Quoted 6 lines
Quoted 1 line
Signed-off-by Leonid Snegirev <[email protected]>Seems like a good idea. However, I suspect this is already covered in userspace by rewriting the pids array prior to calling
sys_restart(). Seeckpt_init_tree()in user-cr'srestart.cand let me know if you agree and/or see any problems with that approach.
Indeed it is in the to-do list to have user-space restart, in the --no-pids case, adjust the pids all over the checkpoint image, i.e. in all objects that reference a pid, not only for task creation.
Perhaps a clean way to do this is to create a pid object for c/r, and then reference this object everywhere a pid is mentioned. This will rid the need to filter the entire checkpoint image.
Oren.
Quoted 78 lines
Do you have a testcase where restart doesn't work because of problems with
tty->pgrprestore? Or was this just something that caught your eye during review?Regardless, thanks for taking the time to put this patch together.
Quoted 38 lines
--- checkpoint/signal.c | 16 +++++++++++++--- include/linux/checkpoint_hdr.h | 2 +- 2 files changed, 14 insertions(+), 4 deletions(-)checkpoint/signal.c +1
-0@@ -323,6 +323,7 @@ static int checkpoint_signal(struct ckpt_ctx *ctx, struct task_struct *t) cputime_t cputime;unsigned long flags;int i, ret =0 ; +pid_t tty_pgrp, prgp_i; h =ckpt_hdr_get_type (ctx,sizeof (*h),CKPT_HDR_SIGNAL );if (!h)checkpoint/signal.c +10
-1@@ -411,7 +412,16 @@ static int checkpoint_signal(struct ckpt_ctx *ctx, struct task_struct *t) if (tty) {/* irq is already disabled */ spin_lock (&tty->ctrl_lock); - h->tty_pgrp =ckpt_pid_nr (ctx, tty->pgrp); + h->tty_pgrp_owner =-1 ; + tty_pgrp =ckpt_pid_nr (ctx, tty->pgrp); +for (i =0 ; i < ctx->nr_tasks; i++) { + pgrp_i =task_pgrp_nr_ns (ctx->tasks_arr[i], + ctx->root_nsproxy->pid_ns); +if (tty_pgrp == pgrp_i) { + h->tty_pgrp_owner = i; +break ; + } + }spin_unlock (&tty->ctrl_lock);tty_kref_put (tty);}checkpoint/signal.c static int restore_signal(struct ckpt_ctx *ctx) · line 550
Quoted 5 lines
checkpoint/signal.c +1
-1@@ -537,13 +547,13 @@ static int restore_signal(struct ckpt_ctx *ctx)if (ret <0 ) goto out;/* now restore the foreground group (job control) */ -if (h->tty_pgrp) { +if (h->tty_pgrp_owner !=-1 ) {Userspace can change
h->tty_pgrp_ownerto any value (bug, cosmic ray, exploit attempt) before passing the checkpoint image to restart. We don't want malicious programs to be able to cause an Oops or worse so we need to make sure the index is less than the number of entries in thepids_arr. Since the value was a prgrp id prior to this patch I doubt that check already exists.checkpoint/signal.c static int restore_signal(struct ckpt_ctx *ctx) · line 559
Quoted 10 lines
checkpoint/signal.c +1
-1/* * If tty_pgrp == CKPT_PID_NULL, below will * fail, so no need for explicit test */ ret =do_tiocspgrp (tty,tty_pair_get_tty (tty), - h->tty_pgrp); + ctx->pids_arr[h->tty_pgrp_owner].vpgid);if (ret <0 ) goto out; }<snip>
Cheers,
-Matt Helsley
_______________________________________________
Containers mailing list Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
https://lists.linux-foundation.org/mailman/listinfo/containers
Quoted 3 lines
Do you have a testcase where restart doesn't work because of problems with
tty->pgrprestore? Or was this just something that caught your eye during review?
Yes, we faced this while trying to restore an Xvnc session (bash running a script + Xvnc + twm + xterm + bash). It was failing with -ESRCH at this particular point, and we have been trying to solve it.
Quoted 6 lines
Userspace can change
h->tty_pgrp_ownerto any value (bug, cosmic ray, exploit attempt) before passing the checkpoint image to restart. We don't want malicious programs to be able to cause an Oops or worse so we need to make sure the index is less than the number of entries in thepids_arr. Since the value was a prgrp id prior to this patch I doubt that check already exists.
Understood.
It should be not
if (h->tty_pgrp_owner != -1), but
but
if ( h->tty_pgrp_owner >=0 && h->tty_pgrp_owner < ctx->nr_tasks)
Quoted 4 lines
However, I suspect this is already covered in userspace by rewriting the pids array prior to calling
sys_restart(). Seeckpt_init_tree()in user-cr'srestart.cand let me know if you agree and/or see any problems with that approach.
This may be indeed better, but it requires userspace to have knowledge about entire checkpoint structure, with (as far as I understand) is not the case currently, and may be hard to maintain while checkpoint format envolves in future.
Nikita