[PATCH] Deny external checkpoint unless frozen

[PATCH] Deny external checkpoint unless frozen

[Sukadev Bhattiprolu]

From: Sukadev Bhattiprolu <sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
Date: Sat, 21 Feb 2009 11:17:07 -0800
Subject: [PATCH] Deny external checkpoint unless task is frozen

Remove a 'FIXME' and ensure that the tasks we are checkpointing are
frozen unless its a self-checkpoint.

Signed-off-by: Sukadev Bhattiprolu <sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
---
 checkpoint/checkpoint.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/checkpoint/checkpoint.c b/checkpoint/checkpoint.c
index 2bbb409..023bd9d 100644
--- a/checkpoint/checkpoint.c
+++ b/checkpoint/checkpoint.c
@@ -19,6 +19,7 @@
 #include <linux/mount.h>
 #include <linux/utsname.h>
 #include <linux/magic.h>
+#include <linux/freezer.h>
 #include <linux/checkpoint.h>
 #include <linux/checkpoint_hdr.h>
 
@@ -280,7 +281,9 @@ static int cr_may_checkpoint_task(struct task_struct *t, struct cr_ctx *ctx)
 	if (!ptrace_may_access(t, PTRACE_MODE_READ))
 		return -EPERM;
 
-	/* FIXME: verify that the task is frozen (unless self) */
+	/* verify that the task is frozen (unless self) */
+	if (t != current && !frozen(t))
+		return -EBUSY;
 
 	/* FIXME: change this for nested containers */
 	if (task_nsproxy(t) != ctx->root_nsproxy)
-- 
1.5.2.5

Re: [PATCH] Deny external checkpoint unless frozen

[Serge E. Hallyn]

Quoting Sukadev Bhattiprolu (sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org):
> 
> From: Sukadev Bhattiprolu <sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> Date: Sat, 21 Feb 2009 11:17:07 -0800
> Subject: [PATCH] Deny external checkpoint unless task is frozen
> 
> Remove a 'FIXME' and ensure that the tasks we are checkpointing are
> frozen unless its a self-checkpoint.
> 
> Signed-off-by: Sukadev Bhattiprolu <sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>

Agreed.  I personally would like to just get rid of support
for t==current, but don't expect to get anywhere with that
argument :)

Acked-by: Serge Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>

> ---
>  checkpoint/checkpoint.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/checkpoint/checkpoint.c b/checkpoint/checkpoint.c
> index 2bbb409..023bd9d 100644
> --- a/checkpoint/checkpoint.c
> +++ b/checkpoint/checkpoint.c
> @@ -19,6 +19,7 @@
>  #include <linux/mount.h>
>  #include <linux/utsname.h>
>  #include <linux/magic.h>
> +#include <linux/freezer.h>
>  #include <linux/checkpoint.h>
>  #include <linux/checkpoint_hdr.h>
> 
> @@ -280,7 +281,9 @@ static int cr_may_checkpoint_task(struct task_struct *t, struct cr_ctx *ctx)
>  	if (!ptrace_may_access(t, PTRACE_MODE_READ))
>  		return -EPERM;
> 
> -	/* FIXME: verify that the task is frozen (unless self) */
> +	/* verify that the task is frozen (unless self) */
> +	if (t != current && !frozen(t))
> +		return -EBUSY;
> 
>  	/* FIXME: change this for nested containers */
>  	if (task_nsproxy(t) != ctx->root_nsproxy)
> -- 
> 1.5.2.5

Re: [PATCH] Deny external checkpoint unless frozen

[Dave Hansen]

On Mon, 2009-02-23 at 17:04 -0600, Serge E. Hallyn wrote:
> Quoting Sukadev Bhattiprolu (sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org):
> > 
> > From: Sukadev Bhattiprolu <sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> > Date: Sat, 21 Feb 2009 11:17:07 -0800
> > Subject: [PATCH] Deny external checkpoint unless task is frozen
> > 
> > Remove a 'FIXME' and ensure that the tasks we are checkpointing are
> > frozen unless its a self-checkpoint.
> > 
> > Signed-off-by: Sukadev Bhattiprolu <sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> 
> Agreed.  I personally would like to just get rid of support
> for t==current, but don't expect to get anywhere with that
> argument :)

Along the lines of what Ingo has been asking for, do we need to expose
this logic in some way?  Do we need a /proc/$$/checkpointable file which
says, "I'm not checkpointable because I'm not frozen"?

Or, is this just a core part of the API: you have to freeze before
checkpointing?  As such, we'll never move to a place where we're not
frozen when checkpointing, so we might as well not even track or expose
it.  

-- Dave

Re: [PATCH] Deny external checkpoint unless frozen

[Serge E. Hallyn]

Quoting Dave Hansen (dave-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org):
> On Mon, 2009-02-23 at 17:04 -0600, Serge E. Hallyn wrote:
> > Quoting Sukadev Bhattiprolu (sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org):
> > > 
> > > From: Sukadev Bhattiprolu <sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> > > Date: Sat, 21 Feb 2009 11:17:07 -0800
> > > Subject: [PATCH] Deny external checkpoint unless task is frozen
> > > 
> > > Remove a 'FIXME' and ensure that the tasks we are checkpointing are
> > > frozen unless its a self-checkpoint.
> > > 
> > > Signed-off-by: Sukadev Bhattiprolu <sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> > 
> > Agreed.  I personally would like to just get rid of support
> > for t==current, but don't expect to get anywhere with that
> > argument :)
> 
> Along the lines of what Ingo has been asking for, do we need to expose
> this logic in some way?  Do we need a /proc/$$/checkpointable file which
> says, "I'm not checkpointable because I'm not frozen"?

I really like that.

> Or, is this just a core part of the API: you have to freeze before
> checkpointing?  As such, we'll never move to a place where we're not
> frozen when checkpointing, so we might as well not even track or expose
> it.  

the only way that would make sense is if sys_checkpoint went ahead
and frozen them all, right?

-serge

Re: [PATCH] Deny external checkpoint unless frozen

[Dave Hansen]

On Mon, 2009-02-23 at 19:09 -0600, Serge E. Hallyn wrote:
> 
> > > Agreed.  I personally would like to just get rid of support
> > > for t==current, but don't expect to get anywhere with that
> > > argument :)
> > 
> > Along the lines of what Ingo has been asking for, do we need to expose
> > this logic in some way?  Do we need a /proc/$$/checkpointable file which
> > says, "I'm not checkpointable because I'm not frozen"?
> 
> I really like that.
> 
> > Or, is this just a core part of the API: you have to freeze before
> > checkpointing?  As such, we'll never move to a place where we're not
> > frozen when checkpointing, so we might as well not even track or expose
> > it.  
> 
> the only way that would make sense is if sys_checkpoint went ahead
> and frozen them all, right?

Yeah, I agree with that.

Does this mean Suka has to do the patch? ;)

-- Dave

Re: [PATCH] Deny external checkpoint unless frozen

[Serge E. Hallyn]

Quoting Dave Hansen (dave-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org):
> On Mon, 2009-02-23 at 19:09 -0600, Serge E. Hallyn wrote:
> > 
> > > > Agreed.  I personally would like to just get rid of support
> > > > for t==current, but don't expect to get anywhere with that
> > > > argument :)
> > > 
> > > Along the lines of what Ingo has been asking for, do we need to expose
> > > this logic in some way?  Do we need a /proc/$$/checkpointable file which
> > > says, "I'm not checkpointable because I'm not frozen"?
> > 
> > I really like that.
> > 
> > > Or, is this just a core part of the API: you have to freeze before
> > > checkpointing?  As such, we'll never move to a place where we're not
> > > frozen when checkpointing, so we might as well not even track or expose
> > > it.  
> > 
> > the only way that would make sense is if sys_checkpoint went ahead
> > and frozen them all, right?
> 
> Yeah, I agree with that.
> 
> Does this mean Suka has to do the patch? ;)

Heh.

Well the patch is mainly on top of your patchset which defines
the string holding reasons for uncheckpointability, right?  I
assume you've modified that since then (too bad we're on a
patch model at the moment) so seems easiest for you to toss it
on top of your set.

Or you can send your latest version and I (or Suka) can write
the /proc/$$/checkpointable file.

-serge

Re: [PATCH] Deny external checkpoint unless frozen

[Dave Hansen]

On Tue, 2009-02-24 at 12:31 -0600, Serge E. Hallyn wrote:
> Well the patch is mainly on top of your patchset which defines
> the string holding reasons for uncheckpointability, right?  I
> assume you've modified that since then (too bad we're on a
> patch model at the moment) so seems easiest for you to toss it
> on top of your set.
> 
> Or you can send your latest version and I (or Suka) can write
> the /proc/$$/checkpointable file.

What I have so far is actually only for fds and uses /proc/$$/fdinfo/*.
I haven't done the per-task file yet.

-- Dave

Re: [PATCH] Deny external checkpoint unless frozen

[Oren Laadan]

Fixed.

Sukadev Bhattiprolu wrote:
> From: Sukadev Bhattiprolu <sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> Date: Sat, 21 Feb 2009 11:17:07 -0800
> Subject: [PATCH] Deny external checkpoint unless task is frozen
> 
> Remove a 'FIXME' and ensure that the tasks we are checkpointing are
> frozen unless its a self-checkpoint.
> 
> Signed-off-by: Sukadev Bhattiprolu <sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> ---
>  checkpoint/checkpoint.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/checkpoint/checkpoint.c b/checkpoint/checkpoint.c
> index 2bbb409..023bd9d 100644
> --- a/checkpoint/checkpoint.c
> +++ b/checkpoint/checkpoint.c
> @@ -19,6 +19,7 @@
>  #include <linux/mount.h>
>  #include <linux/utsname.h>
>  #include <linux/magic.h>
> +#include <linux/freezer.h>
>  #include <linux/checkpoint.h>
>  #include <linux/checkpoint_hdr.h>
>  
> @@ -280,7 +281,9 @@ static int cr_may_checkpoint_task(struct task_struct *t, struct cr_ctx *ctx)
>  	if (!ptrace_may_access(t, PTRACE_MODE_READ))
>  		return -EPERM;
>  
> -	/* FIXME: verify that the task is frozen (unless self) */
> +	/* verify that the task is frozen (unless self) */
> +	if (t != current && !frozen(t))
> +		return -EBUSY;
>  
>  	/* FIXME: change this for nested containers */
>  	if (task_nsproxy(t) != ctx->root_nsproxy)