All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 1/3] kcov: support comparison operands collection
@ 2017-10-09 15:05 ` Alexander Potapenko
  0 siblings, 0 replies; 27+ messages in thread
From: Alexander Potapenko @ 2017-10-09 15:05 UTC (permalink / raw)
  To: akpm, mark.rutland, alex.popov, aryabinin, quentin.casasnovas,
	dvyukov, andreyknvl, keescook, vegard.nossum
  Cc: syzkaller, linux-mm, linux-kernel

From: Victor Chibotaru <tchibo@google.com>

Enables kcov to collect comparison operands from instrumented code.
This is done by using Clang's -fsanitize=trace-cmp instrumentation
(currently not available for GCC).

The comparison operands help a lot in fuzz testing. E.g. they are
used in Syzkaller to cover the interiors of conditional statements
with way less attempts and thus make previously unreachable code
reachable.

To allow separate collection of coverage and comparison operands two
different work modes are implemented. Mode selection is now done via
a KCOV_ENABLE ioctl call with corresponding argument value.

Signed-off-by: Victor Chibotaru <tchibo@google.com>
Signed-off-by: Alexander Potapenko <glider@google.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Andrey Konovalov <andreyknvl@google.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Alexander Popov <alex.popov@linux.com>
Cc: Andrey Ryabinin <aryabinin@virtuozzo.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Vegard Nossum <vegard.nossum@oracle.com>
Cc: Quentin Casasnovas <quentin.casasnovas@oracle.com>
Cc: syzkaller@googlegroups.com
Cc: linux-mm@kvack.org
Cc: linux-kernel@vger.kernel.org
---
Clang instrumentation:
https://clang.llvm.org/docs/SanitizerCoverage.html#tracing-data-flow
Syzkaller:
https://github.com/google/syzkaller

v2: - fix wording (as suggested by Mark Rutland)
    - store caller PCs (as suggested by Andrey Konovalov)
---
 include/linux/kcov.h      |  12 ++-
 include/uapi/linux/kcov.h |  32 +++++++
 kernel/kcov.c             | 208 ++++++++++++++++++++++++++++++++++++++--------
 3 files changed, 214 insertions(+), 38 deletions(-)

diff --git a/include/linux/kcov.h b/include/linux/kcov.h
index 2883ac98c280..87e2a44f1bab 100644
--- a/include/linux/kcov.h
+++ b/include/linux/kcov.h
@@ -7,19 +7,23 @@ struct task_struct;
 
 #ifdef CONFIG_KCOV
 
-void kcov_task_init(struct task_struct *t);
-void kcov_task_exit(struct task_struct *t);
-
 enum kcov_mode {
 	/* Coverage collection is not enabled yet. */
 	KCOV_MODE_DISABLED = 0,
+	/* KCOV was initialized, but tracing mode hasn't been chosen yet. */
+	KCOV_MODE_INIT = 1,
 	/*
 	 * Tracing coverage collection mode.
 	 * Covered PCs are collected in a per-task buffer.
 	 */
-	KCOV_MODE_TRACE = 1,
+	KCOV_MODE_TRACE_PC = 2,
+	/* Collecting comparison operands mode. */
+	KCOV_MODE_TRACE_CMP = 3,
 };
 
+void kcov_task_init(struct task_struct *t);
+void kcov_task_exit(struct task_struct *t);
+
 #else
 
 static inline void kcov_task_init(struct task_struct *t) {}
diff --git a/include/uapi/linux/kcov.h b/include/uapi/linux/kcov.h
index 574e22ec640d..a0bc3e6a5ff7 100644
--- a/include/uapi/linux/kcov.h
+++ b/include/uapi/linux/kcov.h
@@ -7,4 +7,36 @@
 #define KCOV_ENABLE			_IO('c', 100)
 #define KCOV_DISABLE			_IO('c', 101)
 
+enum {
+	/*
+	 * Tracing coverage collection mode.
+	 * Covered PCs are collected in a per-task buffer.
+	 * In new KCOV version the mode is chosen by calling
+	 * ioctl(fd, KCOV_ENABLE, mode). In older versions the mode argument
+	 * was supposed to be 0 in such a call. So, for reasons of backward
+	 * compatibility, we have chosen the value KCOV_TRACE_PC to be 0.
+	 */
+	KCOV_TRACE_PC = 0,
+	/* Collecting comparison operands mode. */
+	KCOV_TRACE_CMP = 1,
+};
+
+/*
+ * Defines the format for the types of collected comparisons.
+ */
+enum kcov_cmp_type {
+	/*
+	 * LSB shows whether one of the arguments is a compile-time constant.
+	 */
+	KCOV_CMP_CONST = 1,
+	/*
+	 * Second and third LSBs contain the size of arguments (1/2/4/8 bytes).
+	 */
+	KCOV_CMP_SIZE1 = 0,
+	KCOV_CMP_SIZE2 = 2,
+	KCOV_CMP_SIZE4 = 4,
+	KCOV_CMP_SIZE8 = 6,
+	KCOV_CMP_SIZE_MASK = 6,
+};
+
 #endif /* _LINUX_KCOV_IOCTLS_H */
diff --git a/kernel/kcov.c b/kernel/kcov.c
index 3f693a0f6f3e..7e70488c0261 100644
--- a/kernel/kcov.c
+++ b/kernel/kcov.c
@@ -21,13 +21,21 @@
 #include <linux/kcov.h>
 #include <asm/setup.h>
 
+/* Number of 64-bit words written per one comparison: */
+#define KCOV_WORDS_PER_CMP 4
+
 /*
  * kcov descriptor (one per opened debugfs file).
  * State transitions of the descriptor:
  *  - initial state after open()
  *  - then there must be a single ioctl(KCOV_INIT_TRACE) call
  *  - then, mmap() call (several calls are allowed but not useful)
- *  - then, repeated enable/disable for a task (only one task a time allowed)
+ *  - then, ioctl(KCOV_ENABLE, arg), where arg is
+ *	KCOV_TRACE_PC - to trace only the PCs
+ *	or
+ *	KCOV_TRACE_CMP - to trace only the comparison operands
+ *  - then, ioctl(KCOV_DISABLE) to disable the task.
+ * Enabling/disabling ioctls can be repeated (only one task a time allowed).
  */
 struct kcov {
 	/*
@@ -47,6 +55,30 @@ struct kcov {
 	struct task_struct	*t;
 };
 
+static bool check_kcov_mode(enum kcov_mode needed_mode, struct task_struct *t)
+{
+	enum kcov_mode mode;
+
+	/*
+	 * We are interested in code coverage as a function of a syscall inputs,
+	 * so we ignore code executed in interrupts.
+	 */
+	if (!t || !in_task())
+		return false;
+	mode = READ_ONCE(t->kcov_mode);
+	/*
+	 * There is some code that runs in interrupts but for which
+	 * in_interrupt() returns false (e.g. preempt_schedule_irq()).
+	 * READ_ONCE()/barrier() effectively provides load-acquire wrt
+	 * interrupts, there are paired barrier()/WRITE_ONCE() in
+	 * kcov_ioctl_locked().
+	 */
+	barrier();
+	if (mode != needed_mode)
+		return false;
+	return true;
+}
+
 /*
  * Entry point from instrumented code.
  * This is called once per basic-block/edge.
@@ -54,44 +86,141 @@ struct kcov {
 void notrace __sanitizer_cov_trace_pc(void)
 {
 	struct task_struct *t;
-	enum kcov_mode mode;
+	unsigned long *area;
+	unsigned long ip = _RET_IP_;
+	unsigned long pos;
 
 	t = current;
-	/*
-	 * We are interested in code coverage as a function of a syscall inputs,
-	 * so we ignore code executed in interrupts.
-	 */
-	if (!t || !in_task())
+	if (!check_kcov_mode(KCOV_MODE_TRACE_PC, t))
 		return;
-	mode = READ_ONCE(t->kcov_mode);
-	if (mode == KCOV_MODE_TRACE) {
-		unsigned long *area;
-		unsigned long pos;
-		unsigned long ip = _RET_IP_;
 
 #ifdef CONFIG_RANDOMIZE_BASE
-		ip -= kaslr_offset();
+	ip -= kaslr_offset();
 #endif
 
-		/*
-		 * There is some code that runs in interrupts but for which
-		 * in_interrupt() returns false (e.g. preempt_schedule_irq()).
-		 * READ_ONCE()/barrier() effectively provides load-acquire wrt
-		 * interrupts, there are paired barrier()/WRITE_ONCE() in
-		 * kcov_ioctl_locked().
-		 */
-		barrier();
-		area = t->kcov_area;
-		/* The first word is number of subsequent PCs. */
-		pos = READ_ONCE(area[0]) + 1;
-		if (likely(pos < t->kcov_size)) {
-			area[pos] = ip;
-			WRITE_ONCE(area[0], pos);
-		}
+	area = t->kcov_area;
+	/* The first 64-bit word is the number of subsequent PCs. */
+	pos = READ_ONCE(area[0]) + 1;
+	if (likely(pos < t->kcov_size)) {
+		area[pos] = ip;
+		WRITE_ONCE(area[0], pos);
 	}
 }
 EXPORT_SYMBOL(__sanitizer_cov_trace_pc);
 
+#ifdef CONFIG_KCOV_ENABLE_COMPARISONS
+static void write_comp_data(u64 type, u64 arg1, u64 arg2, u64 ip)
+{
+	struct task_struct *t;
+	u64 *area;
+	u64 count, start_index, end_pos, max_pos;
+
+	t = current;
+	if (!check_kcov_mode(KCOV_MODE_TRACE_CMP, t))
+		return;
+
+#ifdef CONFIG_RANDOMIZE_BASE
+	ip -= kaslr_offset();
+#endif
+
+	/*
+	 * We write all comparison arguments and types as u64.
+	 * The buffer was allocated for t->kcov_size unsigned longs.
+	 */
+	area = (u64 *)t->kcov_area;
+	max_pos = t->kcov_size * sizeof(unsigned long);
+
+	count = READ_ONCE(area[0]);
+
+	/* Every record is KCOV_WORDS_PER_CMP 64-bit words. */
+	start_index = 1 + count * KCOV_WORDS_PER_CMP;
+	end_pos = (start_index + KCOV_WORDS_PER_CMP) * sizeof(u64);
+	if (likely(end_pos <= max_pos)) {
+		area[start_index] = type;
+		area[start_index + 1] = arg1;
+		area[start_index + 2] = arg2;
+		area[start_index + 3] = ip;
+		WRITE_ONCE(area[0], count + 1);
+	}
+}
+
+void notrace __sanitizer_cov_trace_cmp1(u8 arg1, u8 arg2)
+{
+	write_comp_data(KCOV_CMP_SIZE1, arg1, arg2, _RET_IP_);
+}
+EXPORT_SYMBOL(__sanitizer_cov_trace_cmp1);
+
+void notrace __sanitizer_cov_trace_cmp2(u16 arg1, u16 arg2)
+{
+	write_comp_data(KCOV_CMP_SIZE2, arg1, arg2, _RET_IP_);
+}
+EXPORT_SYMBOL(__sanitizer_cov_trace_cmp2);
+
+void notrace __sanitizer_cov_trace_cmp4(u16 arg1, u16 arg2)
+{
+	write_comp_data(KCOV_CMP_SIZE4, arg1, arg2, _RET_IP_);
+}
+EXPORT_SYMBOL(__sanitizer_cov_trace_cmp4);
+
+void notrace __sanitizer_cov_trace_cmp8(u64 arg1, u64 arg2)
+{
+	write_comp_data(KCOV_CMP_SIZE8, arg1, arg2, _RET_IP_);
+}
+EXPORT_SYMBOL(__sanitizer_cov_trace_cmp8);
+
+void notrace __sanitizer_cov_trace_const_cmp1(u8 arg1, u8 arg2)
+{
+	write_comp_data(KCOV_CMP_SIZE1 | KCOV_CMP_CONST, arg1, arg2, _RET_IP_);
+}
+EXPORT_SYMBOL(__sanitizer_cov_trace_const_cmp1);
+
+void notrace __sanitizer_cov_trace_const_cmp2(u16 arg1, u16 arg2)
+{
+	write_comp_data(KCOV_CMP_SIZE2 | KCOV_CMP_CONST, arg1, arg2, _RET_IP_);
+}
+EXPORT_SYMBOL(__sanitizer_cov_trace_const_cmp2);
+
+void notrace __sanitizer_cov_trace_const_cmp4(u16 arg1, u16 arg2)
+{
+	write_comp_data(KCOV_CMP_SIZE4 | KCOV_CMP_CONST, arg1, arg2, _RET_IP_);
+}
+EXPORT_SYMBOL(__sanitizer_cov_trace_const_cmp4);
+
+void notrace __sanitizer_cov_trace_const_cmp8(u64 arg1, u64 arg2)
+{
+	write_comp_data(KCOV_CMP_SIZE8 | KCOV_CMP_CONST, arg1, arg2, _RET_IP_);
+}
+EXPORT_SYMBOL(__sanitizer_cov_trace_const_cmp8);
+
+void notrace __sanitizer_cov_trace_switch(u64 val, u64 *cases)
+{
+	u64 i;
+	u64 count = cases[0];
+	u64 size = cases[1];
+	u64 type = KCOV_CMP_CONST;
+
+	switch (size) {
+	case 8:
+		type |= KCOV_CMP_SIZE1;
+		break;
+	case 16:
+		type |= KCOV_CMP_SIZE2;
+		break;
+	case 32:
+		type |= KCOV_CMP_SIZE4;
+		break;
+	case 64:
+		type |= KCOV_CMP_SIZE8;
+		break;
+	default:
+		return;
+	}
+	for (i = 0; i < count; i++)
+		write_comp_data(type, cases[i + 2], val, _RET_IP_);
+}
+EXPORT_SYMBOL(__sanitizer_cov_trace_switch);
+#endif /* ifdef CONFIG_KCOV_ENABLE_COMPARISONS */
+
 static void kcov_get(struct kcov *kcov)
 {
 	atomic_inc(&kcov->refcount);
@@ -128,6 +257,7 @@ void kcov_task_exit(struct task_struct *t)
 	/* Just to not leave dangling references behind. */
 	kcov_task_init(t);
 	kcov->t = NULL;
+	kcov->mode = KCOV_MODE_INIT;
 	spin_unlock(&kcov->lock);
 	kcov_put(kcov);
 }
@@ -146,7 +276,7 @@ static int kcov_mmap(struct file *filep, struct vm_area_struct *vma)
 
 	spin_lock(&kcov->lock);
 	size = kcov->size * sizeof(unsigned long);
-	if (kcov->mode == KCOV_MODE_DISABLED || vma->vm_pgoff != 0 ||
+	if (kcov->mode != KCOV_MODE_INIT || vma->vm_pgoff != 0 ||
 	    vma->vm_end - vma->vm_start != size) {
 		res = -EINVAL;
 		goto exit;
@@ -175,6 +305,7 @@ static int kcov_open(struct inode *inode, struct file *filep)
 	kcov = kzalloc(sizeof(*kcov), GFP_KERNEL);
 	if (!kcov)
 		return -ENOMEM;
+	kcov->mode = KCOV_MODE_DISABLED;
 	atomic_set(&kcov->refcount, 1);
 	spin_lock_init(&kcov->lock);
 	filep->private_data = kcov;
@@ -210,7 +341,7 @@ static int kcov_ioctl_locked(struct kcov *kcov, unsigned int cmd,
 		if (size < 2 || size > INT_MAX / sizeof(unsigned long))
 			return -EINVAL;
 		kcov->size = size;
-		kcov->mode = KCOV_MODE_TRACE;
+		kcov->mode = KCOV_MODE_INIT;
 		return 0;
 	case KCOV_ENABLE:
 		/*
@@ -220,17 +351,25 @@ static int kcov_ioctl_locked(struct kcov *kcov, unsigned int cmd,
 		 * at task exit or voluntary by KCOV_DISABLE. After that it can
 		 * be enabled for another task.
 		 */
-		unused = arg;
-		if (unused != 0 || kcov->mode == KCOV_MODE_DISABLED ||
-		    kcov->area == NULL)
+		if (kcov->mode != KCOV_MODE_INIT || !kcov->area)
 			return -EINVAL;
 		if (kcov->t != NULL)
 			return -EBUSY;
+		if (arg == KCOV_TRACE_PC)
+			kcov->mode = KCOV_MODE_TRACE_PC;
+		else if (arg == KCOV_TRACE_CMP)
+#ifdef CONFIG_KCOV_ENABLE_COMPARISONS
+			kcov->mode = KCOV_MODE_TRACE_CMP;
+#else
+		return -ENOTSUPP;
+#endif
+		else
+			return -EINVAL;
 		t = current;
 		/* Cache in task struct for performance. */
 		t->kcov_size = kcov->size;
 		t->kcov_area = kcov->area;
-		/* See comment in __sanitizer_cov_trace_pc(). */
+		/* See comment in check_kcov_mode(). */
 		barrier();
 		WRITE_ONCE(t->kcov_mode, kcov->mode);
 		t->kcov = kcov;
@@ -248,6 +387,7 @@ static int kcov_ioctl_locked(struct kcov *kcov, unsigned int cmd,
 			return -EINVAL;
 		kcov_task_init(t);
 		kcov->t = NULL;
+		kcov->mode = KCOV_MODE_INIT;
 		kcov_put(kcov);
 		return 0;
 	default:
-- 
2.14.2.920.gcf0c67979c-goog

^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH v2 1/3] kcov: support comparison operands collection
@ 2017-10-09 15:05 ` Alexander Potapenko
  0 siblings, 0 replies; 27+ messages in thread
From: Alexander Potapenko @ 2017-10-09 15:05 UTC (permalink / raw)
  To: akpm, mark.rutland, alex.popov, aryabinin, quentin.casasnovas,
	dvyukov, andreyknvl, keescook, vegard.nossum
  Cc: syzkaller, linux-mm, linux-kernel

From: Victor Chibotaru <tchibo@google.com>

Enables kcov to collect comparison operands from instrumented code.
This is done by using Clang's -fsanitize=trace-cmp instrumentation
(currently not available for GCC).

The comparison operands help a lot in fuzz testing. E.g. they are
used in Syzkaller to cover the interiors of conditional statements
with way less attempts and thus make previously unreachable code
reachable.

To allow separate collection of coverage and comparison operands two
different work modes are implemented. Mode selection is now done via
a KCOV_ENABLE ioctl call with corresponding argument value.

Signed-off-by: Victor Chibotaru <tchibo@google.com>
Signed-off-by: Alexander Potapenko <glider@google.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Andrey Konovalov <andreyknvl@google.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Alexander Popov <alex.popov@linux.com>
Cc: Andrey Ryabinin <aryabinin@virtuozzo.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Vegard Nossum <vegard.nossum@oracle.com>
Cc: Quentin Casasnovas <quentin.casasnovas@oracle.com>
Cc: syzkaller@googlegroups.com
Cc: linux-mm@kvack.org
Cc: linux-kernel@vger.kernel.org
---
Clang instrumentation:
https://clang.llvm.org/docs/SanitizerCoverage.html#tracing-data-flow
Syzkaller:
https://github.com/google/syzkaller

v2: - fix wording (as suggested by Mark Rutland)
    - store caller PCs (as suggested by Andrey Konovalov)
---
 include/linux/kcov.h      |  12 ++-
 include/uapi/linux/kcov.h |  32 +++++++
 kernel/kcov.c             | 208 ++++++++++++++++++++++++++++++++++++++--------
 3 files changed, 214 insertions(+), 38 deletions(-)

diff --git a/include/linux/kcov.h b/include/linux/kcov.h
index 2883ac98c280..87e2a44f1bab 100644
--- a/include/linux/kcov.h
+++ b/include/linux/kcov.h
@@ -7,19 +7,23 @@ struct task_struct;
 
 #ifdef CONFIG_KCOV
 
-void kcov_task_init(struct task_struct *t);
-void kcov_task_exit(struct task_struct *t);
-
 enum kcov_mode {
 	/* Coverage collection is not enabled yet. */
 	KCOV_MODE_DISABLED = 0,
+	/* KCOV was initialized, but tracing mode hasn't been chosen yet. */
+	KCOV_MODE_INIT = 1,
 	/*
 	 * Tracing coverage collection mode.
 	 * Covered PCs are collected in a per-task buffer.
 	 */
-	KCOV_MODE_TRACE = 1,
+	KCOV_MODE_TRACE_PC = 2,
+	/* Collecting comparison operands mode. */
+	KCOV_MODE_TRACE_CMP = 3,
 };
 
+void kcov_task_init(struct task_struct *t);
+void kcov_task_exit(struct task_struct *t);
+
 #else
 
 static inline void kcov_task_init(struct task_struct *t) {}
diff --git a/include/uapi/linux/kcov.h b/include/uapi/linux/kcov.h
index 574e22ec640d..a0bc3e6a5ff7 100644
--- a/include/uapi/linux/kcov.h
+++ b/include/uapi/linux/kcov.h
@@ -7,4 +7,36 @@
 #define KCOV_ENABLE			_IO('c', 100)
 #define KCOV_DISABLE			_IO('c', 101)
 
+enum {
+	/*
+	 * Tracing coverage collection mode.
+	 * Covered PCs are collected in a per-task buffer.
+	 * In new KCOV version the mode is chosen by calling
+	 * ioctl(fd, KCOV_ENABLE, mode). In older versions the mode argument
+	 * was supposed to be 0 in such a call. So, for reasons of backward
+	 * compatibility, we have chosen the value KCOV_TRACE_PC to be 0.
+	 */
+	KCOV_TRACE_PC = 0,
+	/* Collecting comparison operands mode. */
+	KCOV_TRACE_CMP = 1,
+};
+
+/*
+ * Defines the format for the types of collected comparisons.
+ */
+enum kcov_cmp_type {
+	/*
+	 * LSB shows whether one of the arguments is a compile-time constant.
+	 */
+	KCOV_CMP_CONST = 1,
+	/*
+	 * Second and third LSBs contain the size of arguments (1/2/4/8 bytes).
+	 */
+	KCOV_CMP_SIZE1 = 0,
+	KCOV_CMP_SIZE2 = 2,
+	KCOV_CMP_SIZE4 = 4,
+	KCOV_CMP_SIZE8 = 6,
+	KCOV_CMP_SIZE_MASK = 6,
+};
+
 #endif /* _LINUX_KCOV_IOCTLS_H */
diff --git a/kernel/kcov.c b/kernel/kcov.c
index 3f693a0f6f3e..7e70488c0261 100644
--- a/kernel/kcov.c
+++ b/kernel/kcov.c
@@ -21,13 +21,21 @@
 #include <linux/kcov.h>
 #include <asm/setup.h>
 
+/* Number of 64-bit words written per one comparison: */
+#define KCOV_WORDS_PER_CMP 4
+
 /*
  * kcov descriptor (one per opened debugfs file).
  * State transitions of the descriptor:
  *  - initial state after open()
  *  - then there must be a single ioctl(KCOV_INIT_TRACE) call
  *  - then, mmap() call (several calls are allowed but not useful)
- *  - then, repeated enable/disable for a task (only one task a time allowed)
+ *  - then, ioctl(KCOV_ENABLE, arg), where arg is
+ *	KCOV_TRACE_PC - to trace only the PCs
+ *	or
+ *	KCOV_TRACE_CMP - to trace only the comparison operands
+ *  - then, ioctl(KCOV_DISABLE) to disable the task.
+ * Enabling/disabling ioctls can be repeated (only one task a time allowed).
  */
 struct kcov {
 	/*
@@ -47,6 +55,30 @@ struct kcov {
 	struct task_struct	*t;
 };
 
+static bool check_kcov_mode(enum kcov_mode needed_mode, struct task_struct *t)
+{
+	enum kcov_mode mode;
+
+	/*
+	 * We are interested in code coverage as a function of a syscall inputs,
+	 * so we ignore code executed in interrupts.
+	 */
+	if (!t || !in_task())
+		return false;
+	mode = READ_ONCE(t->kcov_mode);
+	/*
+	 * There is some code that runs in interrupts but for which
+	 * in_interrupt() returns false (e.g. preempt_schedule_irq()).
+	 * READ_ONCE()/barrier() effectively provides load-acquire wrt
+	 * interrupts, there are paired barrier()/WRITE_ONCE() in
+	 * kcov_ioctl_locked().
+	 */
+	barrier();
+	if (mode != needed_mode)
+		return false;
+	return true;
+}
+
 /*
  * Entry point from instrumented code.
  * This is called once per basic-block/edge.
@@ -54,44 +86,141 @@ struct kcov {
 void notrace __sanitizer_cov_trace_pc(void)
 {
 	struct task_struct *t;
-	enum kcov_mode mode;
+	unsigned long *area;
+	unsigned long ip = _RET_IP_;
+	unsigned long pos;
 
 	t = current;
-	/*
-	 * We are interested in code coverage as a function of a syscall inputs,
-	 * so we ignore code executed in interrupts.
-	 */
-	if (!t || !in_task())
+	if (!check_kcov_mode(KCOV_MODE_TRACE_PC, t))
 		return;
-	mode = READ_ONCE(t->kcov_mode);
-	if (mode == KCOV_MODE_TRACE) {
-		unsigned long *area;
-		unsigned long pos;
-		unsigned long ip = _RET_IP_;
 
 #ifdef CONFIG_RANDOMIZE_BASE
-		ip -= kaslr_offset();
+	ip -= kaslr_offset();
 #endif
 
-		/*
-		 * There is some code that runs in interrupts but for which
-		 * in_interrupt() returns false (e.g. preempt_schedule_irq()).
-		 * READ_ONCE()/barrier() effectively provides load-acquire wrt
-		 * interrupts, there are paired barrier()/WRITE_ONCE() in
-		 * kcov_ioctl_locked().
-		 */
-		barrier();
-		area = t->kcov_area;
-		/* The first word is number of subsequent PCs. */
-		pos = READ_ONCE(area[0]) + 1;
-		if (likely(pos < t->kcov_size)) {
-			area[pos] = ip;
-			WRITE_ONCE(area[0], pos);
-		}
+	area = t->kcov_area;
+	/* The first 64-bit word is the number of subsequent PCs. */
+	pos = READ_ONCE(area[0]) + 1;
+	if (likely(pos < t->kcov_size)) {
+		area[pos] = ip;
+		WRITE_ONCE(area[0], pos);
 	}
 }
 EXPORT_SYMBOL(__sanitizer_cov_trace_pc);
 
+#ifdef CONFIG_KCOV_ENABLE_COMPARISONS
+static void write_comp_data(u64 type, u64 arg1, u64 arg2, u64 ip)
+{
+	struct task_struct *t;
+	u64 *area;
+	u64 count, start_index, end_pos, max_pos;
+
+	t = current;
+	if (!check_kcov_mode(KCOV_MODE_TRACE_CMP, t))
+		return;
+
+#ifdef CONFIG_RANDOMIZE_BASE
+	ip -= kaslr_offset();
+#endif
+
+	/*
+	 * We write all comparison arguments and types as u64.
+	 * The buffer was allocated for t->kcov_size unsigned longs.
+	 */
+	area = (u64 *)t->kcov_area;
+	max_pos = t->kcov_size * sizeof(unsigned long);
+
+	count = READ_ONCE(area[0]);
+
+	/* Every record is KCOV_WORDS_PER_CMP 64-bit words. */
+	start_index = 1 + count * KCOV_WORDS_PER_CMP;
+	end_pos = (start_index + KCOV_WORDS_PER_CMP) * sizeof(u64);
+	if (likely(end_pos <= max_pos)) {
+		area[start_index] = type;
+		area[start_index + 1] = arg1;
+		area[start_index + 2] = arg2;
+		area[start_index + 3] = ip;
+		WRITE_ONCE(area[0], count + 1);
+	}
+}
+
+void notrace __sanitizer_cov_trace_cmp1(u8 arg1, u8 arg2)
+{
+	write_comp_data(KCOV_CMP_SIZE1, arg1, arg2, _RET_IP_);
+}
+EXPORT_SYMBOL(__sanitizer_cov_trace_cmp1);
+
+void notrace __sanitizer_cov_trace_cmp2(u16 arg1, u16 arg2)
+{
+	write_comp_data(KCOV_CMP_SIZE2, arg1, arg2, _RET_IP_);
+}
+EXPORT_SYMBOL(__sanitizer_cov_trace_cmp2);
+
+void notrace __sanitizer_cov_trace_cmp4(u16 arg1, u16 arg2)
+{
+	write_comp_data(KCOV_CMP_SIZE4, arg1, arg2, _RET_IP_);
+}
+EXPORT_SYMBOL(__sanitizer_cov_trace_cmp4);
+
+void notrace __sanitizer_cov_trace_cmp8(u64 arg1, u64 arg2)
+{
+	write_comp_data(KCOV_CMP_SIZE8, arg1, arg2, _RET_IP_);
+}
+EXPORT_SYMBOL(__sanitizer_cov_trace_cmp8);
+
+void notrace __sanitizer_cov_trace_const_cmp1(u8 arg1, u8 arg2)
+{
+	write_comp_data(KCOV_CMP_SIZE1 | KCOV_CMP_CONST, arg1, arg2, _RET_IP_);
+}
+EXPORT_SYMBOL(__sanitizer_cov_trace_const_cmp1);
+
+void notrace __sanitizer_cov_trace_const_cmp2(u16 arg1, u16 arg2)
+{
+	write_comp_data(KCOV_CMP_SIZE2 | KCOV_CMP_CONST, arg1, arg2, _RET_IP_);
+}
+EXPORT_SYMBOL(__sanitizer_cov_trace_const_cmp2);
+
+void notrace __sanitizer_cov_trace_const_cmp4(u16 arg1, u16 arg2)
+{
+	write_comp_data(KCOV_CMP_SIZE4 | KCOV_CMP_CONST, arg1, arg2, _RET_IP_);
+}
+EXPORT_SYMBOL(__sanitizer_cov_trace_const_cmp4);
+
+void notrace __sanitizer_cov_trace_const_cmp8(u64 arg1, u64 arg2)
+{
+	write_comp_data(KCOV_CMP_SIZE8 | KCOV_CMP_CONST, arg1, arg2, _RET_IP_);
+}
+EXPORT_SYMBOL(__sanitizer_cov_trace_const_cmp8);
+
+void notrace __sanitizer_cov_trace_switch(u64 val, u64 *cases)
+{
+	u64 i;
+	u64 count = cases[0];
+	u64 size = cases[1];
+	u64 type = KCOV_CMP_CONST;
+
+	switch (size) {
+	case 8:
+		type |= KCOV_CMP_SIZE1;
+		break;
+	case 16:
+		type |= KCOV_CMP_SIZE2;
+		break;
+	case 32:
+		type |= KCOV_CMP_SIZE4;
+		break;
+	case 64:
+		type |= KCOV_CMP_SIZE8;
+		break;
+	default:
+		return;
+	}
+	for (i = 0; i < count; i++)
+		write_comp_data(type, cases[i + 2], val, _RET_IP_);
+}
+EXPORT_SYMBOL(__sanitizer_cov_trace_switch);
+#endif /* ifdef CONFIG_KCOV_ENABLE_COMPARISONS */
+
 static void kcov_get(struct kcov *kcov)
 {
 	atomic_inc(&kcov->refcount);
@@ -128,6 +257,7 @@ void kcov_task_exit(struct task_struct *t)
 	/* Just to not leave dangling references behind. */
 	kcov_task_init(t);
 	kcov->t = NULL;
+	kcov->mode = KCOV_MODE_INIT;
 	spin_unlock(&kcov->lock);
 	kcov_put(kcov);
 }
@@ -146,7 +276,7 @@ static int kcov_mmap(struct file *filep, struct vm_area_struct *vma)
 
 	spin_lock(&kcov->lock);
 	size = kcov->size * sizeof(unsigned long);
-	if (kcov->mode == KCOV_MODE_DISABLED || vma->vm_pgoff != 0 ||
+	if (kcov->mode != KCOV_MODE_INIT || vma->vm_pgoff != 0 ||
 	    vma->vm_end - vma->vm_start != size) {
 		res = -EINVAL;
 		goto exit;
@@ -175,6 +305,7 @@ static int kcov_open(struct inode *inode, struct file *filep)
 	kcov = kzalloc(sizeof(*kcov), GFP_KERNEL);
 	if (!kcov)
 		return -ENOMEM;
+	kcov->mode = KCOV_MODE_DISABLED;
 	atomic_set(&kcov->refcount, 1);
 	spin_lock_init(&kcov->lock);
 	filep->private_data = kcov;
@@ -210,7 +341,7 @@ static int kcov_ioctl_locked(struct kcov *kcov, unsigned int cmd,
 		if (size < 2 || size > INT_MAX / sizeof(unsigned long))
 			return -EINVAL;
 		kcov->size = size;
-		kcov->mode = KCOV_MODE_TRACE;
+		kcov->mode = KCOV_MODE_INIT;
 		return 0;
 	case KCOV_ENABLE:
 		/*
@@ -220,17 +351,25 @@ static int kcov_ioctl_locked(struct kcov *kcov, unsigned int cmd,
 		 * at task exit or voluntary by KCOV_DISABLE. After that it can
 		 * be enabled for another task.
 		 */
-		unused = arg;
-		if (unused != 0 || kcov->mode == KCOV_MODE_DISABLED ||
-		    kcov->area == NULL)
+		if (kcov->mode != KCOV_MODE_INIT || !kcov->area)
 			return -EINVAL;
 		if (kcov->t != NULL)
 			return -EBUSY;
+		if (arg == KCOV_TRACE_PC)
+			kcov->mode = KCOV_MODE_TRACE_PC;
+		else if (arg == KCOV_TRACE_CMP)
+#ifdef CONFIG_KCOV_ENABLE_COMPARISONS
+			kcov->mode = KCOV_MODE_TRACE_CMP;
+#else
+		return -ENOTSUPP;
+#endif
+		else
+			return -EINVAL;
 		t = current;
 		/* Cache in task struct for performance. */
 		t->kcov_size = kcov->size;
 		t->kcov_area = kcov->area;
-		/* See comment in __sanitizer_cov_trace_pc(). */
+		/* See comment in check_kcov_mode(). */
 		barrier();
 		WRITE_ONCE(t->kcov_mode, kcov->mode);
 		t->kcov = kcov;
@@ -248,6 +387,7 @@ static int kcov_ioctl_locked(struct kcov *kcov, unsigned int cmd,
 			return -EINVAL;
 		kcov_task_init(t);
 		kcov->t = NULL;
+		kcov->mode = KCOV_MODE_INIT;
 		kcov_put(kcov);
 		return 0;
 	default:
-- 
2.14.2.920.gcf0c67979c-goog

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH v2 2/3] Makefile: support flag -fsanitizer-coverage=trace-cmp
  2017-10-09 15:05 ` Alexander Potapenko
@ 2017-10-09 15:05   ` Alexander Potapenko
  -1 siblings, 0 replies; 27+ messages in thread
From: Alexander Potapenko @ 2017-10-09 15:05 UTC (permalink / raw)
  To: akpm, mark.rutland, alex.popov, aryabinin, quentin.casasnovas,
	dvyukov, andreyknvl, keescook, vegard.nossum
  Cc: syzkaller, linux-mm, linux-kernel

From: Victor Chibotaru <tchibo@google.com>

The flag enables Clang instrumentation of comparison operations
(currently not supported by GCC). This instrumentation is needed by the
new KCOV device to collect comparison operands.

Signed-off-by: Victor Chibotaru <tchibo@google.com>
Signed-off-by: Alexander Potapenko <glider@google.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Andrey Konovalov <andreyknvl@google.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Alexander Popov <alex.popov@linux.com>
Cc: Andrey Ryabinin <aryabinin@virtuozzo.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Vegard Nossum <vegard.nossum@oracle.com>
Cc: Quentin Casasnovas <quentin.casasnovas@oracle.com>
Cc: syzkaller@googlegroups.com
Cc: linux-mm@kvack.org
Cc: linux-kernel@vger.kernel.org
---
Clang instrumentation:
https://clang.llvm.org/docs/SanitizerCoverage.html#tracing-data-flow

v2: - updated KCOV_ENABLE_COMPARISONS description
---
 Makefile             |  5 +++--
 lib/Kconfig.debug    | 10 ++++++++++
 scripts/Makefile.lib |  6 ++++++
 3 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index 2835863bdd5a..c2a8e56df748 100644
--- a/Makefile
+++ b/Makefile
@@ -374,7 +374,7 @@ AFLAGS_KERNEL	=
 LDFLAGS_vmlinux =
 CFLAGS_GCOV	:= -fprofile-arcs -ftest-coverage -fno-tree-loop-im $(call cc-disable-warning,maybe-uninitialized,)
 CFLAGS_KCOV	:= $(call cc-option,-fsanitize-coverage=trace-pc,)
-
+CFLAGS_KCOV_COMPS := $(call cc-option,-fsanitize-coverage=trace-cmp,)
 
 # Use USERINCLUDE when you must reference the UAPI directories only.
 USERINCLUDE    := \
@@ -420,7 +420,7 @@ export MAKE AWK GENKSYMS INSTALLKERNEL PERL PYTHON UTS_MACHINE
 export HOSTCXX HOSTCXXFLAGS LDFLAGS_MODULE CHECK CHECKFLAGS
 
 export KBUILD_CPPFLAGS NOSTDINC_FLAGS LINUXINCLUDE OBJCOPYFLAGS LDFLAGS
-export KBUILD_CFLAGS CFLAGS_KERNEL CFLAGS_MODULE CFLAGS_GCOV CFLAGS_KCOV CFLAGS_KASAN CFLAGS_UBSAN
+export KBUILD_CFLAGS CFLAGS_KERNEL CFLAGS_MODULE CFLAGS_GCOV CFLAGS_KCOV CFLAGS_KCOV_COMPS CFLAGS_KASAN CFLAGS_UBSAN
 export KBUILD_AFLAGS AFLAGS_KERNEL AFLAGS_MODULE
 export KBUILD_AFLAGS_MODULE KBUILD_CFLAGS_MODULE KBUILD_LDFLAGS_MODULE
 export KBUILD_AFLAGS_KERNEL KBUILD_CFLAGS_KERNEL
@@ -822,6 +822,7 @@ KBUILD_CFLAGS   += $(call cc-option,-Werror=designated-init)
 KBUILD_ARFLAGS := $(call ar-option,D)
 
 include scripts/Makefile.kasan
+include scripts/Makefile.kcov
 include scripts/Makefile.extrawarn
 include scripts/Makefile.ubsan
 
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 2689b7c50c52..a10eb4e34719 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -759,6 +759,16 @@ config KCOV
 
 	  For more details, see Documentation/dev-tools/kcov.rst.
 
+config KCOV_ENABLE_COMPARISONS
+	bool "Enable comparison operands collection by KCOV"
+	depends on KCOV
+	default n
+	help
+	  KCOV also exposes operands of every comparison in the instrumented
+	  code along with operand sizes and PCs of the comparison instructions.
+	  These operands can be used by fuzzing engines to improve the quality
+	  of fuzzing coverage.
+
 config KCOV_INSTRUMENT_ALL
 	bool "Instrument all code by default"
 	depends on KCOV
diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
index 5e975fee0f5b..7ddd5932c832 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -142,6 +142,12 @@ _c_flags += $(if $(patsubst n%,, \
 	$(CFLAGS_KCOV))
 endif
 
+ifeq ($(CONFIG_KCOV_ENABLE_COMPARISONS),y)
+_c_flags += $(if $(patsubst n%,, \
+	$(KCOV_INSTRUMENT_$(basetarget).o)$(KCOV_INSTRUMENT)$(CONFIG_KCOV_INSTRUMENT_ALL)), \
+	$(CFLAGS_KCOV_COMPS))
+endif
+
 # If building the kernel in a separate objtree expand all occurrences
 # of -Idir to -I$(srctree)/dir except for absolute paths (starting with '/').
 
-- 
2.14.2.920.gcf0c67979c-goog

^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH v2 2/3] Makefile: support flag -fsanitizer-coverage=trace-cmp
@ 2017-10-09 15:05   ` Alexander Potapenko
  0 siblings, 0 replies; 27+ messages in thread
From: Alexander Potapenko @ 2017-10-09 15:05 UTC (permalink / raw)
  To: akpm, mark.rutland, alex.popov, aryabinin, quentin.casasnovas,
	dvyukov, andreyknvl, keescook, vegard.nossum
  Cc: syzkaller, linux-mm, linux-kernel

From: Victor Chibotaru <tchibo@google.com>

The flag enables Clang instrumentation of comparison operations
(currently not supported by GCC). This instrumentation is needed by the
new KCOV device to collect comparison operands.

Signed-off-by: Victor Chibotaru <tchibo@google.com>
Signed-off-by: Alexander Potapenko <glider@google.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Andrey Konovalov <andreyknvl@google.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Alexander Popov <alex.popov@linux.com>
Cc: Andrey Ryabinin <aryabinin@virtuozzo.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Vegard Nossum <vegard.nossum@oracle.com>
Cc: Quentin Casasnovas <quentin.casasnovas@oracle.com>
Cc: syzkaller@googlegroups.com
Cc: linux-mm@kvack.org
Cc: linux-kernel@vger.kernel.org
---
Clang instrumentation:
https://clang.llvm.org/docs/SanitizerCoverage.html#tracing-data-flow

v2: - updated KCOV_ENABLE_COMPARISONS description
---
 Makefile             |  5 +++--
 lib/Kconfig.debug    | 10 ++++++++++
 scripts/Makefile.lib |  6 ++++++
 3 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index 2835863bdd5a..c2a8e56df748 100644
--- a/Makefile
+++ b/Makefile
@@ -374,7 +374,7 @@ AFLAGS_KERNEL	=
 LDFLAGS_vmlinux =
 CFLAGS_GCOV	:= -fprofile-arcs -ftest-coverage -fno-tree-loop-im $(call cc-disable-warning,maybe-uninitialized,)
 CFLAGS_KCOV	:= $(call cc-option,-fsanitize-coverage=trace-pc,)
-
+CFLAGS_KCOV_COMPS := $(call cc-option,-fsanitize-coverage=trace-cmp,)
 
 # Use USERINCLUDE when you must reference the UAPI directories only.
 USERINCLUDE    := \
@@ -420,7 +420,7 @@ export MAKE AWK GENKSYMS INSTALLKERNEL PERL PYTHON UTS_MACHINE
 export HOSTCXX HOSTCXXFLAGS LDFLAGS_MODULE CHECK CHECKFLAGS
 
 export KBUILD_CPPFLAGS NOSTDINC_FLAGS LINUXINCLUDE OBJCOPYFLAGS LDFLAGS
-export KBUILD_CFLAGS CFLAGS_KERNEL CFLAGS_MODULE CFLAGS_GCOV CFLAGS_KCOV CFLAGS_KASAN CFLAGS_UBSAN
+export KBUILD_CFLAGS CFLAGS_KERNEL CFLAGS_MODULE CFLAGS_GCOV CFLAGS_KCOV CFLAGS_KCOV_COMPS CFLAGS_KASAN CFLAGS_UBSAN
 export KBUILD_AFLAGS AFLAGS_KERNEL AFLAGS_MODULE
 export KBUILD_AFLAGS_MODULE KBUILD_CFLAGS_MODULE KBUILD_LDFLAGS_MODULE
 export KBUILD_AFLAGS_KERNEL KBUILD_CFLAGS_KERNEL
@@ -822,6 +822,7 @@ KBUILD_CFLAGS   += $(call cc-option,-Werror=designated-init)
 KBUILD_ARFLAGS := $(call ar-option,D)
 
 include scripts/Makefile.kasan
+include scripts/Makefile.kcov
 include scripts/Makefile.extrawarn
 include scripts/Makefile.ubsan
 
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 2689b7c50c52..a10eb4e34719 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -759,6 +759,16 @@ config KCOV
 
 	  For more details, see Documentation/dev-tools/kcov.rst.
 
+config KCOV_ENABLE_COMPARISONS
+	bool "Enable comparison operands collection by KCOV"
+	depends on KCOV
+	default n
+	help
+	  KCOV also exposes operands of every comparison in the instrumented
+	  code along with operand sizes and PCs of the comparison instructions.
+	  These operands can be used by fuzzing engines to improve the quality
+	  of fuzzing coverage.
+
 config KCOV_INSTRUMENT_ALL
 	bool "Instrument all code by default"
 	depends on KCOV
diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
index 5e975fee0f5b..7ddd5932c832 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -142,6 +142,12 @@ _c_flags += $(if $(patsubst n%,, \
 	$(CFLAGS_KCOV))
 endif
 
+ifeq ($(CONFIG_KCOV_ENABLE_COMPARISONS),y)
+_c_flags += $(if $(patsubst n%,, \
+	$(KCOV_INSTRUMENT_$(basetarget).o)$(KCOV_INSTRUMENT)$(CONFIG_KCOV_INSTRUMENT_ALL)), \
+	$(CFLAGS_KCOV_COMPS))
+endif
+
 # If building the kernel in a separate objtree expand all occurrences
 # of -Idir to -I$(srctree)/dir except for absolute paths (starting with '/').
 
-- 
2.14.2.920.gcf0c67979c-goog

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH v2 3/3] kcov: update documentation
  2017-10-09 15:05 ` Alexander Potapenko
@ 2017-10-09 15:05   ` Alexander Potapenko
  -1 siblings, 0 replies; 27+ messages in thread
From: Alexander Potapenko @ 2017-10-09 15:05 UTC (permalink / raw)
  To: akpm, mark.rutland, alex.popov, aryabinin, quentin.casasnovas,
	dvyukov, andreyknvl, keescook, vegard.nossum
  Cc: syzkaller, linux-mm, linux-kernel

From: Victor Chibotaru <tchibo@google.com>

The updated documentation describes new KCOV mode for collecting
comparison operands.

Signed-off-by: Victor Chibotaru <tchibo@google.com>
Signed-off-by: Alexander Potapenko <glider@google.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Andrey Konovalov <andreyknvl@google.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Alexander Popov <alex.popov@linux.com>
Cc: Andrey Ryabinin <aryabinin@virtuozzo.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Vegard Nossum <vegard.nossum@oracle.com>
Cc: Quentin Casasnovas <quentin.casasnovas@oracle.com>
Cc: syzkaller@googlegroups.com
Cc: linux-mm@kvack.org
Cc: linux-kernel@vger.kernel.org
---
v2: - reflect the changes to kcov.c in the test program.
---
 Documentation/dev-tools/kcov.rst | 103 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 99 insertions(+), 4 deletions(-)

diff --git a/Documentation/dev-tools/kcov.rst b/Documentation/dev-tools/kcov.rst
index 44886c91e112..6ee65c6e2448 100644
--- a/Documentation/dev-tools/kcov.rst
+++ b/Documentation/dev-tools/kcov.rst
@@ -12,19 +12,30 @@ To achieve this goal it does not collect coverage in soft/hard interrupts
 and instrumentation of some inherently non-deterministic parts of kernel is
 disabled (e.g. scheduler, locking).
 
-Usage
------
+kcov is also able to collect comparison operands from the instrumented code
+(this feature currently requires that the kernel is compiled with clang).
+
+Prerequisites
+-------------
 
 Configure the kernel with::
 
         CONFIG_KCOV=y
 
 CONFIG_KCOV requires gcc built on revision 231296 or later.
+
+If the comparison operands need to be collected, set::
+
+	CONFIG_KCOV_ENABLE_COMPARISONS=y
+
 Profiling data will only become accessible once debugfs has been mounted::
 
         mount -t debugfs none /sys/kernel/debug
 
-The following program demonstrates kcov usage from within a test program:
+Coverage collection
+-------------------
+The following program demonstrates coverage collection from within a test
+program using kcov:
 
 .. code-block:: c
 
@@ -44,6 +55,9 @@ The following program demonstrates kcov usage from within a test program:
     #define KCOV_DISABLE			_IO('c', 101)
     #define COVER_SIZE			(64<<10)
 
+    #define KCOV_TRACE_PC  0
+    #define KCOV_TRACE_CMP 1
+
     int main(int argc, char **argv)
     {
 	int fd;
@@ -64,7 +78,7 @@ The following program demonstrates kcov usage from within a test program:
 	if ((void*)cover == MAP_FAILED)
 		perror("mmap"), exit(1);
 	/* Enable coverage collection on the current thread. */
-	if (ioctl(fd, KCOV_ENABLE, 0))
+	if (ioctl(fd, KCOV_ENABLE, KCOV_TRACE_PC))
 		perror("ioctl"), exit(1);
 	/* Reset coverage from the tail of the ioctl() call. */
 	__atomic_store_n(&cover[0], 0, __ATOMIC_RELAXED);
@@ -111,3 +125,84 @@ The interface is fine-grained to allow efficient forking of test processes.
 That is, a parent process opens /sys/kernel/debug/kcov, enables trace mode,
 mmaps coverage buffer and then forks child processes in a loop. Child processes
 only need to enable coverage (disable happens automatically on thread end).
+
+Comparison operands collection
+------------------------------
+Comparison operands collection is similar to coverage collection:
+
+.. code-block:: c
+
+    /* Same includes and defines as above. */
+
+     /* Number of 64-bit words per record. */
+     #define KCOV_WORDS_PER_CMP 4
+
+     enum kcov_cmp_type {
+	/*
+	* LSB shows whether the first argument is a compile-time constant.
+	*/
+	KCOV_CMP_CONST = 1,
+	/*
+	* Second and third LSBs contain the size of arguments (1/2/4/8 bytes).
+	*/
+	KCOV_CMP_SIZE1 = 0,
+	KCOV_CMP_SIZE2 = 2,
+	KCOV_CMP_SIZE4 = 4,
+	KCOV_CMP_SIZE8 = 6,
+	KCOV_CMP_SIZE_MASK = 6,
+    };
+
+    int main(int argc, char **argv)
+    {
+	int fd;
+	uint64_t *cover, type, arg1, arg2, is_const, size;
+	unsigned long n, i;
+
+	fd = open("/sys/kernel/debug/kcov", O_RDWR);
+	if (fd == -1)
+		perror("open"), exit(1);
+	if (ioctl(fd, KCOV_INIT_TRACE, COVER_SIZE))
+		perror("ioctl"), exit(1);
+	/*
+	* Note that the buffer pointer is of type uint64_t*, because all
+	* the comparison operands are promoted to uint64_t.
+	*/
+	cover = (uint64_t *)mmap(NULL, COVER_SIZE * sizeof(unsigned long),
+				     PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
+	if ((void*)cover == MAP_FAILED)
+		perror("mmap"), exit(1);
+	/* Note KCOV_TRACE_CMP instead of KCOV_TRACE_PC. */
+	if (ioctl(fd, KCOV_ENABLE, KCOV_TRACE_CMP))
+		perror("ioctl"), exit(1);
+	__atomic_store_n(&cover[0], 0, __ATOMIC_RELAXED);
+	read(-1, NULL, 0);
+	/* Read number of comparisons collected. */
+	n = __atomic_load_n(&cover[0], __ATOMIC_RELAXED);
+	for (i = 0; i < n; i++) {
+		type = cover[i * KCOV_WORDS_PER_CMP + 1];
+		/* arg1 and arg2 - operands of the comparison. */
+		arg1 = cover[i * KCOV_WORDS_PER_CMP + 2];
+		arg2 = cover[i * KCOV_WORDS_PER_CMP + 3];
+		/* ip - caller address. */
+		ip = cover[i * KCOV_WORDS_PER_CMP + 4];
+		/* size == KCOV_CMP_SIZEi. */
+		size = type & KCOV_CMP_SIZE_MASK;
+		/* is_const - shows whether arg1 is a compile-time constant.*/
+		is_const = type & KCOV_CMP_CONST;
+		printf("ip: 0x%lx type: 0x%lx, arg1: 0x%lx, arg2: 0x%lx, "
+			"size: %lu, %s\n",
+			ip, type, arg1, arg2, size,
+		is_const ? "const" : "non-const");
+	}
+	if (ioctl(fd, KCOV_DISABLE, 0))
+		perror("ioctl"), exit(1);
+	/* Free resources. */
+	if (munmap(cover, COVER_SIZE * sizeof(unsigned long)))
+		perror("munmap"), exit(1);
+	if (close(fd))
+		perror("close"), exit(1);
+	return 0;
+    }
+
+Note that the kcov modes (coverage collection or comparison operands) are
+mutually exclusive.
-- 
2.14.2.920.gcf0c67979c-goog

^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH v2 3/3] kcov: update documentation
@ 2017-10-09 15:05   ` Alexander Potapenko
  0 siblings, 0 replies; 27+ messages in thread
From: Alexander Potapenko @ 2017-10-09 15:05 UTC (permalink / raw)
  To: akpm, mark.rutland, alex.popov, aryabinin, quentin.casasnovas,
	dvyukov, andreyknvl, keescook, vegard.nossum
  Cc: syzkaller, linux-mm, linux-kernel

From: Victor Chibotaru <tchibo@google.com>

The updated documentation describes new KCOV mode for collecting
comparison operands.

Signed-off-by: Victor Chibotaru <tchibo@google.com>
Signed-off-by: Alexander Potapenko <glider@google.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Andrey Konovalov <andreyknvl@google.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Alexander Popov <alex.popov@linux.com>
Cc: Andrey Ryabinin <aryabinin@virtuozzo.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Vegard Nossum <vegard.nossum@oracle.com>
Cc: Quentin Casasnovas <quentin.casasnovas@oracle.com>
Cc: syzkaller@googlegroups.com
Cc: linux-mm@kvack.org
Cc: linux-kernel@vger.kernel.org
---
v2: - reflect the changes to kcov.c in the test program.
---
 Documentation/dev-tools/kcov.rst | 103 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 99 insertions(+), 4 deletions(-)

diff --git a/Documentation/dev-tools/kcov.rst b/Documentation/dev-tools/kcov.rst
index 44886c91e112..6ee65c6e2448 100644
--- a/Documentation/dev-tools/kcov.rst
+++ b/Documentation/dev-tools/kcov.rst
@@ -12,19 +12,30 @@ To achieve this goal it does not collect coverage in soft/hard interrupts
 and instrumentation of some inherently non-deterministic parts of kernel is
 disabled (e.g. scheduler, locking).
 
-Usage
------
+kcov is also able to collect comparison operands from the instrumented code
+(this feature currently requires that the kernel is compiled with clang).
+
+Prerequisites
+-------------
 
 Configure the kernel with::
 
         CONFIG_KCOV=y
 
 CONFIG_KCOV requires gcc built on revision 231296 or later.
+
+If the comparison operands need to be collected, set::
+
+	CONFIG_KCOV_ENABLE_COMPARISONS=y
+
 Profiling data will only become accessible once debugfs has been mounted::
 
         mount -t debugfs none /sys/kernel/debug
 
-The following program demonstrates kcov usage from within a test program:
+Coverage collection
+-------------------
+The following program demonstrates coverage collection from within a test
+program using kcov:
 
 .. code-block:: c
 
@@ -44,6 +55,9 @@ The following program demonstrates kcov usage from within a test program:
     #define KCOV_DISABLE			_IO('c', 101)
     #define COVER_SIZE			(64<<10)
 
+    #define KCOV_TRACE_PC  0
+    #define KCOV_TRACE_CMP 1
+
     int main(int argc, char **argv)
     {
 	int fd;
@@ -64,7 +78,7 @@ The following program demonstrates kcov usage from within a test program:
 	if ((void*)cover == MAP_FAILED)
 		perror("mmap"), exit(1);
 	/* Enable coverage collection on the current thread. */
-	if (ioctl(fd, KCOV_ENABLE, 0))
+	if (ioctl(fd, KCOV_ENABLE, KCOV_TRACE_PC))
 		perror("ioctl"), exit(1);
 	/* Reset coverage from the tail of the ioctl() call. */
 	__atomic_store_n(&cover[0], 0, __ATOMIC_RELAXED);
@@ -111,3 +125,84 @@ The interface is fine-grained to allow efficient forking of test processes.
 That is, a parent process opens /sys/kernel/debug/kcov, enables trace mode,
 mmaps coverage buffer and then forks child processes in a loop. Child processes
 only need to enable coverage (disable happens automatically on thread end).
+
+Comparison operands collection
+------------------------------
+Comparison operands collection is similar to coverage collection:
+
+.. code-block:: c
+
+    /* Same includes and defines as above. */
+
+     /* Number of 64-bit words per record. */
+     #define KCOV_WORDS_PER_CMP 4
+
+     enum kcov_cmp_type {
+	/*
+	* LSB shows whether the first argument is a compile-time constant.
+	*/
+	KCOV_CMP_CONST = 1,
+	/*
+	* Second and third LSBs contain the size of arguments (1/2/4/8 bytes).
+	*/
+	KCOV_CMP_SIZE1 = 0,
+	KCOV_CMP_SIZE2 = 2,
+	KCOV_CMP_SIZE4 = 4,
+	KCOV_CMP_SIZE8 = 6,
+	KCOV_CMP_SIZE_MASK = 6,
+    };
+
+    int main(int argc, char **argv)
+    {
+	int fd;
+	uint64_t *cover, type, arg1, arg2, is_const, size;
+	unsigned long n, i;
+
+	fd = open("/sys/kernel/debug/kcov", O_RDWR);
+	if (fd == -1)
+		perror("open"), exit(1);
+	if (ioctl(fd, KCOV_INIT_TRACE, COVER_SIZE))
+		perror("ioctl"), exit(1);
+	/*
+	* Note that the buffer pointer is of type uint64_t*, because all
+	* the comparison operands are promoted to uint64_t.
+	*/
+	cover = (uint64_t *)mmap(NULL, COVER_SIZE * sizeof(unsigned long),
+				     PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
+	if ((void*)cover == MAP_FAILED)
+		perror("mmap"), exit(1);
+	/* Note KCOV_TRACE_CMP instead of KCOV_TRACE_PC. */
+	if (ioctl(fd, KCOV_ENABLE, KCOV_TRACE_CMP))
+		perror("ioctl"), exit(1);
+	__atomic_store_n(&cover[0], 0, __ATOMIC_RELAXED);
+	read(-1, NULL, 0);
+	/* Read number of comparisons collected. */
+	n = __atomic_load_n(&cover[0], __ATOMIC_RELAXED);
+	for (i = 0; i < n; i++) {
+		type = cover[i * KCOV_WORDS_PER_CMP + 1];
+		/* arg1 and arg2 - operands of the comparison. */
+		arg1 = cover[i * KCOV_WORDS_PER_CMP + 2];
+		arg2 = cover[i * KCOV_WORDS_PER_CMP + 3];
+		/* ip - caller address. */
+		ip = cover[i * KCOV_WORDS_PER_CMP + 4];
+		/* size == KCOV_CMP_SIZEi. */
+		size = type & KCOV_CMP_SIZE_MASK;
+		/* is_const - shows whether arg1 is a compile-time constant.*/
+		is_const = type & KCOV_CMP_CONST;
+		printf("ip: 0x%lx type: 0x%lx, arg1: 0x%lx, arg2: 0x%lx, "
+			"size: %lu, %s\n",
+			ip, type, arg1, arg2, size,
+		is_const ? "const" : "non-const");
+	}
+	if (ioctl(fd, KCOV_DISABLE, 0))
+		perror("ioctl"), exit(1);
+	/* Free resources. */
+	if (munmap(cover, COVER_SIZE * sizeof(unsigned long)))
+		perror("munmap"), exit(1);
+	if (close(fd))
+		perror("close"), exit(1);
+	return 0;
+    }
+
+Note that the kcov modes (coverage collection or comparison operands) are
+mutually exclusive.
-- 
2.14.2.920.gcf0c67979c-goog

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply related	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 1/3] kcov: support comparison operands collection
  2017-10-09 15:05 ` Alexander Potapenko
@ 2017-10-09 15:46   ` Mark Rutland
  -1 siblings, 0 replies; 27+ messages in thread
From: Mark Rutland @ 2017-10-09 15:46 UTC (permalink / raw)
  To: Alexander Potapenko
  Cc: akpm, alex.popov, aryabinin, quentin.casasnovas, dvyukov,
	andreyknvl, keescook, vegard.nossum, syzkaller, linux-mm,
	linux-kernel

Hi,

I look forward to using this! :)

I just have afew comments below.

On Mon, Oct 09, 2017 at 05:05:19PM +0200, Alexander Potapenko wrote:
> +/*
> + * Defines the format for the types of collected comparisons.
> + */
> +enum kcov_cmp_type {
> +	/*
> +	 * LSB shows whether one of the arguments is a compile-time constant.
> +	 */
> +	KCOV_CMP_CONST = 1,
> +	/*
> +	 * Second and third LSBs contain the size of arguments (1/2/4/8 bytes).
> +	 */
> +	KCOV_CMP_SIZE1 = 0,
> +	KCOV_CMP_SIZE2 = 2,
> +	KCOV_CMP_SIZE4 = 4,
> +	KCOV_CMP_SIZE8 = 6,
> +	KCOV_CMP_SIZE_MASK = 6,
> +};

Given that LSB is meant to be OR-ed in, (and hence combinations of
values are meaningful) I don't think it makes sense for this to be an
enum. This would clearer as something like:

/*
 * The format for the types of collected comparisons.
 *
 * Bit 0 shows whether one of the arguments is a compile-time constant.
 * Bits 1 & 2 contain log2 of the argument size, up to 8 bytes.
 */
#define	KCOV_CMP_CONST		(1 << 0)
#define KCOV_CMP_SIZE(n)	((n) << 1)
#define KCOV_CMP_MASK		KCOV_CMP_SIZE(3)

... I note that a few places in the kernel use a 128-bit type. Are
128-bit comparisons not instrumented?

[...]

> +static bool check_kcov_mode(enum kcov_mode needed_mode, struct task_struct *t)
> +{
> +	enum kcov_mode mode;
> +
> +	/*
> +	 * We are interested in code coverage as a function of a syscall inputs,
> +	 * so we ignore code executed in interrupts.
> +	 */
> +	if (!t || !in_task())
> +		return false;

This !t check can go, as with the one in __sanitizer_cov_trace_pc, since
t is always current, and therefore cannot be NULL.

IIRC there's a patch queued for that, which this may conflict with.

> +	mode = READ_ONCE(t->kcov_mode);
> +	/*
> +	 * There is some code that runs in interrupts but for which
> +	 * in_interrupt() returns false (e.g. preempt_schedule_irq()).
> +	 * READ_ONCE()/barrier() effectively provides load-acquire wrt
> +	 * interrupts, there are paired barrier()/WRITE_ONCE() in
> +	 * kcov_ioctl_locked().
> +	 */
> +	barrier();
> +	if (mode != needed_mode)
> +		return false;
> +	return true;

This would be simpler as:
	
	return mode == needed_mode;

[...]

> +	area = t->kcov_area;
> +	/* The first 64-bit word is the number of subsequent PCs. */
> +	pos = READ_ONCE(area[0]) + 1;
> +	if (likely(pos < t->kcov_size)) {
> +		area[pos] = ip;
> +		WRITE_ONCE(area[0], pos);

Not a new problem, but if the area for one thread is mmap'd, and read by
another thread, these two writes could be seen out-of-order, since we
don't have an smp_wmb() between them.

I guess Syzkaller doesn't read the mmap'd kcov file from another thread?

>  	}
>  }
>  EXPORT_SYMBOL(__sanitizer_cov_trace_pc);
>  
> +#ifdef CONFIG_KCOV_ENABLE_COMPARISONS
> +static void write_comp_data(u64 type, u64 arg1, u64 arg2, u64 ip)
> +{
> +	struct task_struct *t;
> +	u64 *area;
> +	u64 count, start_index, end_pos, max_pos;
> +
> +	t = current;
> +	if (!check_kcov_mode(KCOV_MODE_TRACE_CMP, t))
> +		return;
> +
> +#ifdef CONFIG_RANDOMIZE_BASE
> +	ip -= kaslr_offset();
> +#endif

Given we have this in two places, it might make sense to have a helper
like:

unsigned long canonicalize_ip(unsigned long ip)
{
#ifdef CONFIG_RANDOMIZE_BASE
	ip -= kaslr_offset();
#endif
	return ip;
}

... to minimize the ifdeffery elsewhere.

> +
> +	/*
> +	 * We write all comparison arguments and types as u64.
> +	 * The buffer was allocated for t->kcov_size unsigned longs.
> +	 */
> +	area = (u64 *)t->kcov_area;
> +	max_pos = t->kcov_size * sizeof(unsigned long);
> +
> +	count = READ_ONCE(area[0]);
> +
> +	/* Every record is KCOV_WORDS_PER_CMP 64-bit words. */
> +	start_index = 1 + count * KCOV_WORDS_PER_CMP;
> +	end_pos = (start_index + KCOV_WORDS_PER_CMP) * sizeof(u64);
> +	if (likely(end_pos <= max_pos)) {
> +		area[start_index] = type;
> +		area[start_index + 1] = arg1;
> +		area[start_index + 2] = arg2;
> +		area[start_index + 3] = ip;
> +		WRITE_ONCE(area[0], count + 1);

That ordering problem applies here, too.

Thanks,
Mark.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 1/3] kcov: support comparison operands collection
@ 2017-10-09 15:46   ` Mark Rutland
  0 siblings, 0 replies; 27+ messages in thread
From: Mark Rutland @ 2017-10-09 15:46 UTC (permalink / raw)
  To: Alexander Potapenko
  Cc: akpm, alex.popov, aryabinin, quentin.casasnovas, dvyukov,
	andreyknvl, keescook, vegard.nossum, syzkaller, linux-mm,
	linux-kernel

Hi,

I look forward to using this! :)

I just have afew comments below.

On Mon, Oct 09, 2017 at 05:05:19PM +0200, Alexander Potapenko wrote:
> +/*
> + * Defines the format for the types of collected comparisons.
> + */
> +enum kcov_cmp_type {
> +	/*
> +	 * LSB shows whether one of the arguments is a compile-time constant.
> +	 */
> +	KCOV_CMP_CONST = 1,
> +	/*
> +	 * Second and third LSBs contain the size of arguments (1/2/4/8 bytes).
> +	 */
> +	KCOV_CMP_SIZE1 = 0,
> +	KCOV_CMP_SIZE2 = 2,
> +	KCOV_CMP_SIZE4 = 4,
> +	KCOV_CMP_SIZE8 = 6,
> +	KCOV_CMP_SIZE_MASK = 6,
> +};

Given that LSB is meant to be OR-ed in, (and hence combinations of
values are meaningful) I don't think it makes sense for this to be an
enum. This would clearer as something like:

/*
 * The format for the types of collected comparisons.
 *
 * Bit 0 shows whether one of the arguments is a compile-time constant.
 * Bits 1 & 2 contain log2 of the argument size, up to 8 bytes.
 */
#define	KCOV_CMP_CONST		(1 << 0)
#define KCOV_CMP_SIZE(n)	((n) << 1)
#define KCOV_CMP_MASK		KCOV_CMP_SIZE(3)

... I note that a few places in the kernel use a 128-bit type. Are
128-bit comparisons not instrumented?

[...]

> +static bool check_kcov_mode(enum kcov_mode needed_mode, struct task_struct *t)
> +{
> +	enum kcov_mode mode;
> +
> +	/*
> +	 * We are interested in code coverage as a function of a syscall inputs,
> +	 * so we ignore code executed in interrupts.
> +	 */
> +	if (!t || !in_task())
> +		return false;

This !t check can go, as with the one in __sanitizer_cov_trace_pc, since
t is always current, and therefore cannot be NULL.

IIRC there's a patch queued for that, which this may conflict with.

> +	mode = READ_ONCE(t->kcov_mode);
> +	/*
> +	 * There is some code that runs in interrupts but for which
> +	 * in_interrupt() returns false (e.g. preempt_schedule_irq()).
> +	 * READ_ONCE()/barrier() effectively provides load-acquire wrt
> +	 * interrupts, there are paired barrier()/WRITE_ONCE() in
> +	 * kcov_ioctl_locked().
> +	 */
> +	barrier();
> +	if (mode != needed_mode)
> +		return false;
> +	return true;

This would be simpler as:
	
	return mode == needed_mode;

[...]

> +	area = t->kcov_area;
> +	/* The first 64-bit word is the number of subsequent PCs. */
> +	pos = READ_ONCE(area[0]) + 1;
> +	if (likely(pos < t->kcov_size)) {
> +		area[pos] = ip;
> +		WRITE_ONCE(area[0], pos);

Not a new problem, but if the area for one thread is mmap'd, and read by
another thread, these two writes could be seen out-of-order, since we
don't have an smp_wmb() between them.

I guess Syzkaller doesn't read the mmap'd kcov file from another thread?

>  	}
>  }
>  EXPORT_SYMBOL(__sanitizer_cov_trace_pc);
>  
> +#ifdef CONFIG_KCOV_ENABLE_COMPARISONS
> +static void write_comp_data(u64 type, u64 arg1, u64 arg2, u64 ip)
> +{
> +	struct task_struct *t;
> +	u64 *area;
> +	u64 count, start_index, end_pos, max_pos;
> +
> +	t = current;
> +	if (!check_kcov_mode(KCOV_MODE_TRACE_CMP, t))
> +		return;
> +
> +#ifdef CONFIG_RANDOMIZE_BASE
> +	ip -= kaslr_offset();
> +#endif

Given we have this in two places, it might make sense to have a helper
like:

unsigned long canonicalize_ip(unsigned long ip)
{
#ifdef CONFIG_RANDOMIZE_BASE
	ip -= kaslr_offset();
#endif
	return ip;
}

... to minimize the ifdeffery elsewhere.

> +
> +	/*
> +	 * We write all comparison arguments and types as u64.
> +	 * The buffer was allocated for t->kcov_size unsigned longs.
> +	 */
> +	area = (u64 *)t->kcov_area;
> +	max_pos = t->kcov_size * sizeof(unsigned long);
> +
> +	count = READ_ONCE(area[0]);
> +
> +	/* Every record is KCOV_WORDS_PER_CMP 64-bit words. */
> +	start_index = 1 + count * KCOV_WORDS_PER_CMP;
> +	end_pos = (start_index + KCOV_WORDS_PER_CMP) * sizeof(u64);
> +	if (likely(end_pos <= max_pos)) {
> +		area[start_index] = type;
> +		area[start_index + 1] = arg1;
> +		area[start_index + 2] = arg2;
> +		area[start_index + 3] = ip;
> +		WRITE_ONCE(area[0], count + 1);

That ordering problem applies here, too.

Thanks,
Mark.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 2/3] Makefile: support flag -fsanitizer-coverage=trace-cmp
  2017-10-09 15:05   ` Alexander Potapenko
@ 2017-10-09 15:53     ` Andrey Ryabinin
  -1 siblings, 0 replies; 27+ messages in thread
From: Andrey Ryabinin @ 2017-10-09 15:53 UTC (permalink / raw)
  To: Alexander Potapenko, akpm, mark.rutland, alex.popov,
	quentin.casasnovas, dvyukov, andreyknvl, keescook, vegard.nossum
  Cc: syzkaller, linux-mm, linux-kernel



On 10/09/2017 06:05 PM, Alexander Potapenko wrote:

> v2: - updated KCOV_ENABLE_COMPARISONS description
> ---
>  Makefile             |  5 +++--
>  lib/Kconfig.debug    | 10 ++++++++++
>  scripts/Makefile.lib |  6 ++++++
>  3 files changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 2835863bdd5a..c2a8e56df748 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -374,7 +374,7 @@ AFLAGS_KERNEL	=
>  LDFLAGS_vmlinux =
>  CFLAGS_GCOV	:= -fprofile-arcs -ftest-coverage -fno-tree-loop-im $(call cc-disable-warning,maybe-uninitialized,)
>  CFLAGS_KCOV	:= $(call cc-option,-fsanitize-coverage=trace-pc,)
> -
> +CFLAGS_KCOV_COMPS := $(call cc-option,-fsanitize-coverage=trace-cmp,)
>  
>  # Use USERINCLUDE when you must reference the UAPI directories only.
>  USERINCLUDE    := \
> @@ -420,7 +420,7 @@ export MAKE AWK GENKSYMS INSTALLKERNEL PERL PYTHON UTS_MACHINE
>  export HOSTCXX HOSTCXXFLAGS LDFLAGS_MODULE CHECK CHECKFLAGS
>  
>  export KBUILD_CPPFLAGS NOSTDINC_FLAGS LINUXINCLUDE OBJCOPYFLAGS LDFLAGS
> -export KBUILD_CFLAGS CFLAGS_KERNEL CFLAGS_MODULE CFLAGS_GCOV CFLAGS_KCOV CFLAGS_KASAN CFLAGS_UBSAN
> +export KBUILD_CFLAGS CFLAGS_KERNEL CFLAGS_MODULE CFLAGS_GCOV CFLAGS_KCOV CFLAGS_KCOV_COMPS CFLAGS_KASAN CFLAGS_UBSAN
>  export KBUILD_AFLAGS AFLAGS_KERNEL AFLAGS_MODULE
>  export KBUILD_AFLAGS_MODULE KBUILD_CFLAGS_MODULE KBUILD_LDFLAGS_MODULE
>  export KBUILD_AFLAGS_KERNEL KBUILD_CFLAGS_KERNEL
> @@ -822,6 +822,7 @@ KBUILD_CFLAGS   += $(call cc-option,-Werror=designated-init)
>  KBUILD_ARFLAGS := $(call ar-option,D)
>  
>  include scripts/Makefile.kasan
> +include scripts/Makefile.kcov

scripts/Makefile.kcov doesn't exist.



> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> index 5e975fee0f5b..7ddd5932c832 100644
> --- a/scripts/Makefile.lib
> +++ b/scripts/Makefile.lib
> @@ -142,6 +142,12 @@ _c_flags += $(if $(patsubst n%,, \
>  	$(CFLAGS_KCOV))
>  endif
>  
> +ifeq ($(CONFIG_KCOV_ENABLE_COMPARISONS),y)
> +_c_flags += $(if $(patsubst n%,, \
> +	$(KCOV_INSTRUMENT_$(basetarget).o)$(KCOV_INSTRUMENT)$(CONFIG_KCOV_INSTRUMENT_ALL)), \
> +	$(CFLAGS_KCOV_COMPS))
> +endif
> +

Instead of this you could simply add -fsanitize-coverage=trace-cmp to CFLAGS_KCOV.


>  # If building the kernel in a separate objtree expand all occurrences
>  # of -Idir to -I$(srctree)/dir except for absolute paths (starting with '/').
>  
> 

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 2/3] Makefile: support flag -fsanitizer-coverage=trace-cmp
@ 2017-10-09 15:53     ` Andrey Ryabinin
  0 siblings, 0 replies; 27+ messages in thread
From: Andrey Ryabinin @ 2017-10-09 15:53 UTC (permalink / raw)
  To: Alexander Potapenko, akpm, mark.rutland, alex.popov,
	quentin.casasnovas, dvyukov, andreyknvl, keescook, vegard.nossum
  Cc: syzkaller, linux-mm, linux-kernel



On 10/09/2017 06:05 PM, Alexander Potapenko wrote:

> v2: - updated KCOV_ENABLE_COMPARISONS description
> ---
>  Makefile             |  5 +++--
>  lib/Kconfig.debug    | 10 ++++++++++
>  scripts/Makefile.lib |  6 ++++++
>  3 files changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 2835863bdd5a..c2a8e56df748 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -374,7 +374,7 @@ AFLAGS_KERNEL	=
>  LDFLAGS_vmlinux =
>  CFLAGS_GCOV	:= -fprofile-arcs -ftest-coverage -fno-tree-loop-im $(call cc-disable-warning,maybe-uninitialized,)
>  CFLAGS_KCOV	:= $(call cc-option,-fsanitize-coverage=trace-pc,)
> -
> +CFLAGS_KCOV_COMPS := $(call cc-option,-fsanitize-coverage=trace-cmp,)
>  
>  # Use USERINCLUDE when you must reference the UAPI directories only.
>  USERINCLUDE    := \
> @@ -420,7 +420,7 @@ export MAKE AWK GENKSYMS INSTALLKERNEL PERL PYTHON UTS_MACHINE
>  export HOSTCXX HOSTCXXFLAGS LDFLAGS_MODULE CHECK CHECKFLAGS
>  
>  export KBUILD_CPPFLAGS NOSTDINC_FLAGS LINUXINCLUDE OBJCOPYFLAGS LDFLAGS
> -export KBUILD_CFLAGS CFLAGS_KERNEL CFLAGS_MODULE CFLAGS_GCOV CFLAGS_KCOV CFLAGS_KASAN CFLAGS_UBSAN
> +export KBUILD_CFLAGS CFLAGS_KERNEL CFLAGS_MODULE CFLAGS_GCOV CFLAGS_KCOV CFLAGS_KCOV_COMPS CFLAGS_KASAN CFLAGS_UBSAN
>  export KBUILD_AFLAGS AFLAGS_KERNEL AFLAGS_MODULE
>  export KBUILD_AFLAGS_MODULE KBUILD_CFLAGS_MODULE KBUILD_LDFLAGS_MODULE
>  export KBUILD_AFLAGS_KERNEL KBUILD_CFLAGS_KERNEL
> @@ -822,6 +822,7 @@ KBUILD_CFLAGS   += $(call cc-option,-Werror=designated-init)
>  KBUILD_ARFLAGS := $(call ar-option,D)
>  
>  include scripts/Makefile.kasan
> +include scripts/Makefile.kcov

scripts/Makefile.kcov doesn't exist.



> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> index 5e975fee0f5b..7ddd5932c832 100644
> --- a/scripts/Makefile.lib
> +++ b/scripts/Makefile.lib
> @@ -142,6 +142,12 @@ _c_flags += $(if $(patsubst n%,, \
>  	$(CFLAGS_KCOV))
>  endif
>  
> +ifeq ($(CONFIG_KCOV_ENABLE_COMPARISONS),y)
> +_c_flags += $(if $(patsubst n%,, \
> +	$(KCOV_INSTRUMENT_$(basetarget).o)$(KCOV_INSTRUMENT)$(CONFIG_KCOV_INSTRUMENT_ALL)), \
> +	$(CFLAGS_KCOV_COMPS))
> +endif
> +

Instead of this you could simply add -fsanitize-coverage=trace-cmp to CFLAGS_KCOV.


>  # If building the kernel in a separate objtree expand all occurrences
>  # of -Idir to -I$(srctree)/dir except for absolute paths (starting with '/').
>  
> 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 1/3] kcov: support comparison operands collection
  2017-10-09 15:46   ` Mark Rutland
@ 2017-10-09 18:15     ` Dmitry Vyukov
  -1 siblings, 0 replies; 27+ messages in thread
From: Dmitry Vyukov @ 2017-10-09 18:15 UTC (permalink / raw)
  To: Mark Rutland
  Cc: Alexander Potapenko, Andrew Morton, Alexander Popov,
	Andrey Ryabinin, Quentin Casasnovas, andreyknvl, Kees Cook,
	Vegard Nossum, syzkaller, linux-mm, LKML

On Mon, Oct 9, 2017 at 5:46 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> Hi,
>
> I look forward to using this! :)
>
> I just have afew comments below.
>
> On Mon, Oct 09, 2017 at 05:05:19PM +0200, Alexander Potapenko wrote:
>> +/*
>> + * Defines the format for the types of collected comparisons.
>> + */
>> +enum kcov_cmp_type {
>> +     /*
>> +      * LSB shows whether one of the arguments is a compile-time constant.
>> +      */
>> +     KCOV_CMP_CONST = 1,
>> +     /*
>> +      * Second and third LSBs contain the size of arguments (1/2/4/8 bytes).
>> +      */
>> +     KCOV_CMP_SIZE1 = 0,
>> +     KCOV_CMP_SIZE2 = 2,
>> +     KCOV_CMP_SIZE4 = 4,
>> +     KCOV_CMP_SIZE8 = 6,
>> +     KCOV_CMP_SIZE_MASK = 6,
>> +};
>
> Given that LSB is meant to be OR-ed in, (and hence combinations of
> values are meaningful) I don't think it makes sense for this to be an
> enum. This would clearer as something like:
>
> /*
>  * The format for the types of collected comparisons.
>  *
>  * Bit 0 shows whether one of the arguments is a compile-time constant.
>  * Bits 1 & 2 contain log2 of the argument size, up to 8 bytes.
>  */
> #define KCOV_CMP_CONST          (1 << 0)
> #define KCOV_CMP_SIZE(n)        ((n) << 1)
> #define KCOV_CMP_MASK           KCOV_CMP_SIZE(3)
>
> ... I note that a few places in the kernel use a 128-bit type. Are
> 128-bit comparisons not instrumented?

Yes, they are not instrumented.
How many are there? Can you give some examples?



>> +static bool check_kcov_mode(enum kcov_mode needed_mode, struct task_struct *t)
>> +{
>> +     enum kcov_mode mode;
>> +
>> +     /*
>> +      * We are interested in code coverage as a function of a syscall inputs,
>> +      * so we ignore code executed in interrupts.
>> +      */
>> +     if (!t || !in_task())
>> +             return false;
>
> This !t check can go, as with the one in __sanitizer_cov_trace_pc, since
> t is always current, and therefore cannot be NULL.
>
> IIRC there's a patch queued for that, which this may conflict with.
>
>> +     mode = READ_ONCE(t->kcov_mode);
>> +     /*
>> +      * There is some code that runs in interrupts but for which
>> +      * in_interrupt() returns false (e.g. preempt_schedule_irq()).
>> +      * READ_ONCE()/barrier() effectively provides load-acquire wrt
>> +      * interrupts, there are paired barrier()/WRITE_ONCE() in
>> +      * kcov_ioctl_locked().
>> +      */
>> +     barrier();
>> +     if (mode != needed_mode)
>> +             return false;
>> +     return true;
>
> This would be simpler as:
>
>         return mode == needed_mode;
>
> [...]
>
>> +     area = t->kcov_area;
>> +     /* The first 64-bit word is the number of subsequent PCs. */
>> +     pos = READ_ONCE(area[0]) + 1;
>> +     if (likely(pos < t->kcov_size)) {
>> +             area[pos] = ip;
>> +             WRITE_ONCE(area[0], pos);
>
> Not a new problem, but if the area for one thread is mmap'd, and read by
> another thread, these two writes could be seen out-of-order, since we
> don't have an smp_wmb() between them.
>
> I guess Syzkaller doesn't read the mmap'd kcov file from another thread?


Yes, that's the intention. If you read coverage from another thread,
you can't know coverage from what exactly you read. So the usage
pattern is:

reset coverage;
do something;
read coverage;



>
>>       }
>>  }
>>  EXPORT_SYMBOL(__sanitizer_cov_trace_pc);
>>
>> +#ifdef CONFIG_KCOV_ENABLE_COMPARISONS
>> +static void write_comp_data(u64 type, u64 arg1, u64 arg2, u64 ip)
>> +{
>> +     struct task_struct *t;
>> +     u64 *area;
>> +     u64 count, start_index, end_pos, max_pos;
>> +
>> +     t = current;
>> +     if (!check_kcov_mode(KCOV_MODE_TRACE_CMP, t))
>> +             return;
>> +
>> +#ifdef CONFIG_RANDOMIZE_BASE
>> +     ip -= kaslr_offset();
>> +#endif
>
> Given we have this in two places, it might make sense to have a helper
> like:
>
> unsigned long canonicalize_ip(unsigned long ip)
> {
> #ifdef CONFIG_RANDOMIZE_BASE
>         ip -= kaslr_offset();
> #endif
>         return ip;
> }
>
> ... to minimize the ifdeffery elsewhere.
>
>> +
>> +     /*
>> +      * We write all comparison arguments and types as u64.
>> +      * The buffer was allocated for t->kcov_size unsigned longs.
>> +      */
>> +     area = (u64 *)t->kcov_area;
>> +     max_pos = t->kcov_size * sizeof(unsigned long);
>> +
>> +     count = READ_ONCE(area[0]);
>> +
>> +     /* Every record is KCOV_WORDS_PER_CMP 64-bit words. */
>> +     start_index = 1 + count * KCOV_WORDS_PER_CMP;
>> +     end_pos = (start_index + KCOV_WORDS_PER_CMP) * sizeof(u64);
>> +     if (likely(end_pos <= max_pos)) {
>> +             area[start_index] = type;
>> +             area[start_index + 1] = arg1;
>> +             area[start_index + 2] = arg2;
>> +             area[start_index + 3] = ip;
>> +             WRITE_ONCE(area[0], count + 1);
>
> That ordering problem applies here, too.
>
> Thanks,
> Mark.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 1/3] kcov: support comparison operands collection
@ 2017-10-09 18:15     ` Dmitry Vyukov
  0 siblings, 0 replies; 27+ messages in thread
From: Dmitry Vyukov @ 2017-10-09 18:15 UTC (permalink / raw)
  To: Mark Rutland
  Cc: Alexander Potapenko, Andrew Morton, Alexander Popov,
	Andrey Ryabinin, Quentin Casasnovas, andreyknvl, Kees Cook,
	Vegard Nossum, syzkaller, linux-mm, LKML

On Mon, Oct 9, 2017 at 5:46 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> Hi,
>
> I look forward to using this! :)
>
> I just have afew comments below.
>
> On Mon, Oct 09, 2017 at 05:05:19PM +0200, Alexander Potapenko wrote:
>> +/*
>> + * Defines the format for the types of collected comparisons.
>> + */
>> +enum kcov_cmp_type {
>> +     /*
>> +      * LSB shows whether one of the arguments is a compile-time constant.
>> +      */
>> +     KCOV_CMP_CONST = 1,
>> +     /*
>> +      * Second and third LSBs contain the size of arguments (1/2/4/8 bytes).
>> +      */
>> +     KCOV_CMP_SIZE1 = 0,
>> +     KCOV_CMP_SIZE2 = 2,
>> +     KCOV_CMP_SIZE4 = 4,
>> +     KCOV_CMP_SIZE8 = 6,
>> +     KCOV_CMP_SIZE_MASK = 6,
>> +};
>
> Given that LSB is meant to be OR-ed in, (and hence combinations of
> values are meaningful) I don't think it makes sense for this to be an
> enum. This would clearer as something like:
>
> /*
>  * The format for the types of collected comparisons.
>  *
>  * Bit 0 shows whether one of the arguments is a compile-time constant.
>  * Bits 1 & 2 contain log2 of the argument size, up to 8 bytes.
>  */
> #define KCOV_CMP_CONST          (1 << 0)
> #define KCOV_CMP_SIZE(n)        ((n) << 1)
> #define KCOV_CMP_MASK           KCOV_CMP_SIZE(3)
>
> ... I note that a few places in the kernel use a 128-bit type. Are
> 128-bit comparisons not instrumented?

Yes, they are not instrumented.
How many are there? Can you give some examples?



>> +static bool check_kcov_mode(enum kcov_mode needed_mode, struct task_struct *t)
>> +{
>> +     enum kcov_mode mode;
>> +
>> +     /*
>> +      * We are interested in code coverage as a function of a syscall inputs,
>> +      * so we ignore code executed in interrupts.
>> +      */
>> +     if (!t || !in_task())
>> +             return false;
>
> This !t check can go, as with the one in __sanitizer_cov_trace_pc, since
> t is always current, and therefore cannot be NULL.
>
> IIRC there's a patch queued for that, which this may conflict with.
>
>> +     mode = READ_ONCE(t->kcov_mode);
>> +     /*
>> +      * There is some code that runs in interrupts but for which
>> +      * in_interrupt() returns false (e.g. preempt_schedule_irq()).
>> +      * READ_ONCE()/barrier() effectively provides load-acquire wrt
>> +      * interrupts, there are paired barrier()/WRITE_ONCE() in
>> +      * kcov_ioctl_locked().
>> +      */
>> +     barrier();
>> +     if (mode != needed_mode)
>> +             return false;
>> +     return true;
>
> This would be simpler as:
>
>         return mode == needed_mode;
>
> [...]
>
>> +     area = t->kcov_area;
>> +     /* The first 64-bit word is the number of subsequent PCs. */
>> +     pos = READ_ONCE(area[0]) + 1;
>> +     if (likely(pos < t->kcov_size)) {
>> +             area[pos] = ip;
>> +             WRITE_ONCE(area[0], pos);
>
> Not a new problem, but if the area for one thread is mmap'd, and read by
> another thread, these two writes could be seen out-of-order, since we
> don't have an smp_wmb() between them.
>
> I guess Syzkaller doesn't read the mmap'd kcov file from another thread?


Yes, that's the intention. If you read coverage from another thread,
you can't know coverage from what exactly you read. So the usage
pattern is:

reset coverage;
do something;
read coverage;



>
>>       }
>>  }
>>  EXPORT_SYMBOL(__sanitizer_cov_trace_pc);
>>
>> +#ifdef CONFIG_KCOV_ENABLE_COMPARISONS
>> +static void write_comp_data(u64 type, u64 arg1, u64 arg2, u64 ip)
>> +{
>> +     struct task_struct *t;
>> +     u64 *area;
>> +     u64 count, start_index, end_pos, max_pos;
>> +
>> +     t = current;
>> +     if (!check_kcov_mode(KCOV_MODE_TRACE_CMP, t))
>> +             return;
>> +
>> +#ifdef CONFIG_RANDOMIZE_BASE
>> +     ip -= kaslr_offset();
>> +#endif
>
> Given we have this in two places, it might make sense to have a helper
> like:
>
> unsigned long canonicalize_ip(unsigned long ip)
> {
> #ifdef CONFIG_RANDOMIZE_BASE
>         ip -= kaslr_offset();
> #endif
>         return ip;
> }
>
> ... to minimize the ifdeffery elsewhere.
>
>> +
>> +     /*
>> +      * We write all comparison arguments and types as u64.
>> +      * The buffer was allocated for t->kcov_size unsigned longs.
>> +      */
>> +     area = (u64 *)t->kcov_area;
>> +     max_pos = t->kcov_size * sizeof(unsigned long);
>> +
>> +     count = READ_ONCE(area[0]);
>> +
>> +     /* Every record is KCOV_WORDS_PER_CMP 64-bit words. */
>> +     start_index = 1 + count * KCOV_WORDS_PER_CMP;
>> +     end_pos = (start_index + KCOV_WORDS_PER_CMP) * sizeof(u64);
>> +     if (likely(end_pos <= max_pos)) {
>> +             area[start_index] = type;
>> +             area[start_index + 1] = arg1;
>> +             area[start_index + 2] = arg2;
>> +             area[start_index + 3] = ip;
>> +             WRITE_ONCE(area[0], count + 1);
>
> That ordering problem applies here, too.
>
> Thanks,
> Mark.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 1/3] kcov: support comparison operands collection
  2017-10-09 18:15     ` Dmitry Vyukov
@ 2017-10-09 18:37       ` Mark Rutland
  -1 siblings, 0 replies; 27+ messages in thread
From: Mark Rutland @ 2017-10-09 18:37 UTC (permalink / raw)
  To: Dmitry Vyukov
  Cc: Alexander Potapenko, Andrew Morton, Alexander Popov,
	Andrey Ryabinin, Quentin Casasnovas, andreyknvl, Kees Cook,
	Vegard Nossum, syzkaller, linux-mm, LKML

On Mon, Oct 09, 2017 at 08:15:10PM +0200, 'Dmitry Vyukov' via syzkaller wrote:
> On Mon, Oct 9, 2017 at 5:46 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> > On Mon, Oct 09, 2017 at 05:05:19PM +0200, Alexander Potapenko wrote:

> > ... I note that a few places in the kernel use a 128-bit type. Are
> > 128-bit comparisons not instrumented?
> 
> Yes, they are not instrumented.
> How many are there? Can you give some examples?

>From a quick scan, it doesn't looks like there are currently any
comparisons.

It's used as a data type in a few places under arm64:

arch/arm64/include/asm/checksum.h:      __uint128_t tmp;
arch/arm64/include/asm/checksum.h:      tmp = *(const __uint128_t *)iph;
arch/arm64/include/asm/fpsimd.h:                        __uint128_t vregs[32];
arch/arm64/include/uapi/asm/ptrace.h:   __uint128_t     vregs[32];
arch/arm64/include/uapi/asm/sigcontext.h:       __uint128_t vregs[32];
arch/arm64/kernel/signal32.c:   __uint128_t     raw;
arch/arm64/kvm/guest.c: __uint128_t tmp;

[...]

> >> +     area = t->kcov_area;
> >> +     /* The first 64-bit word is the number of subsequent PCs. */
> >> +     pos = READ_ONCE(area[0]) + 1;
> >> +     if (likely(pos < t->kcov_size)) {
> >> +             area[pos] = ip;
> >> +             WRITE_ONCE(area[0], pos);
> >
> > Not a new problem, but if the area for one thread is mmap'd, and read by
> > another thread, these two writes could be seen out-of-order, since we
> > don't have an smp_wmb() between them.
> >
> > I guess Syzkaller doesn't read the mmap'd kcov file from another thread?
> 
> 
> Yes, that's the intention. If you read coverage from another thread,
> you can't know coverage from what exactly you read. So the usage
> pattern is:
> 
> reset coverage;
> do something;
> read coverage;

Ok. I guess without a use-case for reading this from another thread it doesn't
really matter.

Thanks,
Mark.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 1/3] kcov: support comparison operands collection
@ 2017-10-09 18:37       ` Mark Rutland
  0 siblings, 0 replies; 27+ messages in thread
From: Mark Rutland @ 2017-10-09 18:37 UTC (permalink / raw)
  To: Dmitry Vyukov
  Cc: Alexander Potapenko, Andrew Morton, Alexander Popov,
	Andrey Ryabinin, Quentin Casasnovas, andreyknvl, Kees Cook,
	Vegard Nossum, syzkaller, linux-mm, LKML

On Mon, Oct 09, 2017 at 08:15:10PM +0200, 'Dmitry Vyukov' via syzkaller wrote:
> On Mon, Oct 9, 2017 at 5:46 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> > On Mon, Oct 09, 2017 at 05:05:19PM +0200, Alexander Potapenko wrote:

> > ... I note that a few places in the kernel use a 128-bit type. Are
> > 128-bit comparisons not instrumented?
> 
> Yes, they are not instrumented.
> How many are there? Can you give some examples?

>From a quick scan, it doesn't looks like there are currently any
comparisons.

It's used as a data type in a few places under arm64:

arch/arm64/include/asm/checksum.h:      __uint128_t tmp;
arch/arm64/include/asm/checksum.h:      tmp = *(const __uint128_t *)iph;
arch/arm64/include/asm/fpsimd.h:                        __uint128_t vregs[32];
arch/arm64/include/uapi/asm/ptrace.h:   __uint128_t     vregs[32];
arch/arm64/include/uapi/asm/sigcontext.h:       __uint128_t vregs[32];
arch/arm64/kernel/signal32.c:   __uint128_t     raw;
arch/arm64/kvm/guest.c: __uint128_t tmp;

[...]

> >> +     area = t->kcov_area;
> >> +     /* The first 64-bit word is the number of subsequent PCs. */
> >> +     pos = READ_ONCE(area[0]) + 1;
> >> +     if (likely(pos < t->kcov_size)) {
> >> +             area[pos] = ip;
> >> +             WRITE_ONCE(area[0], pos);
> >
> > Not a new problem, but if the area for one thread is mmap'd, and read by
> > another thread, these two writes could be seen out-of-order, since we
> > don't have an smp_wmb() between them.
> >
> > I guess Syzkaller doesn't read the mmap'd kcov file from another thread?
> 
> 
> Yes, that's the intention. If you read coverage from another thread,
> you can't know coverage from what exactly you read. So the usage
> pattern is:
> 
> reset coverage;
> do something;
> read coverage;

Ok. I guess without a use-case for reading this from another thread it doesn't
really matter.

Thanks,
Mark.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 1/3] kcov: support comparison operands collection
  2017-10-09 18:37       ` Mark Rutland
@ 2017-10-09 18:46         ` Dmitry Vyukov
  -1 siblings, 0 replies; 27+ messages in thread
From: Dmitry Vyukov @ 2017-10-09 18:46 UTC (permalink / raw)
  To: Mark Rutland
  Cc: Alexander Potapenko, Andrew Morton, Alexander Popov,
	Andrey Ryabinin, Quentin Casasnovas, andreyknvl, Kees Cook,
	Vegard Nossum, syzkaller, linux-mm, LKML

On Mon, Oct 9, 2017 at 8:37 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Mon, Oct 09, 2017 at 08:15:10PM +0200, 'Dmitry Vyukov' via syzkaller wrote:
>> On Mon, Oct 9, 2017 at 5:46 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>> > On Mon, Oct 09, 2017 at 05:05:19PM +0200, Alexander Potapenko wrote:
>
>> > ... I note that a few places in the kernel use a 128-bit type. Are
>> > 128-bit comparisons not instrumented?
>>
>> Yes, they are not instrumented.
>> How many are there? Can you give some examples?
>
> From a quick scan, it doesn't looks like there are currently any
> comparisons.
>
> It's used as a data type in a few places under arm64:
>
> arch/arm64/include/asm/checksum.h:      __uint128_t tmp;
> arch/arm64/include/asm/checksum.h:      tmp = *(const __uint128_t *)iph;
> arch/arm64/include/asm/fpsimd.h:                        __uint128_t vregs[32];
> arch/arm64/include/uapi/asm/ptrace.h:   __uint128_t     vregs[32];
> arch/arm64/include/uapi/asm/sigcontext.h:       __uint128_t vregs[32];
> arch/arm64/kernel/signal32.c:   __uint128_t     raw;
> arch/arm64/kvm/guest.c: __uint128_t tmp;


Then I think we just continue ignoring them for now :)
In the future we can extend kcov to trace 128-bits values. We will
need to add a special flag and write 2 consecutive entries for them.
Or something along these lines.


>> >> +     area = t->kcov_area;
>> >> +     /* The first 64-bit word is the number of subsequent PCs. */
>> >> +     pos = READ_ONCE(area[0]) + 1;
>> >> +     if (likely(pos < t->kcov_size)) {
>> >> +             area[pos] = ip;
>> >> +             WRITE_ONCE(area[0], pos);
>> >
>> > Not a new problem, but if the area for one thread is mmap'd, and read by
>> > another thread, these two writes could be seen out-of-order, since we
>> > don't have an smp_wmb() between them.
>> >
>> > I guess Syzkaller doesn't read the mmap'd kcov file from another thread?
>>
>>
>> Yes, that's the intention. If you read coverage from another thread,
>> you can't know coverage from what exactly you read. So the usage
>> pattern is:
>>
>> reset coverage;
>> do something;
>> read coverage;
>
> Ok. I guess without a use-case for reading this from another thread it doesn't
> really matter.
>
> Thanks,
> Mark.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 1/3] kcov: support comparison operands collection
@ 2017-10-09 18:46         ` Dmitry Vyukov
  0 siblings, 0 replies; 27+ messages in thread
From: Dmitry Vyukov @ 2017-10-09 18:46 UTC (permalink / raw)
  To: Mark Rutland
  Cc: Alexander Potapenko, Andrew Morton, Alexander Popov,
	Andrey Ryabinin, Quentin Casasnovas, andreyknvl, Kees Cook,
	Vegard Nossum, syzkaller, linux-mm, LKML

On Mon, Oct 9, 2017 at 8:37 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Mon, Oct 09, 2017 at 08:15:10PM +0200, 'Dmitry Vyukov' via syzkaller wrote:
>> On Mon, Oct 9, 2017 at 5:46 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>> > On Mon, Oct 09, 2017 at 05:05:19PM +0200, Alexander Potapenko wrote:
>
>> > ... I note that a few places in the kernel use a 128-bit type. Are
>> > 128-bit comparisons not instrumented?
>>
>> Yes, they are not instrumented.
>> How many are there? Can you give some examples?
>
> From a quick scan, it doesn't looks like there are currently any
> comparisons.
>
> It's used as a data type in a few places under arm64:
>
> arch/arm64/include/asm/checksum.h:      __uint128_t tmp;
> arch/arm64/include/asm/checksum.h:      tmp = *(const __uint128_t *)iph;
> arch/arm64/include/asm/fpsimd.h:                        __uint128_t vregs[32];
> arch/arm64/include/uapi/asm/ptrace.h:   __uint128_t     vregs[32];
> arch/arm64/include/uapi/asm/sigcontext.h:       __uint128_t vregs[32];
> arch/arm64/kernel/signal32.c:   __uint128_t     raw;
> arch/arm64/kvm/guest.c: __uint128_t tmp;


Then I think we just continue ignoring them for now :)
In the future we can extend kcov to trace 128-bits values. We will
need to add a special flag and write 2 consecutive entries for them.
Or something along these lines.


>> >> +     area = t->kcov_area;
>> >> +     /* The first 64-bit word is the number of subsequent PCs. */
>> >> +     pos = READ_ONCE(area[0]) + 1;
>> >> +     if (likely(pos < t->kcov_size)) {
>> >> +             area[pos] = ip;
>> >> +             WRITE_ONCE(area[0], pos);
>> >
>> > Not a new problem, but if the area for one thread is mmap'd, and read by
>> > another thread, these two writes could be seen out-of-order, since we
>> > don't have an smp_wmb() between them.
>> >
>> > I guess Syzkaller doesn't read the mmap'd kcov file from another thread?
>>
>>
>> Yes, that's the intention. If you read coverage from another thread,
>> you can't know coverage from what exactly you read. So the usage
>> pattern is:
>>
>> reset coverage;
>> do something;
>> read coverage;
>
> Ok. I guess without a use-case for reading this from another thread it doesn't
> really matter.
>
> Thanks,
> Mark.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 1/3] kcov: support comparison operands collection
  2017-10-09 18:46         ` Dmitry Vyukov
@ 2017-10-10  9:56           ` Mark Rutland
  -1 siblings, 0 replies; 27+ messages in thread
From: Mark Rutland @ 2017-10-10  9:56 UTC (permalink / raw)
  To: Dmitry Vyukov
  Cc: Alexander Potapenko, Andrew Morton, Alexander Popov,
	Andrey Ryabinin, Quentin Casasnovas, andreyknvl, Kees Cook,
	Vegard Nossum, syzkaller, linux-mm, LKML

On Mon, Oct 09, 2017 at 08:46:18PM +0200, 'Dmitry Vyukov' via syzkaller wrote:
> On Mon, Oct 9, 2017 at 8:37 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> > On Mon, Oct 09, 2017 at 08:15:10PM +0200, 'Dmitry Vyukov' via syzkaller wrote:
> >> On Mon, Oct 9, 2017 at 5:46 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> >> > On Mon, Oct 09, 2017 at 05:05:19PM +0200, Alexander Potapenko wrote:
> >
> >> > ... I note that a few places in the kernel use a 128-bit type. Are
> >> > 128-bit comparisons not instrumented?
> >>
> >> Yes, they are not instrumented.
> >> How many are there? Can you give some examples?
> >
> > From a quick scan, it doesn't looks like there are currently any
> > comparisons.
> >
> > It's used as a data type in a few places under arm64:
> >
> > arch/arm64/include/asm/checksum.h:      __uint128_t tmp;
> > arch/arm64/include/asm/checksum.h:      tmp = *(const __uint128_t *)iph;
> > arch/arm64/include/asm/fpsimd.h:                        __uint128_t vregs[32];
> > arch/arm64/include/uapi/asm/ptrace.h:   __uint128_t     vregs[32];
> > arch/arm64/include/uapi/asm/sigcontext.h:       __uint128_t vregs[32];
> > arch/arm64/kernel/signal32.c:   __uint128_t     raw;
> > arch/arm64/kvm/guest.c: __uint128_t tmp;
> 
> Then I think we just continue ignoring them for now :)
> In the future we can extend kcov to trace 128-bits values. We will
> need to add a special flag and write 2 consecutive entries for them.
> Or something along these lines.

Just wanted to make sure that we weren't backing ourselves into a corner
w.r.t. ABI; that sounds fine to me.

Thanks,
Mark.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 1/3] kcov: support comparison operands collection
@ 2017-10-10  9:56           ` Mark Rutland
  0 siblings, 0 replies; 27+ messages in thread
From: Mark Rutland @ 2017-10-10  9:56 UTC (permalink / raw)
  To: Dmitry Vyukov
  Cc: Alexander Potapenko, Andrew Morton, Alexander Popov,
	Andrey Ryabinin, Quentin Casasnovas, andreyknvl, Kees Cook,
	Vegard Nossum, syzkaller, linux-mm, LKML

On Mon, Oct 09, 2017 at 08:46:18PM +0200, 'Dmitry Vyukov' via syzkaller wrote:
> On Mon, Oct 9, 2017 at 8:37 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> > On Mon, Oct 09, 2017 at 08:15:10PM +0200, 'Dmitry Vyukov' via syzkaller wrote:
> >> On Mon, Oct 9, 2017 at 5:46 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> >> > On Mon, Oct 09, 2017 at 05:05:19PM +0200, Alexander Potapenko wrote:
> >
> >> > ... I note that a few places in the kernel use a 128-bit type. Are
> >> > 128-bit comparisons not instrumented?
> >>
> >> Yes, they are not instrumented.
> >> How many are there? Can you give some examples?
> >
> > From a quick scan, it doesn't looks like there are currently any
> > comparisons.
> >
> > It's used as a data type in a few places under arm64:
> >
> > arch/arm64/include/asm/checksum.h:      __uint128_t tmp;
> > arch/arm64/include/asm/checksum.h:      tmp = *(const __uint128_t *)iph;
> > arch/arm64/include/asm/fpsimd.h:                        __uint128_t vregs[32];
> > arch/arm64/include/uapi/asm/ptrace.h:   __uint128_t     vregs[32];
> > arch/arm64/include/uapi/asm/sigcontext.h:       __uint128_t vregs[32];
> > arch/arm64/kernel/signal32.c:   __uint128_t     raw;
> > arch/arm64/kvm/guest.c: __uint128_t tmp;
> 
> Then I think we just continue ignoring them for now :)
> In the future we can extend kcov to trace 128-bits values. We will
> need to add a special flag and write 2 consecutive entries for them.
> Or something along these lines.

Just wanted to make sure that we weren't backing ourselves into a corner
w.r.t. ABI; that sounds fine to me.

Thanks,
Mark.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 2/3] Makefile: support flag -fsanitizer-coverage=trace-cmp
  2017-10-09 15:53     ` Andrey Ryabinin
@ 2017-10-10 15:28       ` Alexander Potapenko
  -1 siblings, 0 replies; 27+ messages in thread
From: Alexander Potapenko @ 2017-10-10 15:28 UTC (permalink / raw)
  To: Andrey Ryabinin
  Cc: Andrew Morton, Mark Rutland, Alexander Popov, Quentin Casasnovas,
	Dmitriy Vyukov, Andrey Konovalov, Kees Cook, Vegard Nossum,
	syzkaller, Linux Memory Management List, LKML

On Mon, Oct 9, 2017 at 8:53 AM, Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:
>
>
> On 10/09/2017 06:05 PM, Alexander Potapenko wrote:
>
>> v2: - updated KCOV_ENABLE_COMPARISONS description
>> ---
>>  Makefile             |  5 +++--
>>  lib/Kconfig.debug    | 10 ++++++++++
>>  scripts/Makefile.lib |  6 ++++++
>>  3 files changed, 19 insertions(+), 2 deletions(-)
>>
>> diff --git a/Makefile b/Makefile
>> index 2835863bdd5a..c2a8e56df748 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -374,7 +374,7 @@ AFLAGS_KERNEL     =
>>  LDFLAGS_vmlinux =
>>  CFLAGS_GCOV  := -fprofile-arcs -ftest-coverage -fno-tree-loop-im $(call cc-disable-warning,maybe-uninitialized,)
>>  CFLAGS_KCOV  := $(call cc-option,-fsanitize-coverage=trace-pc,)
>> -
>> +CFLAGS_KCOV_COMPS := $(call cc-option,-fsanitize-coverage=trace-cmp,)
>>
>>  # Use USERINCLUDE when you must reference the UAPI directories only.
>>  USERINCLUDE    := \
>> @@ -420,7 +420,7 @@ export MAKE AWK GENKSYMS INSTALLKERNEL PERL PYTHON UTS_MACHINE
>>  export HOSTCXX HOSTCXXFLAGS LDFLAGS_MODULE CHECK CHECKFLAGS
>>
>>  export KBUILD_CPPFLAGS NOSTDINC_FLAGS LINUXINCLUDE OBJCOPYFLAGS LDFLAGS
>> -export KBUILD_CFLAGS CFLAGS_KERNEL CFLAGS_MODULE CFLAGS_GCOV CFLAGS_KCOV CFLAGS_KASAN CFLAGS_UBSAN
>> +export KBUILD_CFLAGS CFLAGS_KERNEL CFLAGS_MODULE CFLAGS_GCOV CFLAGS_KCOV CFLAGS_KCOV_COMPS CFLAGS_KASAN CFLAGS_UBSAN
>>  export KBUILD_AFLAGS AFLAGS_KERNEL AFLAGS_MODULE
>>  export KBUILD_AFLAGS_MODULE KBUILD_CFLAGS_MODULE KBUILD_LDFLAGS_MODULE
>>  export KBUILD_AFLAGS_KERNEL KBUILD_CFLAGS_KERNEL
>> @@ -822,6 +822,7 @@ KBUILD_CFLAGS   += $(call cc-option,-Werror=designated-init)
>>  KBUILD_ARFLAGS := $(call ar-option,D)
>>
>>  include scripts/Makefile.kasan
>> +include scripts/Makefile.kcov
>
> scripts/Makefile.kcov doesn't exist.
Good catch! Will fix.

>
>
>
>> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
>> index 5e975fee0f5b..7ddd5932c832 100644
>> --- a/scripts/Makefile.lib
>> +++ b/scripts/Makefile.lib
>> @@ -142,6 +142,12 @@ _c_flags += $(if $(patsubst n%,, \
>>       $(CFLAGS_KCOV))
>>  endif
>>
>> +ifeq ($(CONFIG_KCOV_ENABLE_COMPARISONS),y)
>> +_c_flags += $(if $(patsubst n%,, \
>> +     $(KCOV_INSTRUMENT_$(basetarget).o)$(KCOV_INSTRUMENT)$(CONFIG_KCOV_INSTRUMENT_ALL)), \
>> +     $(CFLAGS_KCOV_COMPS))
>> +endif
>> +
>
> Instead of this you could simply add -fsanitize-coverage=trace-cmp to CFLAGS_KCOV.
Indeed. I've refactored these bits and moved them to Makefile.kcov.
>
>>  # If building the kernel in a separate objtree expand all occurrences
>>  # of -Idir to -I$(srctree)/dir except for absolute paths (starting with '/').
>>
>>



-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 2/3] Makefile: support flag -fsanitizer-coverage=trace-cmp
@ 2017-10-10 15:28       ` Alexander Potapenko
  0 siblings, 0 replies; 27+ messages in thread
From: Alexander Potapenko @ 2017-10-10 15:28 UTC (permalink / raw)
  To: Andrey Ryabinin
  Cc: Andrew Morton, Mark Rutland, Alexander Popov, Quentin Casasnovas,
	Dmitriy Vyukov, Andrey Konovalov, Kees Cook, Vegard Nossum,
	syzkaller, Linux Memory Management List, LKML

On Mon, Oct 9, 2017 at 8:53 AM, Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:
>
>
> On 10/09/2017 06:05 PM, Alexander Potapenko wrote:
>
>> v2: - updated KCOV_ENABLE_COMPARISONS description
>> ---
>>  Makefile             |  5 +++--
>>  lib/Kconfig.debug    | 10 ++++++++++
>>  scripts/Makefile.lib |  6 ++++++
>>  3 files changed, 19 insertions(+), 2 deletions(-)
>>
>> diff --git a/Makefile b/Makefile
>> index 2835863bdd5a..c2a8e56df748 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -374,7 +374,7 @@ AFLAGS_KERNEL     =
>>  LDFLAGS_vmlinux =
>>  CFLAGS_GCOV  := -fprofile-arcs -ftest-coverage -fno-tree-loop-im $(call cc-disable-warning,maybe-uninitialized,)
>>  CFLAGS_KCOV  := $(call cc-option,-fsanitize-coverage=trace-pc,)
>> -
>> +CFLAGS_KCOV_COMPS := $(call cc-option,-fsanitize-coverage=trace-cmp,)
>>
>>  # Use USERINCLUDE when you must reference the UAPI directories only.
>>  USERINCLUDE    := \
>> @@ -420,7 +420,7 @@ export MAKE AWK GENKSYMS INSTALLKERNEL PERL PYTHON UTS_MACHINE
>>  export HOSTCXX HOSTCXXFLAGS LDFLAGS_MODULE CHECK CHECKFLAGS
>>
>>  export KBUILD_CPPFLAGS NOSTDINC_FLAGS LINUXINCLUDE OBJCOPYFLAGS LDFLAGS
>> -export KBUILD_CFLAGS CFLAGS_KERNEL CFLAGS_MODULE CFLAGS_GCOV CFLAGS_KCOV CFLAGS_KASAN CFLAGS_UBSAN
>> +export KBUILD_CFLAGS CFLAGS_KERNEL CFLAGS_MODULE CFLAGS_GCOV CFLAGS_KCOV CFLAGS_KCOV_COMPS CFLAGS_KASAN CFLAGS_UBSAN
>>  export KBUILD_AFLAGS AFLAGS_KERNEL AFLAGS_MODULE
>>  export KBUILD_AFLAGS_MODULE KBUILD_CFLAGS_MODULE KBUILD_LDFLAGS_MODULE
>>  export KBUILD_AFLAGS_KERNEL KBUILD_CFLAGS_KERNEL
>> @@ -822,6 +822,7 @@ KBUILD_CFLAGS   += $(call cc-option,-Werror=designated-init)
>>  KBUILD_ARFLAGS := $(call ar-option,D)
>>
>>  include scripts/Makefile.kasan
>> +include scripts/Makefile.kcov
>
> scripts/Makefile.kcov doesn't exist.
Good catch! Will fix.

>
>
>
>> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
>> index 5e975fee0f5b..7ddd5932c832 100644
>> --- a/scripts/Makefile.lib
>> +++ b/scripts/Makefile.lib
>> @@ -142,6 +142,12 @@ _c_flags += $(if $(patsubst n%,, \
>>       $(CFLAGS_KCOV))
>>  endif
>>
>> +ifeq ($(CONFIG_KCOV_ENABLE_COMPARISONS),y)
>> +_c_flags += $(if $(patsubst n%,, \
>> +     $(KCOV_INSTRUMENT_$(basetarget).o)$(KCOV_INSTRUMENT)$(CONFIG_KCOV_INSTRUMENT_ALL)), \
>> +     $(CFLAGS_KCOV_COMPS))
>> +endif
>> +
>
> Instead of this you could simply add -fsanitize-coverage=trace-cmp to CFLAGS_KCOV.
Indeed. I've refactored these bits and moved them to Makefile.kcov.
>
>>  # If building the kernel in a separate objtree expand all occurrences
>>  # of -Idir to -I$(srctree)/dir except for absolute paths (starting with '/').
>>
>>



-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 1/3] kcov: support comparison operands collection
  2017-10-09 15:46   ` Mark Rutland
@ 2017-10-10 15:28     ` Alexander Potapenko
  -1 siblings, 0 replies; 27+ messages in thread
From: Alexander Potapenko @ 2017-10-10 15:28 UTC (permalink / raw)
  To: Mark Rutland
  Cc: Andrew Morton, Alexander Popov, Andrey Ryabinin,
	Quentin Casasnovas, Dmitriy Vyukov, Andrey Konovalov, Kees Cook,
	Vegard Nossum, syzkaller, Linux Memory Management List, LKML

On Mon, Oct 9, 2017 at 8:46 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> Hi,
>
> I look forward to using this! :)
>
> I just have afew comments below.
>
> On Mon, Oct 09, 2017 at 05:05:19PM +0200, Alexander Potapenko wrote:
>> +/*
>> + * Defines the format for the types of collected comparisons.
>> + */
>> +enum kcov_cmp_type {
>> +     /*
>> +      * LSB shows whether one of the arguments is a compile-time constant.
>> +      */
>> +     KCOV_CMP_CONST = 1,
>> +     /*
>> +      * Second and third LSBs contain the size of arguments (1/2/4/8 bytes).
>> +      */
>> +     KCOV_CMP_SIZE1 = 0,
>> +     KCOV_CMP_SIZE2 = 2,
>> +     KCOV_CMP_SIZE4 = 4,
>> +     KCOV_CMP_SIZE8 = 6,
>> +     KCOV_CMP_SIZE_MASK = 6,
>> +};
>
> Given that LSB is meant to be OR-ed in, (and hence combinations of
> values are meaningful) I don't think it makes sense for this to be an
> enum. This would clearer as something like:
>
> /*
>  * The format for the types of collected comparisons.
>  *
>  * Bit 0 shows whether one of the arguments is a compile-time constant.
>  * Bits 1 & 2 contain log2 of the argument size, up to 8 bytes.
>  */
> #define KCOV_CMP_CONST          (1 << 0)
> #define KCOV_CMP_SIZE(n)        ((n) << 1)
> #define KCOV_CMP_MASK           KCOV_CMP_SIZE(3)
Agreed.
> ... I note that a few places in the kernel use a 128-bit type. Are
> 128-bit comparisons not instrumented?
>
> [...]
>
>> +static bool check_kcov_mode(enum kcov_mode needed_mode, struct task_struct *t)
>> +{
>> +     enum kcov_mode mode;
>> +
>> +     /*
>> +      * We are interested in code coverage as a function of a syscall inputs,
>> +      * so we ignore code executed in interrupts.
>> +      */
>> +     if (!t || !in_task())
>> +             return false;
>
> This !t check can go, as with the one in __sanitizer_cov_trace_pc, since
> t is always current, and therefore cannot be NULL.
Ok.
> IIRC there's a patch queued for that, which this may conflict with.
Sorry, I don't quite understand what exactly is conflicting here.

>> +     mode = READ_ONCE(t->kcov_mode);
>> +     /*
>> +      * There is some code that runs in interrupts but for which
>> +      * in_interrupt() returns false (e.g. preempt_schedule_irq()).
>> +      * READ_ONCE()/barrier() effectively provides load-acquire wrt
>> +      * interrupts, there are paired barrier()/WRITE_ONCE() in
>> +      * kcov_ioctl_locked().
>> +      */
>> +     barrier();
>> +     if (mode != needed_mode)
>> +             return false;
>> +     return true;
>
> This would be simpler as:
>
>         return mode == needed_mode;

Agreed.

> [...]
>
>> +     area = t->kcov_area;
>> +     /* The first 64-bit word is the number of subsequent PCs. */
>> +     pos = READ_ONCE(area[0]) + 1;
>> +     if (likely(pos < t->kcov_size)) {
>> +             area[pos] = ip;
>> +             WRITE_ONCE(area[0], pos);
>
> Not a new problem, but if the area for one thread is mmap'd, and read by
> another thread, these two writes could be seen out-of-order, since we
> don't have an smp_wmb() between them.
>
> I guess Syzkaller doesn't read the mmap'd kcov file from another thread?
(Dmitry answered this one already)
>>       }
>>  }
>>  EXPORT_SYMBOL(__sanitizer_cov_trace_pc);
>>
>> +#ifdef CONFIG_KCOV_ENABLE_COMPARISONS
>> +static void write_comp_data(u64 type, u64 arg1, u64 arg2, u64 ip)
>> +{
>> +     struct task_struct *t;
>> +     u64 *area;
>> +     u64 count, start_index, end_pos, max_pos;
>> +
>> +     t = current;
>> +     if (!check_kcov_mode(KCOV_MODE_TRACE_CMP, t))
>> +             return;
>> +
>> +#ifdef CONFIG_RANDOMIZE_BASE
>> +     ip -= kaslr_offset();
>> +#endif
>
> Given we have this in two places, it might make sense to have a helper
> like:
>
> unsigned long canonicalize_ip(unsigned long ip)
> {
> #ifdef CONFIG_RANDOMIZE_BASE
>         ip -= kaslr_offset();
> #endif
>         return ip;
> }
Done.
> ... to minimize the ifdeffery elsewhere.
>
>> +
>> +     /*
>> +      * We write all comparison arguments and types as u64.
>> +      * The buffer was allocated for t->kcov_size unsigned longs.
>> +      */
>> +     area = (u64 *)t->kcov_area;
>> +     max_pos = t->kcov_size * sizeof(unsigned long);
>> +
>> +     count = READ_ONCE(area[0]);
>> +
>> +     /* Every record is KCOV_WORDS_PER_CMP 64-bit words. */
>> +     start_index = 1 + count * KCOV_WORDS_PER_CMP;
>> +     end_pos = (start_index + KCOV_WORDS_PER_CMP) * sizeof(u64);
>> +     if (likely(end_pos <= max_pos)) {
>> +             area[start_index] = type;
>> +             area[start_index + 1] = arg1;
>> +             area[start_index + 2] = arg2;
>> +             area[start_index + 3] = ip;
>> +             WRITE_ONCE(area[0], count + 1);
>
> That ordering problem applies here, too.
>
> Thanks,
> Mark.



-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 1/3] kcov: support comparison operands collection
@ 2017-10-10 15:28     ` Alexander Potapenko
  0 siblings, 0 replies; 27+ messages in thread
From: Alexander Potapenko @ 2017-10-10 15:28 UTC (permalink / raw)
  To: Mark Rutland
  Cc: Andrew Morton, Alexander Popov, Andrey Ryabinin,
	Quentin Casasnovas, Dmitriy Vyukov, Andrey Konovalov, Kees Cook,
	Vegard Nossum, syzkaller, Linux Memory Management List, LKML

On Mon, Oct 9, 2017 at 8:46 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> Hi,
>
> I look forward to using this! :)
>
> I just have afew comments below.
>
> On Mon, Oct 09, 2017 at 05:05:19PM +0200, Alexander Potapenko wrote:
>> +/*
>> + * Defines the format for the types of collected comparisons.
>> + */
>> +enum kcov_cmp_type {
>> +     /*
>> +      * LSB shows whether one of the arguments is a compile-time constant.
>> +      */
>> +     KCOV_CMP_CONST = 1,
>> +     /*
>> +      * Second and third LSBs contain the size of arguments (1/2/4/8 bytes).
>> +      */
>> +     KCOV_CMP_SIZE1 = 0,
>> +     KCOV_CMP_SIZE2 = 2,
>> +     KCOV_CMP_SIZE4 = 4,
>> +     KCOV_CMP_SIZE8 = 6,
>> +     KCOV_CMP_SIZE_MASK = 6,
>> +};
>
> Given that LSB is meant to be OR-ed in, (and hence combinations of
> values are meaningful) I don't think it makes sense for this to be an
> enum. This would clearer as something like:
>
> /*
>  * The format for the types of collected comparisons.
>  *
>  * Bit 0 shows whether one of the arguments is a compile-time constant.
>  * Bits 1 & 2 contain log2 of the argument size, up to 8 bytes.
>  */
> #define KCOV_CMP_CONST          (1 << 0)
> #define KCOV_CMP_SIZE(n)        ((n) << 1)
> #define KCOV_CMP_MASK           KCOV_CMP_SIZE(3)
Agreed.
> ... I note that a few places in the kernel use a 128-bit type. Are
> 128-bit comparisons not instrumented?
>
> [...]
>
>> +static bool check_kcov_mode(enum kcov_mode needed_mode, struct task_struct *t)
>> +{
>> +     enum kcov_mode mode;
>> +
>> +     /*
>> +      * We are interested in code coverage as a function of a syscall inputs,
>> +      * so we ignore code executed in interrupts.
>> +      */
>> +     if (!t || !in_task())
>> +             return false;
>
> This !t check can go, as with the one in __sanitizer_cov_trace_pc, since
> t is always current, and therefore cannot be NULL.
Ok.
> IIRC there's a patch queued for that, which this may conflict with.
Sorry, I don't quite understand what exactly is conflicting here.

>> +     mode = READ_ONCE(t->kcov_mode);
>> +     /*
>> +      * There is some code that runs in interrupts but for which
>> +      * in_interrupt() returns false (e.g. preempt_schedule_irq()).
>> +      * READ_ONCE()/barrier() effectively provides load-acquire wrt
>> +      * interrupts, there are paired barrier()/WRITE_ONCE() in
>> +      * kcov_ioctl_locked().
>> +      */
>> +     barrier();
>> +     if (mode != needed_mode)
>> +             return false;
>> +     return true;
>
> This would be simpler as:
>
>         return mode == needed_mode;

Agreed.

> [...]
>
>> +     area = t->kcov_area;
>> +     /* The first 64-bit word is the number of subsequent PCs. */
>> +     pos = READ_ONCE(area[0]) + 1;
>> +     if (likely(pos < t->kcov_size)) {
>> +             area[pos] = ip;
>> +             WRITE_ONCE(area[0], pos);
>
> Not a new problem, but if the area for one thread is mmap'd, and read by
> another thread, these two writes could be seen out-of-order, since we
> don't have an smp_wmb() between them.
>
> I guess Syzkaller doesn't read the mmap'd kcov file from another thread?
(Dmitry answered this one already)
>>       }
>>  }
>>  EXPORT_SYMBOL(__sanitizer_cov_trace_pc);
>>
>> +#ifdef CONFIG_KCOV_ENABLE_COMPARISONS
>> +static void write_comp_data(u64 type, u64 arg1, u64 arg2, u64 ip)
>> +{
>> +     struct task_struct *t;
>> +     u64 *area;
>> +     u64 count, start_index, end_pos, max_pos;
>> +
>> +     t = current;
>> +     if (!check_kcov_mode(KCOV_MODE_TRACE_CMP, t))
>> +             return;
>> +
>> +#ifdef CONFIG_RANDOMIZE_BASE
>> +     ip -= kaslr_offset();
>> +#endif
>
> Given we have this in two places, it might make sense to have a helper
> like:
>
> unsigned long canonicalize_ip(unsigned long ip)
> {
> #ifdef CONFIG_RANDOMIZE_BASE
>         ip -= kaslr_offset();
> #endif
>         return ip;
> }
Done.
> ... to minimize the ifdeffery elsewhere.
>
>> +
>> +     /*
>> +      * We write all comparison arguments and types as u64.
>> +      * The buffer was allocated for t->kcov_size unsigned longs.
>> +      */
>> +     area = (u64 *)t->kcov_area;
>> +     max_pos = t->kcov_size * sizeof(unsigned long);
>> +
>> +     count = READ_ONCE(area[0]);
>> +
>> +     /* Every record is KCOV_WORDS_PER_CMP 64-bit words. */
>> +     start_index = 1 + count * KCOV_WORDS_PER_CMP;
>> +     end_pos = (start_index + KCOV_WORDS_PER_CMP) * sizeof(u64);
>> +     if (likely(end_pos <= max_pos)) {
>> +             area[start_index] = type;
>> +             area[start_index + 1] = arg1;
>> +             area[start_index + 2] = arg2;
>> +             area[start_index + 3] = ip;
>> +             WRITE_ONCE(area[0], count + 1);
>
> That ordering problem applies here, too.
>
> Thanks,
> Mark.



-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 1/3] kcov: support comparison operands collection
  2017-10-10 15:28     ` Alexander Potapenko
@ 2017-10-10 15:34       ` Dmitry Vyukov
  -1 siblings, 0 replies; 27+ messages in thread
From: Dmitry Vyukov @ 2017-10-10 15:34 UTC (permalink / raw)
  To: Alexander Potapenko
  Cc: Mark Rutland, Andrew Morton, Alexander Popov, Andrey Ryabinin,
	Quentin Casasnovas, Andrey Konovalov, Kees Cook, Vegard Nossum,
	syzkaller, Linux Memory Management List, LKML

On Tue, Oct 10, 2017 at 5:28 PM, 'Alexander Potapenko' via syzkaller
<syzkaller@googlegroups.com> wrote:
> On Mon, Oct 9, 2017 at 8:46 AM, Mark Rutland <mark.rutland@arm.com> wrote:
>> Hi,
>>
>> I look forward to using this! :)
>>
>> I just have afew comments below.
>>
>> On Mon, Oct 09, 2017 at 05:05:19PM +0200, Alexander Potapenko wrote:
>>> +/*
>>> + * Defines the format for the types of collected comparisons.
>>> + */
>>> +enum kcov_cmp_type {
>>> +     /*
>>> +      * LSB shows whether one of the arguments is a compile-time constant.
>>> +      */
>>> +     KCOV_CMP_CONST = 1,
>>> +     /*
>>> +      * Second and third LSBs contain the size of arguments (1/2/4/8 bytes).
>>> +      */
>>> +     KCOV_CMP_SIZE1 = 0,
>>> +     KCOV_CMP_SIZE2 = 2,
>>> +     KCOV_CMP_SIZE4 = 4,
>>> +     KCOV_CMP_SIZE8 = 6,
>>> +     KCOV_CMP_SIZE_MASK = 6,
>>> +};
>>
>> Given that LSB is meant to be OR-ed in, (and hence combinations of
>> values are meaningful) I don't think it makes sense for this to be an
>> enum. This would clearer as something like:
>>
>> /*
>>  * The format for the types of collected comparisons.
>>  *
>>  * Bit 0 shows whether one of the arguments is a compile-time constant.
>>  * Bits 1 & 2 contain log2 of the argument size, up to 8 bytes.
>>  */
>> #define KCOV_CMP_CONST          (1 << 0)
>> #define KCOV_CMP_SIZE(n)        ((n) << 1)
>> #define KCOV_CMP_MASK           KCOV_CMP_SIZE(3)
> Agreed.
>> ... I note that a few places in the kernel use a 128-bit type. Are
>> 128-bit comparisons not instrumented?
>>
>> [...]
>>
>>> +static bool check_kcov_mode(enum kcov_mode needed_mode, struct task_struct *t)
>>> +{
>>> +     enum kcov_mode mode;
>>> +
>>> +     /*
>>> +      * We are interested in code coverage as a function of a syscall inputs,
>>> +      * so we ignore code executed in interrupts.
>>> +      */
>>> +     if (!t || !in_task())
>>> +             return false;
>>
>> This !t check can go, as with the one in __sanitizer_cov_trace_pc, since
>> t is always current, and therefore cannot be NULL.
> Ok.
>> IIRC there's a patch queued for that, which this may conflict with.
> Sorry, I don't quite understand what exactly is conflicting here.


This patch should be in mm tree:
https://patchwork.kernel.org/patch/9978383/

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 1/3] kcov: support comparison operands collection
@ 2017-10-10 15:34       ` Dmitry Vyukov
  0 siblings, 0 replies; 27+ messages in thread
From: Dmitry Vyukov @ 2017-10-10 15:34 UTC (permalink / raw)
  To: Alexander Potapenko
  Cc: Mark Rutland, Andrew Morton, Alexander Popov, Andrey Ryabinin,
	Quentin Casasnovas, Andrey Konovalov, Kees Cook, Vegard Nossum,
	syzkaller, Linux Memory Management List, LKML

On Tue, Oct 10, 2017 at 5:28 PM, 'Alexander Potapenko' via syzkaller
<syzkaller@googlegroups.com> wrote:
> On Mon, Oct 9, 2017 at 8:46 AM, Mark Rutland <mark.rutland@arm.com> wrote:
>> Hi,
>>
>> I look forward to using this! :)
>>
>> I just have afew comments below.
>>
>> On Mon, Oct 09, 2017 at 05:05:19PM +0200, Alexander Potapenko wrote:
>>> +/*
>>> + * Defines the format for the types of collected comparisons.
>>> + */
>>> +enum kcov_cmp_type {
>>> +     /*
>>> +      * LSB shows whether one of the arguments is a compile-time constant.
>>> +      */
>>> +     KCOV_CMP_CONST = 1,
>>> +     /*
>>> +      * Second and third LSBs contain the size of arguments (1/2/4/8 bytes).
>>> +      */
>>> +     KCOV_CMP_SIZE1 = 0,
>>> +     KCOV_CMP_SIZE2 = 2,
>>> +     KCOV_CMP_SIZE4 = 4,
>>> +     KCOV_CMP_SIZE8 = 6,
>>> +     KCOV_CMP_SIZE_MASK = 6,
>>> +};
>>
>> Given that LSB is meant to be OR-ed in, (and hence combinations of
>> values are meaningful) I don't think it makes sense for this to be an
>> enum. This would clearer as something like:
>>
>> /*
>>  * The format for the types of collected comparisons.
>>  *
>>  * Bit 0 shows whether one of the arguments is a compile-time constant.
>>  * Bits 1 & 2 contain log2 of the argument size, up to 8 bytes.
>>  */
>> #define KCOV_CMP_CONST          (1 << 0)
>> #define KCOV_CMP_SIZE(n)        ((n) << 1)
>> #define KCOV_CMP_MASK           KCOV_CMP_SIZE(3)
> Agreed.
>> ... I note that a few places in the kernel use a 128-bit type. Are
>> 128-bit comparisons not instrumented?
>>
>> [...]
>>
>>> +static bool check_kcov_mode(enum kcov_mode needed_mode, struct task_struct *t)
>>> +{
>>> +     enum kcov_mode mode;
>>> +
>>> +     /*
>>> +      * We are interested in code coverage as a function of a syscall inputs,
>>> +      * so we ignore code executed in interrupts.
>>> +      */
>>> +     if (!t || !in_task())
>>> +             return false;
>>
>> This !t check can go, as with the one in __sanitizer_cov_trace_pc, since
>> t is always current, and therefore cannot be NULL.
> Ok.
>> IIRC there's a patch queued for that, which this may conflict with.
> Sorry, I don't quite understand what exactly is conflicting here.


This patch should be in mm tree:
https://patchwork.kernel.org/patch/9978383/

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 2/3] Makefile: support flag -fsanitizer-coverage=trace-cmp
  2017-10-09 15:05   ` Alexander Potapenko
  (?)
  (?)
@ 2017-10-10 21:53   ` kbuild test robot
  -1 siblings, 0 replies; 27+ messages in thread
From: kbuild test robot @ 2017-10-10 21:53 UTC (permalink / raw)
  To: Alexander Potapenko
  Cc: kbuild-all, akpm, mark.rutland, alex.popov, aryabinin,
	quentin.casasnovas, dvyukov, andreyknvl, keescook, vegard.nossum,
	syzkaller, linux-mm, linux-kernel

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

Hi Victor,

[auto build test ERROR on linus/master]
[also build test ERROR on v4.14-rc4]
[cannot apply to next-20171009]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Alexander-Potapenko/kcov-support-comparison-operands-collection/20171011-052025
config: i386-tinyconfig (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

>> Makefile:825: scripts/Makefile.kcov: No such file or directory
>> make[1]: *** No rule to make target 'scripts/Makefile.kcov'.
   make[1]: Failed to remake makefile 'scripts/Makefile.kcov'.
>> Makefile:825: scripts/Makefile.kcov: No such file or directory
>> make[1]: *** No rule to make target 'scripts/Makefile.kcov'.
   make[1]: Failed to remake makefile 'scripts/Makefile.kcov'.
   make: *** [sub-make] Error 2

vim +825 Makefile

   823	
   824	include scripts/Makefile.kasan
 > 825	include scripts/Makefile.kcov
   826	include scripts/Makefile.extrawarn
   827	include scripts/Makefile.ubsan
   828	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 6685 bytes --]

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 1/3] kcov: support comparison operands collection
  2017-10-10 15:34       ` Dmitry Vyukov
@ 2017-10-11  9:56         ` Alexander Potapenko
  -1 siblings, 0 replies; 27+ messages in thread
From: Alexander Potapenko @ 2017-10-11  9:56 UTC (permalink / raw)
  To: Dmitry Vyukov
  Cc: Mark Rutland, Andrew Morton, Alexander Popov, Andrey Ryabinin,
	Quentin Casasnovas, Andrey Konovalov, Kees Cook, Vegard Nossum,
	syzkaller, Linux Memory Management List, LKML

On Tue, Oct 10, 2017 at 5:34 PM, 'Dmitry Vyukov' via syzkaller
<syzkaller@googlegroups.com> wrote:
> On Tue, Oct 10, 2017 at 5:28 PM, 'Alexander Potapenko' via syzkaller
> <syzkaller@googlegroups.com> wrote:
>> On Mon, Oct 9, 2017 at 8:46 AM, Mark Rutland <mark.rutland@arm.com> wrote:
>>> Hi,
>>>
>>> I look forward to using this! :)
>>>
>>> I just have afew comments below.
>>>
>>> On Mon, Oct 09, 2017 at 05:05:19PM +0200, Alexander Potapenko wrote:
>>>> +/*
>>>> + * Defines the format for the types of collected comparisons.
>>>> + */
>>>> +enum kcov_cmp_type {
>>>> +     /*
>>>> +      * LSB shows whether one of the arguments is a compile-time constant.
>>>> +      */
>>>> +     KCOV_CMP_CONST = 1,
>>>> +     /*
>>>> +      * Second and third LSBs contain the size of arguments (1/2/4/8 bytes).
>>>> +      */
>>>> +     KCOV_CMP_SIZE1 = 0,
>>>> +     KCOV_CMP_SIZE2 = 2,
>>>> +     KCOV_CMP_SIZE4 = 4,
>>>> +     KCOV_CMP_SIZE8 = 6,
>>>> +     KCOV_CMP_SIZE_MASK = 6,
>>>> +};
>>>
>>> Given that LSB is meant to be OR-ed in, (and hence combinations of
>>> values are meaningful) I don't think it makes sense for this to be an
>>> enum. This would clearer as something like:
>>>
>>> /*
>>>  * The format for the types of collected comparisons.
>>>  *
>>>  * Bit 0 shows whether one of the arguments is a compile-time constant.
>>>  * Bits 1 & 2 contain log2 of the argument size, up to 8 bytes.
>>>  */
>>> #define KCOV_CMP_CONST          (1 << 0)
>>> #define KCOV_CMP_SIZE(n)        ((n) << 1)
>>> #define KCOV_CMP_MASK           KCOV_CMP_SIZE(3)
>> Agreed.
>>> ... I note that a few places in the kernel use a 128-bit type. Are
>>> 128-bit comparisons not instrumented?
>>>
>>> [...]
>>>
>>>> +static bool check_kcov_mode(enum kcov_mode needed_mode, struct task_struct *t)
>>>> +{
>>>> +     enum kcov_mode mode;
>>>> +
>>>> +     /*
>>>> +      * We are interested in code coverage as a function of a syscall inputs,
>>>> +      * so we ignore code executed in interrupts.
>>>> +      */
>>>> +     if (!t || !in_task())
>>>> +             return false;
>>>
>>> This !t check can go, as with the one in __sanitizer_cov_trace_pc, since
>>> t is always current, and therefore cannot be NULL.
>> Ok.
>>> IIRC there's a patch queued for that, which this may conflict with.
>> Sorry, I don't quite understand what exactly is conflicting here.
>
>
> This patch should be in mm tree:
> https://patchwork.kernel.org/patch/9978383/
Ok, I've rebased on top of it, see v4.
> --
> You received this message because you are subscribed to the Google Groups "syzkaller" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.



-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 1/3] kcov: support comparison operands collection
@ 2017-10-11  9:56         ` Alexander Potapenko
  0 siblings, 0 replies; 27+ messages in thread
From: Alexander Potapenko @ 2017-10-11  9:56 UTC (permalink / raw)
  To: Dmitry Vyukov
  Cc: Mark Rutland, Andrew Morton, Alexander Popov, Andrey Ryabinin,
	Quentin Casasnovas, Andrey Konovalov, Kees Cook, Vegard Nossum,
	syzkaller, Linux Memory Management List, LKML

On Tue, Oct 10, 2017 at 5:34 PM, 'Dmitry Vyukov' via syzkaller
<syzkaller@googlegroups.com> wrote:
> On Tue, Oct 10, 2017 at 5:28 PM, 'Alexander Potapenko' via syzkaller
> <syzkaller@googlegroups.com> wrote:
>> On Mon, Oct 9, 2017 at 8:46 AM, Mark Rutland <mark.rutland@arm.com> wrote:
>>> Hi,
>>>
>>> I look forward to using this! :)
>>>
>>> I just have afew comments below.
>>>
>>> On Mon, Oct 09, 2017 at 05:05:19PM +0200, Alexander Potapenko wrote:
>>>> +/*
>>>> + * Defines the format for the types of collected comparisons.
>>>> + */
>>>> +enum kcov_cmp_type {
>>>> +     /*
>>>> +      * LSB shows whether one of the arguments is a compile-time constant.
>>>> +      */
>>>> +     KCOV_CMP_CONST = 1,
>>>> +     /*
>>>> +      * Second and third LSBs contain the size of arguments (1/2/4/8 bytes).
>>>> +      */
>>>> +     KCOV_CMP_SIZE1 = 0,
>>>> +     KCOV_CMP_SIZE2 = 2,
>>>> +     KCOV_CMP_SIZE4 = 4,
>>>> +     KCOV_CMP_SIZE8 = 6,
>>>> +     KCOV_CMP_SIZE_MASK = 6,
>>>> +};
>>>
>>> Given that LSB is meant to be OR-ed in, (and hence combinations of
>>> values are meaningful) I don't think it makes sense for this to be an
>>> enum. This would clearer as something like:
>>>
>>> /*
>>>  * The format for the types of collected comparisons.
>>>  *
>>>  * Bit 0 shows whether one of the arguments is a compile-time constant.
>>>  * Bits 1 & 2 contain log2 of the argument size, up to 8 bytes.
>>>  */
>>> #define KCOV_CMP_CONST          (1 << 0)
>>> #define KCOV_CMP_SIZE(n)        ((n) << 1)
>>> #define KCOV_CMP_MASK           KCOV_CMP_SIZE(3)
>> Agreed.
>>> ... I note that a few places in the kernel use a 128-bit type. Are
>>> 128-bit comparisons not instrumented?
>>>
>>> [...]
>>>
>>>> +static bool check_kcov_mode(enum kcov_mode needed_mode, struct task_struct *t)
>>>> +{
>>>> +     enum kcov_mode mode;
>>>> +
>>>> +     /*
>>>> +      * We are interested in code coverage as a function of a syscall inputs,
>>>> +      * so we ignore code executed in interrupts.
>>>> +      */
>>>> +     if (!t || !in_task())
>>>> +             return false;
>>>
>>> This !t check can go, as with the one in __sanitizer_cov_trace_pc, since
>>> t is always current, and therefore cannot be NULL.
>> Ok.
>>> IIRC there's a patch queued for that, which this may conflict with.
>> Sorry, I don't quite understand what exactly is conflicting here.
>
>
> This patch should be in mm tree:
> https://patchwork.kernel.org/patch/9978383/
Ok, I've rebased on top of it, see v4.
> --
> You received this message because you are subscribed to the Google Groups "syzkaller" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.



-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 27+ messages in thread

end of thread, other threads:[~2017-10-11  9:56 UTC | newest]

Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-09 15:05 [PATCH v2 1/3] kcov: support comparison operands collection Alexander Potapenko
2017-10-09 15:05 ` Alexander Potapenko
2017-10-09 15:05 ` [PATCH v2 2/3] Makefile: support flag -fsanitizer-coverage=trace-cmp Alexander Potapenko
2017-10-09 15:05   ` Alexander Potapenko
2017-10-09 15:53   ` Andrey Ryabinin
2017-10-09 15:53     ` Andrey Ryabinin
2017-10-10 15:28     ` Alexander Potapenko
2017-10-10 15:28       ` Alexander Potapenko
2017-10-10 21:53   ` kbuild test robot
2017-10-09 15:05 ` [PATCH v2 3/3] kcov: update documentation Alexander Potapenko
2017-10-09 15:05   ` Alexander Potapenko
2017-10-09 15:46 ` [PATCH v2 1/3] kcov: support comparison operands collection Mark Rutland
2017-10-09 15:46   ` Mark Rutland
2017-10-09 18:15   ` Dmitry Vyukov
2017-10-09 18:15     ` Dmitry Vyukov
2017-10-09 18:37     ` Mark Rutland
2017-10-09 18:37       ` Mark Rutland
2017-10-09 18:46       ` Dmitry Vyukov
2017-10-09 18:46         ` Dmitry Vyukov
2017-10-10  9:56         ` Mark Rutland
2017-10-10  9:56           ` Mark Rutland
2017-10-10 15:28   ` Alexander Potapenko
2017-10-10 15:28     ` Alexander Potapenko
2017-10-10 15:34     ` Dmitry Vyukov
2017-10-10 15:34       ` Dmitry Vyukov
2017-10-11  9:56       ` Alexander Potapenko
2017-10-11  9:56         ` Alexander Potapenko

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.