[Devel] Re: [PATCH] c/r: restore tty pgrp properly

[Matt Helsley]

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 at lvk.cs.msu.su>

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.

> ---
>  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 at lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers