A few patches in a sake of c/r functionality

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 <[email protected]>

Reviewed-by Oleg Nesterov <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Pavel Emelyanov <[email protected]>

Cc Serge Hallyn <[email protected]>

Cc KAMEZAWA Hiroyuki <[email protected]>

Quoted 11 lines · Cyrill Gorcunov

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 <[email protected]>

Reviewed-by Oleg Nesterov <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Pavel Emelyanov <[email protected]>

Cc Serge Hallyn <[email protected]>

Cc KAMEZAWA Hiroyuki <[email protected]>

Reviewed-by Kees Cook <[email protected]>

fs/proc/array.c int proc_pid_statm(struct seq_file *m, s · line 614

Quoted 8 lines

fs/proc/array.c +8 -0

+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. :)

Quoted 15 lines · Kees Cook

Quoted 11 lines · Cyrill Gorcunov

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 <[email protected]>

Reviewed-by Oleg Nesterov <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Pavel Emelyanov <[email protected]>

Cc Serge Hallyn <[email protected]>

Cc KAMEZAWA Hiroyuki <[email protected]>

Reviewed-by Kees Cook <[email protected]>

Quoted 13 lines

fs/proc/array.c int proc_pid_statm(struct seq_file *m, s · line 614

Quoted 8 lines

fs/proc/array.c +8 -0

+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? ;)

Quoted 4 lines

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?

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 <[email protected]>

Reviewed-by Oleg Nesterov <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Pavel Emelyanov <[email protected]>

Cc Serge Hallyn <[email protected]>

Cc KAMEZAWA Hiroyuki <[email protected]>

From viewpoint I played with seq_file, yesterday.

fs/proc/array.c +3 -0

+static void *children_seq_start(struct seq_file *seq, loff_t *pos)
+{
+	return get_children_pid(seq->private, NULL, *pos);

From viewpoint I played with seq_file, yesterday.

fs/proc/array.c int proc_pid_statm(struct seq_file *m, s · line 628

Quoted 13 lines

fs/proc/array.c +13 -0

+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".)

On Tue, Jan 24, 2012 at 11:07:30AM +0900, KAMEZAWA Hiroyuki wrote:
...

Quoted 19 lines

From viewpoint I played with seq_file, yesterday.

fs/proc/array.c int proc_pid_statm(struct seq_file *m, s · line 628

Quoted 13 lines

fs/proc/array.c +13 -0

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

Quoted 3 lines

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?

Quoted 4 lines

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.

Quoted 10 lines · KAMEZAWA Hiroyuki

Quoted 4 lines

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.

Quoted 2 lines

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

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

Quoted 11 lines · Cyrill Gorcunov

Quoted 10 lines · KAMEZAWA Hiroyuki

Quoted 4 lines

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.

Quoted 4 lines

Quoted 2 lines

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?

Quoted 18 lines · Eric W. Biederman

Quoted 15 lines · Cyrill Gorcunov

Quoted 5 lines · KAMEZAWA Hiroyuki

Quoted 4 lines

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

Quoted 7 lines

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.

Quoted 3 lines

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.

Quoted 23 lines · Cyrill Gorcunov

Quoted 21 lines · Eric W. Biederman

Quoted 11 lines · Cyrill Gorcunov

Quoted 10 lines · KAMEZAWA Hiroyuki

Quoted 4 lines

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.

Quoted 4 lines

Quoted 2 lines

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?

Quoted 3 lines

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?

Quoted 6 lines · KOSAKI Motohiro

Quoted 4 lines

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.

Quoted 8 lines

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.

Quoted 3 lines

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.

Quoted 21 lines · On Tue, 24 Jan 2012 10:53:38 +0400 Cyrill Gorcunov <[email protected]> wrote:

On Tue, Jan 24, 2012 at 11:07:30AM +0900, KAMEZAWA Hiroyuki wrote:
...

Quoted 19 lines

From viewpoint I played with seq_file, yesterday.

Review

Quoted 13 lines

(quoted diff) +13 -0

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

Quoted 7 lines

Quoted 3 lines

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.

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.

Quoted 10 lines · On Mon, 23 Jan 2012 18:20:37 +0400 Cyrill Gorcunov <[email protected]> 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.

Quoted 1 line

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.

arch/x86/kernel/kcmp.c line 21

Quoted 8 lines · Cyrill Gorcunov

arch/x86/kernel/kcmp.c new file +8 -0

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

arch/x86/kernel/kcmp.c new file +7 -0

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

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 <[email protected]>

CC "Eric W. Biederman" <[email protected]>

CC Pavel Emelyanov <[email protected]>

CC Andrey Vagin <[email protected]>

CC KOSAKI Motohiro <[email protected]>

CC Ingo Molnar <[email protected]>

CC H. Peter Anvin <[email protected]>

CC Thomas Gleixner <[email protected]>

CC Glauber Costa <[email protected]>

CC Andi Kleen <[email protected]>

CC Tejun Heo <[email protected]>

CC Matt Helsley <[email protected]>

CC Pekka Enberg <[email protected]>

CC Eric Dumazet <[email protected]>

CC Vasiliy Kulikov <[email protected]>

CC Andrew Morton <[email protected]>

CC Alexey Dobriyan <[email protected]>

CC [email protected]

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

arch/x86/include/asm/kcmp.h new file +7 -0

@@ -0,0 +1,17 @@
+#ifndef _LINUX_KCMP_H
+#define _LINUX_KCMP_H
+
+/* Comparision type */
+enum {
+	KCMP_FILE,
+	KCMP_VM,

Index: linux-2.6.git/arch/x86/include/asm/syscalls.h
===================================================================

arch/x86/include/asm/syscalls.h +4 -0

@@ -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
===================================================================

arch/x86/kernel/Makefile +1 -0

@@ -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
===================================================================

arch/x86/kernel/kcmp.c new file +11 -0

@@ -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>
+

arch/x86/kernel/kcmp.c new file +116 -0

+/*
+ * 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:

Quoted 17 lines

Index: linux-2.6.git/arch/x86/include/asm/kcmp.h
===================================================================

arch/x86/include/asm/kcmp.h new file +11 -0

@@ -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,

arch/x86/include/asm/kcmp.h line 17

Quoted 6 lines

arch/x86/include/asm/kcmp.h new file +6 -0

+	KCMP_SYSVSEM,
+
+	KCMP_TYPES,
+};
+
+#endif /* _LINUX_KCMP_H */

Why under /arch ?

arch/x86/include/asm/kcmp.h

Quoted 6 lines

arch/x86/include/asm/kcmp.h new file +6 -0

+
+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 ?

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

Quoted 3 lines

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

Quoted 4 lines · Cyrill Gorcunov

Quoted 3 lines

Why under /arch ?

Quoted 8 lines

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.

Quoted 14 lines · H. Peter Anvin

Quoted 12 lines · Cyrill Gorcunov

Quoted 3 lines

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

Quoted 3 lines

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/ ?

Quoted 5 lines

Quoted 4 lines

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.

Quoted 2 lines

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 ;)

On Tue, Jan 24, 2012 at 04:20:31PM +0900, KAMEZAWA Hiroyuki wrote:
...

Quoted 10 lines

Quoted 7 lines

Quoted 4 lines

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 ?

Quoted 3 lines

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.

Quoted 16 lines · On Tue, 24 Jan 2012 11:38:42 +0400 Cyrill Gorcunov <[email protected]> wrote:

On Tue, Jan 24, 2012 at 04:20:31PM +0900, KAMEZAWA Hiroyuki wrote:
...

Quoted 13 lines

Quoted 7 lines

Quoted 4 lines

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 ;)

Quoted 5 lines

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.

Quoted 1 line

please do as you like.

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

Quoted 3 lines

Quoted 1 line

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.

Quoted 4 lines · KOSAKI Motohiro

Quoted 3 lines

Quoted 1 line

please do as you like.

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

Quoted 3 lines

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.

Quoted 11 lines · Cyrill Gorcunov

Quoted 7 lines · KOSAKI Motohiro

Quoted 3 lines

Quoted 1 line

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!

Quoted 2 lines

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.

Quoted 16 lines · Eric W. Biederman

Quoted 13 lines · Cyrill Gorcunov

Quoted 7 lines · KOSAKI Motohiro

Quoted 3 lines

Quoted 1 line

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.

Quoted 4 lines

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.

---
From: Cyrill Gorcunov <[email protected]>
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 <[email protected]>

CC "Eric W. Biederman" <[email protected]>

CC Pavel Emelyanov <[email protected]>

CC Andrey Vagin <[email protected]>

CC KOSAKI Motohiro <[email protected]>

CC Ingo Molnar <[email protected]>

CC H. Peter Anvin <[email protected]>

CC Thomas Gleixner <[email protected]>

CC Glauber Costa <[email protected]>

CC Andi Kleen <[email protected]>

CC Tejun Heo <[email protected]>

CC Matt Helsley <[email protected]>

CC Pekka Enberg <[email protected]>

CC Eric Dumazet <[email protected]>

CC Vasiliy Kulikov <[email protected]>

CC Andrew Morton <[email protected]>

CC Alexey Dobriyan <[email protected]>

CC [email protected]

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

arch/x86/include/asm/syscalls.h +4 -0

@@ -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
===================================================================

arch/x86/syscalls/syscall_32.tbl +1 -0

@@ -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
===================================================================

arch/x86/syscalls/syscall_64.tbl +1 -0

@@ -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
===================================================================

include/linux/kcmp.h new file +17 -0

@@ -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
===================================================================

kernel/Makefile +1 -0

@@ -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
===================================================================

kernel/kcmp.c new file +163 -0

@@ -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);

Quoted 2 lines

This one should fit all requirements I guess.

Bahahaha!

Looking I see one more nit.

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

This one should fit all requirements I guess.

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.

...

kernel/kcmp.c new file +5 -0

@@ -0,0 +1,163 @@
+#include <linux/kernel.h>
+#include <linux/syscalls.h>
+#include <linux/fdtable.h>
+#include <linux/string.h>
+#include <linux/random.h>

kernel/kcmp.c new file +7 -0

+ */
+static int kcmp_ptr(long v1, long v2, int type)
+{
+	long ret;
+
+	ret = kptr_obfuscate(v1, type) - kptr_obfuscate(v2, type);
+

kernel/kcmp.c new file +36 -0

+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();

kernel/kcmp.c new file +32 -0

+		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:

kernel/kcmp.c new file +15 -0

+		goto err;
+#endif
+		break;
+	default:
+		ret = -EINVAL;
+		goto err;
+	}
+
+err:
+	put_task_struct(task1);
+	put_task_struct(task2);
+
+	return ret;
+}
+

kernel/kcmp.c new file +5 -0

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

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;

Quoted 5 lines · On Tue, 24 Jan 2012 13:22:22 -0800 Andrew Morton <[email protected]>…

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;

Quoted 12 lines · Andrew Morton

Quoted 5 lines · On Tue, 24 Jan 2012 13:22:22 -0800 Andrew Morton <[email protected]>…

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:

Quoted 7 lines

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?

Quoted 2 lines

Arithmetic on void pointers?

All the world is gcc!

Quoted 4 lines · Andrew Morton

Quoted 2 lines

Arithmetic on void pointers?

All the world is gcc!

Even so, warnings are annoying...

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 ;)

Quoted 1 line

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.

Quoted 1 line

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

Quoted 4 lines

kernel/kcmp.c new file +4 -0

+static long kptr_obfuscate(long v, int type)
+{
+	return (v ^ cookies[type][0]) * cookies[type][1];
+}

kernel/kcmp.c line 26

Quoted 6 lines

kernel/kcmp.c new file +6 -0

+
+/*
+ * 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?

kernel/kcmp.c line 43

Quoted 3 lines

kernel/kcmp.c new file +3 -0

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

Quoted 1 line

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

"The caller must have pinned the task"

kernel/kcmp.c line 91

Quoted 2 lines

kernel/kcmp.c new file +2 -0

+	if (!ptrace_may_access(task1, PTRACE_MODE_READ) ||
+	    !ptrace_may_access(task2, PTRACE_MODE_READ)) {

Add a comment explaining this decision.

ENOENT seems inappropriate here.

Quoted 5 lines

kernel/kcmp.c new file +5 -0

+static __init int kcmp_cookie_init(void)
+{
+	int i, j;
+
+	for (i = 0; i < KCMP_TYPES; i++) {

kernel/kcmp.c line 158

Quoted 5 lines

kernel/kcmp.c new file +5 -0

+		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?

Quoted 3 lines

ENOENT seems inappropriate here.

Which one should be better?

<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 ;)

Quoted 6 lines

kernel/kcmp.c new file +6 -0

+static int kcmp_ptr(long v1, long v2, int type)
+{
+	long ret;
+
+	ret = kptr_obfuscate(v1, type) - kptr_obfuscate(v2, type);
+

kernel/kcmp.c line 43

Quoted 10 lines

kernel/kcmp.c new file +10 -0

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

Simply open coding the comparison would be better.

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

While doing the checkpoint-restore in the userspace

Quoted 1 line · On Wed, 25 Jan 2012 00:50:39 +0400 Cyrill Gorcunov <[email protected]> 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.

Quoted 19 lines · On Tue, 24 Jan 2012 11:17:16 +0400 Cyrill Gorcunov <[email protected]> wrote:

Quoted 17 lines · H. Peter Anvin

Quoted 4 lines · Cyrill Gorcunov

Quoted 3 lines

Why under /arch ?

Quoted 8 lines

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.

Quoted 2 lines

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.

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 <[email protected]>

Cc Pavel Emelyanov <[email protected]>

Cc Serge Hallyn <[email protected]>

Cc Kees Cook <[email protected]>

Cc KAMEZAWA Hiroyuki <[email protected]>

Cc Alexey Dobriyan <[email protected]>

Cc Tejun Heo <[email protected]>

Cc Andrew Vagin <[email protected]>

Cc Vasiliy Kulikov <[email protected]>

Cc Alexey Dobriyan <[email protected]>

Cc "Eric W. Biederman" <[email protected]>

---
 fs/proc/array.c |   10 ++++++++--

 1 file changed, 8 insertions(+), 2 deletions(-)

Index: linux-2.6.git/fs/proc/array.c
===================================================================

fs/proc/array.c +2 -1

@@ -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,

Quoted 45 lines · Cyrill Gorcunov

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 <[email protected]>

Cc Pavel Emelyanov <[email protected]>

Cc Serge Hallyn <[email protected]>

Cc Kees Cook <[email protected]>

Cc KAMEZAWA Hiroyuki <[email protected]>

Cc Alexey Dobriyan <[email protected]>

Cc Tejun Heo <[email protected]>

Cc Andrew Vagin <[email protected]>

Cc Vasiliy Kulikov <[email protected]>

Cc Alexey Dobriyan <[email protected]>

Cc "Eric W. Biederman" <[email protected]>

---
 fs/proc/array.c |   10 ++++++++--

 1 file changed, 8 insertions(+), 2 deletions(-)

Index: linux-2.6.git/fs/proc/array.c
===================================================================

fs/proc/array.c +7 -2

@@ -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,

fs/proc/array.c static int do_task_stat(struct seq_file · line 526

Quoted 4 lines

fs/proc/array.c +1 -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?

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.

fs/proc/array.c +2 -1

@@ -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,

Quoted 6 lines · On Mon, 23 Jan 2012 18:20:39 +0400 Cyrill Gorcunov <[email protected]> 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.

fs/proc/array.c static int do_task_stat(struct seq_file · line 526

Quoted 9 lines

fs/proc/array.c +6 -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.

Quoted 4 lines

Quoted 4 lines

fs/proc/array.c +4 -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,

Quoted 8 lines

fs/proc/array.c static int do_task_stat(struct seq_file · line 526

Quoted 5 lines

fs/proc/array.c +2 -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.

Quoted 8 lines

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?

Quoted 2 lines · On Wed, 25 Jan 2012 10:54:50 +0400 Cyrill Gorcunov <[email protected]> wrote:

Quoted 2 lines

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

Quoted 3 lines

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.

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 <[email protected]>

Cc Michael Kerrisk <[email protected]>

Cc Kees Cook <[email protected]>

Cc Tejun Heo <[email protected]>

Cc Andrew Vagin <[email protected]>

Cc Serge Hallyn <[email protected]>

Cc Pavel Emelyanov <[email protected]>

Cc Vasiliy Kulikov <[email protected]>

Cc KAMEZAWA Hiroyuki <[email protected]>

Cc Michael Kerrisk <[email protected]>

Cc Andrew Morton <[email protected]>

---

Quoted 7 lines · Cyrill Gorcunov

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