[patch cr 0/4] [patch cr 0/@total@]

[patch cr 0/4] [patch cr 0/@total@]

[Cyrill Gorcunov]

Hi,

this is a series where I tried to resolve all concerns and nits
I've got from previous attempt:

 - fs, proc: Introduce /proc/<pid>/task/<tid>/children entry v9
	Now everything is under CONFIG_CHECKPOINT_RESTORE

 - syscalls, x86: Add __NR_kcmp syscall v7
	* EBADF returned if there is no file
	* EOPNOTSUP returned if there is no sysv support compiled in
	* test uses syscall() from libc instead of own syscall implementation
	* man page for this syscall is under progress still

 - c/r: procfs: add arg_start/end, env_start/end and exit_code members to /proc/$pid/stat
	task->exit_code is shown IIF a caller has enough permissions

 - c/r: prctl: Extend PR_SET_MM to set up more mm_struct entries
	when restoring mm->saved_auxv the task_lock is used to serialize access

Please review, if there is something I'm still missing, ping me back.

Thanks,
	Cyrill

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

[Cyrill Gorcunov]

[-- Attachment #1: fs-proc-tid-children-13 --]
[-- Type: text/plain, Size: 7055 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.

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                    |  123 +++++++++++++++++++++++++++++++++++++
 fs/proc/base.c                     |    3 
 fs/proc/internal.h                 |    1 
 4 files changed, 145 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;
 }
+
+#ifdef CONFIG_CHECKPOINT_RESTORE
+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,
+};
+#endif
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,9 @@ 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),
+#ifdef CONFIG_CHECKPOINT_RESTORE
+	REG("children",  S_IRUGO, proc_tid_children_operations),
+#endif
 #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;

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

[Cyrill Gorcunov]

[-- Attachment #1: sys-kcmp-13 --]
[-- Type: text/plain, Size: 11679 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.

Lookups for pids are done in the caller's PID namespace only.

At moment only x86 is supported and tested.

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/syscalls/syscall_32.tbl         |    1 
 arch/x86/syscalls/syscall_64.tbl         |    1 
 include/linux/kcmp.h                     |   17 +++
 include/linux/syscalls.h                 |    2 
 kernel/Makefile                          |    1 
 kernel/kcmp.c                            |  170 +++++++++++++++++++++++++++++++
 tools/testing/selftests/kcmp/Makefile    |   35 ++++++
 tools/testing/selftests/kcmp/kcmp_test.c |   89 ++++++++++++++++
 tools/testing/selftests/run_tests        |    2 
 9 files changed, 317 insertions(+), 1 deletion(-)

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_type {
+	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/include/linux/syscalls.h
===================================================================
--- linux-2.6.git.orig/include/linux/syscalls.h
+++ linux-2.6.git/include/linux/syscalls.h
@@ -857,4 +857,6 @@ asmlinkage long sys_process_vm_writev(pi
 				      unsigned long riovcnt,
 				      unsigned long flags);
 
+asmlinkage long sys_kcmp(pid_t pid1, pid_t pid2, int type,
+			 unsigned long idx1, unsigned long idx2);
 #endif
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,170 @@
+#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>
+
+/*
+ * We don't expose real in-memory order of objects for security
+ * reasons, still the comparision results should be suitable for
+ * sorting. Thus, we obfuscate kernel pointers values (using random
+ * cookies obtaned at early boot stage) and compare the production
+ * instead.
+ */
+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 (reserved for future)
+ */
+static int kcmp_ptr(void *v1, void *v2, enum kcmp_type type)
+{
+	long ret;
+
+	ret = kptr_obfuscate((long)v1, type) - kptr_obfuscate((long)v2, type);
+
+	return (ret < 0) | ((ret > 0) << 1);
+}
+
+/* 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, *task2;
+	int ret;
+
+	rcu_read_lock();
+
+	/*
+	 * Tasks are looked up in caller's
+	 * PID namespace only.
+	 */
+
+	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();
+
+	/*
+	 * One should have enough rights to inspect tasks details.
+	 */
+	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 = -EBADF;
+		break;
+	}
+	case KCMP_VM:
+		ret = kcmp_ptr(task1->mm, task2->mm, KCMP_VM);
+		break;
+	case KCMP_FILES:
+		ret = kcmp_ptr(task1->files, task2->files, KCMP_FILES);
+		break;
+	case KCMP_FS:
+		ret = kcmp_ptr(task1->fs, task2->fs, KCMP_FS);
+		break;
+	case KCMP_SIGHAND:
+		ret = kcmp_ptr(task1->sighand, task2->sighand, KCMP_SIGHAND);
+		break;
+	case KCMP_IO:
+		ret = kcmp_ptr(task1->io_context, task2->io_context, KCMP_IO);
+		break;
+	case KCMP_SYSVSEM:
+#ifdef CONFIG_SYSVIPC
+		ret = kcmp_ptr(task1->sysvsem.undo_list,
+			       task2->sysvsem.undo_list,
+			       KCMP_SYSVSEM);
+#else
+		ret = -EOPNOTSUP;
+#endif
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+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);
Index: linux-2.6.git/tools/testing/selftests/kcmp/Makefile
===================================================================
--- /dev/null
+++ linux-2.6.git/tools/testing/selftests/kcmp/Makefile
@@ -0,0 +1,35 @@
+ifeq ($(strip $(V)),)
+	E = @echo
+	Q = @
+else
+	E = @\#
+	Q =
+endif
+export E Q
+
+uname_M := $(shell uname -m 2>/dev/null || echo not)
+ARCH ?= $(shell echo $(uname_M) | sed -e s/i.86/i386/)
+ifeq ($(ARCH),i386)
+        ARCH := X86
+	CFLAGS := -DCONFIG_X86_32
+endif
+ifeq ($(ARCH),x86_64)
+	ARCH := X86
+	CFLAGS := -DCONFIG_X86_64
+endif
+
+CFLAGS += -I../../../../arch/x86/include/generated/
+CFLAGS += -I../../../../include/
+
+all:
+ifeq ($(ARCH),X86)
+	$(E) "  CC run_test"
+	$(Q) gcc $(CFLAGS) kcmp_test.c -o run_test
+else
+	$(E) "Not an x86 target, can't build breakpoints selftests"
+endif
+
+clean:
+	$(E) "  CLEAN"
+	$(Q) rm -fr ./run_test
+	$(Q) rm -fr ./test-file
Index: linux-2.6.git/tools/testing/selftests/kcmp/kcmp_test.c
===================================================================
--- /dev/null
+++ linux-2.6.git/tools/testing/selftests/kcmp/kcmp_test.c
@@ -0,0 +1,89 @@
+#define _GNU_SOURCE
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <signal.h>
+#include <limits.h>
+#include <unistd.h>
+#include <errno.h>
+#include <string.h>
+#include <fcntl.h>
+
+#ifdef CONFIG_X86_64
+#include <asm/unistd_64.h>
+#else
+#include <asm/unistd_32.h>
+#endif
+
+#include <linux/kcmp.h>
+
+#include <sys/syscall.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <sys/wait.h>
+
+static long sys_kcmp(int pid1, int pid2, int type, int fd1, int fd2)
+{
+	return syscall(__NR_kcmp, (long)pid1, (long)pid2,
+		       (long)type, (long)fd1, (long)fd2);
+}
+
+int main(int argc, char **argv)
+{
+	int pid1, pid2;
+	int fd1, fd2;
+	int status;
+
+	fd1 = open("test-file", O_RDWR | O_CREAT | O_TRUNC, 0644);
+	pid1 = getpid();
+
+	if (fd1 < 0) {
+		perror("Can't create file");
+		exit(1);
+	}
+
+	pid2 = fork();
+	if (pid2 < 0) {
+		perror("fork failed");
+		exit(1);
+	}
+
+	if (!pid2) {
+		int pid2 = getpid();
+		int ret;
+
+		fd2 = open("test-file", O_RDWR, 0644);
+		if (fd2 < 0) {
+			perror("Can't open file");
+			exit(1);
+		}
+
+		/* An example of output and arguments */
+                printf("pid1: %6d pid2: %6d FD: %2d FILES: %2d VM: %2d FS: %2d "
+		       "SIGHAND: %2d IO: %2d SYSVSEM: %2d INV: %2d\n",
+		       pid1, pid2,
+		       sys_kcmp(pid1, pid2, KCMP_FILE,		fd1, fd2),
+		       sys_kcmp(pid1, pid2, KCMP_FILES,		0, 0),
+		       sys_kcmp(pid1, pid2, KCMP_VM,		0, 0),
+		       sys_kcmp(pid1, pid2, KCMP_FS,		0, 0),
+		       sys_kcmp(pid1, pid2, KCMP_SIGHAND,	0, 0),
+		       sys_kcmp(pid1, pid2, KCMP_IO,		0, 0),
+		       sys_kcmp(pid1, pid2, KCMP_SYSVSEM,	0, 0),
+
+			/* This one should fail */
+		       sys_kcmp(pid1, pid2, KCMP_TYPES + 1,	0, 0));
+
+		/* This one should return same fd */
+		ret = sys_kcmp(pid1, pid2, KCMP_FILE, fd1, fd1);
+		if (ret) {
+			printf("FAIL: 0 expected but %d returned\n", ret);
+			ret = -1;
+		} else
+			printf("PASS: 0 returned as expected\n");
+		exit(ret);
+	}
+
+	waitpid(pid2, &status, P_ALL);
+
+	return 0;
+}
Index: linux-2.6.git/tools/testing/selftests/run_tests
===================================================================
--- linux-2.6.git.orig/tools/testing/selftests/run_tests
+++ linux-2.6.git/tools/testing/selftests/run_tests
@@ -1,6 +1,6 @@
 #!/bin/bash
 
-TARGETS=breakpoints
+TARGETS="breakpoints kcmp"
 
 for TARGET in $TARGETS
 do

[patch cr 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 --]
[-- Type: text/plain, Size: 3236 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>
Reviewed-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                    |   25 ++++++++++++++++++++-----
 2 files changed, 25 insertions(+), 5 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
@@ -464,7 +464,7 @@ 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 ",
 		pid_nr_ns(pid, ns),
 		tcomm,
 		state,
@@ -511,10 +511,25 @@ static int do_task_stat(struct seq_file
 		task->policy,
 		(unsigned long long)delayacct_blkio_ticks(task),
 		cputime_to_clock_t(gtime),
-		cputime_to_clock_t(cgtime),
-		(mm && permitted) ? mm->start_data : 0,
-		(mm && permitted) ? mm->end_data : 0,
-		(mm && permitted) ? mm->start_brk : 0);
+		cputime_to_clock_t(cgtime));
+
+		if (mm && permitted) {
+			seq_printf(m, "%lu %lu %lu %lu %lu %lu %lu ",
+				   mm->start_data,
+				   mm->end_data,
+				   mm->start_brk,
+				   mm->arg_start,
+				   mm->arg_end,
+				   mm->env_start,
+				   mm->env_end);
+		} else
+			seq_printf(m, "0 0 0 0 0 0 0 ");
+
+		if (permitted)
+			seq_printf(m, "%d\n", task->exit_code);
+		else
+			seq_printf(m, "0\n");
+
 	if (mm)
 		mmput(mm);
 	return 0;

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

[Cyrill Gorcunov]

[-- Attachment #1: prctl-restore-mm-members-5 --]
[-- Type: text/plain, Size: 5372 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          |   86 ++++++++++++++++++++++++++++++++------------------
 2 files changed, 61 insertions(+), 30 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,23 @@ 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 mm_struct *mm = current->mm;
 	struct vm_area_struct *vma;
 	int error = 0;
-	struct mm_struct *mm = current->mm;
 
-	if (arg4 | arg5)
+	if (arg5 || (arg4 && opt != PR_SET_MM_AUXV))
 		return -EINVAL;
 
 	if (!capable(CAP_SYS_ADMIN))
@@ -1715,7 +1721,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 +1733,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 +1745,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 +1755,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,16 +1779,53 @@ 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: {
+		unsigned long user_auxv[AT_VECTOR_SIZE];
+
+		if (arg4 > sizeof(mm->saved_auxv))
+			goto out;
+		up_read(&mm->mmap_sem);
+
+		if (copy_from_user(user_auxv, (const void __user *)addr, arg4))
+			return EFAULT;
+
+		task_lock(current);
+		memcpy(mm->saved_auxv, user_auxv, arg4);
+		task_unlock(current);
+
+		return 0;
+	}
 	default:
 		error = -EINVAL;
 		goto out;
 	}
 
 	error = 0;
-
 out:
 	up_read(&mm->mmap_sem);
-
 	return error;
 }
 #else /* CONFIG_CHECKPOINT_RESTORE */

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

[Jonathan Corbet]

Just a couple of silly little things that came to mind while I was looking
at the code...

> +/*
> + * We don't expose real in-memory order of objects for security
> + * reasons, still the comparision results should be suitable for
> + * sorting. Thus, we obfuscate kernel pointers values (using random
> + * cookies obtaned at early boot stage) and compare the production
> + * instead.
> + */
> +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 don't understand the purpose of this at all.  Obfuscation will cause a
random shuffling in the ordering of the pointers - it's intended to - so
how is the result "suitable for sorting"?  More to the point, is there
ever a time when a user of this will care about some contrived ordering
value?  It seems like equality is all that really matters.

> +
> +/*
> + * 0 - equal
> + * 1 - less than
> + * 2 - greater than
> + * 3 - not equal but ordering unavailable (reserved for future)
> + */
> +static int kcmp_ptr(void *v1, void *v2, enum kcmp_type type)
> +{
> +	long ret;
> +
> +	ret = kptr_obfuscate((long)v1, type) - kptr_obfuscate((long)v2, type);
> +
> +	return (ret < 0) | ((ret > 0) << 1);
> +}

That's a cute trick, but do we know that every compiler that will ever see
this code will use 1 for a true integer comparison?  Simply spelling it
out with an if statement might be more robust, just as efficient, and, at
the same time, easier for others to understand.

Thanks,

jon

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

[Cyrill Gorcunov]

On Mon, Jan 30, 2012 at 12:58:12PM -0700, Jonathan Corbet wrote:
> Just a couple of silly little things that came to mind while I was looking
> at the code...
> 
> > +/*
> > + * We don't expose real in-memory order of objects for security
> > + * reasons, still the comparision results should be suitable for
> > + * sorting. Thus, we obfuscate kernel pointers values (using random
> > + * cookies obtaned at early boot stage) and compare the production
> > + * instead.
> > + */
> > +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 don't understand the purpose of this at all.  Obfuscation will cause a
> random shuffling in the ordering of the pointers - it's intended to - so
> how is the result "suitable for sorting"?  More to the point, is there
> ever a time when a user of this will care about some contrived ordering
> value?  It seems like equality is all that really matters.
> 

It won't be completely random shuffling but rather re-ordering in some
new order, which means the results might be passed to qsort or anything.
And yes, in c/r we need at least this "re-ordered" order which will help
to figure out shared file descriptors in case of huge number of files opened.

> > +
> > +/*
> > + * 0 - equal
> > + * 1 - less than
> > + * 2 - greater than
> > + * 3 - not equal but ordering unavailable (reserved for future)
> > + */
> > +static int kcmp_ptr(void *v1, void *v2, enum kcmp_type type)
> > +{
> > +	long ret;
> > +
> > +	ret = kptr_obfuscate((long)v1, type) - kptr_obfuscate((long)v2, type);
> > +
> > +	return (ret < 0) | ((ret > 0) << 1);
> > +}
> 
> That's a cute trick, but do we know that every compiler that will ever see
> this code will use 1 for a true integer comparison?  Simply spelling it
> out with an if statement might be more robust, just as efficient, and, at
> the same time, easier for others to understand.

Well, I believe if this become true, and (ret < 0) wont emit 1 -- the
number of places in kernel will be broken as well (for example see
math_div() function). But of course I don't insist and can rewrite
this code in straight fashion if needed.

	Cyrill

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

[H. Peter Anvin]

On 01/30/2012 11:58 AM, Jonathan Corbet wrote:
> 
> That's a cute trick, but do we know that every compiler that will ever see
> this code will use 1 for a true integer comparison?  Simply spelling it
> out with an if statement might be more robust, just as efficient, and, at
> the same time, easier for others to understand.
> 

This has been the case in C from the very beginning and is guaranteed by
the C standard.  The number of places which will break if this isn't
true is enormous.

	-hpa

Re: [patch cr 0/4] [patch cr 0/@total@]

[Andrew Morton]

On Mon, 30 Jan 2012 18:09:05 +0400
Cyrill Gorcunov <gorcunov@openvz.org> wrote:

> Hi,
> 
> this is a series where I tried to resolve all concerns and nits
> I've got from previous attempt:
> 
>  - fs, proc: Introduce /proc/<pid>/task/<tid>/children entry v9
> 	Now everything is under CONFIG_CHECKPOINT_RESTORE
> 
>  - syscalls, x86: Add __NR_kcmp syscall v7
> 	* EBADF returned if there is no file
> 	* EOPNOTSUP returned if there is no sysv support compiled in
> 	* test uses syscall() from libc instead of own syscall implementation
> 	* man page for this syscall is under progress still
> 
>  - c/r: procfs: add arg_start/end, env_start/end and exit_code members to /proc/$pid/stat
> 	task->exit_code is shown IIF a caller has enough permissions
> 
>  - c/r: prctl: Extend PR_SET_MM to set up more mm_struct entries
> 	when restoring mm->saved_auxv the task_lock is used to serialize access
> 
> Please review, if there is something I'm still missing, ping me back.
> 

Looks pretty close to me.  I had a few issues and niggles.  Please cc
me on v10 and unless someone squawks convincingly I will grab it.

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

[Andrew Morton]

On Mon, 30 Jan 2012 18:09:07 +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.
> 
> Lookups for pids are done in the caller's PID namespace only.
> 
> At moment only x86 is supported and tested.
> 
> ...
>
> --- 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

Can we turn this into obj-$(CONFIG_CHECKPOINT_RESTART) and add the
cond_syscall to sys_ni.c?

I keep saying this, because "hey, we can delete it all again" figures
largely in my rationale for merging your code ;)

>
> ...
>

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

[Andrew Morton]

On Mon, 30 Jan 2012 18:09:08 +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.
> 
> ...
>
> --- linux-2.6.git.orig/fs/proc/array.c
> +++ linux-2.6.git/fs/proc/array.c
> @@ -464,7 +464,7 @@ 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 ",
>  		pid_nr_ns(pid, ns),
>  		tcomm,
>  		state,
> @@ -511,10 +511,25 @@ static int do_task_stat(struct seq_file
>  		task->policy,
>  		(unsigned long long)delayacct_blkio_ticks(task),
>  		cputime_to_clock_t(gtime),
> -		cputime_to_clock_t(cgtime),
> -		(mm && permitted) ? mm->start_data : 0,
> -		(mm && permitted) ? mm->end_data : 0,
> -		(mm && permitted) ? mm->start_brk : 0);
> +		cputime_to_clock_t(cgtime));
> +
> +		if (mm && permitted) {
> +			seq_printf(m, "%lu %lu %lu %lu %lu %lu %lu ",
> +				   mm->start_data,
> +				   mm->end_data,
> +				   mm->start_brk,
> +				   mm->arg_start,
> +				   mm->arg_end,
> +				   mm->env_start,
> +				   mm->env_end);
> +		} else
> +			seq_printf(m, "0 0 0 0 0 0 0 ");

heh.

We have a whizzy new num_to_str() in -mm which can later be used to
speed this up.

This actually means that your patch will conflict horridly with
procfs-add-num_to_str-to-speed-up-proc-stat.patch, because that patch
redoes /proc/stat handling.  I guess I can take care of that issue.

> +
> +		if (permitted)
> +			seq_printf(m, "%d\n", task->exit_code);
> +		else
> +			seq_printf(m, "0\n");
> +
>  	if (mm)
>  		mmput(mm);
>  	return 0;

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

[Andrew Morton]

On Mon, 30 Jan 2012 18:09:09 +0400
Cyrill Gorcunov <gorcunov@openvz.org> 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 changelog is pretty darned hard to understand.  Can we have a
version 2 please?

>
> ...
>
> @@ -1753,19 +1755,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,16 +1779,53 @@ static int prctl_set_mm(int opt, unsigne
>  		mm->brk = addr;
>  		break;

Here would be a good place to add some nice comments explaining what
these do.  Although I guess that isn't needed if one can get that info
by typing "man prctl".

> +	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: {
> +		unsigned long user_auxv[AT_VECTOR_SIZE];
> +
> +		if (arg4 > sizeof(mm->saved_auxv))
> +			goto out;
> +		up_read(&mm->mmap_sem);
> +
> +		if (copy_from_user(user_auxv, (const void __user *)addr, arg4))
> +			return EFAULT;
> +
> +		task_lock(current);
> +		memcpy(mm->saved_auxv, user_auxv, arg4);
> +		task_unlock(current);
> +
> +		return 0;
> +	}

I worry a bit about this.  We're giving userspace the ability to modify
various mm_struct fields.  Userspace can already do this via
exec(elf-file), but perhaps this opens up a way in which userspace can
newly trigger kernel bugs.

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

[H. Peter Anvin]

On 02/02/2012 03:26 PM, Andrew Morton wrote:
>>
>> --- 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
>
> Can we turn this into obj-$(CONFIG_CHECKPOINT_RESTART) and add the
> cond_syscall to sys_ni.c?
>
> I keep saying this, because "hey, we can delete it all again" figures
> largely in my rationale for merging your code ;)
>

Ah, yes please.

	-hpa

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

[Cyrill Gorcunov]

On Thu, Feb 02, 2012 at 06:27:02PM -0800, H. Peter Anvin wrote:
> On 02/02/2012 03:26 PM, Andrew Morton wrote:
> >>
> >>--- 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
> >
> >Can we turn this into obj-$(CONFIG_CHECKPOINT_RESTART) and add the
> >cond_syscall to sys_ni.c?
> >
> >I keep saying this, because "hey, we can delete it all again" figures
> >largely in my rationale for merging your code ;)
> >
> 
> Ah, yes please.
> 

Sure, will update, thanks!

	Cyrill

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

[Cyrill Gorcunov]

On Thu, Feb 02, 2012 at 03:26:48PM -0800, Andrew Morton wrote:
...
> > +		} else
> > +			seq_printf(m, "0 0 0 0 0 0 0 ");
> 
> heh.
> 
> We have a whizzy new num_to_str() in -mm which can later be used to
> speed this up.
> 
> This actually means that your patch will conflict horridly with
> procfs-add-num_to_str-to-speed-up-proc-stat.patch, because that patch
> redoes /proc/stat handling.  I guess I can take care of that issue.
> 

I'll check linux-next version as well.

	Cyrill

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

[Cyrill Gorcunov]

On Thu, Feb 02, 2012 at 03:27:05PM -0800, Andrew Morton wrote:
> On Mon, 30 Jan 2012 18:09:09 +0400
> Cyrill Gorcunov <gorcunov@openvz.org> 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 changelog is pretty darned hard to understand.  Can we have a
> version 2 please?
> 

yeah, will update.
...
> > @@ -1790,16 +1779,53 @@ static int prctl_set_mm(int opt, unsigne
> >  		mm->brk = addr;
> >  		break;
> 
> Here would be a good place to add some nice comments explaining what
> these do.  Although I guess that isn't needed if one can get that info
> by typing "man prctl".
> 

I started cooking prctl man pages but found hardness to explain some
regular user who has no ideas about kernel internals why do we modify
mm_struct data, still I'm trying.

And I'll add comment here (since having it here in-place allows reader
to not read man page ;)
...
> 
> I worry a bit about this.  We're giving userspace the ability to modify
> various mm_struct fields.  Userspace can already do this via
> exec(elf-file), but perhaps this opens up a way in which userspace can
> newly trigger kernel bugs.
> 

At moment there is no more way to modify these fields other than elf
handler, but in future... hard to predict what else there will be
done and where also these fields appear in kernel code. but as i said
at moment this modification is pretty safe and even if one write some
buggy values -- he simply get weird output in /proc/ statistics and
such.

	Cyrill

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

[Ingo Molnar]

* Cyrill Gorcunov <gorcunov@openvz.org> wrote:

> +/* Comparision type */

> + * We don't expose real in-memory order of objects for security
> + * reasons, still the comparision results should be suitable for
> + * sorting. Thus, we obfuscate kernel pointers values (using random
> + * cookies obtaned at early boot stage) and compare the production
> + * instead.

> + * 0 - equal
> + * 1 - less than
> + * 2 - greater than
> + * 3 - not equal but ordering unavailable (reserved for future)

Broken spelling in each of those comment blocks. Are these 
comments write-only?

> +	/*
> +	 * Tasks are looked up in caller's
> +	 * PID namespace only.
> +	 */

Could be a single line.

> +
> +	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;
> +	}

This is not the standard pattern of how we do error paths ...

> +	/*
> +	 * Note for all cases but the KCMP_FILE we
> +	 * don't take any locks in a sake of speed.
> +	 */

Spelling.

> +			get_random_bytes(&cookies[i][j],
> +					 sizeof(cookies[i][j]));

ugly line break.

> +late_initcall(kcmp_cookie_init);

any particular reason why this needs to be a late initcall?

> --- /dev/null
> +++ linux-2.6.git/tools/testing/selftests/kcmp/Makefile
> @@ -0,0 +1,35 @@
> +ifeq ($(strip $(V)),)
> +	E = @echo
> +	Q = @
> +else
> +	E = @\#
> +	Q =
> +endif
> +export E Q
> +
> +uname_M := $(shell uname -m 2>/dev/null || echo not)
> +ARCH ?= $(shell echo $(uname_M) | sed -e s/i.86/i386/)
> +ifeq ($(ARCH),i386)
> +        ARCH := X86
> +	CFLAGS := -DCONFIG_X86_32
> +endif
> +ifeq ($(ARCH),x86_64)
> +	ARCH := X86
> +	CFLAGS := -DCONFIG_X86_64
> +endif
> +
> +CFLAGS += -I../../../../arch/x86/include/generated/
> +CFLAGS += -I../../../../include/
> +
> +all:
> +ifeq ($(ARCH),X86)
> +	$(E) "  CC run_test"
> +	$(Q) gcc $(CFLAGS) kcmp_test.c -o run_test
> +else
> +	$(E) "Not an x86 target, can't build breakpoints selftests"
> +endif
> +
> +clean:
> +	$(E) "  CLEAN"
> +	$(Q) rm -fr ./run_test
> +	$(Q) rm -fr ./test-file

Needs buy-in from the kbuild guys.

> +#ifdef CONFIG_X86_64
> +#include <asm/unistd_64.h>
> +#else
> +#include <asm/unistd_32.h>
> +#endif

Why is asm/unistd.h not good?

> +static long sys_kcmp(int pid1, int pid2, int type, int fd1, int fd2)
> +{
> +	return syscall(__NR_kcmp, (long)pid1, (long)pid2,
> +		       (long)type, (long)fd1, (long)fd2);
> +}

Why is a syscall that takes long arguments defined and called 
with int and then cast over to long again?

> +		int pid2 = getpid();
> +		int ret;
> +
> +		fd2 = open("test-file", O_RDWR, 0644);
> +		if (fd2 < 0) {
> +			perror("Can't open file");
> +			exit(1);
> +		}
> +
> +		/* An example of output and arguments */
> +                printf("pid1: %6d pid2: %6d FD: %2d FILES: %2d VM: %2d FS: %2d "
> +		       "SIGHAND: %2d IO: %2d SYSVSEM: %2d INV: %2d\n",

Visibly stray whitespaces.

> +		/* This one should return same fd */
> +		ret = sys_kcmp(pid1, pid2, KCMP_FILE, fd1, fd1);
> +		if (ret) {
> +			printf("FAIL: 0 expected but %d returned\n", ret);
> +			ret = -1;
> +		} else
> +			printf("PASS: 0 returned as expected\n");
> +		exit(ret);

this is main(), what's wrong with the standard pattern of return 
ret?

I don't know whether this code is correct, but the high amount 
of basic cleanliness problems makes me worry about that.

Thanks,

	Ingo

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

[Cyrill Gorcunov]

On Fri, Feb 03, 2012 at 08:46:56AM +0100, Ingo Molnar wrote:
> 
> * Cyrill Gorcunov <gorcunov@openvz.org> wrote:
> 
> > +/* Comparision type */
> 
> > + * We don't expose real in-memory order of objects for security
> > + * reasons, still the comparision results should be suitable for
> > + * sorting. Thus, we obfuscate kernel pointers values (using random
> > + * cookies obtaned at early boot stage) and compare the production
> > + * instead.
> 
> > + * 0 - equal
> > + * 1 - less than
> > + * 2 - greater than
> > + * 3 - not equal but ordering unavailable (reserved for future)
> 
> Broken spelling in each of those comment blocks. Are these 
> comments write-only?

No, they are not write-only. I've fixed typos in first comment block,
though I don't understand what is wrong with 0,1,2,3 comments.

> 
> > +	/*
> > +	 * Tasks are looked up in caller's
> > +	 * PID namespace only.
> > +	 */
> 
> Could be a single line.
> 

Ok, will do so.

> > +
> > +	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;
> > +	}
> 
> This is not the standard pattern of how we do error paths ...

OK, I'll try to make it in standart way.

> 
> > +	/*
> > +	 * Note for all cases but the KCMP_FILE we
> > +	 * don't take any locks in a sake of speed.
> > +	 */
> 
> Spelling.

Not sure what you mean here, but I'll drop this comment
to eliminate this problem.

> 
> > +			get_random_bytes(&cookies[i][j],
> > +					 sizeof(cookies[i][j]));
> 
> ugly line break.
> 

Why? Looks pretty good to me. But sure I'll change it.

> > +late_initcall(kcmp_cookie_init);
> 
> any particular reason why this needs to be a late initcall?
> 

Grr! The late_initcall remained here from versions where I've
been playing with crypto hashes. Thanks, Ingo, I'll fix!

> > +
> > +clean:
> > +	$(E) "  CLEAN"
> > +	$(Q) rm -fr ./run_test
> > +	$(Q) rm -fr ./test-file
> 
> Needs buy-in from the kbuild guys.

I took breakpoint test as example. Maybe I should
send this test case code as a separate patch?

> 
> > +#ifdef CONFIG_X86_64
> > +#include <asm/unistd_64.h>
> > +#else
> > +#include <asm/unistd_32.h>
> > +#endif
> 
> Why is asm/unistd.h not good?
> 

With asm/unistd.h it fails to build because it requires the headers
to be installed first (ie headers_install target) so I though this
way would be more convenient, no?

> > +static long sys_kcmp(int pid1, int pid2, int type, int fd1, int fd2)
> > +{
> > +	return syscall(__NR_kcmp, (long)pid1, (long)pid2,
> > +		       (long)type, (long)fd1, (long)fd2);
> > +}
> 
> Why is a syscall that takes long arguments defined and called 
> with int and then cast over to long again?
> 

Just a habit, the args will be converted to long anyway,
so I don't see a problem here. Still I can drop them.

> > +		int pid2 = getpid();
> > +		int ret;
> > +
> > +		fd2 = open("test-file", O_RDWR, 0644);
> > +		if (fd2 < 0) {
> > +			perror("Can't open file");
> > +			exit(1);
> > +		}
> > +
> > +		/* An example of output and arguments */
> > +                printf("pid1: %6d pid2: %6d FD: %2d FILES: %2d VM: %2d FS: %2d "
> > +		       "SIGHAND: %2d IO: %2d SYSVSEM: %2d INV: %2d\n",
> 
> Visibly stray whitespaces.
> 
> > +		/* This one should return same fd */
> > +		ret = sys_kcmp(pid1, pid2, KCMP_FILE, fd1, fd1);
> > +		if (ret) {
> > +			printf("FAIL: 0 expected but %d returned\n", ret);
> > +			ret = -1;
> > +		} else
> > +			printf("PASS: 0 returned as expected\n");
> > +		exit(ret);
> 
> this is main(), what's wrong with the standard pattern of return 
> ret?
> 

It's fork'ed children.

> I don't know whether this code is correct, but the high amount 
> of basic cleanliness problems makes me worry about that.
> 

Code is correct. I'll clean up the nits you pointed.

	Cyrill

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

[Ingo Molnar]

* Cyrill Gorcunov <gorcunov@openvz.org> wrote:

> > > +			get_random_bytes(&cookies[i][j],
> > > +					 sizeof(cookies[i][j]));
> > 
> > ugly line break.
> > 
> 
> Why? Looks pretty good to me. But sure I'll change it.

It's ugly because it serves no purpose other than pacifying 
checkpatch and makes the code *uglier*.

It's a disease. When checkpatch tells you "this line is too 
long" then consider it a code cleanliness warning!

		And code
	readability and cleanliness
		is not improved
	by random line-
		breaks, right?

'breaking the line' is the *wrong fix* in roughly 90% of the 
cases.

So instead of dumbly breaking the line you need to think about 
*WHY* the line got too long, not just mechanically work around 
the checkpatch warning!

Too long lines can have many reasons, it's usually one of 
several reasons:

     - too much nesting due to too large function.
       solution: break up the function
 or: - too verbose statements with not enough abbreviation
       solution: find a more compact way to write it

or if the code looks compact enough and is not over-nested then 
*leave the line alone*. By breaking it you have not improved - 
you have made it worse.

Thanks,

	Ingo

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

[Andrew Morton]

On Fri, 3 Feb 2012 10:09:29 +0100 Ingo Molnar <mingo@elte.hu> wrote:

> 
> * Cyrill Gorcunov <gorcunov@openvz.org> wrote:
> 
> > > > +			get_random_bytes(&cookies[i][j],
> > > > +					 sizeof(cookies[i][j]));
> > > 
> > > ugly line break.
> > > 
> > 
> > Why? Looks pretty good to me. But sure I'll change it.
> 
> It's ugly because it serves no purpose other than pacifying 
> checkpatch and makes the code *uglier*.

No it doesn't.  For 80-col displays the code is *already wrapped*.  And
that wrapping to column 0 is vastly worse than the above.

If we want to increase the standard to (say) 96 cols then fine, I'd be
happy with that.  But until we do that we should not create such a
gruesome mess for those who use 80 cols.

> It's a disease. When checkpatch tells you "this line is too 
> long" then consider it a code cleanliness warning!

Well yes, if it can be fixed by other means then great.

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

[Cyrill Gorcunov]

On Fri, Feb 03, 2012 at 01:22:41AM -0800, Andrew Morton wrote:
> On Fri, 3 Feb 2012 10:09:29 +0100 Ingo Molnar <mingo@elte.hu> wrote:
> 
> > 
> > * Cyrill Gorcunov <gorcunov@openvz.org> wrote:
> > 
> > > > > +			get_random_bytes(&cookies[i][j],
> > > > > +					 sizeof(cookies[i][j]));
> > > > 
> > > > ugly line break.
> > > > 
> > > 
> > > Why? Looks pretty good to me. But sure I'll change it.
> > 
> > It's ugly because it serves no purpose other than pacifying 
> > checkpatch and makes the code *uglier*.
> 
> No it doesn't.  For 80-col displays the code is *already wrapped*.  And
> that wrapping to column 0 is vastly worse than the above.
> 
> If we want to increase the standard to (say) 96 cols then fine, I'd be
> happy with that.  But until we do that we should not create such a
> gruesome mess for those who use 80 cols.
> 
> > It's a disease. When checkpatch tells you "this line is too 
> > long" then consider it a code cleanliness warning!
> 
> Well yes, if it can be fixed by other means then great.
> 

Guys, I simply made it as

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(long));
		cookies[i][1] |= (~(~0UL >>  1) | 1);
	}

	return 0;
}

I thought in first place that sizeof(cookies[i][j]) would allow me
to change type of cookies in one place (ie at declaration) but
if cookies type will be changed -- the code will need careful review
anway, so sizeof(long) will be enough I think.

On the other hands, maybe more clean variant will be

static __init int kcmp_cookie_init(void)
{
	const int size = sizeof(cookies[0][0]);
	int i, j;

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

	return 0;
}

Hm?

	Cyrill

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

[Ingo Molnar]

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

> On Fri, 3 Feb 2012 10:09:29 +0100 Ingo Molnar <mingo@elte.hu> wrote:
> 
> > 
> > * Cyrill Gorcunov <gorcunov@openvz.org> wrote:
> > 
> > > > > +			get_random_bytes(&cookies[i][j],
> > > > > +					 sizeof(cookies[i][j]));
> > > > 
> > > > ugly line break.
> > > > 
> > > 
> > > Why? Looks pretty good to me. But sure I'll change it.
> > 
> > It's ugly because it serves no purpose other than pacifying 
> > checkpatch and makes the code *uglier*.
> 
> No it doesn't.  For 80-col displays the code is *already 
> wrapped*.  And that wrapping to column 0 is vastly worse than 
> the above.

Have you actually checked how this actual line would look like 
in an 80 cols terminal, if not broken up? I have, it's exactly 
80 cols so it looks just fine.

( It was probably broken up when it was longer and then left 
  this way - making things permanently worse not just by the 
  linebreak but also by the unnecessary curly braces around the 
  inner loop. )

But more importantly, even if the line was genuinely longer, how 
many people are looking at things in an 80-col display? By my 
experience, from looking at what kinds of terminals kernel 
people use, it's below 1%. (I was one of the last ones holding 
out because text consoles are so much faster than just about any 
usable xterm app - but I switched to a larger terminal some two 
years ago.)

Shouldnt't we concentrate on the 99% case which gets uglified by 
the systematic linebreaks?

Also, there are clearly cases where breaking the line 
intelligently improves things. Such as:

+               /* An example of output and arguments */
+               printf("pid1: %6d pid2: %6d FD: %2d FILES: %2d VM: %2d FS: %2d "
+                      "SIGHAND: %2d IO: %2d SYSVSEM: %2d INV: %2d\n",
+                      pid1, pid2,
+                      sys_kcmp(pid1, pid2, KCMP_FILE,          fd1, fd2),
+                      sys_kcmp(pid1, pid2, KCMP_FILES,         0, 0),
+                      sys_kcmp(pid1, pid2, KCMP_VM,            0, 0),
+                      sys_kcmp(pid1, pid2, KCMP_FS,            0, 0),
+                      sys_kcmp(pid1, pid2, KCMP_SIGHAND,	0, 0),
+                      sys_kcmp(pid1, pid2, KCMP_IO,            0, 0),
+                      sys_kcmp(pid1, pid2, KCMP_SYSVSEM,	0, 0),
+
+                       /* This one should fail */
+                      sys_kcmp(pid1, pid2, KCMP_TYPES + 1,     0, 0));

this is vastly more readable because the arguments are lined up 
vertically not just at the beginning but nicely tabulated along 
the way.

Oh, and note that 

Breaking lines is a tool that should be used on a case by case 
basis, not a hard limit.

> If we want to increase the standard to (say) 96 cols then 
> fine, I'd be happy with that.  But until we do that we should 
> not create such a gruesome mess for those who use 80 cols.

The kernel has *already* become a gruesome mess for 80 col users 
long ago. That was the main reason why I stopped using 80 col 
terminals two years ago ...

So lets stop the pretense.

> > It's a disease. When checkpatch tells you "this line is too 
> > long" then consider it a code cleanliness warning!
> 
> Well yes, if it can be fixed by other means then great.

Yes it can.

Thanks,

	Ingo

[PATCH] SubmittingPatches: Increase the line length limit from 80 to 100 colums

[Ingo Molnar]

* Ingo Molnar <mingo@elte.hu> wrote:

> > If we want to increase the standard to (say) 96 cols then 
> > fine, I'd be happy with that.  But until we do that we 
> > should not create such a gruesome mess for those who use 80 
> > cols.
> 
> The kernel has *already* become a gruesome mess for 80 col 
> users long ago. That was the main reason why I stopped using 
> 80 col terminals two years ago ...
> 
> So lets stop the pretense.

in other words:

--------------->
[PATCH] SubmittingPatches: Increase the line length limit from 80 to 100 colums

The overwhelming majority of kernel developers have stopped 
using 80 col terminals years ago.

As far as I'm aware I was the last regular kernel contributor 
who still used a standard VGA text console, but both text 
consoles and using them to read the kernel source code has 
become increasingly gruesome years ago so I switched to a wider 
terminal two years ago.

Worse than that, people are actively uglifying the kernel code 
to fit things into 80 cols mechanically. They are using 
checkpatch and are interpreting the 80 col warnings the wrong 
way again and again, sucking up reviewer bandwidth that could be 
utilized better.

So lets increase the limit to 100 cols - this is a nice round 
limit, and it also happens to match with most developer xterm 
sizes. Code that goes over 100 cols for no good reasons will be 
arguably something worth fixing. (100 cols is also arguably 
closer to various brain limits such as vision of field and 
resolution restrictions, so we'll likely not have to increase 
this limit for a couple of million years, for all retro human 
genome users.)

Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
diff --git a/Documentation/CodingStyle b/Documentation/CodingStyle
index 2b90d32..e87370f 100644
--- a/Documentation/CodingStyle
+++ b/Documentation/CodingStyle
@@ -27,7 +27,7 @@ how the indentation works if you have large indentations.
 
 Now, some people will claim that having 8-character indentations makes
 the code move too far to the right, and makes it hard to read on a
-80-character terminal screen.  The answer to that is that if you need
+100-character terminal screen.  The answer to that is that if you need
 more than 3 levels of indentation, you're screwed anyway, and should fix
 your program.
 
@@ -77,11 +77,11 @@ Get a decent editor and don't leave whitespace at the end of lines.
 Coding style is all about readability and maintainability using commonly
 available tools.
 
-The limit on the length of lines is 80 columns and this is a strongly
+The limit on the length of lines is 100 columns and this is a strongly
 preferred limit.
 
-Statements longer than 80 columns will be broken into sensible chunks, unless
-exceeding 80 columns significantly increases readability and does not hide
+Statements longer than 100 columns will be broken into sensible chunks, unless
+exceeding 100 columns significantly increases readability and does not hide
 information. Descendants are always substantially shorter than the parent and
 are placed substantially to the right. The same applies to function headers
 with a long argument list. However, never break user-visible strings such as
@@ -344,8 +344,7 @@ be directly accessed should _never_ be a typedef.
 		Chapter 6: Functions
 
 Functions should be short and sweet, and do just one thing.  They should
-fit on one or two screenfuls of text (the ISO/ANSI screen size is 80x24,
-as we all know), and do one thing and do that well.
+fit on one or two screenfuls of text, and do one thing and do that well.
 
 The maximum length of a function is inversely proportional to the
 complexity and indentation level of that function.  So, if you have a
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index e3bfcbe..2d0f093 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1726,15 +1726,14 @@ sub process {
 # check we are in a valid source file if not then ignore this hunk
 		next if ($realfile !~ /\.(h|c|s|S|pl|sh)$/);
 
-#80 column limit
+#100 column limit
 		if ($line =~ /^\+/ && $prevrawline !~ /\/\*\*/ &&
 		    $rawline !~ /^.\s*\*\s*\@$Ident\s/ &&
 		    !($line =~ /^\+\s*$logFunctions\s*\(\s*(?:(KERN_\S+\s*|[^"]*))?"[X\t]*"\s*(?:|,|\)\s*;)\s*$/ ||
 		    $line =~ /^\+\s*"[^"]*"\s*(?:\s*|,|\)\s*;)\s*$/) &&
-		    $length > 80)
+		    $length > 100)
 		{
-			WARN("LONG_LINE",
-			     "line over 80 characters\n" . $herecurr);
+			WARN("LONG_LINE", "line over 100 characters\n" . $herecurr);
 		}
 
 # check for spaces before a quoted newline

Re: [PATCH] SubmittingPatches: Increase the line length limit from 80 to 100 colums

[Pekka Enberg]

On Fri, Feb 3, 2012 at 12:07 PM, Ingo Molnar <mingo@elte.hu> wrote:
>> The kernel has *already* become a gruesome mess for 80 col
>> users long ago. That was the main reason why I stopped using
>> 80 col terminals two years ago ...
>>
>> So lets stop the pretense.
>
> in other words:
>
> --------------->
> [PATCH] SubmittingPatches: Increase the line length limit from 80 to 100 colums
>
> The overwhelming majority of kernel developers have stopped
> using 80 col terminals years ago.
>
> As far as I'm aware I was the last regular kernel contributor
> who still used a standard VGA text console, but both text
> consoles and using them to read the kernel source code has
> become increasingly gruesome years ago so I switched to a wider
> terminal two years ago.
>
> Worse than that, people are actively uglifying the kernel code
> to fit things into 80 cols mechanically. They are using
> checkpatch and are interpreting the 80 col warnings the wrong
> way again and again, sucking up reviewer bandwidth that could be
> utilized better.
>
> So lets increase the limit to 100 cols - this is a nice round
> limit, and it also happens to match with most developer xterm
> sizes. Code that goes over 100 cols for no good reasons will be
> arguably something worth fixing. (100 cols is also arguably
> closer to various brain limits such as vision of field and
> resolution restrictions, so we'll likely not have to increase
> this limit for a couple of million years, for all retro human
> genome users.)
>
> Signed-off-by: Ingo Molnar <mingo@elte.hu>

The rationale in the changelog is somewhat over the top but I agree
with the change :-)

Acked-by: Pekka Enberg <penberg@kernel.org>

Re: [PATCH] SubmittingPatches: Increase the line length limit from 80 to 100 colums

[Cyrill Gorcunov]

On Fri, Feb 03, 2012 at 12:17:43PM +0200, Pekka Enberg wrote:
> >
> > Signed-off-by: Ingo Molnar <mingo@elte.hu>
> 
> The rationale in the changelog is somewhat over the top but I agree
> with the change :-)
> 
> Acked-by: Pekka Enberg <penberg@kernel.org>
> 

FWIW, agreed as well, thanks Ingo!

Acked-by: Cyrill Gorcunov <gorcunov@openvz.org>

	Cyrill

Re: [PATCH] SubmittingPatches: Increase the line length limit from 80 to 100 colums

[Alexey Dobriyan]

On Fri, Feb 3, 2012 at 1:07 PM, Ingo Molnar <mingo@elte.hu> wrote:

> [PATCH] SubmittingPatches: Increase the line length limit from 80 to 100 colums

Better don't mention line length at all.

Re: [PATCH] SubmittingPatches: Increase the line length limit from 80 to 100 colums

[Tejun Heo]

Hello,

On Fri, Feb 03, 2012 at 11:07:43AM +0100, Ingo Molnar wrote:
> 
> * Ingo Molnar <mingo@elte.hu> wrote:
> 
> > > If we want to increase the standard to (say) 96 cols then 
> > > fine, I'd be happy with that.  But until we do that we 
> > > should not create such a gruesome mess for those who use 80 
> > > cols.
> > 
> > The kernel has *already* become a gruesome mess for 80 col 
> > users long ago. That was the main reason why I stopped using 
> > 80 col terminals two years ago ...
> > 
> > So lets stop the pretense.

I don't know.  In my experience, a lot of code, especially core part,
mostly follows 80 col limit.  It shouldn't be too difficult to write
up a script to count >80col lines in different parts of the kernel.

> [PATCH] SubmittingPatches: Increase the line length limit from 80 to 100 colums
> 
> The overwhelming majority of kernel developers have stopped 
> using 80 col terminals years ago.
> 
> As far as I'm aware I was the last regular kernel contributor 
> who still used a standard VGA text console, but both text 
> consoles and using them to read the kernel source code has 
> become increasingly gruesome years ago so I switched to a wider 
> terminal two years ago.

People usually place multiple windows horizontally so it's not like
all those extra pixels go wasted.  80col might even have the benefit
of giving overall higher density in terms of pixel usage.

> Worse than that, people are actively uglifying the kernel code 
> to fit things into 80 cols mechanically. They are using 
> checkpatch and are interpreting the 80 col warnings the wrong 
> way again and again, sucking up reviewer bandwidth that could be 
> utilized better.
> 
> So lets increase the limit to 100 cols - this is a nice round 
> limit, and it also happens to match with most developer xterm 
> sizes. Code that goes over 100 cols for no good reasons will be 
> arguably something worth fixing. (100 cols is also arguably 
> closer to various brain limits such as vision of field and 
> resolution restrictions, so we'll likely not have to increase 
> this limit for a couple of million years, for all retro human 
> genome users.)

That said, yeah, 80col is a pain in the ass and lessening the pressure
a bit might make it a non-problem and 100 is one of the nicer numbers
which aren't power of two.

For me, the biggest reason to stick to 80col has been that, while
being widely disliked, it still was the most common limit people were
using and consistency tends to be more beneficial on these issues.  If
we're gonna do this, and I hope we do, let's proactively encourage /
enforce it - ie. let's collectively nag so that 100col quickly becomes
the standard.

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

Re: [PATCH] SubmittingPatches: Increase the line length limit from 80 to 100 colums

[hpanvin@gmail.com]

96 might be a better limit.  Why?  Because 80 and 96 are the easiest line lengths to achieve on a printer.  80 is typically the default, but with a 12 cpi font you get 96.

Tejun Heo <tj@kernel.org> wrote:

>Hello,
>
>On Fri, Feb 03, 2012 at 11:07:43AM +0100, Ingo Molnar wrote:
>> 
>> * Ingo Molnar <mingo@elte.hu> wrote:
>> 
>> > > If we want to increase the standard to (say) 96 cols then 
>> > > fine, I'd be happy with that.  But until we do that we 
>> > > should not create such a gruesome mess for those who use 80 
>> > > cols.
>> > 
>> > The kernel has *already* become a gruesome mess for 80 col 
>> > users long ago. That was the main reason why I stopped using 
>> > 80 col terminals two years ago ...
>> > 
>> > So lets stop the pretense.
>
>I don't know.  In my experience, a lot of code, especially core part,
>mostly follows 80 col limit.  It shouldn't be too difficult to write
>up a script to count >80col lines in different parts of the kernel.
>
>> [PATCH] SubmittingPatches: Increase the line length limit from 80 to
>100 colums
>> 
>> The overwhelming majority of kernel developers have stopped 
>> using 80 col terminals years ago.
>> 
>> As far as I'm aware I was the last regular kernel contributor 
>> who still used a standard VGA text console, but both text 
>> consoles and using them to read the kernel source code has 
>> become increasingly gruesome years ago so I switched to a wider 
>> terminal two years ago.
>
>People usually place multiple windows horizontally so it's not like
>all those extra pixels go wasted.  80col might even have the benefit
>of giving overall higher density in terms of pixel usage.
>
>> Worse than that, people are actively uglifying the kernel code 
>> to fit things into 80 cols mechanically. They are using 
>> checkpatch and are interpreting the 80 col warnings the wrong 
>> way again and again, sucking up reviewer bandwidth that could be 
>> utilized better.
>> 
>> So lets increase the limit to 100 cols - this is a nice round 
>> limit, and it also happens to match with most developer xterm 
>> sizes. Code that goes over 100 cols for no good reasons will be 
>> arguably something worth fixing. (100 cols is also arguably 
>> closer to various brain limits such as vision of field and 
>> resolution restrictions, so we'll likely not have to increase 
>> this limit for a couple of million years, for all retro human 
>> genome users.)
>
>That said, yeah, 80col is a pain in the ass and lessening the pressure
>a bit might make it a non-problem and 100 is one of the nicer numbers
>which aren't power of two.
>
>For me, the biggest reason to stick to 80col has been that, while
>being widely disliked, it still was the most common limit people were
>using and consistency tends to be more beneficial on these issues.  If
>we're gonna do this, and I hope we do, let's proactively encourage /
>enforce it - ie. let's collectively nag so that 100col quickly becomes
>the standard.
>
>Acked-by: Tejun Heo <tj@kernel.org>
>
>Thanks.
>
>-- 
>tejun

-- 
Sent from my Android phone with K-9 Mail. Please excuse my brevity.

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

[H. Peter Anvin]

On 02/03/2012 01:28 AM, Cyrill Gorcunov wrote:
> 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(long));
> 		cookies[i][1] |= (~(~0UL>>   1) | 1);
> 	}
>
> 	return 0;
> }
>
> I thought in first place that sizeof(cookies[i][j]) would allow me
> to change type of cookies in one place (ie at declaration) but
> if cookies type will be changed -- the code will need careful review
> anway, so sizeof(long) will be enough I think.
>
> On the other hands, maybe more clean variant will be
>
> static __init int kcmp_cookie_init(void)
> {
> 	const int size = sizeof(cookies[0][0]);
> 	int i, j;
>
> 	for (i = 0; i<  KCMP_TYPES; i++) {
> 		for (j = 0; j<  2; j++)
> 			get_random_bytes(&cookies[i][j], size);
> 		cookies[i][1] |= (~(~0UL>>   1) | 1);
> 	}
>

How about:

	const int size = sizeof(cookies[0]);

	get_random_bytes(&cookies[i], size);

... and skip the completely unnecessary for loop?

	-hpa

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

[H. Peter Anvin]

On 02/03/2012 09:32 AM, H. Peter Anvin wrote:
> On 02/03/2012 01:28 AM, Cyrill Gorcunov wrote:
>> 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(long));
>> cookies[i][1] |= (~(~0UL>> 1) | 1);
>> }
>>
>> return 0;
>> }
>>
>> I thought in first place that sizeof(cookies[i][j]) would allow me
>> to change type of cookies in one place (ie at declaration) but
>> if cookies type will be changed -- the code will need careful review
>> anway, so sizeof(long) will be enough I think.
>>
>> On the other hands, maybe more clean variant will be
>>
>> static __init int kcmp_cookie_init(void)
>> {
>> const int size = sizeof(cookies[0][0]);
>> int i, j;
>>
>> for (i = 0; i< KCMP_TYPES; i++) {
>> for (j = 0; j< 2; j++)
>> get_random_bytes(&cookies[i][j], size);
>> cookies[i][1] |= (~(~0UL>> 1) | 1);
>> }
>>
>
> How about:
>
> const int size = sizeof(cookies[0]);
>
> get_random_bytes(&cookies[i], size);
>
> ... and skip the completely unnecessary for loop?
>

Even better:

static __init int kcmp_cookie_init(void)
{
	int i;

	get_random_bytes(cookies, sizeof cookies);

	for (i = 0; i < KCMP_TYPES; i++)
		cookies[i][1] |= ~(~0UL >> 1) | 1;
}

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

[Cyrill Gorcunov]

On Fri, Feb 03, 2012 at 09:35:05AM -0800, H. Peter Anvin wrote:
...
> >How about:
> >
> >const int size = sizeof(cookies[0]);
> >
> >get_random_bytes(&cookies[i], size);
> >
> >... and skip the completely unnecessary for loop?
> >
> 
> Even better:
> 
> static __init int kcmp_cookie_init(void)
> {
> 	int i;
> 
> 	get_random_bytes(cookies, sizeof cookies);
> 
> 	for (i = 0; i < KCMP_TYPES; i++)
> 		cookies[i][1] |= ~(~0UL >> 1) | 1;
> }
> 

Oh, cool! It's a way simplier (I somehow forgot that
get_random_bytes operates with stream of bytes).

	Cyrill
---
From: Cyrill Gorcunov <gorcunov@openvz.org>
Subject: syscalls, x86: Add __NR_kcmp syscall v8

While doing the checkpoint-restore in the user space 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
'comparison' syscall might be the best candidate. So here is it --
__NR_kcmp.

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

Lookups for pids are done in the caller's PID namespace only.

At moment only x86 is supported and tested.

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
CC: Michal Marek <mmarek@suse.cz>
---
 arch/x86/syscalls/syscall_32.tbl         |    1 
 arch/x86/syscalls/syscall_64.tbl         |    1 
 include/linux/kcmp.h                     |   17 +++
 include/linux/syscalls.h                 |    2 
 kernel/Makefile                          |    3 
 kernel/kcmp.c                            |  155 +++++++++++++++++++++++++++++++
 kernel/sys_ni.c                          |    3 
 tools/testing/selftests/kcmp/Makefile    |   36 +++++++
 tools/testing/selftests/kcmp/kcmp_test.c |   84 ++++++++++++++++
 tools/testing/selftests/run_tests        |    2 
 10 files changed, 303 insertions(+), 1 deletion(-)

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
+
+/* Comparison type */
+enum kcmp_type {
+	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/include/linux/syscalls.h
===================================================================
--- linux-2.6.git.orig/include/linux/syscalls.h
+++ linux-2.6.git/include/linux/syscalls.h
@@ -857,4 +857,6 @@ asmlinkage long sys_process_vm_writev(pi
 				      unsigned long riovcnt,
 				      unsigned long flags);
 
+asmlinkage long sys_kcmp(pid_t pid1, pid_t pid2, int type,
+			 unsigned long idx1, unsigned long idx2);
 #endif
Index: linux-2.6.git/kernel/Makefile
===================================================================
--- linux-2.6.git.orig/kernel/Makefile
+++ linux-2.6.git/kernel/Makefile
@@ -25,6 +25,9 @@ endif
 obj-y += sched/
 obj-y += power/
 
+ifeq ($(CONFIG_CHECKPOINT_RESTORE),y)
+obj-$(CONFIG_X86) += kcmp.o
+endif
 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,155 @@
+#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>
+
+/*
+ * We don't expose real in-memory order of objects for security
+ * reasons, still the comparison results should be suitable for
+ * sorting. Thus, we obfuscate kernel pointers values and compare
+ * the production instead.
+ */
+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, i.e. v1 = v2
+ * 1 - less than, i.e. v1 < v2
+ * 2 - greater than, i.e. v1 > v2
+ * 3 - not equal but ordering unavailable (reserved for future)
+ */
+static int kcmp_ptr(void *v1, void *v2, enum kcmp_type type)
+{
+	long ret;
+
+	ret = kptr_obfuscate((long)v1, type) - kptr_obfuscate((long)v2, type);
+
+	return (ret < 0) | ((ret > 0) << 1);
+}
+
+/* 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, *task2;
+	int ret;
+
+	rcu_read_lock();
+
+	/*
+	 * Tasks are looked up in caller's PID namespace only.
+	 */
+	task1 = find_task_by_vpid(pid1);
+	task2 = find_task_by_vpid(pid2);
+	if (!task1 || !task2)
+		goto err_no_task;
+
+	get_task_struct(task1);
+	get_task_struct(task2);
+
+	rcu_read_unlock();
+
+	/*
+	 * One should have enough rights to inspect task details.
+	 */
+	if (!ptrace_may_access(task1, PTRACE_MODE_READ) ||
+	    !ptrace_may_access(task2, PTRACE_MODE_READ)) {
+		ret = -EACCES;
+		goto err;
+	}
+
+	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 = -EBADF;
+		break;
+	}
+	case KCMP_VM:
+		ret = kcmp_ptr(task1->mm, task2->mm, KCMP_VM);
+		break;
+	case KCMP_FILES:
+		ret = kcmp_ptr(task1->files, task2->files, KCMP_FILES);
+		break;
+	case KCMP_FS:
+		ret = kcmp_ptr(task1->fs, task2->fs, KCMP_FS);
+		break;
+	case KCMP_SIGHAND:
+		ret = kcmp_ptr(task1->sighand, task2->sighand, KCMP_SIGHAND);
+		break;
+	case KCMP_IO:
+		ret = kcmp_ptr(task1->io_context, task2->io_context, KCMP_IO);
+		break;
+	case KCMP_SYSVSEM:
+#ifdef CONFIG_SYSVIPC
+		ret = kcmp_ptr(task1->sysvsem.undo_list,
+			       task2->sysvsem.undo_list,
+			       KCMP_SYSVSEM);
+#else
+		ret = -EOPNOTSUP;
+#endif
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+err:
+	put_task_struct(task1);
+	put_task_struct(task2);
+
+	return ret;
+
+err_no_task:
+	rcu_read_unlock();
+	return -ESRCH;
+}
+
+static __init int kcmp_cookies_init(void)
+{
+	int i;
+
+	get_random_bytes(cookies, sizeof(cookies));
+
+	for (i = 0; i < KCMP_TYPES; i++)
+		cookies[i][1] |= (~(~0UL >>  1) | 1);
+
+	return 0;
+}
+arch_initcall(kcmp_cookies_init);
Index: linux-2.6.git/kernel/sys_ni.c
===================================================================
--- linux-2.6.git.orig/kernel/sys_ni.c
+++ linux-2.6.git/kernel/sys_ni.c
@@ -203,3 +203,6 @@ cond_syscall(sys_fanotify_mark);
 cond_syscall(sys_name_to_handle_at);
 cond_syscall(sys_open_by_handle_at);
 cond_syscall(compat_sys_open_by_handle_at);
+
+/* compare kernel pointers */
+cond_syscall(sys_kcmp);
Index: linux-2.6.git/tools/testing/selftests/kcmp/Makefile
===================================================================
--- /dev/null
+++ linux-2.6.git/tools/testing/selftests/kcmp/Makefile
@@ -0,0 +1,36 @@
+ifeq ($(strip $(V)),)
+	E = @echo
+	Q = @
+else
+	E = @\#
+	Q =
+endif
+export E Q
+
+uname_M := $(shell uname -m 2>/dev/null || echo not)
+ARCH ?= $(shell echo $(uname_M) | sed -e s/i.86/i386/)
+ifeq ($(ARCH),i386)
+        ARCH := X86
+	CFLAGS := -DCONFIG_X86_32 -D__i386__
+endif
+ifeq ($(ARCH),x86_64)
+	ARCH := X86
+	CFLAGS := -DCONFIG_X86_64 -D__x86_64__
+endif
+
+CFLAGS += -I../../../../arch/x86/include/generated/
+CFLAGS += -I../../../../include/
+CFLAGS += -I../../../../usr/include/
+
+all:
+ifeq ($(ARCH),X86)
+	$(E) "  CC run_test"
+	$(Q) gcc $(CFLAGS) kcmp_test.c -o run_test
+else
+	$(E) "Not an x86 target, can't build kcmp selftest"
+endif
+
+clean:
+	$(E) "  CLEAN"
+	$(Q) rm -fr ./run_test
+	$(Q) rm -fr ./test-file
Index: linux-2.6.git/tools/testing/selftests/kcmp/kcmp_test.c
===================================================================
--- /dev/null
+++ linux-2.6.git/tools/testing/selftests/kcmp/kcmp_test.c
@@ -0,0 +1,84 @@
+#define _GNU_SOURCE
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <signal.h>
+#include <limits.h>
+#include <unistd.h>
+#include <errno.h>
+#include <string.h>
+#include <fcntl.h>
+
+#include <linux/unistd.h>
+#include <linux/kcmp.h>
+
+#include <sys/syscall.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <sys/wait.h>
+
+static long sys_kcmp(int pid1, int pid2, int type, int fd1, int fd2)
+{
+	return syscall(__NR_kcmp, pid1, pid2, type, fd1, fd2);
+}
+
+int main(int argc, char **argv)
+{
+	const char kpath[] = "kcmp-test-file";
+	int pid1, pid2;
+	int fd1, fd2;
+	int status;
+
+	fd1 = open(kpath, O_RDWR | O_CREAT | O_TRUNC, 0644);
+	pid1 = getpid();
+
+	if (fd1 < 0) {
+		perror("Can't create file");
+		exit(1);
+	}
+
+	pid2 = fork();
+	if (pid2 < 0) {
+		perror("fork failed");
+		exit(1);
+	}
+
+	if (!pid2) {
+		int pid2 = getpid();
+		int ret;
+
+		fd2 = open(kpath, O_RDWR, 0644);
+		if (fd2 < 0) {
+			perror("Can't open file");
+			exit(1);
+		}
+
+		/* An example of output and arguments */
+		printf("pid1: %6d pid2: %6d FD: %2d FILES: %2d VM: %2d FS: %2d "
+		       "SIGHAND: %2d IO: %2d SYSVSEM: %2d INV: %2d\n",
+		       pid1, pid2,
+		       sys_kcmp(pid1, pid2, KCMP_FILE,		fd1, fd2),
+		       sys_kcmp(pid1, pid2, KCMP_FILES,		0, 0),
+		       sys_kcmp(pid1, pid2, KCMP_VM,		0, 0),
+		       sys_kcmp(pid1, pid2, KCMP_FS,		0, 0),
+		       sys_kcmp(pid1, pid2, KCMP_SIGHAND,	0, 0),
+		       sys_kcmp(pid1, pid2, KCMP_IO,		0, 0),
+		       sys_kcmp(pid1, pid2, KCMP_SYSVSEM,	0, 0),
+
+			/* This one should fail */
+		       sys_kcmp(pid1, pid2, KCMP_TYPES + 1,	0, 0));
+
+		/* This one should return same fd */
+		ret = sys_kcmp(pid1, pid2, KCMP_FILE, fd1, fd1);
+		if (ret) {
+			printf("FAIL: 0 expected but %d returned\n", ret);
+			ret = -1;
+		} else
+			printf("PASS: 0 returned as expected\n");
+		exit(ret);
+	}
+
+	waitpid(pid2, &status, P_ALL);
+
+	return 0;
+}
Index: linux-2.6.git/tools/testing/selftests/run_tests
===================================================================
--- linux-2.6.git.orig/tools/testing/selftests/run_tests
+++ linux-2.6.git/tools/testing/selftests/run_tests
@@ -1,6 +1,6 @@
 #!/bin/bash
 
-TARGETS=breakpoints
+TARGETS="breakpoints kcmp"
 
 for TARGET in $TARGETS
 do

Re: [PATCH] SubmittingPatches: Increase the line length limit from 80 to 100 colums

[Andi Kleen]

> So lets increase the limit to 100 cols - this is a nice round 
> limit, and it also happens to match with most developer xterm 
> sizes. Code that goes over 100 cols for no good reasons will be 
> arguably something worth fixing. (100 cols is also arguably 
> closer to various brain limits such as vision of field and 
> resolution restrictions, so we'll likely not have to increase 
> this limit for a couple of million years, for all retro human 
> genome users.)

I suppose the future computer to brain interfaces will just
do away with the concept of lines @)

> 
> Signed-off-by: Ingo Molnar <mingo@elte.hu>

Acked-by: Andi Kleen <ak@linux.intel.com>

-Andi

Re: [PATCH] SubmittingPatches: Increase the line length limit from 80 to 100 colums

[Andrew Morton]

On Fri, 3 Feb 2012 11:07:43 +0100
Ingo Molnar <mingo@elte.hu> wrote:

> [PATCH] SubmittingPatches: Increase the line length limit from 80 to 100 colums
> 
> The overwhelming majority of kernel developers have stopped 
> using 80 col terminals years ago.
> 
> As far as I'm aware I was the last regular kernel contributor 
> who still used a standard VGA text console, but both text 
> consoles and using them to read the kernel source code has 
> become increasingly gruesome years ago so I switched to a wider 
> terminal two years ago.

I always use 80-cols, everywhere.  Not because I particularly like it -
I find it a bit too small.  I use it because it is the standard, and
using it helps me see where and how badly we violate the standard.

We've actually done pretty well - linewrap in 80 cols rarely causes me
problems.  It's sufficiently rare that when it *does* happen, it really
stands out.

IOW, the changelog is quite the exaggeration.

> So lets increase the limit to 100 cols

I think that's going too far - 96 will be enough and it's a multiple of 8.

The multiple-of-8 thing seems pleasing but probably doesn't matter
much.  It means that things like


if (foo) {
	if (foo) {
		if (foo) {
			if (foo) {
				if (foo) {
					if (foo) {
						if (foo) {
							if (foo) {
								if (foo) {
									if (foo) {
										if (foo) {
											if (foo) {
												if (foo) {
													if (foo) {
														if (foo) {
												

will line up properly.


If we really want to improve the world we should jump into a time
machine and set tabstops to 4.  Sigh.

Re: [PATCH] SubmittingPatches: Increase the line length limit from 80 to 100 colums

[H. Peter Anvin]

On 02/03/2012 12:57 PM, Andrew Morton wrote:
> 
> If we really want to improve the world we should jump into a time
> machine and set tabstops to 4.  Sigh.
> 

Most editors these days will happy do 4-space stops expanded to 8-space
tab characters.

I have adopted the "Linux kernel formatting with 4-space indentation"
for most of my projects these days.

	-hpa

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

Re: [PATCH] SubmittingPatches: Increase the line length limit from 80 to 100 colums

[H. Peter Anvin]

On 02/03/2012 12:57 PM, Andrew Morton wrote:
> 
> I always use 80-cols, everywhere.  Not because I particularly like it -
> I find it a bit too small.  I use it because it is the standard, and
> using it helps me see where and how badly we violate the standard.
> 

Width matters to me more for printing than it does on the screen.
80-column text can be printed on a landscape piece of paper two columns
wide without being too hard to read (e.g. enscript -2r).  I haven't
tried that with 96 columns, but more than that would almost certainly be
illegible.

	-hpa

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

Re: [PATCH] SubmittingPatches: Increase the line length limit from 80 to 100 colums

[Linus Torvalds]

On Fri, Feb 3, 2012 at 2:07 AM, Ingo Molnar <mingo@elte.hu> wrote:
>
> The overwhelming majority of kernel developers have stopped
> using 80 col terminals years ago.

Quite frankly, I think we should still keep it at 80 columns.

The problem is not the 80 columns, it's that damn patch-check script
that warns about people *occasionally* going over 80 columns.

But usually it's bettre to have the *occasional* 80+ column line, than
try to split it up. So we do have lines that are longer than 80
columns, but that's not because 100 columns is ok - it's because 80+
columns is better than the alternative.

So it's a trade-off. Thinking that there is a hard limit is the
problem. And extending that hard limit (and thinking that it's "ok" to
be over 80 columns) is *also* a problem.

So no, 100-char columns are not ok.

                   Linus

[PATCH] checkpatch: Warn on code with 6+ tab indentation

[Joe Perches]

Overly indented code should be refactored.

Suggest refactoring excessive indentation of
of if/else/for/do/while/switch statements.

For example:

$ cat t.c
#include <stdio.h>
#include <stdlib.h>

int main(int argc, char **argv)
{

	if (1)
		if (2)
			if (3)
				if (4)
					if (5)
						if (6)
							if (7)
								if (8)
									;
	return 0;
}

$ ./scripts/checkpatch.pl -f t.c
WARNING: Too many leading tabs - consider code refactoring
#12: FILE: t.c:12:
+						if (6)

WARNING: Too many leading tabs - consider code refactoring
#13: FILE: t.c:13:
+							if (7)

WARNING: Too many leading tabs - consider code refactoring
#14: FILE: t.c:14:
+								if (8)

total: 0 errors, 3 warnings, 17 lines checked

t.c has style problems, please review.

If any of these errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.

Signed-off-by: Joe Perches <joe@perches.com>
---
On Fri, 2012-02-03 at 13:27 -0800, Linus Torvalds wrote:
> So no, 100-char columns are not ok.

Perhaps this is a reasonable alternative.

Another might be to limit variable name length to something
shorter than say 10 characters.

 scripts/checkpatch.pl |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 2b52aeb..89d24b3 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1924,6 +1924,12 @@ sub process {
 			my $pre_ctx = "$1$2";
 
 			my ($level, @ctx) = ctx_statement_level($linenr, $realcnt, 0);
+
+			if ($line =~ /^\+\t{6,}/) {
+				WARN("DEEP_INDENTATION",
+				     "Too many leading tabs - consider code refactoring\n" . $herecurr);
+			}
+
 			my $ctx_cnt = $realcnt - $#ctx - 1;
 			my $ctx = join("\n", @ctx);
 

Re: [PATCH] SubmittingPatches: Increase the line length limit from 80 to 100 colums

[Randy Dunlap]

On 02/03/2012 01:27 PM, Linus Torvalds wrote:
> On Fri, Feb 3, 2012 at 2:07 AM, Ingo Molnar <mingo@elte.hu> wrote:
>>
>> The overwhelming majority of kernel developers have stopped
>> using 80 col terminals years ago.

There was more interesting text there.  ;)

> Quite frankly, I think we should still keep it at 80 columns.
> 
...

> 
> So no, 100-char columns are not ok.

Thank you.

-- 
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [PATCH] checkpatch: Warn on code with 6+ tab indentation

[Linus Torvalds]

On Fri, Feb 3, 2012 at 3:20 PM, Joe Perches <joe@perches.com> wrote:
> Overly indented code should be refactored.
>
> Suggest refactoring excessive indentation of
> of if/else/for/do/while/switch statements.

I hate this patch.

Why? Because mindless checks like this would just lead to people
making things worse and intermixing spaces there instead.

That's the same reason the 80-character check has been a total
disaster. People shut it up by splitting long strings etc, and we've
had to change that 80-character test many times just to avoid the
crazy workarounds.

Don't warn about things that will just result in people working around
the warnings with worse code!

                 Linus

Re: [PATCH] checkpatch: Warn on code with 6+ tab indentation

[Joe Perches]

On Fri, 2012-02-03 at 17:27 -0800, Linus Torvalds wrote:
> On Fri, Feb 3, 2012 at 3:20 PM, Joe Perches <joe@perches.com> wrote:
> > Overly indented code should be refactored.
> > Suggest refactoring excessive indentation of
> > of if/else/for/do/while/switch statements.
> I hate this patch.

You liked the same premise, but a worse
implementation, a couple years ago.
https://lkml.org/lkml/2009/12/18/289

> Why? Because mindless checks like this would just lead to people
> making things worse and intermixing spaces there instead.

Can't happen.
They'll get a different warning about mixing tabs
and spaces or starting a line with spaces only.

> Don't warn about things that will just result in people working around
> the warnings with worse code!

It merely suggests refactoring. ie: better code.

Re: [PATCH] checkpatch: Warn on code with 6+ tab indentation

[Andrew Morton]

On Fri, 3 Feb 2012 17:27:36 -0800 Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Fri, Feb 3, 2012 at 3:20 PM, Joe Perches <joe@perches.com> wrote:
> > Overly indented code should be refactored.
> >
> > Suggest refactoring excessive indentation of
> > of if/else/for/do/while/switch statements.
> 
> I hate this patch.
> 
> Why? Because mindless checks like this would just lead to people
> making things worse and intermixing spaces there instead.
> 
> That's the same reason the 80-character check has been a total
> disaster. People shut it up by splitting long strings etc, and we've
> had to change that 80-character test many times just to avoid the
> crazy workarounds.
> 
> Don't warn about things that will just result in people working around
> the warnings with worse code!
> 

Sampling bias ;) You notice the 80-col fixups which resulted in
poor-looking code and not the fixups which resulted in decent-looking
code.

Re: [PATCH] checkpatch: Warn on code with 6+ tab indentation

[Eric W. Biederman]

Joe Perches <joe@perches.com> writes:

> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 2b52aeb..89d24b3 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -1924,6 +1924,12 @@ sub process {
>  			my $pre_ctx = "$1$2";
>  
>  			my ($level, @ctx) = ctx_statement_level($linenr, $realcnt, 0);
> +
> +			if ($line =~ /^\+\t{6,}/) {
> +				WARN("DEEP_INDENTATION",
> +				     "Too many leading tabs - consider code refactoring\n" . $herecurr);
> +			}

By any chance have you run this patch against itself? I find it comical
that there is a line 104 characters long suggesting people use shorter
lines.

>  			my $ctx_cnt = $realcnt - $#ctx - 1;
>  			my $ctx = join("\n", @ctx);
>  

Eric

Re: [PATCH] checkpatch: Warn on code with 6+ tab indentation

[Joe Perches]

On Fri, 2012-02-03 at 18:40 -0800, Eric W. Biederman wrote:
> Joe Perches <joe@perches.com> writes:
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> > @@ -1924,6 +1924,12 @@ sub process {
[]
> > +
> > +			if ($line =~ /^\+\t{6,}/) {
> > +				WARN("DEEP_INDENTATION",
> > +				     "Too many leading tabs - consider code refactoring\n" . $herecurr);
> > +			}
> By any chance have you run this patch against itself?

Nope.  perl isn't a c.  Logic doesn't apply.
Some might claim perl logic an oxymoron.

checkpatch should be used for c sources only.

> I find it comical
> that there is a line 104 characters long suggesting people use shorter
> lines.

You're welcome to line wrap it if you like...
I don't actually use checkpatch much.

cheers, Joe

Re: [PATCH] checkpatch: Warn on code with 6+ tab indentation

[Linus Torvalds]

On Fri, Feb 3, 2012 at 5:33 PM, Joe Perches <joe@perches.com> wrote:
>
> You liked the same premise, but a worse
> implementation, a couple years ago.
> https://lkml.org/lkml/2009/12/18/289

I have grown to dislike debatable parts of checkpatch over the years,
which probably colors my reaction to things like this.

I'd prefer for checkpatch to check things that are fairly black-and-white.

>> Why? Because mindless checks like this would just lead to people
>> making things worse and intermixing spaces there instead.
>
> Can't happen.
> They'll get a different warning about mixing tabs
> and spaces or starting a line with spaces only.

Ok,. fair enough. That makes it more likely to work.

That said, I did a grep for six tabs, and we do seem to have quite a
bit of code like that. 67 _thousand_ lines just in C files, if I did
my grep right.

And many of them seem reasonable. Quite a lot of them seem to be to
just line things up with the previous line. Although some really are
due to excessively deep indentation. But it doesn't seem exactly
black-and-white to me.

                       Linus

Re: [PATCH] checkpatch: Warn on code with 6+ tab indentation

[Joe Perches]

On Fri, 2012-02-03 at 19:09 -0800, Linus Torvalds wrote:
> On Fri, Feb 3, 2012 at 5:33 PM, Joe Perches <joe@perches.com> wrote:
> > You liked the same premise, but a worse
> > implementation, a couple years ago.
> > https://lkml.org/lkml/2009/12/18/289
> I have grown to dislike debatable parts of checkpatch over the years,
> which probably colors my reaction to things like this.
> I'd prefer for checkpatch to check things that are fairly black-and-white.
> >> Why? Because mindless checks like this would just lead to people
> >> making things worse and intermixing spaces there instead.
> > Can't happen.
> > They'll get a different warning about mixing tabs
> > and spaces or starting a line with spaces only.
> Ok,. fair enough. That makes it more likely to work.
> That said, I did a grep for six tabs, and we do seem to have quite a
> bit of code like that. 67 _thousand_ lines just in C files, if I did
> my grep right

I think that's a bad test.
It finds a _lot_ of line continuations.

The right test is _only_ for 6 or more tabs
followed by (if|for|while|do|else|switch)

$ grep -rP --include=*.[ch] "^\t{6,}(if|for|while|do|else|switch)" * | \
  wc -l
1509

Re: [PATCH] checkpatch: Warn on code with 6+ tab indentation

[Linus Torvalds]

On Fri, Feb 3, 2012 at 7:21 PM, Joe Perches <joe@perches.com> wrote:
>
> I think that's a bad test.
> It finds a _lot_ of line continuations.
>
> The right test is _only_ for 6 or more tabs
> followed by (if|for|while|do|else|switch)

Fair enough.

And I have to admit that doing your grep with an additional -4 to see
some context does make a fairly strong argument for your patch.

The code in drivers/isdn/hisax/hfc_usb.c that triggers is absolutely
disgusting, and does crazy things due to the long lines.

As is some other code that grep shows.

In fact, I think that grep convinced me that you are right about this
particular pattern. Brr. Now I need to go dig my eyes out with a
spoon.

                    Linus

>
> $ grep -rP --include=*.[ch] "^\t{6,}(if|for|while|do|else|switch)" * | \
>  wc -l
> 1509
>
>

Re: [PATCH] checkpatch: Warn on code with 6+ tab indentation

[Joe Perches]

On Fri, 2012-02-03 at 19:35 -0800, Linus Torvalds wrote:
> I think that grep convinced me that you are right about this
> particular pattern.

Oh good.

> Brr. Now I need to go dig my eyes out with a
> spoon.

Maybe you can keep that thought for your next
Halloween display for the local urchins...

Re: [PATCH] checkpatch: Warn on code with 6+ tab indentation

[Tony Luck]

On Fri, Feb 3, 2012 at 3:20 PM, Joe Perches <joe@perches.com> wrote:
> Another might be to limit variable name length to something
> shorter than say 10 characters.

10? We have a few cases of variables over 100 (not sure how you are supposed
to use them with an 80 character max line length). Current longest is:

eOpenLogicalChannelAck_reverseLogicalChannelParameters_multiplexParameters_h2250LogicalChannelParameters

at 104 characters.

-Tony

Re: [PATCH] checkpatch: Warn on code with 6+ tab indentation

[Joe Perches]

On Fri, 2012-02-03 at 20:45 -0800, Tony Luck wrote:
> On Fri, Feb 3, 2012 at 3:20 PM, Joe Perches <joe@perches.com> wrote:
> > Another might be to limit variable name length to something
> > shorter than say 10 characters.
> 
> 10? We have a few cases of variables over 100 (not sure how you are supposed
> to use them with an 80 character max line length). Current longest is:
> 
> eOpenLogicalChannelAck_reverseLogicalChannelParameters_multiplexParameters_h2250LogicalChannelParameters

EUey.  (err, well maybe ITUey)

Remind me never to look at that code again.

[PATCH, v2] checkpatch: Warn on code with 6+ tab indentation, remove 80col warning

[Ingo Molnar]

* Joe Perches <joe@perches.com> wrote:

> Overly indented code should be refactored.

_AND_ the 80 cols warning should be removed. The overwhelming 
majority of developers either ignore the 80 cols warning or make 
the code worse as a result of the warning.

So something like the patch below.

Thanks,

	Ingo

--------------------->
Subject: checkpatch: Warn on code with 6+ tab indentation, remove 80col warning

It's better to warn about too deeply indented code than about 
too long lines, as the too long line tends to cause people to 
think about *that line*, instead of the surrounding code, fixing 
it by breaking the line unnecessarily, etc.

If we warn about too deep indentation then the fix will be a 
natural one: people will reduce code complexity, which is an 
almost black and white good thing.

The few false positives can be ignored.

Signed-off-by: Ingo Molnar <mingo@elte.hu>

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index e3bfcbe..5406011 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1726,17 +1726,6 @@ sub process {
 # check we are in a valid source file if not then ignore this hunk
 		next if ($realfile !~ /\.(h|c|s|S|pl|sh)$/);
 
-#80 column limit
-		if ($line =~ /^\+/ && $prevrawline !~ /\/\*\*/ &&
-		    $rawline !~ /^.\s*\*\s*\@$Ident\s/ &&
-		    !($line =~ /^\+\s*$logFunctions\s*\(\s*(?:(KERN_\S+\s*|[^"]*))?"[X\t]*"\s*(?:|,|\)\s*;)\s*$/ ||
-		    $line =~ /^\+\s*"[^"]*"\s*(?:\s*|,|\)\s*;)\s*$/) &&
-		    $length > 80)
-		{
-			WARN("LONG_LINE",
-			     "line over 80 characters\n" . $herecurr);
-		}
-
 # check for spaces before a quoted newline
 		if ($rawline =~ /^.*\".*\s\\n/) {
 			WARN("QUOTED_WHITESPACE_BEFORE_NEWLINE",
@@ -1924,6 +1913,12 @@ sub process {
 			my $pre_ctx = "$1$2";
 
 			my ($level, @ctx) = ctx_statement_level($linenr, $realcnt, 0);
+
+			if ($line =~ /^\+\t{6,}/) {
+				WARN("DEEP_INDENTATION",
+				     "Too many leading tabs - code should probably be split up\n" . $herecurr);
+			}
+
 			my $ctx_cnt = $realcnt - $#ctx - 1;
 			my $ctx = join("\n", @ctx);
 

Re: [PATCH] SubmittingPatches: Increase the line length limit from 80 to 100 colums

[Ingo Molnar]

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

> On Fri, 3 Feb 2012 11:07:43 +0100
> Ingo Molnar <mingo@elte.hu> wrote:
> 
> > [PATCH] SubmittingPatches: Increase the line length limit from 80 to 100 colums
> > 
> > The overwhelming majority of kernel developers have stopped 
> > using 80 col terminals years ago.
> > 
> > As far as I'm aware I was the last regular kernel 
> > contributor who still used a standard VGA text console, but 
> > both text consoles and using them to read the kernel source 
> > code has become increasingly gruesome years ago so I 
> > switched to a wider terminal two years ago.
> 
> I always use 80-cols, everywhere.  Not because I particularly 
> like it - I find it a bit too small.  I use it because it is 
> the standard, and using it helps me see where and how badly we 
> violate the standard.
> 
> We've actually done pretty well - linewrap in 80 cols rarely 
> causes me problems.  It's sufficiently rare that when it 
> *does* happen, it really stands out.

Maybe it got better after the introduction of checkpatch - I 
stopped using 80col terminals because the situation *was* 
getting so bad and because i did not feel like fighting a 
thousand other kernel developers who had different preferences 
;-)

> IOW, the changelog is quite the exaggeration.

You are right about that.

> > So lets increase the limit to 100 cols
> 
> I think that's going too far - 96 will be enough and it's a 
> multiple of 8.
> 
> The multiple-of-8 thing seems pleasing but probably doesn't 
> matter much.  It means that things like
> 
> 
> if (foo) {
> 	if (foo) {
> 		if (foo) {
> 			if (foo) {
> 				if (foo) {
> 					if (foo) {
> 						if (foo) {
> 							if (foo) {
> 								if (foo) {
> 									if (foo) {
> 										if (foo) {
> 											if (foo) {
> 												if (foo) {
> 													if (foo) {
> 														if (foo) {
> 												
> 
> will line up properly.
> 
> If we really want to improve the world we should jump into a 
> time machine and set tabstops to 4.  Sigh.

I think that would be a distinctly bad decision - people could 
have crazy, 10 levels nesting in a function and be technically 
'compliant'.

8 col tabs _forces clean code_ more often than not. I know about 
very few functions in the kernel that legitimately need more 
than 3 or 4 levels of nesting.

And that is why I agree with the 6-tab based warning approach - 
then we can remove the 80col warning which is really making 
things actively worse.

Thanks,

	Ingo

Re: [PATCH, v2] checkpatch: Warn on code with 6+ tab indentation, remove 80col warning

[Joe Perches]

On Sat, 2012-02-04 at 14:03 +0100, Ingo Molnar wrote:
> * Joe Perches <joe@perches.com> wrote:
> > Overly indented code should be refactored.
> _AND_ the 80 cols warning should be removed. The overwhelming 
> majority of developers either ignore the 80 cols warning or make 
> the code worse as a result of the warning.

Perhaps, but it should be a separate patch
and you'd still need to update CodingStyle.

Re: [PATCH, v2] checkpatch: Warn on code with 6+ tab indentation, remove 80col warning

[Ingo Molnar]

* Joe Perches <joe@perches.com> wrote:

> On Sat, 2012-02-04 at 14:03 +0100, Ingo Molnar wrote:
> > * Joe Perches <joe@perches.com> wrote:
> > > Overly indented code should be refactored.
> > _AND_ the 80 cols warning should be removed. The overwhelming 
> > majority of developers either ignore the 80 cols warning or make 
> > the code worse as a result of the warning.
> 
> Perhaps, but it should be a separate patch
> and you'd still need to update CodingStyle.

Why would I have to update CodingStyle? The 80col limit remains, 
it's just removed from *checkpatch*.

Thanks,

	Ingo

Re: [PATCH, v2] checkpatch: Warn on code with 6+ tab indentation, remove 80col warning

[Joe Perches]

On Sat, 2012-02-04 at 19:02 +0100, Ingo Molnar wrote:
> * Joe Perches <joe@perches.com> wrote:
> 
> > On Sat, 2012-02-04 at 14:03 +0100, Ingo Molnar wrote:
> > > * Joe Perches <joe@perches.com> wrote:
> > > > Overly indented code should be refactored.
> > > _AND_ the 80 cols warning should be removed. The overwhelming 
> > > majority of developers either ignore the 80 cols warning or make 
> > > the code worse as a result of the warning.
> > 
> > Perhaps, but it should be a separate patch
> > and you'd still need to update CodingStyle.
> 
> Why would I have to update CodingStyle? The 80col limit remains, 
> it's just removed from *checkpatch*.

If that's your intent, I disagree with your patch.

There are some truly _ugly_ > 80 char lines that
people attempt where checkpatch should issue some
"don't do that" message.

Re: [PATCH, v2] checkpatch: Warn on code with 6+ tab indentation, remove 80col warning

[Pekka Enberg]

On Sat, Feb 4, 2012 at 8:48 PM, Joe Perches <joe@perches.com> wrote:
> There are some truly _ugly_ > 80 char lines that
> people attempt where checkpatch should issue some
> "don't do that" message.

The practical downsides of the current warning are much more severe. I
personally don't use checkpatch much these days because of the silly
fixed limit.

Re: [PATCH, v2] checkpatch: Warn on code with 6+ tab indentation, remove 80col warning

[Joe Perches]

On Sat, 2012-02-04 at 20:54 +0200, Pekka Enberg wrote:
> On Sat, Feb 4, 2012 at 8:48 PM, Joe Perches <joe@perches.com> wrote:
> > There are some truly _ugly_ > 80 char lines that
> > people attempt where checkpatch should issue some
> > "don't do that" message.
> 
> The practical downsides of the current warning are much more severe. I
> personally don't use checkpatch much these days because of the silly
> fixed limit.

More likely you developed a keen sense of
kernel style appropriateness and don't need
any checkpatch nannying.

I don't disagree with something like setting
the normally allowed line length to 100 and
maybe keeping a --strict limit to 80.

Something like:

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 2b52aeb..7602bce 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -34,6 +34,9 @@ my @ignore = ();
 my $help = 0;
 my $configuration_file = ".checkpatch.conf";
 
+my $max_line_length_warn = 100;		# normal cases
+my $max_line_length_strict = 80;	# when using --strict
+
 sub help {
 	my ($exitcode) = @_;
 
@@ -1726,15 +1729,19 @@ sub process {
 # check we are in a valid source file if not then ignore this hunk
 		next if ($realfile !~ /\.(h|c|s|S|pl|sh)$/);
 
-#80 column limit
+# check line length limit
 		if ($line =~ /^\+/ && $prevrawline !~ /\/\*\*/ &&
 		    $rawline !~ /^.\s*\*\s*\@$Ident\s/ &&
 		    !($line =~ /^\+\s*$logFunctions\s*\(\s*(?:(KERN_\S+\s*|[^"]*))?"[X\t]*"\s*(?:|,|\)\s*;)\s*$/ ||
-		    $line =~ /^\+\s*"[^"]*"\s*(?:\s*|,|\)\s*;)\s*$/) &&
-		    $length > 80)
-		{
-			WARN("LONG_LINE",
-			     "line over 80 characters\n" . $herecurr);
+		    $line =~ /^\+\s*"[^"]*"\s*(?:\s*|,|\)\s*;)\s*$/) {
+			if ($length > $max_line_len_strict) {
+				CHK("LONG_LINE",
+				     "line over $max_line_len_strict characters\n" . $herecurr);
+			}
+			if ($length > $max_line_len_warn) {
+				WARN("LONG_LINE",
+				     "line over $max_line_len_warn characters\n" . $herecurr);
+			}
 		}
 
 # check for spaces before a quoted newline

Re: [PATCH, v2] checkpatch: Warn on code with 6+ tab indentation, remove 80col warning

[Pekka Enberg]

On Sat, Feb 4, 2012 at 9:27 PM, Joe Perches <joe@perches.com> wrote:
>> The practical downsides of the current warning are much more severe. I
>> personally don't use checkpatch much these days because of the silly
>> fixed limit.
>
> More likely you developed a keen sense of
> kernel style appropriateness and don't need
> any checkpatch nannying.

No, it just means more trivially fixable issues slip through the cracks.

On Sat, Feb 4, 2012 at 9:27 PM, Joe Perches <joe@perches.com> wrote:
> I don't disagree with something like setting
> the normally allowed line length to 100 and
> maybe keeping a --strict limit to 80.
>
> Something like:
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 2b52aeb..7602bce 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -34,6 +34,9 @@ my @ignore = ();
>  my $help = 0;
>  my $configuration_file = ".checkpatch.conf";
>
> +my $max_line_length_warn = 100;                # normal cases
> +my $max_line_length_strict = 80;       # when using --strict
> +
>  sub help {
>        my ($exitcode) = @_;
>
> @@ -1726,15 +1729,19 @@ sub process {
>  # check we are in a valid source file if not then ignore this hunk
>                next if ($realfile !~ /\.(h|c|s|S|pl|sh)$/);
>
> -#80 column limit
> +# check line length limit
>                if ($line =~ /^\+/ && $prevrawline !~ /\/\*\*/ &&
>                    $rawline !~ /^.\s*\*\s*\@$Ident\s/ &&
>                    !($line =~ /^\+\s*$logFunctions\s*\(\s*(?:(KERN_\S+\s*|[^"]*))?"[X\t]*"\s*(?:|,|\)\s*;)\s*$/ ||
> -                   $line =~ /^\+\s*"[^"]*"\s*(?:\s*|,|\)\s*;)\s*$/) &&
> -                   $length > 80)
> -               {
> -                       WARN("LONG_LINE",
> -                            "line over 80 characters\n" . $herecurr);
> +                   $line =~ /^\+\s*"[^"]*"\s*(?:\s*|,|\)\s*;)\s*$/) {
> +                       if ($length > $max_line_len_strict) {
> +                               CHK("LONG_LINE",
> +                                    "line over $max_line_len_strict characters\n" . $herecurr);
> +                       }
> +                       if ($length > $max_line_len_warn) {
> +                               WARN("LONG_LINE",
> +                                    "line over $max_line_len_warn characters\n" . $herecurr);
> +                       }
>                }
>
>  # check for spaces before a quoted newline

I'm fine with something like this too.

Re: [PATCH, v2] checkpatch: Warn on code with 6+ tab indentation, remove 80col warning

[Ingo Molnar]

* Joe Perches <joe@perches.com> wrote:

> +		    $line =~ /^\+\s*"[^"]*"\s*(?:\s*|,|\)\s*;)\s*$/) {
> +			if ($length > $max_line_len_strict) {
> +				CHK("LONG_LINE",
> +				     "line over $max_line_len_strict characters\n" . $herecurr);
> +			}
> +			if ($length > $max_line_len_warn) {
> +				WARN("LONG_LINE",
> +				     "line over $max_line_len_warn characters\n" . $herecurr);
> +			}

In practice patch submitters take warnings just as seriously.
If it is emitted by the default checkpatch run, it's acted
upon. That is the human behavior that is a given.

If warnings are often crap and should not be acted upon then 
frankly, don't emit them by default.

I don't care *how* that warning is removed - whether it's 
removed from checkpatch or just hidden from the default view - 
but it needs to be removed or people like Pekka (or me) stop 
using it.

Thanks,

	Ingo

Re: [PATCH, v2] checkpatch: Warn on code with 6+ tab indentation, remove 80col warning

[Joe Perches]

On Sun, 2012-02-05 at 12:38 +0100, Ingo Molnar wrote:
> In practice patch submitters take warnings just as seriously.

In practice, that's not necessarily bad.
I do think there should be some line length limit.
80 might be a bit short.

> If it is emitted by the default checkpatch run, it's acted
> upon. That is the human behavior that is a given.

Look at some of the lines in drivers/staging
that have _really_ long lines that would not
get any warning if this were removed.

$ git ls-files "drivers/staging/*.[ch]" | xargs cat | \
  awk '{print length($0),$0}' | sort -rn | head -50

gotta love the first few...

A few lines in arch/x86 might benefit from
some wrapping too.

Re: [PATCH, v2] checkpatch: Warn on code with 6+ tab indentation, remove 80col warning

[Ingo Molnar]

* Joe Perches <joe@perches.com> wrote:

> On Sun, 2012-02-05 at 12:38 +0100, Ingo Molnar wrote:
> >
> > In practice patch submitters take warnings just as 
> > seriously.
> 
> In practice, that's not necessarily bad.

In practice it *is* bad, and I say that as a maintainer who is 
receiving many checkpatch 'fixes' on a daily basis. Many 
checkpatch warnings are legitimate - but the col80 one is bogus 
in many cases.

Bogus warnings pollute the output of the tool, reducing the 
utility of the *other* warnings.

( GCC frequently made this mistake in the past, emitting dubious 
  warnings by default - it's been getting somewhat better 
  lately. )

So if your patch emits no warning for the col80 thing then 
that's a step forward in the right direction in my opinion.

Thanks,

	Ingo

[PATCH] checkpatch: Add line-length options, set default to 100

[Joe Perches]

80 column line lengths can be a bit constraining.

Make the default 100 and still emit a warning
when that length is exceeded.

When option --strict is set, emit a check when
the length is > 80 too.

Add a command line option --line-length to set
the standard line length allowed.

Using command line option --ignore=LINE_LENGTH
will not emit any line-length messages.

Adding a .checkpatch.conf file with appropriate
command line options would also avoid line-length
messages.

Signed-off-by: Joe Perches <joe@perches.com>
---
 scripts/checkpatch.pl |   21 +++++++++++++++------
 1 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 2b52aeb..6923270 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -34,6 +34,9 @@ my @ignore = ();
 my $help = 0;
 my $configuration_file = ".checkpatch.conf";
 
+my $max_line_length_warn = 100;		# normal cases
+my $max_line_length_strict = 80;	# when using --strict
+
 sub help {
 	my ($exitcode) = @_;
 
@@ -50,6 +53,7 @@ Options:
   --terse                    one line per report
   -f, --file                 treat FILE as regular source file
   --subjective, --strict     enable more subjective tests
+  --line-length=val          set line length to emit a warning when exceeded
   --ignore TYPE(,TYPE2...)   ignore various comma separated message types
   --show-types               show the message "types" in the output
   --root=PATH                PATH to the kernel tree root
@@ -105,6 +109,7 @@ GetOptions(
 	'f|file!'	=> \$file,
 	'subjective!'	=> \$check,
 	'strict!'	=> \$check,
+	'line-length=i'	=> \$max_line_length_warn,
 	'ignore=s'	=> \@ignore,
 	'show-types!'	=> \$show_types,
 	'root=s'	=> \$root,
@@ -1726,15 +1731,19 @@ sub process {
 # check we are in a valid source file if not then ignore this hunk
 		next if ($realfile !~ /\.(h|c|s|S|pl|sh)$/);
 
-#80 column limit
+# check line length limits
 		if ($line =~ /^\+/ && $prevrawline !~ /\/\*\*/ &&
 		    $rawline !~ /^.\s*\*\s*\@$Ident\s/ &&
 		    !($line =~ /^\+\s*$logFunctions\s*\(\s*(?:(KERN_\S+\s*|[^"]*))?"[X\t]*"\s*(?:|,|\)\s*;)\s*$/ ||
-		    $line =~ /^\+\s*"[^"]*"\s*(?:\s*|,|\)\s*;)\s*$/) &&
-		    $length > 80)
-		{
-			WARN("LONG_LINE",
-			     "line over 80 characters\n" . $herecurr);
+		    $line =~ /^\+\s*"[^"]*"\s*(?:\s*|,|\)\s*;)\s*$/)) {
+			if ($length > $max_line_length_strict) {
+				CHK("LONG_LINE",
+				    "line longer than $max_line_length_strict characters\n" . $herecurr);
+			}
+			if ($length > $max_line_length_warn) {
+				WARN("LONG_LINE",
+				     "line longer than $max_line_length_warn characters\n" . $herecurr);
+			}
 		}
 
 # check for spaces before a quoted newline

Re: [PATCH] checkpatch: Add line-length options, set default to 100

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 287 bytes --]

It doesn't make sense for everyone to pick a different standard.

I do hate the check, but if it were disabled by default when people
used the -f option, that would make me happy.  The -f option is
mostly used by newbies trying to write a My First Kernel Patch.

regards,
dan carpenter


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] SubmittingPatches: Increase the line length limit from 80 to 100 colums

[Jan Engelhardt]

On Friday 2012-02-03 11:07, Ingo Molnar wrote:

>[PATCH] SubmittingPatches: Increase the line length limit from 80 to 100 colums
>
>As far as I'm aware I was the last regular kernel contributor who
>still used a standard VGA text console, but both text (100 cols is
>also arguably closer to various brain limits such as vision of field
>and resolution restrictions

Please, no.

(Corollary 1) The older you get, the larger you like your glyphs to
be (...and the FOV is likely to lower as well).

14pt and up usually. And there is not a whole lot that fit on
10.1-inch netbook screens that way.

(Corollary 2) Travelers desire minimizing size and weight.

-----

As for checkpatch, I am throwing in the suggestion to augment checkpatch 
such that it does not look at whether single lines are over $limit, but 
whether a certain percentage of lines of a file is over $limit. That, 
together with a badness value that is e.g. following some power law to 
the amount of chars too much, but not when the line cannot be broken
in the first place. Maybe along the lines of

 #perlish#
 foreach (<>) {
     /^\s+\S+/;
     if (length($_) > length($&)) {
         push @candidate, $_;
         $badness += (length($_) - length($&)) ** 1.5;
     }
 }
 if ($badness > $threshold) {
     warn about @candidate_lines;
 }

Re: [PATCH] SubmittingPatches: Increase the line length limit from 80 to 100 colums

[Joe Perches]

On Thu, 2012-02-09 at 22:55 +0100, Jan Engelhardt wrote:
> I am throwing in the suggestion to augment checkpatch 
> such that it does not look at whether single lines are over $limit, but 
> whether a certain percentage of lines of a file is over $limit. That, 
> together with a badness value that is e.g. following some power law to 
> the amount of chars too much, but not when the line cannot be broken
> in the first place. Maybe along the lines of
> 
>  #perlish#
>  foreach (<>) {
>      /^\s+\S+/;
>      if (length($_) > length($&)) {
>          push @candidate, $_;
>          $badness += (length($_) - length($&)) ** 1.5;
>      }
>  }
>  if ($badness > $threshold) {
>      warn about @candidate_lines;
>  }

I'd be OK with something like this while making the
current line length check a --strict only option.

Re: [PATCH] SubmittingPatches: Increase the line length limit from 80 to 100 colums

[Mark Brown]

On Thu, Feb 09, 2012 at 10:55:45PM +0100, Jan Engelhardt wrote:
> On Friday 2012-02-03 11:07, Ingo Molnar wrote:
> >[PATCH] SubmittingPatches: Increase the line length limit from 80 to 100 colums

> >As far as I'm aware I was the last regular kernel contributor who
> >still used a standard VGA text console, but both text (100 cols is
> >also arguably closer to various brain limits such as vision of field
> >and resolution restrictions

> Please, no.

> (Corollary 1) The older you get, the larger you like your glyphs to
> be (...and the FOV is likely to lower as well).

> 14pt and up usually. And there is not a whole lot that fit on
> 10.1-inch netbook screens that way.

> (Corollary 2) Travelers desire minimizing size and weight.

Or your vision is a bit better but you like multiple things over the
screen width...