[patch 0/4] A few patches in a sake of c/r functionality

[patch 0/4] A few patches in a sake of c/r functionality

[Cyrill Gorcunov]

Hi,

here are a few patches in a sake of c/r. Nothing really
serious except a new kcmp syscall I think. Please review,
comments are really appreciated!

	Cyrill

[patch 1/4] fs, proc: Introduce /proc/<pid>/task/<tid>/children entry v8

[Cyrill Gorcunov]

[-- Attachment #1: fs-proc-tid-children-12 --]
[-- Type: text/plain, Size: 8035 bytes --]

When we do checkpoint of a task we need to know the list of children
the task, has but there is no easy and fast way to generate reverse
parent->children chain from arbitrary <pid> (while a parent pid is
provided in "PPid" field of /proc/<pid>/status).

So instead of walking over all pids in the system (creating one big process
tree in memory, just to figure out which children a task has) -- we add
explicit /proc/<pid>/task/<tid>/children entry, because the kernel already has
this kind of information but it is not yet exported.

This is a first level children, not the whole process tree.

v2:
 - Kame suggested to use a separated /proc/<pid>/children entry
   instead of poking /proc/<pid>/status
 - Andew suggested to use rcu facility instead of locking
   tasklist_lock
 - Tejun pointed that non-seekable seq file might not be
   enough for tasks with large number of children

v3:
 - To be on a safe side use %lu format for pid_t printing

v4:
 - New line get printed when sequence ends not at seq->stop,
   a nit pointed by Tejun
 - Documentation update
 - tasklist_lock is back, Oleg pointed that ->children list
   is actually not rcu-safe

v5:
 - Oleg suggested to make /proc/<pid>/task/<tid>/children
   instead of global /proc/<pid>/children, which eliminates
   hardness related to threads and children migration, and
   allows patch to be a way simplier.

v6:
 - Drop ptrace_may_access tests, pids are can be found anyway
   so nothing to protect here.
 - Update comments and docs, pointed by Oleg.

v7:
 - Use get_pid over proc-pid directly, to simplify
   code, pointed by Oleg.

v8:
 - Obtain a starting pid from the proc's inode directly.

Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
Reviewed-by: Oleg Nesterov <oleg@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Pavel Emelyanov <xemul@parallels.com>
Cc: Serge Hallyn <serge.hallyn@canonical.com>
Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
---
 Documentation/filesystems/proc.txt |   18 +++++
 fs/proc/array.c                    |  123 +++++++++++++++++++++++++++++++++++++
 fs/proc/base.c                     |    1 
 fs/proc/internal.h                 |    1 
 4 files changed, 143 insertions(+)

Index: linux-2.6.git/Documentation/filesystems/proc.txt
===================================================================
--- linux-2.6.git.orig/Documentation/filesystems/proc.txt
+++ linux-2.6.git/Documentation/filesystems/proc.txt
@@ -40,6 +40,7 @@ Table of Contents
   3.4	/proc/<pid>/coredump_filter - Core dump filtering settings
   3.5	/proc/<pid>/mountinfo - Information about mounts
   3.6	/proc/<pid>/comm  & /proc/<pid>/task/<tid>/comm
+  3.7   /proc/<pid>/task/<tid>/children - Information about task children
 
   4	Configuring procfs
   4.1	Mount options
@@ -1549,6 +1550,23 @@ then the kernel's TASK_COMM_LEN (current
 comm value.
 
 
+3.7	/proc/<pid>/task/<tid>/children - Information about task children
+-------------------------------------------------------------------------
+This file provides a fast way to retrieve first level children pids
+of a task pointed by <pid>/<tid> pair. The format is a space separated
+stream of pids.
+
+Note the "first level" here -- if a child has own children they will
+not be listed here, one needs to read /proc/<children-pid>/task/<tid>/children
+to obtain the descendants.
+
+Since this interface is intended to be fast and cheap it doesn't
+guarantee to provide precise results and some children might be
+skipped, especially if they've exited right after we printed their
+pids, so one need to either stop or freeze processes being inspected
+if precise results are needed.
+
+
 ------------------------------------------------------------------------------
 Configuring procfs
 ------------------------------------------------------------------------------
Index: linux-2.6.git/fs/proc/array.c
===================================================================
--- linux-2.6.git.orig/fs/proc/array.c
+++ linux-2.6.git/fs/proc/array.c
@@ -547,3 +547,126 @@ int proc_pid_statm(struct seq_file *m, s
 
 	return 0;
 }
+
+static struct pid *
+get_children_pid(struct inode *inode, struct pid *pid_prev, loff_t pos)
+{
+	struct task_struct *start, *task;
+	struct pid *pid = NULL;
+
+	read_lock(&tasklist_lock);
+
+	start = pid_task(proc_pid(inode), PIDTYPE_PID);
+	if (!start)
+		goto out;
+
+	/*
+	 * Lets try to continue searching first, this gives
+	 * us significant speedup on children-rich processes.
+	 */
+	if (pid_prev) {
+		task = pid_task(pid_prev, PIDTYPE_PID);
+		if (task && task->real_parent == start &&
+		    !(list_empty(&task->sibling))) {
+			if (list_is_last(&task->sibling, &start->children))
+				goto out;
+			task = list_first_entry(&task->sibling,
+						struct task_struct, sibling);
+			pid = get_pid(task_pid(task));
+			goto out;
+		}
+	}
+
+	/*
+	 * Slow search case.
+	 *
+	 * We might miss some children here if children
+	 * are exited while we were not holding the lock,
+	 * but it was never promised to be accurate that
+	 * much.
+	 *
+	 * "Just suppose that the parent sleeps, but N children
+	 *  exit after we printed their tids. Now the slow paths
+	 *  skips N extra children, we miss N tasks." (c)
+	 *
+	 * So one need to stop or freeze the leader and all
+	 * its children to get a precise result.
+	 */
+	list_for_each_entry(task, &start->children, sibling) {
+		if (pos-- == 0) {
+			pid = get_pid(task_pid(task));
+			break;
+		}
+	}
+
+out:
+	read_unlock(&tasklist_lock);
+	return pid;
+}
+
+static int children_seq_show(struct seq_file *seq, void *v)
+{
+	struct inode *inode = seq->private;
+	pid_t pid;
+
+	pid = pid_nr_ns(v, inode->i_sb->s_fs_info);
+	return seq_printf(seq, " %d", pid);
+}
+
+static void *children_seq_start(struct seq_file *seq, loff_t *pos)
+{
+	return get_children_pid(seq->private, NULL, *pos);
+}
+
+static void *children_seq_next(struct seq_file *seq, void *v, loff_t *pos)
+{
+	struct pid *pid = NULL;
+
+	pid = get_children_pid(seq->private, v, *pos + 1);
+	if (!pid)
+		seq_printf(seq, "\n");
+	put_pid(v);
+
+	++*pos;
+	return pid;
+}
+
+static void children_seq_stop(struct seq_file *seq, void *v)
+{
+	put_pid(v);
+}
+
+static const struct seq_operations children_seq_ops = {
+	.start	= children_seq_start,
+	.next	= children_seq_next,
+	.stop	= children_seq_stop,
+	.show	= children_seq_show,
+};
+
+static int children_seq_open(struct inode *inode, struct file *file)
+{
+	struct seq_file *m;
+	int ret;
+
+	ret = seq_open(file, &children_seq_ops);
+	if (ret)
+		return ret;
+
+	m = file->private_data;
+	m->private = inode;
+
+	return ret;
+}
+
+int children_seq_release(struct inode *inode, struct file *file)
+{
+	seq_release(inode, file);
+	return 0;
+}
+
+const struct file_operations proc_tid_children_operations = {
+	.open    = children_seq_open,
+	.read    = seq_read,
+	.llseek  = seq_lseek,
+	.release = children_seq_release,
+};
Index: linux-2.6.git/fs/proc/base.c
===================================================================
--- linux-2.6.git.orig/fs/proc/base.c
+++ linux-2.6.git/fs/proc/base.c
@@ -3384,6 +3384,7 @@ static const struct pid_entry tid_base_s
 	ONE("stat",      S_IRUGO, proc_tid_stat),
 	ONE("statm",     S_IRUGO, proc_pid_statm),
 	REG("maps",      S_IRUGO, proc_maps_operations),
+	REG("children",  S_IRUGO, proc_tid_children_operations),
 #ifdef CONFIG_NUMA
 	REG("numa_maps", S_IRUGO, proc_numa_maps_operations),
 #endif
Index: linux-2.6.git/fs/proc/internal.h
===================================================================
--- linux-2.6.git.orig/fs/proc/internal.h
+++ linux-2.6.git/fs/proc/internal.h
@@ -53,6 +53,7 @@ extern int proc_pid_statm(struct seq_fil
 				struct pid *pid, struct task_struct *task);
 extern loff_t mem_lseek(struct file *file, loff_t offset, int orig);
 
+extern const struct file_operations proc_tid_children_operations;
 extern const struct file_operations proc_maps_operations;
 extern const struct file_operations proc_numa_maps_operations;
 extern const struct file_operations proc_smaps_operations;

Re: [patch 1/4] fs, proc: Introduce /proc/<pid>/task/<tid>/children entry v8

[Kees Cook]

On Mon, Jan 23, 2012 at 6:20 AM, Cyrill Gorcunov <gorcunov@openvz.org> wrote:
> When we do checkpoint of a task we need to know the list of children
> the task, has but there is no easy and fast way to generate reverse
> parent->children chain from arbitrary <pid> (while a parent pid is
> provided in "PPid" field of /proc/<pid>/status).
> [...]
> Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
> Reviewed-by: Oleg Nesterov <oleg@redhat.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Pavel Emelyanov <xemul@parallels.com>
> Cc: Serge Hallyn <serge.hallyn@canonical.com>
> Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

Reviewed-by: Kees Cook <keescook@chromium.org>

> +static int children_seq_show(struct seq_file *seq, void *v)
> +{
> +       struct inode *inode = seq->private;
> +       pid_t pid;
> +
> +       pid = pid_nr_ns(v, inode->i_sb->s_fs_info);
> +       return seq_printf(seq, " %d", pid);
> +}

Does this mean the file contents always starts with a space? I think
I'd prefer a trailing space than a leading one. Better yet, neither.
:)

-Kees

-- 
Kees Cook
ChromeOS Security

Re: [patch 1/4] fs, proc: Introduce /proc/<pid>/task/<tid>/children entry v8

[Cyrill Gorcunov]

On Mon, Jan 23, 2012 at 10:54:53AM -0800, Kees Cook wrote:
> On Mon, Jan 23, 2012 at 6:20 AM, Cyrill Gorcunov <gorcunov@openvz.org> wrote:
> > When we do checkpoint of a task we need to know the list of children
> > the task, has but there is no easy and fast way to generate reverse
> > parent->children chain from arbitrary <pid> (while a parent pid is
> > provided in "PPid" field of /proc/<pid>/status).
> > [...]
> > Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
> > Reviewed-by: Oleg Nesterov <oleg@redhat.com>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Pavel Emelyanov <xemul@parallels.com>
> > Cc: Serge Hallyn <serge.hallyn@canonical.com>
> > Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> 
> Reviewed-by: Kees Cook <keescook@chromium.org>
> 
> > +static int children_seq_show(struct seq_file *seq, void *v)
> > +{
> > +       struct inode *inode = seq->private;
> > +       pid_t pid;
> > +
> > +       pid = pid_nr_ns(v, inode->i_sb->s_fs_info);
> > +       return seq_printf(seq, " %d", pid);
> > +}
> 
> Does this mean the file contents always starts with a space? I think
> I'd prefer a trailing space than a leading one. Better yet, neither.
> :)
> 

Yeah, it there children, they will be in say " 1 2 3 4\n" format.
To drop this space completely i'll have to add more code, which I
actually trying to escape. Can we live with it? ;)

	Cyrill

Re: [patch 1/4] fs, proc: Introduce /proc/<pid>/task/<tid>/children entry v8

[Kees Cook]

On Mon, Jan 23, 2012 at 11:33 AM, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> On Mon, Jan 23, 2012 at 10:54:53AM -0800, Kees Cook wrote:
>> On Mon, Jan 23, 2012 at 6:20 AM, Cyrill Gorcunov <gorcunov@openvz.org> wrote:
>> > When we do checkpoint of a task we need to know the list of children
>> > the task, has but there is no easy and fast way to generate reverse
>> > parent->children chain from arbitrary <pid> (while a parent pid is
>> > provided in "PPid" field of /proc/<pid>/status).
>> > [...]
>> > Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
>> > Reviewed-by: Oleg Nesterov <oleg@redhat.com>
>> > Cc: Andrew Morton <akpm@linux-foundation.org>
>> > Cc: Pavel Emelyanov <xemul@parallels.com>
>> > Cc: Serge Hallyn <serge.hallyn@canonical.com>
>> > Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
>>
>> Reviewed-by: Kees Cook <keescook@chromium.org>
>>
>> > +static int children_seq_show(struct seq_file *seq, void *v)
>> > +{
>> > +       struct inode *inode = seq->private;
>> > +       pid_t pid;
>> > +
>> > +       pid = pid_nr_ns(v, inode->i_sb->s_fs_info);
>> > +       return seq_printf(seq, " %d", pid);
>> > +}
>>
>> Does this mean the file contents always starts with a space? I think
>> I'd prefer a trailing space than a leading one. Better yet, neither.
>> :)
>>
>
> Yeah, it there children, they will be in say " 1 2 3 4\n" format.
> To drop this space completely i'll have to add more code, which I
> actually trying to escape. Can we live with it? ;)

How about just:

return seq_printf(seq, "%d ", pid);

instead?

-Kees

-- 
Kees Cook
ChromeOS Security

Re: [patch 1/4] fs, proc: Introduce /proc/<pid>/task/<tid>/children entry v8

[Cyrill Gorcunov]

On Mon, Jan 23, 2012 at 12:29:52PM -0800, Kees Cook wrote:
> >
> > Yeah, it there children, they will be in say " 1 2 3 4\n" format.
> > To drop this space completely i'll have to add more code, which I
> > actually trying to escape. Can we live with it? ;)
> 
> How about just:
> 
> return seq_printf(seq, "%d ", pid);
> 
> instead?
> 

ok, will update.

	Cyrill

Re: [patch 1/4] fs, proc: Introduce /proc/<pid>/task/<tid>/children entry v8

[KAMEZAWA Hiroyuki]

On Mon, 23 Jan 2012 18:20:37 +0400
Cyrill Gorcunov <gorcunov@openvz.org> wrote:

> When we do checkpoint of a task we need to know the list of children
> the task, has but there is no easy and fast way to generate reverse
> parent->children chain from arbitrary <pid> (while a parent pid is
> provided in "PPid" field of /proc/<pid>/status).
> 
> So instead of walking over all pids in the system (creating one big process
> tree in memory, just to figure out which children a task has) -- we add
> explicit /proc/<pid>/task/<tid>/children entry, because the kernel already has
> this kind of information but it is not yet exported.
> 
> This is a first level children, not the whole process tree.
> 
> v2:
>  - Kame suggested to use a separated /proc/<pid>/children entry
>    instead of poking /proc/<pid>/status
>  - Andew suggested to use rcu facility instead of locking
>    tasklist_lock
>  - Tejun pointed that non-seekable seq file might not be
>    enough for tasks with large number of children
> 
> v3:
>  - To be on a safe side use %lu format for pid_t printing
> 
> v4:
>  - New line get printed when sequence ends not at seq->stop,
>    a nit pointed by Tejun
>  - Documentation update
>  - tasklist_lock is back, Oleg pointed that ->children list
>    is actually not rcu-safe
> 
> v5:
>  - Oleg suggested to make /proc/<pid>/task/<tid>/children
>    instead of global /proc/<pid>/children, which eliminates
>    hardness related to threads and children migration, and
>    allows patch to be a way simplier.
> 
> v6:
>  - Drop ptrace_may_access tests, pids are can be found anyway
>    so nothing to protect here.
>  - Update comments and docs, pointed by Oleg.
> 
> v7:
>  - Use get_pid over proc-pid directly, to simplify
>    code, pointed by Oleg.
> 
> v8:
>  - Obtain a starting pid from the proc's inode directly.
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
> Reviewed-by: Oleg Nesterov <oleg@redhat.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Pavel Emelyanov <xemul@parallels.com>
> Cc: Serge Hallyn <serge.hallyn@canonical.com>
> Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

>From viewpoint I played with seq_file, yesterday.

> +static void *children_seq_start(struct seq_file *seq, loff_t *pos)
> +{
> +	return get_children_pid(seq->private, NULL, *pos);
> +}
> +
> +static void *children_seq_next(struct seq_file *seq, void *v, loff_t *pos)
> +{
> +	struct pid *pid = NULL;
> +
> +	pid = get_children_pid(seq->private, v, *pos + 1);
> +	if (!pid)
> +		seq_printf(seq, "\n");
> +	put_pid(v);

Because seq_printf() may fail. This seems dangeorus.

If seq_printf() fails and returns NULL, "\n" will not be
printed out and user land parser will go wrong.

I think all seq_printf() should be handled in ->show().
(And you can use seq_putc() for "\n".)

Thanks,
-Kame

Re: [patch 1/4] fs, proc: Introduce /proc/<pid>/task/<tid>/children entry v8

[Cyrill Gorcunov]

On Tue, Jan 24, 2012 at 11:07:30AM +0900, KAMEZAWA Hiroyuki wrote:
...
> 
> From viewpoint I played with seq_file, yesterday.
> 
> > +static void *children_seq_start(struct seq_file *seq, loff_t *pos)
> > +{
> > +	return get_children_pid(seq->private, NULL, *pos);
> > +}
> > +
> > +static void *children_seq_next(struct seq_file *seq, void *v, loff_t *pos)
> > +{
> > +	struct pid *pid = NULL;
> > +
> > +	pid = get_children_pid(seq->private, v, *pos + 1);
> > +	if (!pid)
> > +		seq_printf(seq, "\n");
> > +	put_pid(v);
> 
> Because seq_printf() may fail. This seems dangeorus.
> 
> If seq_printf() fails and returns NULL, "\n" will not be
> printed out and user land parser will go wrong.
>

Hmm. But userspace app will get eof, so frankly I don't see
a problem here. Or maybe I miss something?

> I think all seq_printf() should be handled in ->show().
> (And you can use seq_putc() for "\n".)
> 

Sure, i'll change it to seq_putc.

	Cyrill

Re: [patch 1/4] fs, proc: Introduce /proc/<pid>/task/<tid>/children entry v8

[KAMEZAWA Hiroyuki]

On Tue, 24 Jan 2012 10:53:38 +0400
Cyrill Gorcunov <gorcunov@gmail.com> wrote:

> On Tue, Jan 24, 2012 at 11:07:30AM +0900, KAMEZAWA Hiroyuki wrote:
> ...
> > 
> > From viewpoint I played with seq_file, yesterday.
> > 
> > > +static void *children_seq_start(struct seq_file *seq, loff_t *pos)
> > > +{
> > > +	return get_children_pid(seq->private, NULL, *pos);
> > > +}
> > > +
> > > +static void *children_seq_next(struct seq_file *seq, void *v, loff_t *pos)
> > > +{
> > > +	struct pid *pid = NULL;
> > > +
> > > +	pid = get_children_pid(seq->private, v, *pos + 1);
> > > +	if (!pid)
> > > +		seq_printf(seq, "\n");
> > > +	put_pid(v);
> > 
> > Because seq_printf() may fail. This seems dangeorus.
> > 
> > If seq_printf() fails and returns NULL, "\n" will not be
> > printed out and user land parser will go wrong.
> >
> 
> Hmm. But userspace app will get eof, so frankly I don't see
> a problem here. Or maybe I miss something?
> 

Userspace need to take care of whether there may be"\n" or not even
if read() returns EOF.
As an interface, it's BUG to say "\n" will be there if you're lucky!"
(*) I know script language can handle this but we shouldn't assume that.

How about just remove "\n" at EOF  ? I think it's unnecessary.

Thanks,
-Kame

Re: [patch 1/4] fs, proc: Introduce /proc/<pid>/task/<tid>/children entry v8

[Cyrill Gorcunov]

On Tue, Jan 24, 2012 at 04:07:09PM +0900, KAMEZAWA Hiroyuki wrote:
> > 
> > Hmm. But userspace app will get eof, so frankly I don't see
> > a problem here. Or maybe I miss something?
> > 
> 
> Userspace need to take care of whether there may be"\n" or not even
> if read() returns EOF.
> As an interface, it's BUG to say "\n" will be there if you're lucky!"
> (*) I know script language can handle this but we shouldn't assume that.
> 
> How about just remove "\n" at EOF  ? I think it's unnecessary.
> 

Sure thing, it's not a problem to remove it completely.

	Cyrill

Re: [patch 1/4] fs, proc: Introduce /proc/<pid>/task/<tid>/children entry v8

[Eric W. Biederman]

Cyrill Gorcunov <gorcunov@gmail.com> writes:

> On Tue, Jan 24, 2012 at 04:07:09PM +0900, KAMEZAWA Hiroyuki wrote:
>> > 
>> > Hmm. But userspace app will get eof, so frankly I don't see
>> > a problem here. Or maybe I miss something?
>> > 
>> 
>> Userspace need to take care of whether there may be"\n" or not even
>> if read() returns EOF.
>> As an interface, it's BUG to say "\n" will be there if you're lucky!"
>> (*) I know script language can handle this but we shouldn't assume that.
>> 
>> How about just remove "\n" at EOF  ? I think it's unnecessary.
>> 
>
> Sure thing, it's not a problem to remove it completely.

Foolish question.  Is there any reason why this is a file instead
of being the obvious directory full of symlinks?

Eric

Re: [patch 1/4] fs, proc: Introduce /proc/<pid>/task/<tid>/children entry v8

[Cyrill Gorcunov]

On Tue, Jan 24, 2012 at 12:52:03AM -0800, Eric W. Biederman wrote:
> Cyrill Gorcunov <gorcunov@gmail.com> writes:
> 
> > On Tue, Jan 24, 2012 at 04:07:09PM +0900, KAMEZAWA Hiroyuki wrote:
> >> > 
> >> > Hmm. But userspace app will get eof, so frankly I don't see
> >> > a problem here. Or maybe I miss something?
> >> > 
> >> 
> >> Userspace need to take care of whether there may be"\n" or not even
> >> if read() returns EOF.
> >> As an interface, it's BUG to say "\n" will be there if you're lucky!"
> >> (*) I know script language can handle this but we shouldn't assume that.
> >> 
> >> How about just remove "\n" at EOF  ? I think it's unnecessary.
> >> 
> >
> > Sure thing, it's not a problem to remove it completely.
> 
> Foolish question.  Is there any reason why this is a file instead
> of being the obvious directory full of symlinks?
> 

How would these symlinks look like? "../../pid"? There were a conversation
about such things (https://lkml.org/lkml/2011/12/2/142) but I suppose we
were agree on children with pids as consensus.

	Cyrill

Re: [patch 1/4] fs, proc: Introduce /proc/<pid>/task/<tid>/children entry v8

[KOSAKI Motohiro]

(1/24/12 4:11 AM), Cyrill Gorcunov wrote:
> On Tue, Jan 24, 2012 at 12:52:03AM -0800, Eric W. Biederman wrote:
>> Cyrill Gorcunov<gorcunov@gmail.com>  writes:
>>
>>> On Tue, Jan 24, 2012 at 04:07:09PM +0900, KAMEZAWA Hiroyuki wrote:
>>>>>
>>>>> Hmm. But userspace app will get eof, so frankly I don't see
>>>>> a problem here. Or maybe I miss something?
>>>>>
>>>>
>>>> Userspace need to take care of whether there may be"\n" or not even
>>>> if read() returns EOF.
>>>> As an interface, it's BUG to say "\n" will be there if you're lucky!"
>>>> (*) I know script language can handle this but we shouldn't assume that.
>>>>
>>>> How about just remove "\n" at EOF  ? I think it's unnecessary.
>>>>
>>>
>>> Sure thing, it's not a problem to remove it completely.
>>
>> Foolish question.  Is there any reason why this is a file instead
>> of being the obvious directory full of symlinks?
>>
>
> How would these symlinks look like? "../../pid"? There were a conversation
> about such things (https://lkml.org/lkml/2011/12/2/142) but I suppose we
> were agree on children with pids as consensus.

I couldn't find any agreement in this link. Suppose wrong url?

Re: [patch 1/4] fs, proc: Introduce /proc/<pid>/task/<tid>/children entry v8

[Eric W. Biederman]

KOSAKI Motohiro <kosaki.motohiro@gmail.com> writes:

> (1/24/12 4:11 AM), Cyrill Gorcunov wrote:
>> On Tue, Jan 24, 2012 at 12:52:03AM -0800, Eric W. Biederman wrote:
>>> Cyrill Gorcunov<gorcunov@gmail.com>  writes:
>>>
>>>> On Tue, Jan 24, 2012 at 04:07:09PM +0900, KAMEZAWA Hiroyuki wrote:
>>>>>>
>>>>>> Hmm. But userspace app will get eof, so frankly I don't see
>>>>>> a problem here. Or maybe I miss something?
>>>>>>
>>>>>
>>>>> Userspace need to take care of whether there may be"\n" or not even
>>>>> if read() returns EOF.
>>>>> As an interface, it's BUG to say "\n" will be there if you're lucky!"
>>>>> (*) I know script language can handle this but we shouldn't assume that.
>>>>>
>>>>> How about just remove "\n" at EOF  ? I think it's unnecessary.
>>>>>
>>>>
>>>> Sure thing, it's not a problem to remove it completely.
>>>
>>> Foolish question.  Is there any reason why this is a file instead
>>> of being the obvious directory full of symlinks?
>>>
>>
>> How would these symlinks look like? "../../pid"? There were a conversation
>> about such things (https://lkml.org/lkml/2011/12/2/142) but I suppose we
>> were agree on children with pids as consensus.
>
> I couldn't find any agreement in this link. Suppose wrong url?

Now that you have reminded me of this thread.  I can say that the
link would need to look like ../../pid.  Our children will always
be thread group leaders, so we can safely point to the /proc/<pid>
directories.  So readlink would return ../../<pid> or however many
dots are needed.  Follow link could just warp us to that directory
as it does for the other magic proc symlinks.

My feeling is that a children subdirectory would be a lot more useful
than a simple file that lists the children.

Eric

Re: [patch 1/4] fs, proc: Introduce /proc/<pid>/task/<tid>/children entry v8

[Cyrill Gorcunov]

On Tue, Jan 24, 2012 at 06:11:21PM -0800, Eric W. Biederman wrote:
> KOSAKI Motohiro <kosaki.motohiro@gmail.com> writes:
> >>
> >> How would these symlinks look like? "../../pid"? There were a conversation
> >> about such things (https://lkml.org/lkml/2011/12/2/142) but I suppose we
> >> were agree on children with pids as consensus.
> >
> > I couldn't find any agreement in this link. Suppose wrong url?
> 
> Now that you have reminded me of this thread.  I can say that the
> link would need to look like ../../pid.  Our children will always
> be thread group leaders, so we can safely point to the /proc/<pid>
> directories.  So readlink would return ../../<pid> or however many
> dots are needed.  Follow link could just warp us to that directory
> as it does for the other magic proc symlinks.
> 
> My feeling is that a children subdirectory would be a lot more useful
> than a simple file that lists the children.
> 

I'll check what I can do, thanks.

	Cyrill

Re: [patch 1/4] fs, proc: Introduce /proc/<pid>/task/<tid>/children entry v8

[Cyrill Gorcunov]

On Wed, Jan 25, 2012 at 10:55:51AM +0400, Cyrill Gorcunov wrote:
> > 
> > Now that you have reminded me of this thread.  I can say that the
> > link would need to look like ../../pid.  Our children will always
> > be thread group leaders, so we can safely point to the /proc/<pid>
> > directories.  So readlink would return ../../<pid> or however many
> > dots are needed.  Follow link could just warp us to that directory
> > as it does for the other magic proc symlinks.
> > 
> > My feeling is that a children subdirectory would be a lot more useful
> > than a simple file that lists the children.
> > 
> 
> I'll check what I can do, thanks.
> 

Sigh. This will require complete code rewrite. And since I've just
made it to live under CONFIG_CHECKPOINT_RESTORE I would prefer if
we stick with children file. So if there are no strong objections
agains 'children' as a file, could we leave it as is, ie in
stream-of-pids form?

	Cyrill

Re: [patch 1/4] fs, proc: Introduce /proc/<pid>/task/<tid>/children entry v8

[Cyrill Gorcunov]

On Tue, Jan 24, 2012 at 04:07:09PM +0900, KAMEZAWA Hiroyuki wrote:
> On Tue, 24 Jan 2012 10:53:38 +0400
> Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> 
> > On Tue, Jan 24, 2012 at 11:07:30AM +0900, KAMEZAWA Hiroyuki wrote:
> > ...
> > > 
> > > From viewpoint I played with seq_file, yesterday.
> > > 
> > > > +static void *children_seq_start(struct seq_file *seq, loff_t *pos)
> > > > +{
> > > > +	return get_children_pid(seq->private, NULL, *pos);
> > > > +}
> > > > +
> > > > +static void *children_seq_next(struct seq_file *seq, void *v, loff_t *pos)
> > > > +{
> > > > +	struct pid *pid = NULL;
> > > > +
> > > > +	pid = get_children_pid(seq->private, v, *pos + 1);
> > > > +	if (!pid)
> > > > +		seq_printf(seq, "\n");
> > > > +	put_pid(v);
> > > 
> > > Because seq_printf() may fail. This seems dangeorus.
> > > 
> > > If seq_printf() fails and returns NULL, "\n" will not be
> > > printed out and user land parser will go wrong.
> > >
> > 
> > Hmm. But userspace app will get eof, so frankly I don't see
> > a problem here. Or maybe I miss something?
> > 
> 
> Userspace need to take care of whether there may be"\n" or not even
> if read() returns EOF.
> As an interface, it's BUG to say "\n" will be there if you're lucky!"
> (*) I know script language can handle this but we shouldn't assume that.
> 
> How about just remove "\n" at EOF  ? I think it's unnecessary.
> 

This one should fit both "%d " and no "\n" requirements.

	Cyrill
---
From: Cyrill Gorcunov <gorcunov@openvz.org>
Subject: fs, proc: Introduce /proc/<pid>/task/<tid>/children entry v9

When we do checkpoint of a task we need to know the list of children
the task, has but there is no easy and fast way to generate reverse
parent->children chain from arbitrary <pid> (while a parent pid is
provided in "PPid" field of /proc/<pid>/status).

So instead of walking over all pids in the system (creating one big process
tree in memory, just to figure out which children a task has) -- we add
explicit /proc/<pid>/task/<tid>/children entry, because the kernel already has
this kind of information but it is not yet exported.

This is a first level children, not the whole process tree.

Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
Reviewed-by: Oleg Nesterov <oleg@redhat.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Pavel Emelyanov <xemul@parallels.com>
Cc: Serge Hallyn <serge.hallyn@canonical.com>
Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
---
 Documentation/filesystems/proc.txt |   18 +++++
 fs/proc/array.c                    |  121 +++++++++++++++++++++++++++++++++++++
 fs/proc/base.c                     |    1 
 fs/proc/internal.h                 |    1 
 4 files changed, 141 insertions(+)

Index: linux-2.6.git/Documentation/filesystems/proc.txt
===================================================================
--- linux-2.6.git.orig/Documentation/filesystems/proc.txt
+++ linux-2.6.git/Documentation/filesystems/proc.txt
@@ -40,6 +40,7 @@ Table of Contents
   3.4	/proc/<pid>/coredump_filter - Core dump filtering settings
   3.5	/proc/<pid>/mountinfo - Information about mounts
   3.6	/proc/<pid>/comm  & /proc/<pid>/task/<tid>/comm
+  3.7   /proc/<pid>/task/<tid>/children - Information about task children
 
   4	Configuring procfs
   4.1	Mount options
@@ -1549,6 +1550,23 @@ then the kernel's TASK_COMM_LEN (current
 comm value.
 
 
+3.7	/proc/<pid>/task/<tid>/children - Information about task children
+-------------------------------------------------------------------------
+This file provides a fast way to retrieve first level children pids
+of a task pointed by <pid>/<tid> pair. The format is a space separated
+stream of pids.
+
+Note the "first level" here -- if a child has own children they will
+not be listed here, one needs to read /proc/<children-pid>/task/<tid>/children
+to obtain the descendants.
+
+Since this interface is intended to be fast and cheap it doesn't
+guarantee to provide precise results and some children might be
+skipped, especially if they've exited right after we printed their
+pids, so one need to either stop or freeze processes being inspected
+if precise results are needed.
+
+
 ------------------------------------------------------------------------------
 Configuring procfs
 ------------------------------------------------------------------------------
Index: linux-2.6.git/fs/proc/array.c
===================================================================
--- linux-2.6.git.orig/fs/proc/array.c
+++ linux-2.6.git/fs/proc/array.c
@@ -547,3 +547,124 @@ int proc_pid_statm(struct seq_file *m, s
 
 	return 0;
 }
+
+static struct pid *
+get_children_pid(struct inode *inode, struct pid *pid_prev, loff_t pos)
+{
+	struct task_struct *start, *task;
+	struct pid *pid = NULL;
+
+	read_lock(&tasklist_lock);
+
+	start = pid_task(proc_pid(inode), PIDTYPE_PID);
+	if (!start)
+		goto out;
+
+	/*
+	 * Lets try to continue searching first, this gives
+	 * us significant speedup on children-rich processes.
+	 */
+	if (pid_prev) {
+		task = pid_task(pid_prev, PIDTYPE_PID);
+		if (task && task->real_parent == start &&
+		    !(list_empty(&task->sibling))) {
+			if (list_is_last(&task->sibling, &start->children))
+				goto out;
+			task = list_first_entry(&task->sibling,
+						struct task_struct, sibling);
+			pid = get_pid(task_pid(task));
+			goto out;
+		}
+	}
+
+	/*
+	 * Slow search case.
+	 *
+	 * We might miss some children here if children
+	 * are exited while we were not holding the lock,
+	 * but it was never promised to be accurate that
+	 * much.
+	 *
+	 * "Just suppose that the parent sleeps, but N children
+	 *  exit after we printed their tids. Now the slow paths
+	 *  skips N extra children, we miss N tasks." (c)
+	 *
+	 * So one need to stop or freeze the leader and all
+	 * its children to get a precise result.
+	 */
+	list_for_each_entry(task, &start->children, sibling) {
+		if (pos-- == 0) {
+			pid = get_pid(task_pid(task));
+			break;
+		}
+	}
+
+out:
+	read_unlock(&tasklist_lock);
+	return pid;
+}
+
+static int children_seq_show(struct seq_file *seq, void *v)
+{
+	struct inode *inode = seq->private;
+	pid_t pid;
+
+	pid = pid_nr_ns(v, inode->i_sb->s_fs_info);
+	return seq_printf(seq, "%d ", pid);
+}
+
+static void *children_seq_start(struct seq_file *seq, loff_t *pos)
+{
+	return get_children_pid(seq->private, NULL, *pos);
+}
+
+static void *children_seq_next(struct seq_file *seq, void *v, loff_t *pos)
+{
+	struct pid *pid;
+
+	pid = get_children_pid(seq->private, v, *pos + 1);
+	put_pid(v);
+
+	++*pos;
+	return pid;
+}
+
+static void children_seq_stop(struct seq_file *seq, void *v)
+{
+	put_pid(v);
+}
+
+static const struct seq_operations children_seq_ops = {
+	.start	= children_seq_start,
+	.next	= children_seq_next,
+	.stop	= children_seq_stop,
+	.show	= children_seq_show,
+};
+
+static int children_seq_open(struct inode *inode, struct file *file)
+{
+	struct seq_file *m;
+	int ret;
+
+	ret = seq_open(file, &children_seq_ops);
+	if (ret)
+		return ret;
+
+	m = file->private_data;
+	m->private = inode;
+
+	return ret;
+}
+
+int children_seq_release(struct inode *inode, struct file *file)
+{
+	seq_release(inode, file);
+	return 0;
+}
+
+const struct file_operations proc_tid_children_operations = {
+	.open    = children_seq_open,
+	.read    = seq_read,
+	.llseek  = seq_lseek,
+	.release = children_seq_release,
+};
Index: linux-2.6.git/fs/proc/base.c
===================================================================
--- linux-2.6.git.orig/fs/proc/base.c
+++ linux-2.6.git/fs/proc/base.c
@@ -3384,6 +3384,7 @@ static const struct pid_entry tid_base_s
 	ONE("stat",      S_IRUGO, proc_tid_stat),
 	ONE("statm",     S_IRUGO, proc_pid_statm),
 	REG("maps",      S_IRUGO, proc_maps_operations),
+	REG("children",  S_IRUGO, proc_tid_children_operations),
 #ifdef CONFIG_NUMA
 	REG("numa_maps", S_IRUGO, proc_numa_maps_operations),
 #endif
Index: linux-2.6.git/fs/proc/internal.h
===================================================================
--- linux-2.6.git.orig/fs/proc/internal.h
+++ linux-2.6.git/fs/proc/internal.h
@@ -53,6 +53,7 @@ extern int proc_pid_statm(struct seq_fil
 				struct pid *pid, struct task_struct *task);
 extern loff_t mem_lseek(struct file *file, loff_t offset, int orig);
 
+extern const struct file_operations proc_tid_children_operations;
 extern const struct file_operations proc_maps_operations;
 extern const struct file_operations proc_numa_maps_operations;
 extern const struct file_operations proc_smaps_operations;

Re: [patch 1/4] fs, proc: Introduce /proc/<pid>/task/<tid>/children entry v8

[Andrew Morton]

On Mon, 23 Jan 2012 18:20:37 +0400
Cyrill Gorcunov <gorcunov@openvz.org> wrote:

> When we do checkpoint of a task we need to know the list of children
> the task, has but there is no easy and fast way to generate reverse
> parent->children chain from arbitrary <pid> (while a parent pid is
> provided in "PPid" field of /proc/<pid>/status).
> 
> So instead of walking over all pids in the system (creating one big process
> tree in memory, just to figure out which children a task has) -- we add
> explicit /proc/<pid>/task/<tid>/children entry, because the kernel already has
> this kind of information but it is not yet exported.
> 
> This is a first level children, not the whole process tree.

Is there a reason for not putting this new code inside
CONFIG_CHECKPOINT_RESTART?  If so, please changelog that.

We could add

/* #ifdef CONFIG_CHECKPOINT_RESTART (reasoning goes here) */

so that if we should decide to pull all this code out again, a grep
will flag this code for consideration.

Re: [patch 1/4] fs, proc: Introduce /proc/<pid>/task/<tid>/children entry v8

[Cyrill Gorcunov]

On Tue, Jan 24, 2012 at 03:53:03PM -0800, Andrew Morton wrote:
> On Mon, 23 Jan 2012 18:20:37 +0400
> Cyrill Gorcunov <gorcunov@openvz.org> wrote:
> 
> > When we do checkpoint of a task we need to know the list of children
> > the task, has but there is no easy and fast way to generate reverse
> > parent->children chain from arbitrary <pid> (while a parent pid is
> > provided in "PPid" field of /proc/<pid>/status).
> > 
> > So instead of walking over all pids in the system (creating one big process
> > tree in memory, just to figure out which children a task has) -- we add
> > explicit /proc/<pid>/task/<tid>/children entry, because the kernel already has
> > this kind of information but it is not yet exported.
> > 
> > This is a first level children, not the whole process tree.
> 
> Is there a reason for not putting this new code inside
> CONFIG_CHECKPOINT_RESTART?  If so, please changelog that.
> 

I don't know if someone might need it as well, so I've asked in one of
previous iterations if this file is to be under CONFIG_CHECKPOINT_RESTART.
So, sure, I'll wrap it this way.

> We could add
> 
> /* #ifdef CONFIG_CHECKPOINT_RESTART (reasoning goes here) */
> 
> so that if we should decide to pull all this code out again, a grep
> will flag this code for consideration.
> 

	Cyrill

[patch 2/4] [RFC] syscalls, x86: Add __NR_kcmp syscall v4

[Cyrill Gorcunov]

[-- Attachment #1: sys-kcmp-7 --]
[-- Type: text/plain, Size: 7857 bytes --]

While doing the checkpoint-restore in the userspace one need to determine
whether various kernel objects (like mm_struct-s of file_struct-s) are shared
between tasks and restore this state.

The 2nd step can be solved by using appropriate CLONE_ flags and the unshare
syscall, while there's currently no ways for solving the 1st one.

One of the ways for checking whether two tasks share e.g. mm_struct is to
provide some mm_struct ID of a task to its proc file, but showing such
info considered to be not that good for security reasons.

Thus after some debates we end up in conclusion that using that named
'comparision' syscall might be the best candidate. So here is it --
__NR_kcmp.

It takes up to 5 agruments - the pids of the two tasks (which
characteristics should be compared), the comparision type and
(in case of comparision of files) two file descriptors.

At moment only x86 is supported.

Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
CC: "Eric W. Biederman" <ebiederm@xmission.com>
CC: Pavel Emelyanov <xemul@parallels.com>
CC: Andrey Vagin <avagin@openvz.org>
CC: KOSAKI Motohiro <kosaki.motohiro@gmail.com>
CC: Ingo Molnar <mingo@elte.hu>
CC: H. Peter Anvin <hpa@zytor.com>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Glauber Costa <glommer@parallels.com>
CC: Andi Kleen <andi@firstfloor.org>
CC: Tejun Heo <tj@kernel.org>
CC: Matt Helsley <matthltc@us.ibm.com>
CC: Pekka Enberg <penberg@kernel.org>
CC: Eric Dumazet <eric.dumazet@gmail.com>
CC: Vasiliy Kulikov <segoon@openwall.com>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: Alexey Dobriyan <adobriyan@gmail.com>
CC: Valdis.Kletnieks@vt.edu
---
 arch/x86/include/asm/kcmp.h      |   17 ++++
 arch/x86/include/asm/syscalls.h  |    4 
 arch/x86/kernel/Makefile         |    1 
 arch/x86/kernel/kcmp.c           |  163 +++++++++++++++++++++++++++++++++++++++
 arch/x86/syscalls/syscall_32.tbl |    1 
 arch/x86/syscalls/syscall_64.tbl |    1 
 6 files changed, 187 insertions(+)

Index: linux-2.6.git/arch/x86/include/asm/kcmp.h
===================================================================
--- /dev/null
+++ linux-2.6.git/arch/x86/include/asm/kcmp.h
@@ -0,0 +1,17 @@
+#ifndef _LINUX_KCMP_H
+#define _LINUX_KCMP_H
+
+/* Comparision type */
+enum {
+	KCMP_FILE,
+	KCMP_VM,
+	KCMP_FILES,
+	KCMP_FS,
+	KCMP_SIGHAND,
+	KCMP_IO,
+	KCMP_SYSVSEM,
+
+	KCMP_TYPES,
+};
+
+#endif /* _LINUX_KCMP_H */
Index: linux-2.6.git/arch/x86/include/asm/syscalls.h
===================================================================
--- linux-2.6.git.orig/arch/x86/include/asm/syscalls.h
+++ linux-2.6.git/arch/x86/include/asm/syscalls.h
@@ -42,6 +42,10 @@ long sys_sigaltstack(const stack_t __use
 asmlinkage int sys_set_thread_area(struct user_desc __user *);
 asmlinkage int sys_get_thread_area(struct user_desc __user *);
 
+/* kenrel/kcmp.c */
+asmlinkage long sys_kcmp(pid_t pid1, pid_t pid2, int type,
+			 unsigned long idx1, unsigned long idx2);
+
 /* X86_32 only */
 #ifdef CONFIG_X86_32
 
Index: linux-2.6.git/arch/x86/kernel/Makefile
===================================================================
--- linux-2.6.git.orig/arch/x86/kernel/Makefile
+++ linux-2.6.git/arch/x86/kernel/Makefile
@@ -34,6 +34,7 @@ obj-y			+= alternative.o i8253.o pci-nom
 obj-y			+= tsc.o io_delay.o rtc.o
 obj-y			+= pci-iommu_table.o
 obj-y			+= resource.o
+obj-y			+= kcmp.o
 
 obj-y				+= trampoline.o trampoline_$(BITS).o
 obj-y				+= process.o
Index: linux-2.6.git/arch/x86/kernel/kcmp.c
===================================================================
--- /dev/null
+++ linux-2.6.git/arch/x86/kernel/kcmp.c
@@ -0,0 +1,163 @@
+#include <linux/kernel.h>
+#include <linux/syscalls.h>
+#include <linux/fdtable.h>
+#include <linux/string.h>
+#include <linux/random.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/cache.h>
+#include <linux/bug.h>
+#include <linux/err.h>
+
+#include <asm/unistd.h>
+#include <asm/kcmp.h>
+
+static unsigned long cookies[KCMP_TYPES][2] __read_mostly;
+
+static long kptr_obfuscate(long v, int type)
+{
+	return (v + cookies[type][0]) ^ cookies[type][1];
+}
+
+/*
+ * 0 - equal
+ * 1 - less than
+ * 2 - greater than
+ * 3 - not equal but ordering unavailable
+ */
+static int kcmp_ptr(long v1, long v2, int type)
+{
+	long ret;
+
+	ret = kptr_obfuscate(v1, type) - kptr_obfuscate(v2, type);
+
+	return (ret < 0) | ((ret > 0) << 1);
+}
+
+#define KCMP_TASK_PTR(task1, task2, member, type)	\
+	kcmp_ptr((long)(task1)->member,			\
+		 (long)(task2)->member,			\
+		 type)
+
+#define KCMP_PTR(ptr1, ptr2, type)			\
+	kcmp_ptr((long)ptr1, (long)ptr2, type)
+
+/* A caller must be sure the task is presented in memory */
+static struct file *
+get_file_raw_ptr(struct task_struct *task, unsigned int idx)
+{
+	struct fdtable *fdt;
+	struct file *file;
+
+	spin_lock(&task->files->file_lock);
+	fdt = files_fdtable(task->files);
+	if (idx < fdt->max_fds)
+		file = fdt->fd[idx];
+	else
+		file = NULL;
+	spin_unlock(&task->files->file_lock);
+
+	return file;
+}
+
+SYSCALL_DEFINE5(kcmp, pid_t, pid1, pid_t, pid2, int, type,
+		unsigned long, idx1, unsigned long, idx2)
+{
+	struct task_struct *task1;
+	struct task_struct *task2;
+	int ret = 0;
+
+	rcu_read_lock();
+
+	task1 = find_task_by_vpid(pid1);
+	if (!task1) {
+		rcu_read_unlock();
+		return -ESRCH;
+	}
+
+	task2 = find_task_by_vpid(pid2);
+	if (!task2) {
+		put_task_struct(task1);
+		rcu_read_unlock();
+		return -ESRCH;
+	}
+
+	get_task_struct(task1);
+	get_task_struct(task2);
+
+	rcu_read_unlock();
+
+	if (!ptrace_may_access(task1, PTRACE_MODE_READ) ||
+	    !ptrace_may_access(task2, PTRACE_MODE_READ)) {
+		ret = -EACCES;
+		goto err;
+	}
+
+	/*
+	 * Note for all cases but the KCMP_FILE we
+	 * don't take any locks in a sake of speed.
+	 */
+
+	switch (type) {
+	case KCMP_FILE: {
+		struct file *filp1, *filp2;
+
+		filp1 = get_file_raw_ptr(task1, idx1);
+		filp2 = get_file_raw_ptr(task2, idx2);
+
+		if (filp1 && filp2)
+			ret = KCMP_PTR(filp1, filp2, KCMP_FILE);
+		else
+			ret = -ENOENT;
+		break;
+	}
+	case KCMP_VM:
+		ret = KCMP_TASK_PTR(task1, task2, mm, KCMP_VM);
+		break;
+	case KCMP_FILES:
+		ret = KCMP_TASK_PTR(task1, task2, files, KCMP_FILES);
+		break;
+	case KCMP_FS:
+		ret = KCMP_TASK_PTR(task1, task2, fs, KCMP_FS);
+		break;
+	case KCMP_SIGHAND:
+		ret = KCMP_TASK_PTR(task1, task2, sighand, KCMP_SIGHAND);
+		break;
+	case KCMP_IO:
+		ret = KCMP_TASK_PTR(task1, task2, io_context, KCMP_IO);
+		break;
+	case KCMP_SYSVSEM:
+#ifdef CONFIG_SYSVIPC
+		ret = KCMP_TASK_PTR(task1, task2, sysvsem.undo_list, KCMP_SYSVSEM);
+#else
+		ret = -ENOENT;
+		goto err;
+#endif
+		break;
+	default:
+		ret = -EINVAL;
+		goto err;
+	}
+
+err:
+	put_task_struct(task1);
+	put_task_struct(task2);
+
+	return ret;
+}
+
+static __init int kcmp_cookie_init(void)
+{
+	int i, j;
+
+	for (i = 0; i < KCMP_TYPES; i++) {
+		for (j = 0; j < 2; j++) {
+			get_random_bytes(&cookies[i][j],
+					 sizeof(cookies[i][j]));
+			cookies[i][j] |= (~(~0UL >>  1) | 1);
+		}
+	}
+
+	return 0;
+}
+late_initcall(kcmp_cookie_init);
Index: linux-2.6.git/arch/x86/syscalls/syscall_32.tbl
===================================================================
--- linux-2.6.git.orig/arch/x86/syscalls/syscall_32.tbl
+++ linux-2.6.git/arch/x86/syscalls/syscall_32.tbl
@@ -355,3 +355,4 @@
 346	i386	setns			sys_setns
 347	i386	process_vm_readv	sys_process_vm_readv		compat_sys_process_vm_readv
 348	i386	process_vm_writev	sys_process_vm_writev		compat_sys_process_vm_writev
+349	i386	kcmp			sys_kcmp
Index: linux-2.6.git/arch/x86/syscalls/syscall_64.tbl
===================================================================
--- linux-2.6.git.orig/arch/x86/syscalls/syscall_64.tbl
+++ linux-2.6.git/arch/x86/syscalls/syscall_64.tbl
@@ -318,3 +318,4 @@
 309	64	getcpu			sys_getcpu
 310	64	process_vm_readv	sys_process_vm_readv
 311	64	process_vm_writev	sys_process_vm_writev
+312	64	kcmp			sys_kcmp

Re: [patch 2/4] [RFC] syscalls, x86: Add __NR_kcmp syscall v4

[H. Peter Anvin]

On 01/23/2012 06:20 AM, Cyrill Gorcunov wrote:
> +
> +static unsigned long cookies[KCMP_TYPES][2] __read_mostly;
> +
> +static long kptr_obfuscate(long v, int type)
> +{
> +	return (v + cookies[type][0]) ^ cookies[type][1];
> +}
> +

Arf... when I said to use xor I meant instead of the add, not instead of
the multiply, so:

	return (v ^ cookies[type][0]) * cookies[type][1];

Otherwise you have absolutely no source of diffusion at all (symmetric
cryptography is about combinations of diffusion -- spreading the content
-- and confusion -- scrambling individual bits of content.)

+	for (i = 0; i < KCMP_TYPES; i++) {
+		for (j = 0; j < 2; j++) {
+			get_random_bytes(&cookies[i][j],
+					 sizeof(cookies[i][j]));
+			cookies[i][j] |= (~(~0UL >>  1) | 1);
+		}
+	}

Only cookies[1] -- being used as a multiplicative constant -- needs the OR.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [patch 2/4] [RFC] syscalls, x86: Add __NR_kcmp syscall v4

[Cyrill Gorcunov]

On Mon, Jan 23, 2012 at 10:48:57AM -0800, H. Peter Anvin wrote:
> On 01/23/2012 06:20 AM, Cyrill Gorcunov wrote:
> > +
> > +static unsigned long cookies[KCMP_TYPES][2] __read_mostly;
> > +
> > +static long kptr_obfuscate(long v, int type)
> > +{
> > +	return (v + cookies[type][0]) ^ cookies[type][1];
> > +}
> > +
> 
> Arf... when I said to use xor I meant instead of the add, not instead of
> the multiply, so:
> 
> 	return (v ^ cookies[type][0]) * cookies[type][1];
> 
> Otherwise you have absolutely no source of diffusion at all (symmetric
> cryptography is about combinations of diffusion -- spreading the content
> -- and confusion -- scrambling individual bits of content.)
> 
> +	for (i = 0; i < KCMP_TYPES; i++) {
> +		for (j = 0; j < 2; j++) {
> +			get_random_bytes(&cookies[i][j],
> +					 sizeof(cookies[i][j]));
> +			cookies[i][j] |= (~(~0UL >>  1) | 1);
> +		}
> +	}
> 
> Only cookies[1] -- being used as a multiplicative constant -- needs the OR.

OK. Thanks. I'll update.

	Cyrill

Re: [patch 2/4] [RFC] syscalls, x86: Add __NR_kcmp syscall v4

[KAMEZAWA Hiroyuki]

On Mon, 23 Jan 2012 18:20:38 +0400
Cyrill Gorcunov <gorcunov@openvz.org> wrote:

> While doing the checkpoint-restore in the userspace one need to determine
> whether various kernel objects (like mm_struct-s of file_struct-s) are shared
> between tasks and restore this state.
> 
> The 2nd step can be solved by using appropriate CLONE_ flags and the unshare
> syscall, while there's currently no ways for solving the 1st one.
> 
> One of the ways for checking whether two tasks share e.g. mm_struct is to
> provide some mm_struct ID of a task to its proc file, but showing such
> info considered to be not that good for security reasons.
> 
> Thus after some debates we end up in conclusion that using that named
> 'comparision' syscall might be the best candidate. So here is it --
> __NR_kcmp.
> 
> It takes up to 5 agruments - the pids of the two tasks (which
> characteristics should be compared), the comparision type and
> (in case of comparision of files) two file descriptors.
> 
> At moment only x86 is supported.
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
> CC: "Eric W. Biederman" <ebiederm@xmission.com>
> CC: Pavel Emelyanov <xemul@parallels.com>
> CC: Andrey Vagin <avagin@openvz.org>
> CC: KOSAKI Motohiro <kosaki.motohiro@gmail.com>
> CC: Ingo Molnar <mingo@elte.hu>
> CC: H. Peter Anvin <hpa@zytor.com>
> CC: Thomas Gleixner <tglx@linutronix.de>
> CC: Glauber Costa <glommer@parallels.com>
> CC: Andi Kleen <andi@firstfloor.org>
> CC: Tejun Heo <tj@kernel.org>
> CC: Matt Helsley <matthltc@us.ibm.com>
> CC: Pekka Enberg <penberg@kernel.org>
> CC: Eric Dumazet <eric.dumazet@gmail.com>
> CC: Vasiliy Kulikov <segoon@openwall.com>
> CC: Andrew Morton <akpm@linux-foundation.org>
> CC: Alexey Dobriyan <adobriyan@gmail.com>
> CC: Valdis.Kletnieks@vt.edu
> ---
>  arch/x86/include/asm/kcmp.h      |   17 ++++
>  arch/x86/include/asm/syscalls.h  |    4 
>  arch/x86/kernel/Makefile         |    1 
>  arch/x86/kernel/kcmp.c           |  163 +++++++++++++++++++++++++++++++++++++++
>  arch/x86/syscalls/syscall_32.tbl |    1 
>  arch/x86/syscalls/syscall_64.tbl |    1 
>  6 files changed, 187 insertions(+)
> 
> Index: linux-2.6.git/arch/x86/include/asm/kcmp.h
> ===================================================================
> --- /dev/null
> +++ linux-2.6.git/arch/x86/include/asm/kcmp.h
> @@ -0,0 +1,17 @@
> +#ifndef _LINUX_KCMP_H
> +#define _LINUX_KCMP_H
> +
> +/* Comparision type */
> +enum {
> +	KCMP_FILE,
> +	KCMP_VM,
> +	KCMP_FILES,
> +	KCMP_FS,
> +	KCMP_SIGHAND,
> +	KCMP_IO,
> +	KCMP_SYSVSEM,
> +
> +	KCMP_TYPES,
> +};
> +
> +#endif /* _LINUX_KCMP_H */

Why under /arch ?





> Index: linux-2.6.git/arch/x86/include/asm/syscalls.h
> ===================================================================
> --- linux-2.6.git.orig/arch/x86/include/asm/syscalls.h
> +++ linux-2.6.git/arch/x86/include/asm/syscalls.h
> @@ -42,6 +42,10 @@ long sys_sigaltstack(const stack_t __use
>  asmlinkage int sys_set_thread_area(struct user_desc __user *);
>  asmlinkage int sys_get_thread_area(struct user_desc __user *);
>  
> +/* kenrel/kcmp.c */
> +asmlinkage long sys_kcmp(pid_t pid1, pid_t pid2, int type,
> +			 unsigned long idx1, unsigned long idx2);
> +
>  /* X86_32 only */
>  #ifdef CONFIG_X86_32
>  
> Index: linux-2.6.git/arch/x86/kernel/Makefile
> ===================================================================
> --- linux-2.6.git.orig/arch/x86/kernel/Makefile
> +++ linux-2.6.git/arch/x86/kernel/Makefile
> @@ -34,6 +34,7 @@ obj-y			+= alternative.o i8253.o pci-nom
>  obj-y			+= tsc.o io_delay.o rtc.o
>  obj-y			+= pci-iommu_table.o
>  obj-y			+= resource.o
> +obj-y			+= kcmp.o
>  
>  obj-y				+= trampoline.o trampoline_$(BITS).o
>  obj-y				+= process.o
> Index: linux-2.6.git/arch/x86/kernel/kcmp.c
> ===================================================================
> --- /dev/null
> +++ linux-2.6.git/arch/x86/kernel/kcmp.c
> @@ -0,0 +1,163 @@
> +#include <linux/kernel.h>
> +#include <linux/syscalls.h>
> +#include <linux/fdtable.h>
> +#include <linux/string.h>
> +#include <linux/random.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/cache.h>
> +#include <linux/bug.h>
> +#include <linux/err.h>
> +
> +#include <asm/unistd.h>
> +#include <asm/kcmp.h>
> +
> +static unsigned long cookies[KCMP_TYPES][2] __read_mostly;
> +
> +static long kptr_obfuscate(long v, int type)
> +{
> +	return (v + cookies[type][0]) ^ cookies[type][1];
> +}
> +

I'm sorry could you add comments to swho what this does ?



> +/*
> + * 0 - equal
> + * 1 - less than
> + * 2 - greater than
> + * 3 - not equal but ordering unavailable
> + */
> +static int kcmp_ptr(long v1, long v2, int type)
> +{
> +	long ret;
> +
> +	ret = kptr_obfuscate(v1, type) - kptr_obfuscate(v2, type);
> +
> +	return (ret < 0) | ((ret > 0) << 1);
> +}
> +
> +#define KCMP_TASK_PTR(task1, task2, member, type)	\
> +	kcmp_ptr((long)(task1)->member,			\
> +		 (long)(task2)->member,			\
> +		 type)
> +
> +#define KCMP_PTR(ptr1, ptr2, type)			\
> +	kcmp_ptr((long)ptr1, (long)ptr2, type)
> +
> +/* A caller must be sure the task is presented in memory */
> +static struct file *
> +get_file_raw_ptr(struct task_struct *task, unsigned int idx)
> +{
> +	struct fdtable *fdt;
> +	struct file *file;
> +
> +	spin_lock(&task->files->file_lock);
> +	fdt = files_fdtable(task->files);
> +	if (idx < fdt->max_fds)
> +		file = fdt->fd[idx];
> +	else
> +		file = NULL;
> +	spin_unlock(&task->files->file_lock);
> +
> +	return file;
> +}
> +
> +SYSCALL_DEFINE5(kcmp, pid_t, pid1, pid_t, pid2, int, type,
> +		unsigned long, idx1, unsigned long, idx2)
> +{
> +	struct task_struct *task1;
> +	struct task_struct *task2;
> +	int ret = 0;
> +
> +	rcu_read_lock();
> +
> +	task1 = find_task_by_vpid(pid1);
> +	if (!task1) {
> +		rcu_read_unlock();
> +		return -ESRCH;
> +	}
> +
> +	task2 = find_task_by_vpid(pid2);
> +	if (!task2) {
> +		put_task_struct(task1);
> +		rcu_read_unlock();
> +		return -ESRCH;
> +	}
> +
> +	get_task_struct(task1);
> +	get_task_struct(task2);
> +
> +	rcu_read_unlock();
> +
> +	if (!ptrace_may_access(task1, PTRACE_MODE_READ) ||
> +	    !ptrace_may_access(task2, PTRACE_MODE_READ)) {
> +		ret = -EACCES;
> +		goto err;
> +	}
> +
> +	/*
> +	 * Note for all cases but the KCMP_FILE we
> +	 * don't take any locks in a sake of speed.
> +	 */
> +
> +	switch (type) {
> +	case KCMP_FILE: {
> +		struct file *filp1, *filp2;
> +
> +		filp1 = get_file_raw_ptr(task1, idx1);
> +		filp2 = get_file_raw_ptr(task2, idx2);
> +
> +		if (filp1 && filp2)
> +			ret = KCMP_PTR(filp1, filp2, KCMP_FILE);
> +		else
> +			ret = -ENOENT;
> +		break;
> +	}
> +	case KCMP_VM:
> +		ret = KCMP_TASK_PTR(task1, task2, mm, KCMP_VM);
> +		break;
> +	case KCMP_FILES:
> +		ret = KCMP_TASK_PTR(task1, task2, files, KCMP_FILES);
> +		break;
> +	case KCMP_FS:
> +		ret = KCMP_TASK_PTR(task1, task2, fs, KCMP_FS);
> +		break;
> +	case KCMP_SIGHAND:
> +		ret = KCMP_TASK_PTR(task1, task2, sighand, KCMP_SIGHAND);
> +		break;
> +	case KCMP_IO:
> +		ret = KCMP_TASK_PTR(task1, task2, io_context, KCMP_IO);
> +		break;
> +	case KCMP_SYSVSEM:
> +#ifdef CONFIG_SYSVIPC
> +		ret = KCMP_TASK_PTR(task1, task2, sysvsem.undo_list, KCMP_SYSVSEM);
> +#else
> +		ret = -ENOENT;
> +		goto err;
> +#endif
> +		break;
> +	default:
> +		ret = -EINVAL;
> +		goto err;
> +	}
> +
> +err:
> +	put_task_struct(task1);
> +	put_task_struct(task2);
> +
> +	return ret;
> +}

It seems this function itself doesn't depend on arch.

Thanks,
-Kame

Re: [patch 2/4] [RFC] syscalls, x86: Add __NR_kcmp syscall v4

[Cyrill Gorcunov]

On Tue, Jan 24, 2012 at 11:16:55AM +0900, KAMEZAWA Hiroyuki wrote:
...
> > 
> > Index: linux-2.6.git/arch/x86/include/asm/kcmp.h
> > ===================================================================
> > --- /dev/null
> > +++ linux-2.6.git/arch/x86/include/asm/kcmp.h
> > @@ -0,0 +1,17 @@
> > +#ifndef _LINUX_KCMP_H
> > +#define _LINUX_KCMP_H
> > +
> > +/* Comparision type */
> > +enum {
> > +	KCMP_FILE,
> > +	KCMP_VM,
> > +	KCMP_FILES,
> > +	KCMP_FS,
> > +	KCMP_SIGHAND,
> > +	KCMP_IO,
> > +	KCMP_SYSVSEM,
> > +
> > +	KCMP_TYPES,
> > +};
> > +
> > +#endif /* _LINUX_KCMP_H */
> 
> Why under /arch ?
> 

Hi Kame,

because I've tested it under x86 only. Once someone
confirm it's needed on some else arch and does work
as expected -- it wont be a problem to make it system
wide. Until then -- I think better to stick with
at least tested case.
...
> > +
> > +static long kptr_obfuscate(long v, int type)
> > +{
> > +	return (v + cookies[type][0]) ^ cookies[type][1];
> > +}
> > +
> 
> I'm sorry could you add comments to swho what this does ?

The idea is disorder the in-memory order but remain order
suitable for sorting. Anywa, as Peter pointed this helper
must be redone. I'll post updated version.

> 
> It seems this function itself doesn't depend on arch.
> 

Yes, it doesn't. But as I said if someone confirm it's
needed on non-x86 and get tested, that will not be a
problem to make it system-side.

	Cyrill

Re: [patch 2/4] [RFC] syscalls, x86: Add __NR_kcmp syscall v4

[H. Peter Anvin]

On 01/23/2012 10:47 PM, Cyrill Gorcunov wrote:
>>
>> Why under /arch ?
>>
> 
> Hi Kame,
> 
> because I've tested it under x86 only. Once someone
> confirm it's needed on some else arch and does work
> as expected -- it wont be a problem to make it system
> wide. Until then -- I think better to stick with
> at least tested case.
> ...

That's not a reason to put it in arch/ ... that's possibly a reason to
not map the system call on other architectures yet.

	-hpa

Re: [patch 2/4] [RFC] syscalls, x86: Add __NR_kcmp syscall v4

[Cyrill Gorcunov]

On Mon, Jan 23, 2012 at 11:04:19PM -0800, H. Peter Anvin wrote:
> On 01/23/2012 10:47 PM, Cyrill Gorcunov wrote:
> >>
> >> Why under /arch ?
> >>
> > 
> > Hi Kame,
> > 
> > because I've tested it under x86 only. Once someone
> > confirm it's needed on some else arch and does work
> > as expected -- it wont be a problem to make it system
> > wide. Until then -- I think better to stick with
> > at least tested case.
> > ...
> 
> That's not a reason to put it in arch/ ... that's possibly a reason to
> not map the system call on other architectures yet.
> 

Where it should live then? In kernel/ or mm/ ?

	Cyrill

Re: [patch 2/4] [RFC] syscalls, x86: Add __NR_kcmp syscall v4

[KAMEZAWA Hiroyuki]

On Tue, 24 Jan 2012 11:17:16 +0400
Cyrill Gorcunov <gorcunov@gmail.com> wrote:

> On Mon, Jan 23, 2012 at 11:04:19PM -0800, H. Peter Anvin wrote:
> > On 01/23/2012 10:47 PM, Cyrill Gorcunov wrote:
> > >>
> > >> Why under /arch ?
> > >>
> > > 
> > > Hi Kame,
> > > 
> > > because I've tested it under x86 only. Once someone
> > > confirm it's needed on some else arch and does work
> > > as expected -- it wont be a problem to make it system
> > > wide. Until then -- I think better to stick with
> > > at least tested case.
> > > ...
> > 
> > That's not a reason to put it in arch/ ... that's possibly a reason to
> > not map the system call on other architectures yet.
> > 
> 
> Where it should live then? In kernel/ or mm/ ?
> 

kernel/checkpoint_restart ?

gathering related changes to a directory may help developpers joins later....
To me, this makes seeing git log easy ;)
 
Thanks,
-Kame

Re: [patch 2/4] [RFC] syscalls, x86: Add __NR_kcmp syscall v4

[Cyrill Gorcunov]

On Tue, Jan 24, 2012 at 04:20:31PM +0900, KAMEZAWA Hiroyuki wrote:
...
> > > 
> > > That's not a reason to put it in arch/ ... that's possibly a reason to
> > > not map the system call on other architectures yet.
> > > 
> > 
> > Where it should live then? In kernel/ or mm/ ?
> > 
> 
> kernel/checkpoint_restart ?
> 
> gathering related changes to a directory may help developpers joins later....
> To me, this makes seeing git log easy ;)
>  

Such separate directory implies everything related to c/r should live there,
it also implies (I think) that code which is under CHECKPOINT_RESTORE should
be moved there as well, so I'm not sure, Kame ;) I guess mm/ will be a good
place since these are operations with memory pointers.

	Cyrill

Re: [patch 2/4] [RFC] syscalls, x86: Add __NR_kcmp syscall v4

[KAMEZAWA Hiroyuki]

On Tue, 24 Jan 2012 11:38:42 +0400
Cyrill Gorcunov <gorcunov@gmail.com> wrote:

> On Tue, Jan 24, 2012 at 04:20:31PM +0900, KAMEZAWA Hiroyuki wrote:
> ...
> > > > 
> > > > That's not a reason to put it in arch/ ... that's possibly a reason to
> > > > not map the system call on other architectures yet.
> > > > 
> > > 
> > > Where it should live then? In kernel/ or mm/ ?
> > > 
> > 
> > kernel/checkpoint_restart ?
> > 
> > gathering related changes to a directory may help developpers joins later....
> > To me, this makes seeing git log easy ;)
> >  
> 
> Such separate directory implies everything related to c/r should live there,
> it also implies (I think) that code which is under CHECKPOINT_RESTORE should
> be moved there as well, so I'm not sure, Kame ;) I guess mm/ will be a good
> place since these are operations with memory pointers.
> 

please do as you like. 
-Kame

Re: [patch 2/4] [RFC] syscalls, x86: Add __NR_kcmp syscall v4

[Cyrill Gorcunov]

On Tue, Jan 24, 2012 at 04:40:08PM +0900, KAMEZAWA Hiroyuki wrote:
> On Tue, 24 Jan 2012 11:38:42 +0400
> Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> 
> > On Tue, Jan 24, 2012 at 04:20:31PM +0900, KAMEZAWA Hiroyuki wrote:
> > ...
> > > > > 
> > > > > That's not a reason to put it in arch/ ... that's possibly a reason to
> > > > > not map the system call on other architectures yet.
> > > > > 
> > > > 
> > > > Where it should live then? In kernel/ or mm/ ?
> > > > 
> > > 
> > > kernel/checkpoint_restart ?
> > > 
> > > gathering related changes to a directory may help developpers joins later....
> > > To me, this makes seeing git log easy ;)
> > >  
> > 
> > Such separate directory implies everything related to c/r should live there,
> > it also implies (I think) that code which is under CHECKPOINT_RESTORE should
> > be moved there as well, so I'm not sure, Kame ;) I guess mm/ will be a good
> > place since these are operations with memory pointers.
> > 
> 
> please do as you like. 

So it should be something like below I think...

	Cyrill
---
From: Cyrill Gorcunov <gorcunov@openvz.org>
Subject: [RFC] syscalls, x86: Add __NR_kcmp syscall v5

While doing the checkpoint-restore in the userspace one need to determine
whether various kernel objects (like mm_struct-s of file_struct-s) are shared
between tasks and restore this state.

The 2nd step can be solved by using appropriate CLONE_ flags and the unshare
syscall, while there's currently no ways for solving the 1st one.

One of the ways for checking whether two tasks share e.g. mm_struct is to
provide some mm_struct ID of a task to its proc file, but showing such
info considered to be not that good for security reasons.

Thus after some debates we end up in conclusion that using that named
'comparision' syscall might be the best candidate. So here is it --
__NR_kcmp.

It takes up to 5 agruments - the pids of the two tasks (which
characteristics should be compared), the comparision type and
(in case of comparision of files) two file descriptors.

At moment only x86 is supported.

Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
CC: "Eric W. Biederman" <ebiederm@xmission.com>
CC: Pavel Emelyanov <xemul@parallels.com>
CC: Andrey Vagin <avagin@openvz.org>
CC: KOSAKI Motohiro <kosaki.motohiro@gmail.com>
CC: Ingo Molnar <mingo@elte.hu>
CC: H. Peter Anvin <hpa@zytor.com>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Glauber Costa <glommer@parallels.com>
CC: Andi Kleen <andi@firstfloor.org>
CC: Tejun Heo <tj@kernel.org>
CC: Matt Helsley <matthltc@us.ibm.com>
CC: Pekka Enberg <penberg@kernel.org>
CC: Eric Dumazet <eric.dumazet@gmail.com>
CC: Vasiliy Kulikov <segoon@openwall.com>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: Alexey Dobriyan <adobriyan@gmail.com>
CC: Valdis.Kletnieks@vt.edu
---
 arch/x86/include/asm/syscalls.h  |    4 
 arch/x86/syscalls/syscall_32.tbl |    1 
 arch/x86/syscalls/syscall_64.tbl |    1 
 include/linux/kcmp.h             |   17 ++++
 mm/Makefile                      |    1 
 mm/kcmp.c                        |  163 +++++++++++++++++++++++++++++++++++++++
 6 files changed, 187 insertions(+)

Index: linux-2.6.git/arch/x86/include/asm/syscalls.h
===================================================================
--- linux-2.6.git.orig/arch/x86/include/asm/syscalls.h
+++ linux-2.6.git/arch/x86/include/asm/syscalls.h
@@ -42,6 +42,10 @@ long sys_sigaltstack(const stack_t __use
 asmlinkage int sys_set_thread_area(struct user_desc __user *);
 asmlinkage int sys_get_thread_area(struct user_desc __user *);
 
+/* mm/kcmp.c */
+asmlinkage long sys_kcmp(pid_t pid1, pid_t pid2, int type,
+			 unsigned long idx1, unsigned long idx2);
+
 /* X86_32 only */
 #ifdef CONFIG_X86_32
 
Index: linux-2.6.git/arch/x86/syscalls/syscall_32.tbl
===================================================================
--- linux-2.6.git.orig/arch/x86/syscalls/syscall_32.tbl
+++ linux-2.6.git/arch/x86/syscalls/syscall_32.tbl
@@ -355,3 +355,4 @@
 346	i386	setns			sys_setns
 347	i386	process_vm_readv	sys_process_vm_readv		compat_sys_process_vm_readv
 348	i386	process_vm_writev	sys_process_vm_writev		compat_sys_process_vm_writev
+349	i386	kcmp			sys_kcmp
Index: linux-2.6.git/arch/x86/syscalls/syscall_64.tbl
===================================================================
--- linux-2.6.git.orig/arch/x86/syscalls/syscall_64.tbl
+++ linux-2.6.git/arch/x86/syscalls/syscall_64.tbl
@@ -318,3 +318,4 @@
 309	64	getcpu			sys_getcpu
 310	64	process_vm_readv	sys_process_vm_readv
 311	64	process_vm_writev	sys_process_vm_writev
+312	64	kcmp			sys_kcmp
Index: linux-2.6.git/include/linux/kcmp.h
===================================================================
--- /dev/null
+++ linux-2.6.git/include/linux/kcmp.h
@@ -0,0 +1,17 @@
+#ifndef _LINUX_KCMP_H
+#define _LINUX_KCMP_H
+
+/* Comparision type */
+enum {
+	KCMP_FILE,
+	KCMP_VM,
+	KCMP_FILES,
+	KCMP_FS,
+	KCMP_SIGHAND,
+	KCMP_IO,
+	KCMP_SYSVSEM,
+
+	KCMP_TYPES,
+};
+
+#endif /* _LINUX_KCMP_H */
Index: linux-2.6.git/mm/Makefile
===================================================================
--- linux-2.6.git.orig/mm/Makefile
+++ linux-2.6.git/mm/Makefile
@@ -51,3 +51,4 @@ obj-$(CONFIG_HWPOISON_INJECT) += hwpoiso
 obj-$(CONFIG_DEBUG_KMEMLEAK) += kmemleak.o
 obj-$(CONFIG_DEBUG_KMEMLEAK_TEST) += kmemleak-test.o
 obj-$(CONFIG_CLEANCACHE) += cleancache.o
+obj-$(CONFIG_X86) += kcmp.o
Index: linux-2.6.git/mm/kcmp.c
===================================================================
--- /dev/null
+++ linux-2.6.git/mm/kcmp.c
@@ -0,0 +1,163 @@
+#include <linux/kernel.h>
+#include <linux/syscalls.h>
+#include <linux/fdtable.h>
+#include <linux/string.h>
+#include <linux/random.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/cache.h>
+#include <linux/bug.h>
+#include <linux/err.h>
+#include <linux/kcmp.h>
+
+#include <asm/unistd.h>
+
+static unsigned long cookies[KCMP_TYPES][2] __read_mostly;
+
+static long kptr_obfuscate(long v, int type)
+{
+	return (v ^ cookies[type][0]) * cookies[type][1];
+}
+
+/*
+ * 0 - equal
+ * 1 - less than
+ * 2 - greater than
+ * 3 - not equal but ordering unavailable
+ */
+static int kcmp_ptr(long v1, long v2, int type)
+{
+	long ret;
+
+	ret = kptr_obfuscate(v1, type) - kptr_obfuscate(v2, type);
+
+	return (ret < 0) | ((ret > 0) << 1);
+}
+
+#define KCMP_TASK_PTR(task1, task2, member, type)	\
+	kcmp_ptr((long)(task1)->member,			\
+		 (long)(task2)->member,			\
+		 type)
+
+#define KCMP_PTR(ptr1, ptr2, type)			\
+	kcmp_ptr((long)ptr1, (long)ptr2, type)
+
+/* A caller must be sure the task is presented in memory */
+static struct file *
+get_file_raw_ptr(struct task_struct *task, unsigned int idx)
+{
+	struct fdtable *fdt;
+	struct file *file;
+
+	spin_lock(&task->files->file_lock);
+	fdt = files_fdtable(task->files);
+	if (idx < fdt->max_fds)
+		file = fdt->fd[idx];
+	else
+		file = NULL;
+	spin_unlock(&task->files->file_lock);
+
+	return file;
+}
+
+SYSCALL_DEFINE5(kcmp, pid_t, pid1, pid_t, pid2, int, type,
+		unsigned long, idx1, unsigned long, idx2)
+{
+	struct task_struct *task1;
+	struct task_struct *task2;
+	int ret = 0;
+
+	rcu_read_lock();
+
+	task1 = find_task_by_vpid(pid1);
+	if (!task1) {
+		rcu_read_unlock();
+		return -ESRCH;
+	}
+
+	task2 = find_task_by_vpid(pid2);
+	if (!task2) {
+		put_task_struct(task1);
+		rcu_read_unlock();
+		return -ESRCH;
+	}
+
+	get_task_struct(task1);
+	get_task_struct(task2);
+
+	rcu_read_unlock();
+
+	if (!ptrace_may_access(task1, PTRACE_MODE_READ) ||
+	    !ptrace_may_access(task2, PTRACE_MODE_READ)) {
+		ret = -EACCES;
+		goto err;
+	}
+
+	/*
+	 * Note for all cases but the KCMP_FILE we
+	 * don't take any locks in a sake of speed.
+	 */
+
+	switch (type) {
+	case KCMP_FILE: {
+		struct file *filp1, *filp2;
+
+		filp1 = get_file_raw_ptr(task1, idx1);
+		filp2 = get_file_raw_ptr(task2, idx2);
+
+		if (filp1 && filp2)
+			ret = KCMP_PTR(filp1, filp2, KCMP_FILE);
+		else
+			ret = -ENOENT;
+		break;
+	}
+	case KCMP_VM:
+		ret = KCMP_TASK_PTR(task1, task2, mm, KCMP_VM);
+		break;
+	case KCMP_FILES:
+		ret = KCMP_TASK_PTR(task1, task2, files, KCMP_FILES);
+		break;
+	case KCMP_FS:
+		ret = KCMP_TASK_PTR(task1, task2, fs, KCMP_FS);
+		break;
+	case KCMP_SIGHAND:
+		ret = KCMP_TASK_PTR(task1, task2, sighand, KCMP_SIGHAND);
+		break;
+	case KCMP_IO:
+		ret = KCMP_TASK_PTR(task1, task2, io_context, KCMP_IO);
+		break;
+	case KCMP_SYSVSEM:
+#ifdef CONFIG_SYSVIPC
+		ret = KCMP_TASK_PTR(task1, task2, sysvsem.undo_list, KCMP_SYSVSEM);
+#else
+		ret = -ENOENT;
+		goto err;
+#endif
+		break;
+	default:
+		ret = -EINVAL;
+		goto err;
+	}
+
+err:
+	put_task_struct(task1);
+	put_task_struct(task2);
+
+	return ret;
+}
+
+static __init int kcmp_cookie_init(void)
+{
+	int i, j;
+
+	for (i = 0; i < KCMP_TYPES; i++) {
+		for (j = 0; j < 2; j++) {
+			get_random_bytes(&cookies[i][j],
+					 sizeof(cookies[i][j]));
+		}
+		cookies[i][1] |= (~(~0UL >>  1) | 1);
+	}
+
+	return 0;
+}
+late_initcall(kcmp_cookie_init);

Re: [patch 2/4] [RFC] syscalls, x86: Add __NR_kcmp syscall v4

[KOSAKI Motohiro]

>> please do as you like.
>
> So it should be something like below I think...

Looks ok this version to me. So, if you fix other developers pointed
issue, I'll ack this.

thanks.

Re: [patch 2/4] [RFC] syscalls, x86: Add __NR_kcmp syscall v4

[Cyrill Gorcunov]

On Tue, Jan 24, 2012 at 03:20:26PM -0500, KOSAKI Motohiro wrote:
> >> please do as you like.
> >
> > So it should be something like below I think...
> 
> Looks ok this version to me. So, if you fix other developers pointed
> issue, I'll ack this.
> 

Thanks!

Eric, so mm/ would be fine or I still should move it to kernel/
instead? I've addressed other issues I hope.

	Cyrill

Re: [patch 2/4] [RFC] syscalls, x86: Add __NR_kcmp syscall v4

[Eric W. Biederman]

Cyrill Gorcunov <gorcunov@gmail.com> writes:

> On Tue, Jan 24, 2012 at 03:20:26PM -0500, KOSAKI Motohiro wrote:
>> >> please do as you like.
>> >
>> > So it should be something like below I think...
>> 
>> Looks ok this version to me. So, if you fix other developers pointed
>> issue, I'll ack this.
>> 
>
> Thanks!
>
> Eric, so mm/ would be fine or I still should move it to kernel/
> instead? I've addressed other issues I hope.

The world won't fall apart if the code lands in mm.  I have a strong
preference for kernel/.  I just don't see anything at all memory
management like about that code.  Even the fact that you are
comparing pointers is an implementation detail.

Eric

Re: [patch 2/4] [RFC] syscalls, x86: Add __NR_kcmp syscall v4

[Cyrill Gorcunov]

On Tue, Jan 24, 2012 at 12:44:59PM -0800, Eric W. Biederman wrote:
> Cyrill Gorcunov <gorcunov@gmail.com> writes:
> 
> > On Tue, Jan 24, 2012 at 03:20:26PM -0500, KOSAKI Motohiro wrote:
> >> >> please do as you like.
> >> >
> >> > So it should be something like below I think...
> >> 
> >> Looks ok this version to me. So, if you fix other developers pointed
> >> issue, I'll ack this.
> >> 
> >
> > Thanks!
> >
> > Eric, so mm/ would be fine or I still should move it to kernel/
> > instead? I've addressed other issues I hope.
> 
> The world won't fall apart if the code lands in mm.  I have a strong
> preference for kernel/.  I just don't see anything at all memory
> management like about that code.  Even the fact that you are
> comparing pointers is an implementation detail.

This one should fit all requirements I guess.

	Cyrill
---
From: Cyrill Gorcunov <gorcunov@openvz.org>
Subject: [RFC] syscalls, x86: Add __NR_kcmp syscall v6

While doing the checkpoint-restore in the userspace one need to determine
whether various kernel objects (like mm_struct-s of file_struct-s) are shared
between tasks and restore this state.

The 2nd step can be solved by using appropriate CLONE_ flags and the unshare
syscall, while there's currently no ways for solving the 1st one.

One of the ways for checking whether two tasks share e.g. mm_struct is to
provide some mm_struct ID of a task to its proc file, but showing such
info considered to be not that good for security reasons.

Thus after some debates we end up in conclusion that using that named
'comparision' syscall might be the best candidate. So here is it --
__NR_kcmp.

It takes up to 5 agruments - the pids of the two tasks (which
characteristics should be compared), the comparision type and
(in case of comparision of files) two file descriptors.

At moment only x86 is supported.

Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
CC: "Eric W. Biederman" <ebiederm@xmission.com>
CC: Pavel Emelyanov <xemul@parallels.com>
CC: Andrey Vagin <avagin@openvz.org>
CC: KOSAKI Motohiro <kosaki.motohiro@gmail.com>
CC: Ingo Molnar <mingo@elte.hu>
CC: H. Peter Anvin <hpa@zytor.com>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Glauber Costa <glommer@parallels.com>
CC: Andi Kleen <andi@firstfloor.org>
CC: Tejun Heo <tj@kernel.org>
CC: Matt Helsley <matthltc@us.ibm.com>
CC: Pekka Enberg <penberg@kernel.org>
CC: Eric Dumazet <eric.dumazet@gmail.com>
CC: Vasiliy Kulikov <segoon@openwall.com>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: Alexey Dobriyan <adobriyan@gmail.com>
CC: Valdis.Kletnieks@vt.edu
---
 arch/x86/include/asm/syscalls.h  |    4 
 arch/x86/syscalls/syscall_32.tbl |    1 
 arch/x86/syscalls/syscall_64.tbl |    1 
 include/linux/kcmp.h             |   17 ++++
 kernel/Makefile                  |    1 
 kernel/kcmp.c                    |  163 +++++++++++++++++++++++++++++++++++++++
 6 files changed, 187 insertions(+)

Index: linux-2.6.git/arch/x86/include/asm/syscalls.h
===================================================================
--- linux-2.6.git.orig/arch/x86/include/asm/syscalls.h
+++ linux-2.6.git/arch/x86/include/asm/syscalls.h
@@ -42,6 +42,10 @@ long sys_sigaltstack(const stack_t __use
 asmlinkage int sys_set_thread_area(struct user_desc __user *);
 asmlinkage int sys_get_thread_area(struct user_desc __user *);
 
+/* kernel/kcmp.c */
+asmlinkage long sys_kcmp(pid_t pid1, pid_t pid2, int type,
+			 unsigned long idx1, unsigned long idx2);
+
 /* X86_32 only */
 #ifdef CONFIG_X86_32
 
Index: linux-2.6.git/arch/x86/syscalls/syscall_32.tbl
===================================================================
--- linux-2.6.git.orig/arch/x86/syscalls/syscall_32.tbl
+++ linux-2.6.git/arch/x86/syscalls/syscall_32.tbl
@@ -355,3 +355,4 @@
 346	i386	setns			sys_setns
 347	i386	process_vm_readv	sys_process_vm_readv		compat_sys_process_vm_readv
 348	i386	process_vm_writev	sys_process_vm_writev		compat_sys_process_vm_writev
+349	i386	kcmp			sys_kcmp
Index: linux-2.6.git/arch/x86/syscalls/syscall_64.tbl
===================================================================
--- linux-2.6.git.orig/arch/x86/syscalls/syscall_64.tbl
+++ linux-2.6.git/arch/x86/syscalls/syscall_64.tbl
@@ -318,3 +318,4 @@
 309	64	getcpu			sys_getcpu
 310	64	process_vm_readv	sys_process_vm_readv
 311	64	process_vm_writev	sys_process_vm_writev
+312	64	kcmp			sys_kcmp
Index: linux-2.6.git/include/linux/kcmp.h
===================================================================
--- /dev/null
+++ linux-2.6.git/include/linux/kcmp.h
@@ -0,0 +1,17 @@
+#ifndef _LINUX_KCMP_H
+#define _LINUX_KCMP_H
+
+/* Comparision type */
+enum {
+	KCMP_FILE,
+	KCMP_VM,
+	KCMP_FILES,
+	KCMP_FS,
+	KCMP_SIGHAND,
+	KCMP_IO,
+	KCMP_SYSVSEM,
+
+	KCMP_TYPES,
+};
+
+#endif /* _LINUX_KCMP_H */
Index: linux-2.6.git/kernel/Makefile
===================================================================
--- linux-2.6.git.orig/kernel/Makefile
+++ linux-2.6.git/kernel/Makefile
@@ -25,6 +25,7 @@ endif
 obj-y += sched/
 obj-y += power/
 
+obj-$(CONFIG_X86) += kcmp.o
 obj-$(CONFIG_FREEZER) += freezer.o
 obj-$(CONFIG_PROFILING) += profile.o
 obj-$(CONFIG_SYSCTL_SYSCALL_CHECK) += sysctl_check.o
Index: linux-2.6.git/kernel/kcmp.c
===================================================================
--- /dev/null
+++ linux-2.6.git/kernel/kcmp.c
@@ -0,0 +1,163 @@
+#include <linux/kernel.h>
+#include <linux/syscalls.h>
+#include <linux/fdtable.h>
+#include <linux/string.h>
+#include <linux/random.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/cache.h>
+#include <linux/bug.h>
+#include <linux/err.h>
+#include <linux/kcmp.h>
+
+#include <asm/unistd.h>
+
+static unsigned long cookies[KCMP_TYPES][2] __read_mostly;
+
+static long kptr_obfuscate(long v, int type)
+{
+	return (v ^ cookies[type][0]) * cookies[type][1];
+}
+
+/*
+ * 0 - equal
+ * 1 - less than
+ * 2 - greater than
+ * 3 - not equal but ordering unavailable
+ */
+static int kcmp_ptr(long v1, long v2, int type)
+{
+	long ret;
+
+	ret = kptr_obfuscate(v1, type) - kptr_obfuscate(v2, type);
+
+	return (ret < 0) | ((ret > 0) << 1);
+}
+
+#define KCMP_TASK_PTR(task1, task2, member, type)	\
+	kcmp_ptr((long)(task1)->member,			\
+		 (long)(task2)->member,			\
+		 type)
+
+#define KCMP_PTR(ptr1, ptr2, type)			\
+	kcmp_ptr((long)ptr1, (long)ptr2, type)
+
+/* A caller must be sure the task is presented in memory */
+static struct file *
+get_file_raw_ptr(struct task_struct *task, unsigned int idx)
+{
+	struct fdtable *fdt;
+	struct file *file;
+
+	spin_lock(&task->files->file_lock);
+	fdt = files_fdtable(task->files);
+	if (idx < fdt->max_fds)
+		file = fdt->fd[idx];
+	else
+		file = NULL;
+	spin_unlock(&task->files->file_lock);
+
+	return file;
+}
+
+SYSCALL_DEFINE5(kcmp, pid_t, pid1, pid_t, pid2, int, type,
+		unsigned long, idx1, unsigned long, idx2)
+{
+	struct task_struct *task1;
+	struct task_struct *task2;
+	int ret = 0;
+
+	rcu_read_lock();
+
+	task1 = find_task_by_vpid(pid1);
+	if (!task1) {
+		rcu_read_unlock();
+		return -ESRCH;
+	}
+
+	task2 = find_task_by_vpid(pid2);
+	if (!task2) {
+		put_task_struct(task1);
+		rcu_read_unlock();
+		return -ESRCH;
+	}
+
+	get_task_struct(task1);
+	get_task_struct(task2);
+
+	rcu_read_unlock();
+
+	if (!ptrace_may_access(task1, PTRACE_MODE_READ) ||
+	    !ptrace_may_access(task2, PTRACE_MODE_READ)) {
+		ret = -EACCES;
+		goto err;
+	}
+
+	/*
+	 * Note for all cases but the KCMP_FILE we
+	 * don't take any locks in a sake of speed.
+	 */
+
+	switch (type) {
+	case KCMP_FILE: {
+		struct file *filp1, *filp2;
+
+		filp1 = get_file_raw_ptr(task1, idx1);
+		filp2 = get_file_raw_ptr(task2, idx2);
+
+		if (filp1 && filp2)
+			ret = KCMP_PTR(filp1, filp2, KCMP_FILE);
+		else
+			ret = -ENOENT;
+		break;
+	}
+	case KCMP_VM:
+		ret = KCMP_TASK_PTR(task1, task2, mm, KCMP_VM);
+		break;
+	case KCMP_FILES:
+		ret = KCMP_TASK_PTR(task1, task2, files, KCMP_FILES);
+		break;
+	case KCMP_FS:
+		ret = KCMP_TASK_PTR(task1, task2, fs, KCMP_FS);
+		break;
+	case KCMP_SIGHAND:
+		ret = KCMP_TASK_PTR(task1, task2, sighand, KCMP_SIGHAND);
+		break;
+	case KCMP_IO:
+		ret = KCMP_TASK_PTR(task1, task2, io_context, KCMP_IO);
+		break;
+	case KCMP_SYSVSEM:
+#ifdef CONFIG_SYSVIPC
+		ret = KCMP_TASK_PTR(task1, task2, sysvsem.undo_list, KCMP_SYSVSEM);
+#else
+		ret = -ENOENT;
+		goto err;
+#endif
+		break;
+	default:
+		ret = -EINVAL;
+		goto err;
+	}
+
+err:
+	put_task_struct(task1);
+	put_task_struct(task2);
+
+	return ret;
+}
+
+static __init int kcmp_cookie_init(void)
+{
+	int i, j;
+
+	for (i = 0; i < KCMP_TYPES; i++) {
+		for (j = 0; j < 2; j++) {
+			get_random_bytes(&cookies[i][j],
+					 sizeof(cookies[i][j]));
+		}
+		cookies[i][1] |= (~(~0UL >>  1) | 1);
+	}
+
+	return 0;
+}
+late_initcall(kcmp_cookie_init);

Re: [patch 2/4] [RFC] syscalls, x86: Add __NR_kcmp syscall v4

[Eric W. Biederman]

Cyrill Gorcunov <gorcunov@gmail.com> writes:

> On Tue, Jan 24, 2012 at 12:44:59PM -0800, Eric W. Biederman wrote:
>> Cyrill Gorcunov <gorcunov@gmail.com> writes:
>> 
>> > On Tue, Jan 24, 2012 at 03:20:26PM -0500, KOSAKI Motohiro wrote:
>> >> >> please do as you like.
>> >> >
>> >> > So it should be something like below I think...
>> >> 
>> >> Looks ok this version to me. So, if you fix other developers pointed
>> >> issue, I'll ack this.
>> >> 
>> >
>> > Thanks!
>> >
>> > Eric, so mm/ would be fine or I still should move it to kernel/
>> > instead? I've addressed other issues I hope.
>> 
>> The world won't fall apart if the code lands in mm.  I have a strong
>> preference for kernel/.  I just don't see anything at all memory
>> management like about that code.  Even the fact that you are
>> comparing pointers is an implementation detail.
>
> This one should fit all requirements I guess.

Bahahaha!

Looking I see one  more nit.

You need an entry in include/linux/syscalls.h

Eric

> ---
> From: Cyrill Gorcunov <gorcunov@openvz.org>
> Subject: [RFC] syscalls, x86: Add __NR_kcmp syscall v6
>
> While doing the checkpoint-restore in the userspace one need to determine
> whether various kernel objects (like mm_struct-s of file_struct-s) are shared
> between tasks and restore this state.
>
> The 2nd step can be solved by using appropriate CLONE_ flags and the unshare
> syscall, while there's currently no ways for solving the 1st one.
>
> One of the ways for checking whether two tasks share e.g. mm_struct is to
> provide some mm_struct ID of a task to its proc file, but showing such
> info considered to be not that good for security reasons.
>
> Thus after some debates we end up in conclusion that using that named
> 'comparision' syscall might be the best candidate. So here is it --
> __NR_kcmp.
>
> It takes up to 5 agruments - the pids of the two tasks (which
> characteristics should be compared), the comparision type and
> (in case of comparision of files) two file descriptors.
>
> At moment only x86 is supported.
>
> Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
> CC: "Eric W. Biederman" <ebiederm@xmission.com>
> CC: Pavel Emelyanov <xemul@parallels.com>
> CC: Andrey Vagin <avagin@openvz.org>
> CC: KOSAKI Motohiro <kosaki.motohiro@gmail.com>
> CC: Ingo Molnar <mingo@elte.hu>
> CC: H. Peter Anvin <hpa@zytor.com>
> CC: Thomas Gleixner <tglx@linutronix.de>
> CC: Glauber Costa <glommer@parallels.com>
> CC: Andi Kleen <andi@firstfloor.org>
> CC: Tejun Heo <tj@kernel.org>
> CC: Matt Helsley <matthltc@us.ibm.com>
> CC: Pekka Enberg <penberg@kernel.org>
> CC: Eric Dumazet <eric.dumazet@gmail.com>
> CC: Vasiliy Kulikov <segoon@openwall.com>
> CC: Andrew Morton <akpm@linux-foundation.org>
> CC: Alexey Dobriyan <adobriyan@gmail.com>
> CC: Valdis.Kletnieks@vt.edu
> ---
>  arch/x86/include/asm/syscalls.h  |    4 
>  arch/x86/syscalls/syscall_32.tbl |    1 
>  arch/x86/syscalls/syscall_64.tbl |    1 
>  include/linux/kcmp.h             |   17 ++++
>  kernel/Makefile                  |    1 
>  kernel/kcmp.c                    |  163 +++++++++++++++++++++++++++++++++++++++
>  6 files changed, 187 insertions(+)
>
> Index: linux-2.6.git/arch/x86/include/asm/syscalls.h
> ===================================================================
> --- linux-2.6.git.orig/arch/x86/include/asm/syscalls.h
> +++ linux-2.6.git/arch/x86/include/asm/syscalls.h
> @@ -42,6 +42,10 @@ long sys_sigaltstack(const stack_t __use
>  asmlinkage int sys_set_thread_area(struct user_desc __user *);
>  asmlinkage int sys_get_thread_area(struct user_desc __user *);
>  
> +/* kernel/kcmp.c */
> +asmlinkage long sys_kcmp(pid_t pid1, pid_t pid2, int type,
> +			 unsigned long idx1, unsigned long idx2);
> +
>  /* X86_32 only */
>  #ifdef CONFIG_X86_32
>  
> Index: linux-2.6.git/arch/x86/syscalls/syscall_32.tbl
> ===================================================================
> --- linux-2.6.git.orig/arch/x86/syscalls/syscall_32.tbl
> +++ linux-2.6.git/arch/x86/syscalls/syscall_32.tbl
> @@ -355,3 +355,4 @@
>  346	i386	setns			sys_setns
>  347	i386	process_vm_readv	sys_process_vm_readv		compat_sys_process_vm_readv
>  348	i386	process_vm_writev	sys_process_vm_writev		compat_sys_process_vm_writev
> +349	i386	kcmp			sys_kcmp
> Index: linux-2.6.git/arch/x86/syscalls/syscall_64.tbl
> ===================================================================
> --- linux-2.6.git.orig/arch/x86/syscalls/syscall_64.tbl
> +++ linux-2.6.git/arch/x86/syscalls/syscall_64.tbl
> @@ -318,3 +318,4 @@
>  309	64	getcpu			sys_getcpu
>  310	64	process_vm_readv	sys_process_vm_readv
>  311	64	process_vm_writev	sys_process_vm_writev
> +312	64	kcmp			sys_kcmp
> Index: linux-2.6.git/include/linux/kcmp.h
> ===================================================================
> --- /dev/null
> +++ linux-2.6.git/include/linux/kcmp.h
> @@ -0,0 +1,17 @@
> +#ifndef _LINUX_KCMP_H
> +#define _LINUX_KCMP_H
> +
> +/* Comparision type */
> +enum {
> +	KCMP_FILE,
> +	KCMP_VM,
> +	KCMP_FILES,
> +	KCMP_FS,
> +	KCMP_SIGHAND,
> +	KCMP_IO,
> +	KCMP_SYSVSEM,
> +
> +	KCMP_TYPES,
> +};
> +
> +#endif /* _LINUX_KCMP_H */
> Index: linux-2.6.git/kernel/Makefile
> ===================================================================
> --- linux-2.6.git.orig/kernel/Makefile
> +++ linux-2.6.git/kernel/Makefile
> @@ -25,6 +25,7 @@ endif
>  obj-y += sched/
>  obj-y += power/
>  
> +obj-$(CONFIG_X86) += kcmp.o
>  obj-$(CONFIG_FREEZER) += freezer.o
>  obj-$(CONFIG_PROFILING) += profile.o
>  obj-$(CONFIG_SYSCTL_SYSCALL_CHECK) += sysctl_check.o
> Index: linux-2.6.git/kernel/kcmp.c
> ===================================================================
> --- /dev/null
> +++ linux-2.6.git/kernel/kcmp.c
> @@ -0,0 +1,163 @@
> +#include <linux/kernel.h>
> +#include <linux/syscalls.h>
> +#include <linux/fdtable.h>
> +#include <linux/string.h>
> +#include <linux/random.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/cache.h>
> +#include <linux/bug.h>
> +#include <linux/err.h>
> +#include <linux/kcmp.h>
> +
> +#include <asm/unistd.h>
> +
> +static unsigned long cookies[KCMP_TYPES][2] __read_mostly;
> +
> +static long kptr_obfuscate(long v, int type)
> +{
> +	return (v ^ cookies[type][0]) * cookies[type][1];
> +}
> +
> +/*
> + * 0 - equal
> + * 1 - less than
> + * 2 - greater than
> + * 3 - not equal but ordering unavailable
> + */
> +static int kcmp_ptr(long v1, long v2, int type)
> +{
> +	long ret;
> +
> +	ret = kptr_obfuscate(v1, type) - kptr_obfuscate(v2, type);
> +
> +	return (ret < 0) | ((ret > 0) << 1);
> +}
> +
> +#define KCMP_TASK_PTR(task1, task2, member, type)	\
> +	kcmp_ptr((long)(task1)->member,			\
> +		 (long)(task2)->member,			\
> +		 type)
> +
> +#define KCMP_PTR(ptr1, ptr2, type)			\
> +	kcmp_ptr((long)ptr1, (long)ptr2, type)
> +
> +/* A caller must be sure the task is presented in memory */
> +static struct file *
> +get_file_raw_ptr(struct task_struct *task, unsigned int idx)
> +{
> +	struct fdtable *fdt;
> +	struct file *file;
> +
> +	spin_lock(&task->files->file_lock);
> +	fdt = files_fdtable(task->files);
> +	if (idx < fdt->max_fds)
> +		file = fdt->fd[idx];
> +	else
> +		file = NULL;
> +	spin_unlock(&task->files->file_lock);
> +
> +	return file;
> +}
> +
> +SYSCALL_DEFINE5(kcmp, pid_t, pid1, pid_t, pid2, int, type,
> +		unsigned long, idx1, unsigned long, idx2)
> +{
> +	struct task_struct *task1;
> +	struct task_struct *task2;
> +	int ret = 0;
> +
> +	rcu_read_lock();
> +
> +	task1 = find_task_by_vpid(pid1);
> +	if (!task1) {
> +		rcu_read_unlock();
> +		return -ESRCH;
> +	}
> +
> +	task2 = find_task_by_vpid(pid2);
> +	if (!task2) {
> +		put_task_struct(task1);
> +		rcu_read_unlock();
> +		return -ESRCH;
> +	}
> +
> +	get_task_struct(task1);
> +	get_task_struct(task2);
> +
> +	rcu_read_unlock();
> +
> +	if (!ptrace_may_access(task1, PTRACE_MODE_READ) ||
> +	    !ptrace_may_access(task2, PTRACE_MODE_READ)) {
> +		ret = -EACCES;
> +		goto err;
> +	}
> +
> +	/*
> +	 * Note for all cases but the KCMP_FILE we
> +	 * don't take any locks in a sake of speed.
> +	 */
> +
> +	switch (type) {
> +	case KCMP_FILE: {
> +		struct file *filp1, *filp2;
> +
> +		filp1 = get_file_raw_ptr(task1, idx1);
> +		filp2 = get_file_raw_ptr(task2, idx2);
> +
> +		if (filp1 && filp2)
> +			ret = KCMP_PTR(filp1, filp2, KCMP_FILE);
> +		else
> +			ret = -ENOENT;
> +		break;
> +	}
> +	case KCMP_VM:
> +		ret = KCMP_TASK_PTR(task1, task2, mm, KCMP_VM);
> +		break;
> +	case KCMP_FILES:
> +		ret = KCMP_TASK_PTR(task1, task2, files, KCMP_FILES);
> +		break;
> +	case KCMP_FS:
> +		ret = KCMP_TASK_PTR(task1, task2, fs, KCMP_FS);
> +		break;
> +	case KCMP_SIGHAND:
> +		ret = KCMP_TASK_PTR(task1, task2, sighand, KCMP_SIGHAND);
> +		break;
> +	case KCMP_IO:
> +		ret = KCMP_TASK_PTR(task1, task2, io_context, KCMP_IO);
> +		break;
> +	case KCMP_SYSVSEM:
> +#ifdef CONFIG_SYSVIPC
> +		ret = KCMP_TASK_PTR(task1, task2, sysvsem.undo_list, KCMP_SYSVSEM);
> +#else
> +		ret = -ENOENT;
> +		goto err;
> +#endif
> +		break;
> +	default:
> +		ret = -EINVAL;
> +		goto err;
> +	}
> +
> +err:
> +	put_task_struct(task1);
> +	put_task_struct(task2);
> +
> +	return ret;
> +}
> +
> +static __init int kcmp_cookie_init(void)
> +{
> +	int i, j;
> +
> +	for (i = 0; i < KCMP_TYPES; i++) {
> +		for (j = 0; j < 2; j++) {
> +			get_random_bytes(&cookies[i][j],
> +					 sizeof(cookies[i][j]));
> +		}
> +		cookies[i][1] |= (~(~0UL >>  1) | 1);
> +	}
> +
> +	return 0;
> +}
> +late_initcall(kcmp_cookie_init);

Re: [patch 2/4] [RFC] syscalls, x86: Add __NR_kcmp syscall v4

[Cyrill Gorcunov]

On Tue, Jan 24, 2012 at 01:20:12PM -0800, Eric W. Biederman wrote:
...
> >
> > This one should fit all requirements I guess.
> 
> Bahahaha!
> 
> Looking I see one  more nit.
> 
> You need an entry in include/linux/syscalls.h

Sigh.. ;) Not just this. I'll post updated version to address
Andrew's comments too once I finish it (not today probably).

	Cyrill

Re: [patch 2/4] [RFC] syscalls, x86: Add __NR_kcmp syscall v4

[Andrew Morton]

On Wed, 25 Jan 2012 00:50:39 +0400
Cyrill Gorcunov <gorcunov@gmail.com> wrote:

> This one should fit all requirements I guess.

[wakes up]

> While doing the checkpoint-restore in the userspace one need to determine
> whether various kernel objects (like mm_struct-s of file_struct-s) are shared
> between tasks and restore this state.
> 
> The 2nd step can be solved by using appropriate CLONE_ flags and the unshare
> syscall, while there's currently no ways for solving the 1st one.
> 
> One of the ways for checking whether two tasks share e.g. mm_struct is to
> provide some mm_struct ID of a task to its proc file, but showing such
> info considered to be not that good for security reasons.
> 
> Thus after some debates we end up in conclusion that using that named
> 'comparision' syscall might be the best candidate. So here is it --
> __NR_kcmp.
> 
> It takes up to 5 agruments - the pids of the two tasks (which
> characteristics should be compared), the comparision type and
> (in case of comparision of files) two file descriptors.

PIDs are not unique.  One wonders what happens in this syscall if the
same pid appears in two namespaces.

<reads the code>

Seems that it performs lookups only in the caller's PID namespace. 
Maybe this is appropriate but it should be described and justified in
the changelog and in code comments, please.  And in the forthcoming
manpage ;)

> At moment only x86 is supported.

Presumably you have a test app.  Please let's include that app in
tools/testing/selftests/ for arch maintainers and others to use and
maintain.

>
> ...
>
> --- /dev/null
> +++ linux-2.6.git/kernel/kcmp.c
> @@ -0,0 +1,163 @@
> +#include <linux/kernel.h>
> +#include <linux/syscalls.h>
> +#include <linux/fdtable.h>
> +#include <linux/string.h>
> +#include <linux/random.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/cache.h>
> +#include <linux/bug.h>
> +#include <linux/err.h>
> +#include <linux/kcmp.h>
> +
> +#include <asm/unistd.h>
> +
> +static unsigned long cookies[KCMP_TYPES][2] __read_mostly;

This reader of this code doesn't understand why all this cookie stuff
is in here.  Please include code comments which explain the reason for
the existence of this code.

> +static long kptr_obfuscate(long v, int type)
> +{
> +	return (v ^ cookies[type][0]) * cookies[type][1];
> +}
> +
> +/*
> + * 0 - equal
> + * 1 - less than
> + * 2 - greater than
> + * 3 - not equal but ordering unavailable

what the heck does case 3 mean?  Why is it here?

> + */
> +static int kcmp_ptr(long v1, long v2, int type)
> +{
> +	long ret;
> +
> +	ret = kptr_obfuscate(v1, type) - kptr_obfuscate(v2, type);
> +
> +	return (ret < 0) | ((ret > 0) << 1);
> +}
> +
> +#define KCMP_TASK_PTR(task1, task2, member, type)	\
> +	kcmp_ptr((long)(task1)->member,			\
> +		 (long)(task2)->member,			\
> +		 type)
> +
> +#define KCMP_PTR(ptr1, ptr2, type)			\
> +	kcmp_ptr((long)ptr1, (long)ptr2, type)

ugh.  This:

static long kptr_obfuscate(void *p, enum you_forgot_to_name_the_enum type)
{
	return ((long)p ^ cookies[type][0]) * cookies[type][1];
}

static int kcmp_task_pointers(void *task1, void *task2, size_t field_offset,
				enum you_forgot_to_name_the_enum type)
{
	void **field1 = t1 + field_offset;	/* points to a pointer in the task_struct */
	void **field2 = t1 + field_offset;
	long diff;

	diff = kptr_obfuscate(*field1, type) - kptr_obfuscate(*field2, type);
	return (diff < 0) | ((diff > 0) << 1);
}

	...
	ret = kcmp_task_pointers(task1, task2, offsetof(task_struct, mm),
				KCMP_VM);
	...

see?  No nasty macros, it's type-correct and it uses only a single
explicit typecast.

> +/* A caller must be sure the task is presented in memory */

"The caller must have pinned the task"

> +static struct file *
> +get_file_raw_ptr(struct task_struct *task, unsigned int idx)
> +{
> +	struct fdtable *fdt;
> +	struct file *file;
> +
> +	spin_lock(&task->files->file_lock);
> +	fdt = files_fdtable(task->files);
> +	if (idx < fdt->max_fds)
> +		file = fdt->fd[idx];
> +	else
> +		file = NULL;
> +	spin_unlock(&task->files->file_lock);
> +
> +	return file;
> +}
> +
> +SYSCALL_DEFINE5(kcmp, pid_t, pid1, pid_t, pid2, int, type,
> +		unsigned long, idx1, unsigned long, idx2)
> +{
> +	struct task_struct *task1;
> +	struct task_struct *task2;
> +	int ret = 0;
> +
> +	rcu_read_lock();
> +
> +	task1 = find_task_by_vpid(pid1);
> +	if (!task1) {
> +		rcu_read_unlock();
> +		return -ESRCH;
> +	}
> +
> +	task2 = find_task_by_vpid(pid2);
> +	if (!task2) {
> +		put_task_struct(task1);
> +		rcu_read_unlock();
> +		return -ESRCH;
> +	}
> +
> +	get_task_struct(task1);
> +	get_task_struct(task2);
> +
> +	rcu_read_unlock();
> +
> +	if (!ptrace_may_access(task1, PTRACE_MODE_READ) ||
> +	    !ptrace_may_access(task2, PTRACE_MODE_READ)) {

Add a comment explaining this decision.

> +		ret = -EACCES;
> +		goto err;
> +	}
> +
> +	/*
> +	 * Note for all cases but the KCMP_FILE we
> +	 * don't take any locks in a sake of speed.
> +	 */
> +
> +	switch (type) {
> +	case KCMP_FILE: {
> +		struct file *filp1, *filp2;
> +
> +		filp1 = get_file_raw_ptr(task1, idx1);
> +		filp2 = get_file_raw_ptr(task2, idx2);
> +
> +		if (filp1 && filp2)
> +			ret = KCMP_PTR(filp1, filp2, KCMP_FILE);
> +		else
> +			ret = -ENOENT;
> +		break;
> +	}
> +	case KCMP_VM:
> +		ret = KCMP_TASK_PTR(task1, task2, mm, KCMP_VM);
> +		break;
> +	case KCMP_FILES:
> +		ret = KCMP_TASK_PTR(task1, task2, files, KCMP_FILES);
> +		break;
> +	case KCMP_FS:
> +		ret = KCMP_TASK_PTR(task1, task2, fs, KCMP_FS);
> +		break;
> +	case KCMP_SIGHAND:
> +		ret = KCMP_TASK_PTR(task1, task2, sighand, KCMP_SIGHAND);
> +		break;
> +	case KCMP_IO:
> +		ret = KCMP_TASK_PTR(task1, task2, io_context, KCMP_IO);
> +		break;
> +	case KCMP_SYSVSEM:
> +#ifdef CONFIG_SYSVIPC
> +		ret = KCMP_TASK_PTR(task1, task2, sysvsem.undo_list, KCMP_SYSVSEM);
> +#else
> +		ret = -ENOENT;

ENOENT seems inappropriate here.

> +		goto err;
> +#endif
> +		break;
> +	default:
> +		ret = -EINVAL;
> +		goto err;
> +	}
> +
> +err:
> +	put_task_struct(task1);
> +	put_task_struct(task2);
> +
> +	return ret;
> +}
> +
> +static __init int kcmp_cookie_init(void)
> +{
> +	int i, j;
> +
> +	for (i = 0; i < KCMP_TYPES; i++) {
> +		for (j = 0; j < 2; j++) {
> +			get_random_bytes(&cookies[i][j],
> +					 sizeof(cookies[i][j]));
> +		}
> +		cookies[i][1] |= (~(~0UL >>  1) | 1);

hm, what's the point in writing a random number to cookies[i][1] and
then immediately overwriting that with a constant?

> +	}
> +
> +	return 0;
> +}
> +late_initcall(kcmp_cookie_init);

Re: [patch 2/4] [RFC] syscalls, x86: Add __NR_kcmp syscall v4

[Andrew Morton]

On Tue, 24 Jan 2012 13:22:22 -0800
Andrew Morton <akpm@linux-foundation.org> wrote:

> static int kcmp_task_pointers(void *task1, void *task2, size_t field_offset,
> 				enum you_forgot_to_name_the_enum type)
> {
> 	void **field1 = t1 + field_offset;	/* points to a pointer in the task_struct */
> 	void **field2 = t1 + field_offset;

On reflection, this was being too cute.  It would be better to make the
function type-safer and just put up with the local typecasts:

static int kcmp_task_pointers(struct task_struct *task1,
			struct task_struct *task2, size_t field_offset,
			enum you_forgot_to_name_the_enum type)
{
	void **field1 = (void *)t1 + field_offset;
	void **field2 = (void *)t2 + field_offset;

Re: [patch 2/4] [RFC] syscalls, x86: Add __NR_kcmp syscall v4

[H. Peter Anvin]

On 01/24/2012 01:45 PM, Andrew Morton wrote:
> On Tue, 24 Jan 2012 13:22:22 -0800
> Andrew Morton <akpm@linux-foundation.org> wrote:
> 
>> static int kcmp_task_pointers(void *task1, void *task2, size_t field_offset,
>> 				enum you_forgot_to_name_the_enum type)
>> {
>> 	void **field1 = t1 + field_offset;	/* points to a pointer in the task_struct */
>> 	void **field2 = t1 + field_offset;
> 
> On reflection, this was being too cute.  It would be better to make the
> function type-safer and just put up with the local typecasts:
> 
> static int kcmp_task_pointers(struct task_struct *task1,
> 			struct task_struct *task2, size_t field_offset,
> 			enum you_forgot_to_name_the_enum type)
> {
> 	void **field1 = (void *)t1 + field_offset;
> 	void **field2 = (void *)t2 + field_offset;
> 

Arithmetic on void pointers?

	-hpa

Re: [patch 2/4] [RFC] syscalls, x86: Add __NR_kcmp syscall v4

[Andrew Morton]

On Tue, 24 Jan 2012 13:46:31 -0800
"H. Peter Anvin" <hpa@zytor.com> wrote:

> On 01/24/2012 01:45 PM, Andrew Morton wrote:
> > On Tue, 24 Jan 2012 13:22:22 -0800
> > Andrew Morton <akpm@linux-foundation.org> wrote:
> > 
> >> static int kcmp_task_pointers(void *task1, void *task2, size_t field_offset,
> >> 				enum you_forgot_to_name_the_enum type)
> >> {
> >> 	void **field1 = t1 + field_offset;	/* points to a pointer in the task_struct */
> >> 	void **field2 = t1 + field_offset;
> > 
> > On reflection, this was being too cute.  It would be better to make the
> > function type-safer and just put up with the local typecasts:
> > 
> > static int kcmp_task_pointers(struct task_struct *task1,
> > 			struct task_struct *task2, size_t field_offset,
> > 			enum you_forgot_to_name_the_enum type)
> > {
> > 	void **field1 = (void *)t1 + field_offset;
> > 	void **field2 = (void *)t2 + field_offset;
> > 
> 
> Arithmetic on void pointers?
> 

All the world is gcc!

Re: [patch 2/4] [RFC] syscalls, x86: Add __NR_kcmp syscall v4

[H. Peter Anvin]

On 01/24/2012 02:00 PM, Andrew Morton wrote:
>>
>> Arithmetic on void pointers?
> 
> All the world is gcc!

Even so, warnings are annoying...

	-hpa

Re: [patch 2/4] [RFC] syscalls, x86: Add __NR_kcmp syscall v4

[Andrew Morton]

On Tue, 24 Jan 2012 14:52:32 -0800
"H. Peter Anvin" <hpa@zytor.com> wrote:

> On 01/24/2012 02:00 PM, Andrew Morton wrote:
> >>
> >> Arithmetic on void pointers?
> > 
> > All the world is gcc!
> 
> Even so, warnings are annoying...
> 

We already do arithmetic on void* in many places.  Run "make
W=3" and admire..

Re: [patch 2/4] [RFC] syscalls, x86: Add __NR_kcmp syscall v4

[Cyrill Gorcunov]

On Tue, Jan 24, 2012 at 01:22:22PM -0800, Andrew Morton wrote:
> 
> PIDs are not unique.  One wonders what happens in this syscall if the
> same pid appears in two namespaces.
> 
> <reads the code>
> 
> Seems that it performs lookups only in the caller's PID namespace. 
> Maybe this is appropriate but it should be described and justified in
> the changelog and in code comments, please.  And in the forthcoming
> manpage ;)
> 

Yes, caller's namespace was used intentionally, will add comments (manpage
makes me shiver).

> > At moment only x86 is supported.
> 
> Presumably you have a test app.  Please let's include that app in
> tools/testing/selftests/ for arch maintainers and others to use and
> maintain.

ok

> > +static unsigned long cookies[KCMP_TYPES][2] __read_mostly;
> 
> This reader of this code doesn't understand why all this cookie stuff
> is in here.  Please include code comments which explain the reason for
> the existence of this code.
> 

ok

> > +static long kptr_obfuscate(long v, int type)
> > +{
> > +	return (v ^ cookies[type][0]) * cookies[type][1];
> > +}
> > +
> > +/*
> > + * 0 - equal
> > + * 1 - less than
> > + * 2 - greater than
> > + * 3 - not equal but ordering unavailable
> 
> what the heck does case 3 mean?  Why is it here?
> 

I'll add a comment. It's reserved for case where we might
need to disable gt/lt comparision result. Probably in future.

> > +
> > +#define KCMP_PTR(ptr1, ptr2, type)			\
> > +	kcmp_ptr((long)ptr1, (long)ptr2, type)
> 
> ugh.  This:
> 
> static long kptr_obfuscate(void *p, enum you_forgot_to_name_the_enum type)
> {
> 	return ((long)p ^ cookies[type][0]) * cookies[type][1];
> }
> 
> static int kcmp_task_pointers(void *task1, void *task2, size_t field_offset,
> 				enum you_forgot_to_name_the_enum type)
> {
> 	void **field1 = t1 + field_offset;	/* points to a pointer in the task_struct */
> 	void **field2 = t1 + field_offset;
> 	long diff;
> 
> 	diff = kptr_obfuscate(*field1, type) - kptr_obfuscate(*field2, type);
> 	return (diff < 0) | ((diff > 0) << 1);
> }
> 
> 	...
> 	ret = kcmp_task_pointers(task1, task2, offsetof(task_struct, mm),
> 				KCMP_VM);
> 	...
> 
> see?  No nasty macros, it's type-correct and it uses only a single
> explicit typecast.
> 

ok, i'll change it of course, but I personally like macros version more.

> > +/* A caller must be sure the task is presented in memory */
> "The caller must have pinned the task"
> 
> > +	if (!ptrace_may_access(task1, PTRACE_MODE_READ) ||
> > +	    !ptrace_may_access(task2, PTRACE_MODE_READ)) {
> 
> Add a comment explaining this decision.
> 

OK.

> 
> ENOENT seems inappropriate here.
>

Which one should be better?

> > +static __init int kcmp_cookie_init(void)
> > +{
> > +	int i, j;
> > +
> > +	for (i = 0; i < KCMP_TYPES; i++) {
> > +		for (j = 0; j < 2; j++) {
> > +			get_random_bytes(&cookies[i][j],
> > +					 sizeof(cookies[i][j]));
> > +		}
> > +		cookies[i][1] |= (~(~0UL >>  1) | 1);
> 
> hm, what's the point in writing a random number to cookies[i][1] and
> then immediately overwriting that with a constant?

It's '|=' , not '='.

	Cyrill

Re: [patch 2/4] [RFC] syscalls, x86: Add __NR_kcmp syscall v4

[Andrew Morton]

On Wed, 25 Jan 2012 01:46:30 +0400
Cyrill Gorcunov <gorcunov@gmail.com> wrote:

> > 
> > ENOENT seems inappropriate here.
> >
> 
> Which one should be better?

EINVAL, I guess.

Re: [patch 2/4] [RFC] syscalls, x86: Add __NR_kcmp syscall v4

[Eric W. Biederman]

Andrew Morton <akpm@linux-foundation.org> writes:


> <reads the code>
>
> Seems that it performs lookups only in the caller's PID namespace. 
> Maybe this is appropriate but it should be described and justified in
> the changelog and in code comments, please.  And in the forthcoming
> manpage ;)

Well pids should always and only be looked up in the callers pid
namespace.  Any other behavior is broken.  It is probably worth
a mention in a manpage but you should not need to justify using
abstractions as they were designed to be used.


>> +static int kcmp_ptr(long v1, long v2, int type)
>> +{
>> +	long ret;
>> +
>> +	ret = kptr_obfuscate(v1, type) - kptr_obfuscate(v2, type);
>> +
>> +	return (ret < 0) | ((ret > 0) << 1);
>> +}
>> +
>> +#define KCMP_TASK_PTR(task1, task2, member, type)	\
>> +	kcmp_ptr((long)(task1)->member,			\
>> +		 (long)(task2)->member,			\
>> +		 type)
>> +
>> +#define KCMP_PTR(ptr1, ptr2, type)			\
>> +	kcmp_ptr((long)ptr1, (long)ptr2, type)
>
> ugh.  This:
>
> static long kptr_obfuscate(void *p, enum you_forgot_to_name_the_enum type)
> {
> 	return ((long)p ^ cookies[type][0]) * cookies[type][1];
> }
>
> static int kcmp_task_pointers(void *task1, void *task2, size_t field_offset,
> 				enum you_forgot_to_name_the_enum type)
> {
> 	void **field1 = t1 + field_offset;	/* points to a pointer in the task_struct */
> 	void **field2 = t1 + field_offset;
> 	long diff;
>
> 	diff = kptr_obfuscate(*field1, type) - kptr_obfuscate(*field2, type);
> 	return (diff < 0) | ((diff > 0) << 1);
> }
>
> 	...
> 	ret = kcmp_task_pointers(task1, task2, offsetof(task_struct, mm),
> 				KCMP_VM);
> 	...
>
> see?  No nasty macros, it's type-correct and it uses only a single
> explicit typecast.

Seriously?  Simply open coding the comparison would be better. 

        ret = kcmp_ptr(task1->files, task2->files, type);

All pointers are not encoded the same as void * pointers.  Admittedly
the only case I can think of are function pointers on Itanium, but
what is a little wrong today can easily become a lot wrong tomorrow.

Making the kcmp_ptr arguments void * seems the way to go though.

Now there is one interesting case we are not handling properly.
If any of our pointers can be NULL which I think happens in the
file case we should return -EBADF instead of reporting two NULL
pointers point to the same object.

Eric

Re: [patch 2/4] [RFC] syscalls, x86: Add __NR_kcmp syscall v4

[Andrew Morton]

On Tue, 24 Jan 2012 14:54:18 -0800
ebiederm@xmission.com (Eric W. Biederman) wrote:

> Simply open coding the comparison would be better. 
> 
>         ret = kcmp_ptr(task1->files, task2->files, type);

yup, that's nice.

Re: [patch 2/4] [RFC] syscalls, x86: Add __NR_kcmp syscall v4

[Andrew Morton]

On Wed, 25 Jan 2012 00:50:39 +0400
Cyrill Gorcunov <gorcunov@gmail.com> wrote:

> While doing the checkpoint-restore in the userspace

btw, some questions regarding the overall project: do you guys actually
have working userspace code which can at least partially perform a c/r?
What state is all this in?

I'd be a bit concerned if we're putting code into mainline which has
not been demonstrated to achieve any form of successful
checkpoint/restore.

Re: [patch 2/4] [RFC] syscalls, x86: Add __NR_kcmp syscall v4

[Cyrill Gorcunov]

On Tue, Jan 24, 2012 at 01:25:16PM -0800, Andrew Morton wrote:
> On Wed, 25 Jan 2012 00:50:39 +0400
> Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> 
> > While doing the checkpoint-restore in the userspace
> 
> btw, some questions regarding the overall project: do you guys actually
> have working userspace code which can at least partially perform a c/r?
> What state is all this in?
> 

Yes, we have a number of test-cases which do c/r. See details on
http://criu.org/ . We don't announce it yet since everything is under
heavy development.

> I'd be a bit concerned if we're putting code into mainline which has
> not been demonstrated to achieve any form of successful checkpoint/restore.

	Cyrill

Re: [patch 2/4] [RFC] syscalls, x86: Add __NR_kcmp syscall v4

[Eric W. Biederman]

KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> writes:

> On Tue, 24 Jan 2012 11:17:16 +0400
> Cyrill Gorcunov <gorcunov@gmail.com> wrote:
>
>> On Mon, Jan 23, 2012 at 11:04:19PM -0800, H. Peter Anvin wrote:
>> > On 01/23/2012 10:47 PM, Cyrill Gorcunov wrote:
>> > >>
>> > >> Why under /arch ?
>> > >>
>> > > 
>> > > Hi Kame,
>> > > 
>> > > because I've tested it under x86 only. Once someone
>> > > confirm it's needed on some else arch and does work
>> > > as expected -- it wont be a problem to make it system
>> > > wide. Until then -- I think better to stick with
>> > > at least tested case.
>> > > ...
>> > 
>> > That's not a reason to put it in arch/ ... that's possibly a reason to
>> > not map the system call on other architectures yet.
>> > 
>> 
>> Where it should live then? In kernel/ or mm/ ?
>> 
>
> kernel/checkpoint_restart ?
>
> gathering related changes to a directory may help developpers joins later....
> To me, this makes seeing git log easy ;)

kernel/

The dominant use may be checkpoint restart but the code is not at all
checkpoint restart specific.

Eric

Re: [patch 2/4] [RFC] syscalls, x86: Add __NR_kcmp syscall v4

[Cyrill Gorcunov]

On Tue, Jan 24, 2012 at 12:49:27AM -0800, Eric W. Biederman wrote:
...
> 
> kernel/
> 
> The dominant use may be checkpoint restart but the code is not at all
> checkpoint restart specific.
> 

Crap, just sent out mm/ version :)

	Cyrill

[patch 3/4] c/r: procfs: add arg_start/end, env_start/end and exit_code members to /proc/$pid/stat

[Cyrill Gorcunov]

[-- Attachment #1: fs-proc-extend-state --]
[-- Type: text/plain, Size: 1871 bytes --]

We would like to have an ability to restore command line
arguments and envirion pointers so the task being restored
would print appropriate values in /proc/pid/cmdline and
/proc/pid/envirion. The exit_code is needed to restore
zombie tasks.

Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
Cc: Pavel Emelyanov <xemul@parallels.com>
Cc: Serge Hallyn <serge.hallyn@canonical.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
Cc: Alexey Dobriyan <adobriyan@gmail.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: Andrew Vagin <avagin@openvz.org>
Cc: Vasiliy Kulikov <segoon@openwall.com>
Cc: Alexey Dobriyan <adobriyan@gmail.com>
Cc: "Eric W. Biederman" <ebiederm@xmission.com>
---
 fs/proc/array.c |   10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Index: linux-2.6.git/fs/proc/array.c
===================================================================
--- linux-2.6.git.orig/fs/proc/array.c
+++ linux-2.6.git/fs/proc/array.c
@@ -464,7 +464,8 @@ static int do_task_stat(struct seq_file
 
 	seq_printf(m, "%d (%s) %c %d %d %d %d %d %u %lu \
 %lu %lu %lu %lu %lu %ld %ld %ld %ld %d 0 %llu %lu %ld %lu %lu %lu %lu %lu \
-%lu %lu %lu %lu %lu %lu %lu %lu %d %d %u %u %llu %lu %ld %lu %lu %lu\n",
+%lu %lu %lu %lu %lu %lu %lu %lu %d %d %u %u %llu %lu %ld %lu %lu %lu \
+%lu %lu %lu %lu %d\n",
 		pid_nr_ns(pid, ns),
 		tcomm,
 		state,
@@ -514,7 +515,12 @@ static int do_task_stat(struct seq_file
 		cputime_to_clock_t(cgtime),
 		(mm && permitted) ? mm->start_data : 0,
 		(mm && permitted) ? mm->end_data : 0,
-		(mm && permitted) ? mm->start_brk : 0);
+		(mm && permitted) ? mm->start_brk : 0,
+		(mm && permitted) ? mm->arg_start : 0,
+		(mm && permitted) ? mm->arg_end : 0,
+		(mm && permitted) ? mm->env_start : 0,
+		(mm && permitted) ? mm->env_end : 0,
+		task->exit_code);
 	if (mm)
 		mmput(mm);
 	return 0;

Re: [patch 3/4] c/r: procfs: add arg_start/end, env_start/end and exit_code members to /proc/$pid/stat

[Kees Cook]

On Mon, Jan 23, 2012 at 6:20 AM, Cyrill Gorcunov <gorcunov@openvz.org> wrote:
> We would like to have an ability to restore command line
> arguments and envirion pointers so the task being restored
> would print appropriate values in /proc/pid/cmdline and
> /proc/pid/envirion. The exit_code is needed to restore
> zombie tasks.
>
> Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
> Cc: Pavel Emelyanov <xemul@parallels.com>
> Cc: Serge Hallyn <serge.hallyn@canonical.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> Cc: Alexey Dobriyan <adobriyan@gmail.com>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Andrew Vagin <avagin@openvz.org>
> Cc: Vasiliy Kulikov <segoon@openwall.com>
> Cc: Alexey Dobriyan <adobriyan@gmail.com>
> Cc: "Eric W. Biederman" <ebiederm@xmission.com>
> ---
>  fs/proc/array.c |   10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> Index: linux-2.6.git/fs/proc/array.c
> ===================================================================
> --- linux-2.6.git.orig/fs/proc/array.c
> +++ linux-2.6.git/fs/proc/array.c
> @@ -464,7 +464,8 @@ static int do_task_stat(struct seq_file
>
>        seq_printf(m, "%d (%s) %c %d %d %d %d %d %u %lu \
>  %lu %lu %lu %lu %lu %ld %ld %ld %ld %d 0 %llu %lu %ld %lu %lu %lu %lu %lu \
> -%lu %lu %lu %lu %lu %lu %lu %lu %d %d %u %u %llu %lu %ld %lu %lu %lu\n",
> +%lu %lu %lu %lu %lu %lu %lu %lu %d %d %u %u %llu %lu %ld %lu %lu %lu \
> +%lu %lu %lu %lu %d\n",
>                pid_nr_ns(pid, ns),
>                tcomm,
>                state,
> @@ -514,7 +515,12 @@ static int do_task_stat(struct seq_file
>                cputime_to_clock_t(cgtime),
>                (mm && permitted) ? mm->start_data : 0,
>                (mm && permitted) ? mm->end_data : 0,
> -               (mm && permitted) ? mm->start_brk : 0);
> +               (mm && permitted) ? mm->start_brk : 0,
> +               (mm && permitted) ? mm->arg_start : 0,
> +               (mm && permitted) ? mm->arg_end : 0,
> +               (mm && permitted) ? mm->env_start : 0,
> +               (mm && permitted) ? mm->env_end : 0,
> +               task->exit_code);
>        if (mm)
>                mmput(mm);
>        return 0;

You're not exposing auxv here? In your testing, what situations ended
up using auxv after initial startup? Or is your intention to be able
to freeze a process potentially before libc has examined auxv?

-Kees

-- 
Kees Cook
ChromeOS Security

Re: [patch 3/4] c/r: procfs: add arg_start/end, env_start/end and exit_code members to /proc/$pid/stat

[Cyrill Gorcunov]

On Mon, Jan 23, 2012 at 12:42:28PM -0800, Kees Cook wrote:
> On Mon, Jan 23, 2012 at 6:20 AM, Cyrill Gorcunov <gorcunov@openvz.org> wrote:
> > We would like to have an ability to restore command line
> > arguments and envirion pointers so the task being restored
> > would print appropriate values in /proc/pid/cmdline and
> > /proc/pid/envirion. The exit_code is needed to restore
> > zombie tasks.
> >
> > Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
> > Cc: Pavel Emelyanov <xemul@parallels.com>
> > Cc: Serge Hallyn <serge.hallyn@canonical.com>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> > Cc: Alexey Dobriyan <adobriyan@gmail.com>
> > Cc: Tejun Heo <tj@kernel.org>
> > Cc: Andrew Vagin <avagin@openvz.org>
> > Cc: Vasiliy Kulikov <segoon@openwall.com>
> > Cc: Alexey Dobriyan <adobriyan@gmail.com>
> > Cc: "Eric W. Biederman" <ebiederm@xmission.com>
> > ---
> >  fs/proc/array.c |   10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > Index: linux-2.6.git/fs/proc/array.c
> > ===================================================================
> > --- linux-2.6.git.orig/fs/proc/array.c
> > +++ linux-2.6.git/fs/proc/array.c
> > @@ -464,7 +464,8 @@ static int do_task_stat(struct seq_file
> >
> >        seq_printf(m, "%d (%s) %c %d %d %d %d %d %u %lu \
> >  %lu %lu %lu %lu %lu %ld %ld %ld %ld %d 0 %llu %lu %ld %lu %lu %lu %lu %lu \
> > -%lu %lu %lu %lu %lu %lu %lu %lu %d %d %u %u %llu %lu %ld %lu %lu %lu\n",
> > +%lu %lu %lu %lu %lu %lu %lu %lu %d %d %u %u %llu %lu %ld %lu %lu %lu \
> > +%lu %lu %lu %lu %d\n",
> >                pid_nr_ns(pid, ns),
> >                tcomm,
> >                state,
> > @@ -514,7 +515,12 @@ static int do_task_stat(struct seq_file
> >                cputime_to_clock_t(cgtime),
> >                (mm && permitted) ? mm->start_data : 0,
> >                (mm && permitted) ? mm->end_data : 0,
> > -               (mm && permitted) ? mm->start_brk : 0);
> > +               (mm && permitted) ? mm->start_brk : 0,
> > +               (mm && permitted) ? mm->arg_start : 0,
> > +               (mm && permitted) ? mm->arg_end : 0,
> > +               (mm && permitted) ? mm->env_start : 0,
> > +               (mm && permitted) ? mm->env_end : 0,
> > +               task->exit_code);
> >        if (mm)
> >                mmput(mm);
> >        return 0;
> 
> You're not exposing auxv here? In your testing, what situations ended
> up using auxv after initial startup? Or is your intention to be able
> to freeze a process potentially before libc has examined auxv?
> 

auxv already available via /proc/pid/auxv so I thought it would be
redundant to put it into different place. Or you meant something
else?

In testing I simply read auxv values from /proc/pid/auxv and restore
them back via prctl at restore time. (prctl patch is in this series
but it's slightly buggy so I post correct version later).

And intention is simply dump this vector at checkpoint time and
restore when needed.

	Cyrill

Re: [patch 3/4] c/r: procfs: add arg_start/end, env_start/end and exit_code members to /proc/$pid/stat

[Andrew Morton]

On Mon, 23 Jan 2012 18:20:39 +0400
Cyrill Gorcunov <gorcunov@openvz.org> wrote:

> We would like to have an ability to restore command line
> arguments and envirion pointers so the task being restored
> would print appropriate values in /proc/pid/cmdline and
> /proc/pid/envirion. The exit_code is needed to restore
> zombie tasks.
> 

Please update Documentation/filesystems/proc.txt.

>
> --- linux-2.6.git.orig/fs/proc/array.c
> +++ linux-2.6.git/fs/proc/array.c
> @@ -464,7 +464,8 @@ static int do_task_stat(struct seq_file
>  
>  	seq_printf(m, "%d (%s) %c %d %d %d %d %d %u %lu \
>  %lu %lu %lu %lu %lu %ld %ld %ld %ld %d 0 %llu %lu %ld %lu %lu %lu %lu %lu \
> -%lu %lu %lu %lu %lu %lu %lu %lu %d %d %u %u %llu %lu %ld %lu %lu %lu\n",
> +%lu %lu %lu %lu %lu %lu %lu %lu %d %d %u %u %llu %lu %ld %lu %lu %lu \
> +%lu %lu %lu %lu %d\n",
>  		pid_nr_ns(pid, ns),
>  		tcomm,
>  		state,
> @@ -514,7 +515,12 @@ static int do_task_stat(struct seq_file
>  		cputime_to_clock_t(cgtime),
>  		(mm && permitted) ? mm->start_data : 0,
>  		(mm && permitted) ? mm->end_data : 0,
> -		(mm && permitted) ? mm->start_brk : 0);
> +		(mm && permitted) ? mm->start_brk : 0,
> +		(mm && permitted) ? mm->arg_start : 0,
> +		(mm && permitted) ? mm->arg_end : 0,
> +		(mm && permitted) ? mm->env_start : 0,
> +		(mm && permitted) ? mm->env_end : 0,
> +		task->exit_code);
>  	if (mm)
>  		mmput(mm);
>  	return 0;

/proc/pid/stat is getting out of control.  People are now sending patches
because reading from this thing already takes too long.

The problem is that if you only want one field, you have to incur the
cost of preparing all the fields.  The magnitude of this problem
increases exponentially over time!

I'm unsure what to do about it really.  Perhaps add a new
/proc/pid/mmstat for MM-specific things.  We could put the above six
fields in there, as long as we move quickly.

Re: [patch 3/4] c/r: procfs: add arg_start/end, env_start/end and exit_code members to /proc/$pid/stat

[Cyrill Gorcunov]

On Tue, Jan 24, 2012 at 03:59:41PM -0800, Andrew Morton wrote:
> On Mon, 23 Jan 2012 18:20:39 +0400
> Cyrill Gorcunov <gorcunov@openvz.org> wrote:
> 
> > We would like to have an ability to restore command line
> > arguments and envirion pointers so the task being restored
> > would print appropriate values in /proc/pid/cmdline and
> > /proc/pid/envirion. The exit_code is needed to restore
> > zombie tasks.
> > 
> 
> Please update Documentation/filesystems/proc.txt.

ok

> > +		(mm && permitted) ? mm->start_brk : 0,
> > +		(mm && permitted) ? mm->arg_start : 0,
> > +		(mm && permitted) ? mm->arg_end : 0,
> > +		(mm && permitted) ? mm->env_start : 0,
> > +		(mm && permitted) ? mm->env_end : 0,
> > +		task->exit_code);
> >  	if (mm)
> >  		mmput(mm);
> >  	return 0;
> 
> /proc/pid/stat is getting out of control.  People are now sending patches
> because reading from this thing already takes too long.
> 
> The problem is that if you only want one field, you have to incur the
> cost of preparing all the fields.  The magnitude of this problem
> increases exponentially over time!
> 
> I'm unsure what to do about it really.  Perhaps add a new
> /proc/pid/mmstat for MM-specific things.  We could put the above six
> fields in there, as long as we move quickly.
> 

I can add prctl PR_GET_MM with subcodes, since PR_SET_MM is already here
and wrapped with CHECKPOINT_RESTORE. Would this be better?

	Cyrill

Re: [patch 3/4] c/r: procfs: add arg_start/end, env_start/end and exit_code members to /proc/$pid/stat

[Andrew Morton]

On Wed, 25 Jan 2012 10:54:50 +0400 Cyrill Gorcunov <gorcunov@gmail.com> wrote:

> > > +		(mm && permitted) ? mm->start_brk : 0,
> > > +		(mm && permitted) ? mm->arg_start : 0,
> > > +		(mm && permitted) ? mm->arg_end : 0,
> > > +		(mm && permitted) ? mm->env_start : 0,
> > > +		(mm && permitted) ? mm->env_end : 0,
> > > +		task->exit_code);
> > >  	if (mm)
> > >  		mmput(mm);
> > >  	return 0;
> > 
> > /proc/pid/stat is getting out of control.  People are now sending patches
> > because reading from this thing already takes too long.

err, actually, that was /proc/stat/

> > The problem is that if you only want one field, you have to incur the
> > cost of preparing all the fields.  The magnitude of this problem
> > increases exponentially over time!
> > 
> > I'm unsure what to do about it really.  Perhaps add a new
> > /proc/pid/mmstat for MM-specific things.  We could put the above six
> > fields in there, as long as we move quickly.
> > 
> 
> I can add prctl PR_GET_MM with subcodes, since PR_SET_MM is already here
> and wrapped with CHECKPOINT_RESTORE. Would this be better?

mm, not really - /proc is the logical/expected place for it.

I'm thinking that perhaps we should start again with all of this and
export all this information in brand new, well-designed procfs files. 
We'd still maintain /proc/stat and /proc/pid/stat but people should
migrate off them.  Eventually (10 years?) everyone will be setting their
CONFIG_PROC_[PID_]STAT to 'n' and perhaps we can retire the things.

Meanwhile, I suppose you may as well continue to make /proc/pid/stat
even crazier :(  It isn't as bad as /proc/stat!


btw, do we really need to do "(mm && permitted)" so many times?  ie,
can we split that seq_printf up and do

	if (mm && permitted) {
		seq_printf(m + offset, "%lu", mm->start_data);
		...
	} else {
		seq_printf(m + offset, "0 0 0 0 0 0 0 0 0");
	}

?  Although this probably won't help much.

Re: [patch 3/4] c/r: procfs: add arg_start/end, env_start/end and exit_code members to /proc/$pid/stat

[Cyrill Gorcunov]

On Tue, Jan 24, 2012 at 11:12:45PM -0800, Andrew Morton wrote:
> On Wed, 25 Jan 2012 10:54:50 +0400 Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> 
> > > /proc/pid/stat is getting out of control.  People are now sending patches
> > > because reading from this thing already takes too long.
> 
> err, actually, that was /proc/stat/
> 

Ah! (I saw those thread, but then I get confused and thought maybe there were
for /proc/pid/stat as well but I simply missed it ;)

> > 
> > I can add prctl PR_GET_MM with subcodes, since PR_SET_MM is already here
> > and wrapped with CHECKPOINT_RESTORE. Would this be better?
> 
> mm, not really - /proc is the logical/expected place for it.
> 
> I'm thinking that perhaps we should start again with all of this and
> export all this information in brand new, well-designed procfs files. 
> We'd still maintain /proc/stat and /proc/pid/stat but people should
> migrate off them.  Eventually (10 years?) everyone will be setting their
> CONFIG_PROC_[PID_]STAT to 'n' and perhaps we can retire the things.
> 
> Meanwhile, I suppose you may as well continue to make /proc/pid/stat
> even crazier :(  It isn't as bad as /proc/stat!
> 

At moment indeed it's not that bloated... yet.

> btw, do we really need to do "(mm && permitted)" so many times?  ie,
> can we split that seq_printf up and do
> 
> 	if (mm && permitted) {
> 		seq_printf(m + offset, "%lu", mm->start_data);
> 		...
> 	} else {
> 		seq_printf(m + offset, "0 0 0 0 0 0 0 0 0");
> 	}
> 
> ?  Although this probably won't help much.
> 

Yeah we can. I'll update.

	Cyrill

[patch 4/4] c/r: prctl: Extend PR_SET_MM to set up more mm_struct entries

[Cyrill Gorcunov]

[-- Attachment #1: prctl-restore-mm-members-2 --]
[-- Type: text/plain, Size: 5062 bytes --]

After restore we would like the 'ps' command show the command
line and evironment exactly the same it was at checkpoint time.

So this additional PR_SET_MM_ allow us to do so. Note that
these members of mm_struct is rather used for output in
procfs, except auxv vector which is used by ld.so mostly.

Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
Cc: Michael Kerrisk <mtk.manpages@gmail.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Tejun Heo <tj@kernel.org>
Cc: Andrew Vagin <avagin@openvz.org>
Cc: Serge Hallyn <serge.hallyn@canonical.com>
Cc: Pavel Emelyanov <xemul@parallels.com>
Cc: Vasiliy Kulikov <segoon@openwall.com>
Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
Cc: Michael Kerrisk <mtk.manpages@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
 include/linux/prctl.h |    5 +++
 kernel/sys.c          |   73 +++++++++++++++++++++++++++++++-------------------
 2 files changed, 51 insertions(+), 27 deletions(-)

Index: linux-2.6.git/include/linux/prctl.h
===================================================================
--- linux-2.6.git.orig/include/linux/prctl.h
+++ linux-2.6.git/include/linux/prctl.h
@@ -113,5 +113,10 @@
 # define PR_SET_MM_START_STACK		5
 # define PR_SET_MM_START_BRK		6
 # define PR_SET_MM_BRK			7
+# define PR_SET_MM_ARG_START		8
+# define PR_SET_MM_ARG_END		9
+# define PR_SET_MM_ENV_START		10
+# define PR_SET_MM_ENV_END		11
+# define PR_SET_MM_AUXV			12
 
 #endif /* _LINUX_PRCTL_H */
Index: linux-2.6.git/kernel/sys.c
===================================================================
--- linux-2.6.git.orig/kernel/sys.c
+++ linux-2.6.git/kernel/sys.c
@@ -1693,17 +1693,25 @@ SYSCALL_DEFINE1(umask, int, mask)
 }
 
 #ifdef CONFIG_CHECKPOINT_RESTORE
+static bool vma_flags_mismatch(struct vm_area_struct *vma,
+			       unsigned long required,
+			       unsigned long banned)
+{
+	return (vma->vm_flags & required) != required ||
+		(vma->vm_flags & banned);
+}
+
 static int prctl_set_mm(int opt, unsigned long addr,
 			unsigned long arg4, unsigned long arg5)
 {
 	unsigned long rlim = rlimit(RLIMIT_DATA);
-	unsigned long vm_req_flags;
-	unsigned long vm_bad_flags;
 	struct vm_area_struct *vma;
 	int error = 0;
 	struct mm_struct *mm = current->mm;
 
-	if (arg4 | arg5)
+	if (arg4 && opt != PR_SET_MM_AUXV)
+		return -EINVAL;
+	else if (arg4 | arg5)
 		return -EINVAL;
 
 	if (!capable(CAP_SYS_ADMIN))
@@ -1715,7 +1723,9 @@ static int prctl_set_mm(int opt, unsigne
 	down_read(&mm->mmap_sem);
 	vma = find_vma(mm, addr);
 
-	if (opt != PR_SET_MM_START_BRK && opt != PR_SET_MM_BRK) {
+	if (opt != PR_SET_MM_START_BRK &&
+	    opt != PR_SET_MM_BRK &&
+	    opt != PR_SET_MM_AUXV) {
 		/* It must be existing VMA */
 		if (!vma || vma->vm_start > addr)
 			goto out;
@@ -1725,11 +1735,8 @@ static int prctl_set_mm(int opt, unsigne
 	switch (opt) {
 	case PR_SET_MM_START_CODE:
 	case PR_SET_MM_END_CODE:
-		vm_req_flags = VM_READ | VM_EXEC;
-		vm_bad_flags = VM_WRITE | VM_MAYSHARE;
-
-		if ((vma->vm_flags & vm_req_flags) != vm_req_flags ||
-		    (vma->vm_flags & vm_bad_flags))
+		if (vma_flags_mismatch(vma, VM_READ | VM_EXEC,
+				       VM_WRITE | VM_MAYSHARE))
 			goto out;
 
 		if (opt == PR_SET_MM_START_CODE)
@@ -1740,11 +1747,8 @@ static int prctl_set_mm(int opt, unsigne
 
 	case PR_SET_MM_START_DATA:
 	case PR_SET_MM_END_DATA:
-		vm_req_flags = VM_READ | VM_WRITE;
-		vm_bad_flags = VM_EXEC | VM_MAYSHARE;
-
-		if ((vma->vm_flags & vm_req_flags) != vm_req_flags ||
-		    (vma->vm_flags & vm_bad_flags))
+		if (vma_flags_mismatch(vma, VM_READ | VM_WRITE,
+				       VM_EXEC | VM_MAYSHARE))
 			goto out;
 
 		if (opt == PR_SET_MM_START_DATA)
@@ -1753,19 +1757,6 @@ static int prctl_set_mm(int opt, unsigne
 			mm->end_data = addr;
 		break;
 
-	case PR_SET_MM_START_STACK:
-
-#ifdef CONFIG_STACK_GROWSUP
-		vm_req_flags = VM_READ | VM_WRITE | VM_GROWSUP;
-#else
-		vm_req_flags = VM_READ | VM_WRITE | VM_GROWSDOWN;
-#endif
-		if ((vma->vm_flags & vm_req_flags) != vm_req_flags)
-			goto out;
-
-		mm->start_stack = addr;
-		break;
-
 	case PR_SET_MM_START_BRK:
 		if (addr <= mm->end_data)
 			goto out;
@@ -1790,6 +1781,34 @@ static int prctl_set_mm(int opt, unsigne
 		mm->brk = addr;
 		break;
 
+	case PR_SET_MM_START_STACK:
+	case PR_SET_MM_ARG_START:
+	case PR_SET_MM_ARG_END:
+	case PR_SET_MM_ENV_START:
+	case PR_SET_MM_ENV_END:
+#ifdef CONFIG_STACK_GROWSUP
+		if (vma_flags_mismatch(vma, VM_READ | VM_WRITE | VM_GROWSUP, 0))
+#else
+		if (vma_flags_mismatch(vma, VM_READ | VM_WRITE | VM_GROWSDOWN, 0))
+#endif
+			goto out;
+		if (opt == PR_SET_MM_ARG_START)
+			mm->start_stack = addr;
+		else if (opt == PR_SET_MM_ARG_START)
+			mm->arg_start = addr;
+		else if (opt == PR_SET_MM_ARG_END)
+			mm->arg_end = addr;
+		else if (opt == PR_SET_MM_ENV_START)
+			mm->env_start = addr;
+		else if (opt == PR_SET_MM_ENV_END)
+			mm->env_end = addr;
+		break;
+
+	case PR_SET_MM_AUXV:
+		if (arg4 > sizeof(mm->saved_auxv))
+			goto out;
+		error = copy_from_user(mm->saved_auxv, (const void __user *)addr, arg4);
+		break;
 	default:
 		error = -EINVAL;
 		goto out;

Re: [patch 4/4] c/r: prctl: Extend PR_SET_MM to set up more mm_struct entries

[Cyrill Gorcunov]

On Mon, Jan 23, 2012 at 06:20:40PM +0400, Cyrill Gorcunov wrote:
> After restore we would like the 'ps' command show the command
> line and evironment exactly the same it was at checkpoint time.
> 
> So this additional PR_SET_MM_ allow us to do so. Note that
> these members of mm_struct is rather used for output in
> procfs, except auxv vector which is used by ld.so mostly.
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
> Cc: Michael Kerrisk <mtk.manpages@gmail.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Andrew Vagin <avagin@openvz.org>
> Cc: Serge Hallyn <serge.hallyn@canonical.com>
> Cc: Pavel Emelyanov <xemul@parallels.com>
> Cc: Vasiliy Kulikov <segoon@openwall.com>
> Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> Cc: Michael Kerrisk <mtk.manpages@gmail.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> ---

This one is with typo fixed.

	Cyrill
---
From: Cyrill Gorcunov <gorcunov@openvz.org>
Subject: c/r: prctl: Extend PR_SET_MM to set up more mm_struct entries

After restore we would like the 'ps' command show the command
line and evironment exactly the same it was at checkpoint time.

So this additional PR_SET_MM_ allow us to do so. Note that
these members of mm_struct is rather used for output in
procfs, except auxv vector which is used by ld.so mostly.

Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
Cc: Michael Kerrisk <mtk.manpages@gmail.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Tejun Heo <tj@kernel.org>
Cc: Andrew Vagin <avagin@openvz.org>
Cc: Serge Hallyn <serge.hallyn@canonical.com>
Cc: Pavel Emelyanov <xemul@parallels.com>
Cc: Vasiliy Kulikov <segoon@openwall.com>
Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
Cc: Michael Kerrisk <mtk.manpages@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
 include/linux/prctl.h |    5 +++
 kernel/sys.c          |   73 +++++++++++++++++++++++++++++++-------------------
 2 files changed, 51 insertions(+), 27 deletions(-)

Index: linux-2.6.git/include/linux/prctl.h
===================================================================
--- linux-2.6.git.orig/include/linux/prctl.h
+++ linux-2.6.git/include/linux/prctl.h
@@ -113,5 +113,10 @@
 # define PR_SET_MM_START_STACK		5
 # define PR_SET_MM_START_BRK		6
 # define PR_SET_MM_BRK			7
+# define PR_SET_MM_ARG_START		8
+# define PR_SET_MM_ARG_END		9
+# define PR_SET_MM_ENV_START		10
+# define PR_SET_MM_ENV_END		11
+# define PR_SET_MM_AUXV			12
 
 #endif /* _LINUX_PRCTL_H */
Index: linux-2.6.git/kernel/sys.c
===================================================================
--- linux-2.6.git.orig/kernel/sys.c
+++ linux-2.6.git/kernel/sys.c
@@ -1693,17 +1693,25 @@ SYSCALL_DEFINE1(umask, int, mask)
 }
 
 #ifdef CONFIG_CHECKPOINT_RESTORE
+static bool vma_flags_mismatch(struct vm_area_struct *vma,
+			       unsigned long required,
+			       unsigned long banned)
+{
+	return (vma->vm_flags & required) != required ||
+		(vma->vm_flags & banned);
+}
+
 static int prctl_set_mm(int opt, unsigned long addr,
 			unsigned long arg4, unsigned long arg5)
 {
 	unsigned long rlim = rlimit(RLIMIT_DATA);
-	unsigned long vm_req_flags;
-	unsigned long vm_bad_flags;
 	struct vm_area_struct *vma;
 	int error = 0;
 	struct mm_struct *mm = current->mm;
 
-	if (arg4 | arg5)
+	if (arg4 && opt != PR_SET_MM_AUXV)
+		return -EINVAL;
+	else if (arg4 | arg5)
 		return -EINVAL;
 
 	if (!capable(CAP_SYS_ADMIN))
@@ -1715,7 +1723,9 @@ static int prctl_set_mm(int opt, unsigne
 	down_read(&mm->mmap_sem);
 	vma = find_vma(mm, addr);
 
-	if (opt != PR_SET_MM_START_BRK && opt != PR_SET_MM_BRK) {
+	if (opt != PR_SET_MM_START_BRK &&
+	    opt != PR_SET_MM_BRK &&
+	    opt != PR_SET_MM_AUXV) {
 		/* It must be existing VMA */
 		if (!vma || vma->vm_start > addr)
 			goto out;
@@ -1725,11 +1735,8 @@ static int prctl_set_mm(int opt, unsigne
 	switch (opt) {
 	case PR_SET_MM_START_CODE:
 	case PR_SET_MM_END_CODE:
-		vm_req_flags = VM_READ | VM_EXEC;
-		vm_bad_flags = VM_WRITE | VM_MAYSHARE;
-
-		if ((vma->vm_flags & vm_req_flags) != vm_req_flags ||
-		    (vma->vm_flags & vm_bad_flags))
+		if (vma_flags_mismatch(vma, VM_READ | VM_EXEC,
+				       VM_WRITE | VM_MAYSHARE))
 			goto out;
 
 		if (opt == PR_SET_MM_START_CODE)
@@ -1740,11 +1747,8 @@ static int prctl_set_mm(int opt, unsigne
 
 	case PR_SET_MM_START_DATA:
 	case PR_SET_MM_END_DATA:
-		vm_req_flags = VM_READ | VM_WRITE;
-		vm_bad_flags = VM_EXEC | VM_MAYSHARE;
-
-		if ((vma->vm_flags & vm_req_flags) != vm_req_flags ||
-		    (vma->vm_flags & vm_bad_flags))
+		if (vma_flags_mismatch(vma, VM_READ | VM_WRITE,
+				       VM_EXEC | VM_MAYSHARE))
 			goto out;
 
 		if (opt == PR_SET_MM_START_DATA)
@@ -1753,19 +1757,6 @@ static int prctl_set_mm(int opt, unsigne
 			mm->end_data = addr;
 		break;
 
-	case PR_SET_MM_START_STACK:
-
-#ifdef CONFIG_STACK_GROWSUP
-		vm_req_flags = VM_READ | VM_WRITE | VM_GROWSUP;
-#else
-		vm_req_flags = VM_READ | VM_WRITE | VM_GROWSDOWN;
-#endif
-		if ((vma->vm_flags & vm_req_flags) != vm_req_flags)
-			goto out;
-
-		mm->start_stack = addr;
-		break;
-
 	case PR_SET_MM_START_BRK:
 		if (addr <= mm->end_data)
 			goto out;
@@ -1790,6 +1781,34 @@ static int prctl_set_mm(int opt, unsigne
 		mm->brk = addr;
 		break;
 
+	case PR_SET_MM_START_STACK:
+	case PR_SET_MM_ARG_START:
+	case PR_SET_MM_ARG_END:
+	case PR_SET_MM_ENV_START:
+	case PR_SET_MM_ENV_END:
+#ifdef CONFIG_STACK_GROWSUP
+		if (vma_flags_mismatch(vma, VM_READ | VM_WRITE | VM_GROWSUP, 0))
+#else
+		if (vma_flags_mismatch(vma, VM_READ | VM_WRITE | VM_GROWSDOWN, 0))
+#endif
+			goto out;
+		if (opt == PR_SET_MM_START_STACK)
+			mm->start_stack = addr;
+		else if (opt == PR_SET_MM_ARG_START)
+			mm->arg_start = addr;
+		else if (opt == PR_SET_MM_ARG_END)
+			mm->arg_end = addr;
+		else if (opt == PR_SET_MM_ENV_START)
+			mm->env_start = addr;
+		else if (opt == PR_SET_MM_ENV_END)
+			mm->env_end = addr;
+		break;
+
+	case PR_SET_MM_AUXV:
+		if (arg4 > sizeof(mm->saved_auxv))
+			goto out;
+		error = copy_from_user(mm->saved_auxv, (const void __user *)addr, arg4);
+		break;
 	default:
 		error = -EINVAL;
 		goto out;

Re: [patch 4/4] c/r: prctl: Extend PR_SET_MM to set up more mm_struct entries

[Cyrill Gorcunov]

On Mon, Jan 23, 2012 at 07:55:43PM +0400, Cyrill Gorcunov wrote:
> On Mon, Jan 23, 2012 at 06:20:40PM +0400, Cyrill Gorcunov wrote:
> > After restore we would like the 'ps' command show the command
> > line and evironment exactly the same it was at checkpoint time.
> > 
> > So this additional PR_SET_MM_ allow us to do so. Note that
> > these members of mm_struct is rather used for output in
> > procfs, except auxv vector which is used by ld.so mostly.
> > 
> 
> This one is with typo fixed.
> ---

After some more testing I found there is a proble, so I'll send
updated version later. Please dont review ;)

	Cyrill

[patch 0/4] [patch 0/4] A pile in c/r sake v2

[Cyrill Gorcunov]

Hi, hope this time I've addressed more or less all concerns,
still please say me if something is not yet good enough.

 - sys_kcmp now depends on CONFIG_CHECKPOINT_RESTORE
   (Ingo, I suppose arch_initcall for kcmp_cookies_init
    should be better than late_initcall, but I'm not
    sure)

 - the extension of /proc/pid/stat now done against
   linux-next/master

Please review, thanks,

	Cyrill

[patch 3/4] c/r: procfs: add arg_start/end, env_start/end and exit_code members to /proc/$pid/stat

[Cyrill Gorcunov]

[-- Attachment #1: fs-proc-extend-state-2-linux-next --]
[-- Type: text/plain, Size: 3079 bytes --]

We would like to have an ability to restore command line
arguments and program environment pointers but first we
need to obtain them somehow. Thus we put these values into
/proc/$pid/stat. The exit_code is needed to restore zombie
tasks.

Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Pavel Emelyanov <xemul@parallels.com>
Cc: Serge Hallyn <serge.hallyn@canonical.com>
Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
Cc: Alexey Dobriyan <adobriyan@gmail.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: Andrew Vagin <avagin@openvz.org>
Cc: Vasiliy Kulikov <segoon@openwall.com>
Cc: Alexey Dobriyan <adobriyan@gmail.com>
Cc: "Eric W. Biederman" <ebiederm@xmission.com>
---
 Documentation/filesystems/proc.txt |    5 +++++
 fs/proc/array.c                    |   20 +++++++++++++++++---
 2 files changed, 22 insertions(+), 3 deletions(-)

Index: linux-2.6.git/Documentation/filesystems/proc.txt
===================================================================
--- linux-2.6.git.orig/Documentation/filesystems/proc.txt
+++ linux-2.6.git/Documentation/filesystems/proc.txt
@@ -311,6 +311,11 @@ Table 1-4: Contents of the stat files (a
   start_data    address above which program data+bss is placed
   end_data      address below which program data+bss is placed
   start_brk     address above which program heap can be expanded with brk()
+  arg_start     address above which program command line is placed
+  arg_end       address below which program command line is placed
+  env_start     address above which program environment is placed
+  env_end       address below which program environment is placed
+  exit_code     the thread's exit_code in the form reported by the waitpid system call
 ..............................................................................
 
 The /proc/PID/maps file containing the currently mapped memory regions and
Index: linux-2.6.git/fs/proc/array.c
===================================================================
--- linux-2.6.git.orig/fs/proc/array.c
+++ linux-2.6.git/fs/proc/array.c
@@ -508,9 +508,23 @@ static int do_task_stat(struct seq_file
 	seq_put_decimal_ull(m, ' ', delayacct_blkio_ticks(task));
 	seq_put_decimal_ull(m, ' ', cputime_to_clock_t(gtime));
 	seq_put_decimal_ll(m, ' ', cputime_to_clock_t(cgtime));
-	seq_put_decimal_ull(m, ' ', (mm && permitted) ? mm->start_data : 0);
-	seq_put_decimal_ull(m, ' ', (mm && permitted) ? mm->end_data : 0);
-	seq_put_decimal_ull(m, ' ', (mm && permitted) ? mm->start_brk : 0);
+
+	if (mm && permitted) {
+		seq_put_decimal_ull(m, ' ', mm->start_data);
+		seq_put_decimal_ull(m, ' ', mm->end_data);
+		seq_put_decimal_ull(m, ' ', mm->start_brk);
+		seq_put_decimal_ull(m, ' ', mm->arg_start);
+		seq_put_decimal_ull(m, ' ', mm->arg_end);
+		seq_put_decimal_ull(m, ' ', mm->env_start);
+		seq_put_decimal_ull(m, ' ', mm->env_end);
+	} else
+		seq_printf(m, " 0 0 0 0 0 0 0");
+
+	if (permitted)
+		seq_put_decimal_ll(m, ' ', task->exit_code);
+	else
+		seq_put_decimal_ll(m, ' ', 0);
+
 	seq_putc(m, '\n');
 	if (mm)
 		mmput(mm);

Re: [patch 3/4] c/r: procfs: add arg_start/end, env_start/end and exit_code members to /proc/$pid/stat

[Kees Cook]

On Fri, Feb 3, 2012 at 7:19 AM, Cyrill Gorcunov <gorcunov@openvz.org> wrote:
> We would like to have an ability to restore command line
> arguments and program environment pointers but first we
> need to obtain them somehow. Thus we put these values into
> /proc/$pid/stat. The exit_code is needed to restore zombie
> tasks.
>
> Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>

Acked-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook
ChromeOS Security

[patch 0/4] Resending, c/r series v2

[Cyrill Gorcunov]

Hi, this series hopefully in a good shape

 - sys_kcmp now depends on CONFIG_CHECKPOINT_RESTORE

 - the extension of /proc/pid/stat now done against
   linux-next/master

Please letme know if I've missed something.

Thanks,
	Cyrill

[patch 3/4] c/r: procfs: add arg_start/end, env_start/end and exit_code members to /proc/$pid/stat

[Cyrill Gorcunov]

[-- Attachment #1: fs-proc-extend-state-2-linux-next --]
[-- Type: text/plain, Size: 3085 bytes --]

We would like to have an ability to restore command line
arguments and program environment pointers but first we
need to obtain them somehow. Thus we put these values into
/proc/$pid/stat. The exit_code is needed to restore zombie
tasks.

Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
Acked-by: Kees Cook <keescook@chromium.org>
Cc: Pavel Emelyanov <xemul@parallels.com>
Cc: Serge Hallyn <serge.hallyn@canonical.com>
Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
Cc: Alexey Dobriyan <adobriyan@gmail.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: Andrew Vagin <avagin@openvz.org>
Cc: Vasiliy Kulikov <segoon@openwall.com>
Cc: Alexey Dobriyan <adobriyan@gmail.com>
Cc: "Eric W. Biederman" <ebiederm@xmission.com>
---
 Documentation/filesystems/proc.txt |    5 +++++
 fs/proc/array.c                    |   20 +++++++++++++++++---
 2 files changed, 22 insertions(+), 3 deletions(-)

Index: linux-2.6.git/Documentation/filesystems/proc.txt
===================================================================
--- linux-2.6.git.orig/Documentation/filesystems/proc.txt
+++ linux-2.6.git/Documentation/filesystems/proc.txt
@@ -311,6 +311,11 @@ Table 1-4: Contents of the stat files (a
   start_data    address above which program data+bss is placed
   end_data      address below which program data+bss is placed
   start_brk     address above which program heap can be expanded with brk()
+  arg_start     address above which program command line is placed
+  arg_end       address below which program command line is placed
+  env_start     address above which program environment is placed
+  env_end       address below which program environment is placed
+  exit_code     the thread's exit_code in the form reported by the waitpid system call
 ..............................................................................
 
 The /proc/PID/maps file containing the currently mapped memory regions and
Index: linux-2.6.git/fs/proc/array.c
===================================================================
--- linux-2.6.git.orig/fs/proc/array.c
+++ linux-2.6.git/fs/proc/array.c
@@ -508,9 +508,23 @@ static int do_task_stat(struct seq_file
 	seq_put_decimal_ull(m, ' ', delayacct_blkio_ticks(task));
 	seq_put_decimal_ull(m, ' ', cputime_to_clock_t(gtime));
 	seq_put_decimal_ll(m, ' ', cputime_to_clock_t(cgtime));
-	seq_put_decimal_ull(m, ' ', (mm && permitted) ? mm->start_data : 0);
-	seq_put_decimal_ull(m, ' ', (mm && permitted) ? mm->end_data : 0);
-	seq_put_decimal_ull(m, ' ', (mm && permitted) ? mm->start_brk : 0);
+
+	if (mm && permitted) {
+		seq_put_decimal_ull(m, ' ', mm->start_data);
+		seq_put_decimal_ull(m, ' ', mm->end_data);
+		seq_put_decimal_ull(m, ' ', mm->start_brk);
+		seq_put_decimal_ull(m, ' ', mm->arg_start);
+		seq_put_decimal_ull(m, ' ', mm->arg_end);
+		seq_put_decimal_ull(m, ' ', mm->env_start);
+		seq_put_decimal_ull(m, ' ', mm->env_end);
+	} else
+		seq_printf(m, " 0 0 0 0 0 0 0");
+
+	if (permitted)
+		seq_put_decimal_ll(m, ' ', task->exit_code);
+	else
+		seq_put_decimal_ll(m, ' ', 0);
+
 	seq_putc(m, '\n');
 	if (mm)
 		mmput(mm);