Matt Helsley wrote: > On Thu, Apr 08, 2010 at 03:50:56PM +0000, Leonid Snegirev wrote: >> 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. >> >> Signed-off-by: Leonid Snegirev <leo@xxxxxxxxxxxxx> > > 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. 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. > > 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. > >> --- >> checkpoint/signal.c | 16 +++++++++++++--- >> include/linux/checkpoint_hdr.h | 2 +- >> 2 files changed, 14 insertions(+), 4 deletions(-) >> >> diff --git a/checkpoint/signal.c b/checkpoint/signal.c >> index 9d0e9c3..9c90e96 100644 >> --- a/checkpoint/signal.c >> +++ b/checkpoint/signal.c >> @@ -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) { > > 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. > >> /* >> * 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@xxxxxxxxxxxxxxxxxxxxxxxxxx > https://lists.linux-foundation.org/mailman/listinfo/containers > _______________________________________________ Containers mailing list Containers@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linux-foundation.org/mailman/listinfo/containers