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

[Oren Laadan]

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 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.

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