All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] KCOV: Introduced tracing unique covered PCs
@ 2021-02-11  8:07 Alexander Lochmann
  2021-02-12 12:54 ` Dmitry Vyukov
  0 siblings, 1 reply; 13+ messages in thread
From: Alexander Lochmann @ 2021-02-11  8:07 UTC (permalink / raw)
  Cc: Alexander Lochmann, Dmitry Vyukov, Andrey Konovalov,
	Jonathan Corbet, Andrew Morton, Wei Yongjun, Maciej Grochowski,
	kasan-dev, linux-doc, linux-kernel

Introduced new tracing mode KCOV_MODE_UNIQUE.
It simply stores the executed PCs.
The execution order is discarded.
Each bit in the shared buffer represents every fourth
byte of the text segment.
Since a call instruction on every supported
architecture is at least four bytes, it is safe
to just store every fourth byte of the text segment.
In contrast to KCOV_MODE_TRACE_PC, the shared buffer
cannot overflow. Thus, all executed PCs are recorded.

Signed-off-by: Alexander Lochmann <info@alexander-lochmann.de>
---
 Documentation/dev-tools/kcov.rst | 80 ++++++++++++++++++++++++++++++++
 include/linux/kcov.h             |  4 +-
 include/uapi/linux/kcov.h        | 10 ++++
 kernel/kcov.c                    | 67 ++++++++++++++++++++------
 4 files changed, 147 insertions(+), 14 deletions(-)

diff --git a/Documentation/dev-tools/kcov.rst b/Documentation/dev-tools/kcov.rst
index 8548b0b04e43..4712a730a06a 100644
--- a/Documentation/dev-tools/kcov.rst
+++ b/Documentation/dev-tools/kcov.rst
@@ -127,6 +127,86 @@ 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).
 
+If someone is interested in a set of executed PCs, and does not care about
+execution order, he or she can advise KCOV to do so:
+
+.. code-block:: c
+
+    #include <stdio.h>
+    #include <stddef.h>
+    #include <stdint.h>
+    #include <stdlib.h>
+    #include <sys/types.h>
+    #include <sys/stat.h>
+    #include <sys/ioctl.h>
+    #include <sys/mman.h>
+    #include <unistd.h>
+    #include <fcntl.h>
+
+    #define KCOV_INIT_TRACE			_IOR('c', 1, unsigned long)
+    #define KCOV_INIT_UNIQUE                _IOR('c', 2, unsigned long)
+    #define KCOV_ENABLE			_IO('c', 100)
+    #define KCOV_DISABLE			_IO('c', 101)
+
+    #define BITS_PER_LONG 64
+    #define KCOV_TRACE_PC  0
+    #define KCOV_TRACE_CMP 1
+    #define KCOV_UNIQUE_PC 2
+    /*
+     * Determine start of text segment via 'nm vmlinux | grep _stext | cut -d " " -f1',
+     * and fill in.
+     */
+    #define STEXT_START 0xffffffff81000000
+
+
+
+    int main(int argc, char **argv)
+    {
+	int fd;
+	unsigned long *cover, n, i;
+
+	/* A single fd descriptor allows coverage collection on a single
+	 * thread.
+	 */
+	fd = open("/sys/kernel/debug/kcov", O_RDWR);
+	if (fd == -1)
+		perror("open"), exit(1);
+	/* Setup trace mode and trace size. */
+	if ((n = ioctl(fd, KCOV_INIT_UNIQUE, 0)) < 0)
+		perror("ioctl"), exit(1);
+	/* Mmap buffer shared between kernel- and user-space. */
+	cover = (unsigned long*)mmap(NULL, n,
+				     PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
+	if ((void*)cover == MAP_FAILED)
+		perror("mmap"), exit(1);
+	/* Enable coverage collection on the current thread. */
+	if (ioctl(fd, KCOV_ENABLE, KCOV_UNIQUE_PC))
+		perror("ioctl"), exit(1);
+	/* That's the target syscal call. */
+	read(-1, NULL, 0);
+	/* Disable coverage collection for the current thread. After this call
+	 * coverage can be enabled for a different thread.
+	 */
+	if (ioctl(fd, KCOV_DISABLE, 0))
+		perror("ioctl"), exit(1);
+        /* Convert byte size into element size */
+        n /= sizeof(unsigned long);
+        /* Print executed PCs in sorted order */
+        for (i = 0; i < n; i++) {
+            for (int j = 0; j < BITS_PER_LONG; j++) {
+                if (cover[i] & (1L << j)) {
+                    printf("0x%jx\n", (uintmax_t)(STEXT_START + (i * BITS_PER_LONG + j) * 4));
+                }
+            }
+        }
+	/* Free resources. */
+	if (munmap(cover, n * sizeof(unsigned long)))
+		perror("munmap"), exit(1);
+	if (close(fd))
+		perror("close"), exit(1);
+	return 0;
+    }
+
 Comparison operands collection
 ------------------------------
 
diff --git a/include/linux/kcov.h b/include/linux/kcov.h
index a10e84707d82..aa0c8bcf8299 100644
--- a/include/linux/kcov.h
+++ b/include/linux/kcov.h
@@ -19,7 +19,9 @@ enum kcov_mode {
 	 */
 	KCOV_MODE_TRACE_PC = 2,
 	/* Collecting comparison operands mode. */
-	KCOV_MODE_TRACE_CMP = 3,
+	KCOV_MODE_TRACE_CMP = 4,
+	/* Collecting unique covered PCs. Execution order is not saved. */
+	KCOV_MODE_UNIQUE_PC = 8,
 };
 
 #define KCOV_IN_CTXSW	(1 << 30)
diff --git a/include/uapi/linux/kcov.h b/include/uapi/linux/kcov.h
index 1d0350e44ae3..5b99b6d1a1ac 100644
--- a/include/uapi/linux/kcov.h
+++ b/include/uapi/linux/kcov.h
@@ -19,6 +19,7 @@ struct kcov_remote_arg {
 #define KCOV_REMOTE_MAX_HANDLES		0x100
 
 #define KCOV_INIT_TRACE			_IOR('c', 1, unsigned long)
+#define KCOV_INIT_UNIQUE		_IOR('c', 2, unsigned long)
 #define KCOV_ENABLE			_IO('c', 100)
 #define KCOV_DISABLE			_IO('c', 101)
 #define KCOV_REMOTE_ENABLE		_IOW('c', 102, struct kcov_remote_arg)
@@ -35,6 +36,15 @@ enum {
 	KCOV_TRACE_PC = 0,
 	/* Collecting comparison operands mode. */
 	KCOV_TRACE_CMP = 1,
+	/*
+	 * Unique coverage collection mode.
+	 * Unique covered PCs are collected in a per-task buffer.
+	 * De-duplicates the collected PCs. Execution order is *not* saved.
+	 * Each bit in the buffer represents every fourth byte of the text segment.
+	 * Since a call instruction is at least four bytes on every supported
+	 * architecture, storing just every fourth byte is sufficient.
+	 */
+	KCOV_UNIQUE_PC = 2,
 };
 
 /*
diff --git a/kernel/kcov.c b/kernel/kcov.c
index 6b8368be89c8..8f00ba6e672a 100644
--- a/kernel/kcov.c
+++ b/kernel/kcov.c
@@ -24,6 +24,7 @@
 #include <linux/refcount.h>
 #include <linux/log2.h>
 #include <asm/setup.h>
+#include <asm/sections.h>
 
 #define kcov_debug(fmt, ...) pr_debug("%s: " fmt, __func__, ##__VA_ARGS__)
 
@@ -171,7 +172,7 @@ static notrace bool check_kcov_mode(enum kcov_mode needed_mode, struct task_stru
 	 * kcov_start().
 	 */
 	barrier();
-	return mode == needed_mode;
+	return (mode & needed_mode) && !(mode & KCOV_IN_CTXSW);
 }
 
 static notrace unsigned long canonicalize_ip(unsigned long ip)
@@ -191,18 +192,26 @@ void notrace __sanitizer_cov_trace_pc(void)
 	struct task_struct *t;
 	unsigned long *area;
 	unsigned long ip = canonicalize_ip(_RET_IP_);
-	unsigned long pos;
+	unsigned long pos, idx;
 
 	t = current;
-	if (!check_kcov_mode(KCOV_MODE_TRACE_PC, t))
+	if (!check_kcov_mode(KCOV_MODE_TRACE_PC | KCOV_MODE_UNIQUE_PC, t))
 		return;
 
 	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);
+	if (likely(t->kcov_mode == KCOV_MODE_TRACE_PC)) {
+		/* 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);
+		}
+	} else {
+		idx = (ip - canonicalize_ip((unsigned long)&_stext)) / 4;
+		pos = idx % BITS_PER_LONG;
+		idx /= BITS_PER_LONG;
+		if (likely(idx < t->kcov_size))
+			WRITE_ONCE(area[idx], READ_ONCE(area[idx]) | 1L << pos);
 	}
 }
 EXPORT_SYMBOL(__sanitizer_cov_trace_pc);
@@ -474,6 +483,7 @@ static int kcov_mmap(struct file *filep, struct vm_area_struct *vma)
 		goto exit;
 	}
 	if (!kcov->area) {
+		kcov_debug("mmap(): Allocating 0x%lx bytes\n", size);
 		kcov->area = area;
 		vma->vm_flags |= VM_DONTEXPAND;
 		spin_unlock_irqrestore(&kcov->lock, flags);
@@ -515,6 +525,8 @@ static int kcov_get_mode(unsigned long arg)
 {
 	if (arg == KCOV_TRACE_PC)
 		return KCOV_MODE_TRACE_PC;
+	else if (arg == KCOV_UNIQUE_PC)
+		return KCOV_MODE_UNIQUE_PC;
 	else if (arg == KCOV_TRACE_CMP)
 #ifdef CONFIG_KCOV_ENABLE_COMPARISONS
 		return KCOV_MODE_TRACE_CMP;
@@ -562,12 +574,13 @@ static int kcov_ioctl_locked(struct kcov *kcov, unsigned int cmd,
 {
 	struct task_struct *t;
 	unsigned long size, unused;
-	int mode, i;
+	int mode, i, text_size, ret = 0;
 	struct kcov_remote_arg *remote_arg;
 	struct kcov_remote *remote;
 	unsigned long flags;
 
 	switch (cmd) {
+	case KCOV_INIT_UNIQUE:
 	case KCOV_INIT_TRACE:
 		/*
 		 * Enable kcov in trace mode and setup buffer size.
@@ -581,11 +594,39 @@ static int kcov_ioctl_locked(struct kcov *kcov, unsigned int cmd,
 		 * that must not overflow.
 		 */
 		size = arg;
-		if (size < 2 || size > INT_MAX / sizeof(unsigned long))
-			return -EINVAL;
-		kcov->size = size;
+		if (cmd == KCOV_INIT_UNIQUE) {
+			if (size != 0)
+				return -EINVAL;
+			text_size = (canonicalize_ip((unsigned long)&_etext) - canonicalize_ip((unsigned long)&_stext));
+			/**
+			 * A call instr is at least four bytes on every supported architecture.
+			 * Hence, just every fourth instruction can potentially be a call.
+			 */
+			text_size /= 4;
+			/*
+			 * Round up size of text segment to multiple of BITS_PER_LONG.
+			 * Otherwise, we cannot track
+			 * the last (text_size % BITS_PER_LONG) addresses.
+			 */
+			text_size = roundup(text_size, BITS_PER_LONG);
+			/* Get the amount of bytes needed */
+			text_size = text_size / 8;
+			/* mmap() requires size to be a multiple of PAGE_SIZE */
+			text_size = roundup(text_size, PAGE_SIZE);
+			/* Get the cover size (= amount of longs stored) */
+			ret = text_size;
+			kcov->size = text_size / sizeof(unsigned long);
+			kcov_debug("text size = 0x%lx, roundup = 0x%x, kcov->size = 0x%x\n",
+					((unsigned long)&_etext) - ((unsigned long)&_stext),
+					text_size,
+					kcov->size);
+		} else {
+			if (size < 2 || size > INT_MAX / sizeof(unsigned long))
+				return -EINVAL;
+			kcov->size = size;
+		}
 		kcov->mode = KCOV_MODE_INIT;
-		return 0;
+		return ret;
 	case KCOV_ENABLE:
 		/*
 		 * Enable coverage for the current task.
-- 
2.30.0


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

* Re: [PATCH] KCOV: Introduced tracing unique covered PCs
  2021-02-11  8:07 [PATCH] KCOV: Introduced tracing unique covered PCs Alexander Lochmann
@ 2021-02-12 12:54 ` Dmitry Vyukov
  2021-03-14 21:29   ` Alexander Lochmann
  0 siblings, 1 reply; 13+ messages in thread
From: Dmitry Vyukov @ 2021-02-12 12:54 UTC (permalink / raw)
  To: info
  Cc: Andrey Konovalov, Jonathan Corbet, Andrew Morton, Wei Yongjun,
	Maciej Grochowski, kasan-dev, open list:DOCUMENTATION, LKML,
	syzkaller

On Thu, Feb 11, 2021 at 9:07 AM Alexander Lochmann
<info@alexander-lochmann.de> wrote:
>
> Introduced new tracing mode KCOV_MODE_UNIQUE.
> It simply stores the executed PCs.
> The execution order is discarded.
> Each bit in the shared buffer represents every fourth
> byte of the text segment.
> Since a call instruction on every supported
> architecture is at least four bytes, it is safe
> to just store every fourth byte of the text segment.
> In contrast to KCOV_MODE_TRACE_PC, the shared buffer
> cannot overflow. Thus, all executed PCs are recorded.
>
> Signed-off-by: Alexander Lochmann <info@alexander-lochmann.de>
> ---
>  Documentation/dev-tools/kcov.rst | 80 ++++++++++++++++++++++++++++++++
>  include/linux/kcov.h             |  4 +-
>  include/uapi/linux/kcov.h        | 10 ++++
>  kernel/kcov.c                    | 67 ++++++++++++++++++++------
>  4 files changed, 147 insertions(+), 14 deletions(-)
>
> diff --git a/Documentation/dev-tools/kcov.rst b/Documentation/dev-tools/kcov.rst
> index 8548b0b04e43..4712a730a06a 100644
> --- a/Documentation/dev-tools/kcov.rst
> +++ b/Documentation/dev-tools/kcov.rst
> @@ -127,6 +127,86 @@ 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).
>
> +If someone is interested in a set of executed PCs, and does not care about
> +execution order, he or she can advise KCOV to do so:
> +
> +.. code-block:: c
> +
> +    #include <stdio.h>
> +    #include <stddef.h>
> +    #include <stdint.h>
> +    #include <stdlib.h>
> +    #include <sys/types.h>
> +    #include <sys/stat.h>
> +    #include <sys/ioctl.h>
> +    #include <sys/mman.h>
> +    #include <unistd.h>
> +    #include <fcntl.h>
> +
> +    #define KCOV_INIT_TRACE                    _IOR('c', 1, unsigned long)
> +    #define KCOV_INIT_UNIQUE                _IOR('c', 2, unsigned long)
> +    #define KCOV_ENABLE                        _IO('c', 100)
> +    #define KCOV_DISABLE                       _IO('c', 101)
> +
> +    #define BITS_PER_LONG 64
> +    #define KCOV_TRACE_PC  0
> +    #define KCOV_TRACE_CMP 1
> +    #define KCOV_UNIQUE_PC 2
> +    /*
> +     * Determine start of text segment via 'nm vmlinux | grep _stext | cut -d " " -f1',
> +     * and fill in.
> +     */
> +    #define STEXT_START 0xffffffff81000000
> +
> +
> +
> +    int main(int argc, char **argv)
> +    {
> +       int fd;
> +       unsigned long *cover, n, i;
> +
> +       /* A single fd descriptor allows coverage collection on a single
> +        * thread.
> +        */
> +       fd = open("/sys/kernel/debug/kcov", O_RDWR);
> +       if (fd == -1)
> +               perror("open"), exit(1);
> +       /* Setup trace mode and trace size. */
> +       if ((n = ioctl(fd, KCOV_INIT_UNIQUE, 0)) < 0)
> +               perror("ioctl"), exit(1);
> +       /* Mmap buffer shared between kernel- and user-space. */
> +       cover = (unsigned long*)mmap(NULL, n,
> +                                    PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
> +       if ((void*)cover == MAP_FAILED)
> +               perror("mmap"), exit(1);
> +       /* Enable coverage collection on the current thread. */
> +       if (ioctl(fd, KCOV_ENABLE, KCOV_UNIQUE_PC))
> +               perror("ioctl"), exit(1);
> +       /* That's the target syscal call. */
> +       read(-1, NULL, 0);
> +       /* Disable coverage collection for the current thread. After this call
> +        * coverage can be enabled for a different thread.
> +        */
> +       if (ioctl(fd, KCOV_DISABLE, 0))
> +               perror("ioctl"), exit(1);
> +        /* Convert byte size into element size */
> +        n /= sizeof(unsigned long);
> +        /* Print executed PCs in sorted order */
> +        for (i = 0; i < n; i++) {
> +            for (int j = 0; j < BITS_PER_LONG; j++) {
> +                if (cover[i] & (1L << j)) {
> +                    printf("0x%jx\n", (uintmax_t)(STEXT_START + (i * BITS_PER_LONG + j) * 4));
> +                }
> +            }
> +        }
> +       /* Free resources. */
> +       if (munmap(cover, n * sizeof(unsigned long)))
> +               perror("munmap"), exit(1);
> +       if (close(fd))
> +               perror("close"), exit(1);
> +       return 0;
> +    }
> +
>  Comparison operands collection
>  ------------------------------
>
> diff --git a/include/linux/kcov.h b/include/linux/kcov.h
> index a10e84707d82..aa0c8bcf8299 100644
> --- a/include/linux/kcov.h
> +++ b/include/linux/kcov.h
> @@ -19,7 +19,9 @@ enum kcov_mode {
>          */
>         KCOV_MODE_TRACE_PC = 2,
>         /* Collecting comparison operands mode. */
> -       KCOV_MODE_TRACE_CMP = 3,
> +       KCOV_MODE_TRACE_CMP = 4,
> +       /* Collecting unique covered PCs. Execution order is not saved. */
> +       KCOV_MODE_UNIQUE_PC = 8,
>  };
>
>  #define KCOV_IN_CTXSW  (1 << 30)
> diff --git a/include/uapi/linux/kcov.h b/include/uapi/linux/kcov.h
> index 1d0350e44ae3..5b99b6d1a1ac 100644
> --- a/include/uapi/linux/kcov.h
> +++ b/include/uapi/linux/kcov.h
> @@ -19,6 +19,7 @@ struct kcov_remote_arg {
>  #define KCOV_REMOTE_MAX_HANDLES                0x100
>
>  #define KCOV_INIT_TRACE                        _IOR('c', 1, unsigned long)
> +#define KCOV_INIT_UNIQUE               _IOR('c', 2, unsigned long)
>  #define KCOV_ENABLE                    _IO('c', 100)
>  #define KCOV_DISABLE                   _IO('c', 101)
>  #define KCOV_REMOTE_ENABLE             _IOW('c', 102, struct kcov_remote_arg)
> @@ -35,6 +36,15 @@ enum {
>         KCOV_TRACE_PC = 0,
>         /* Collecting comparison operands mode. */
>         KCOV_TRACE_CMP = 1,
> +       /*
> +        * Unique coverage collection mode.
> +        * Unique covered PCs are collected in a per-task buffer.
> +        * De-duplicates the collected PCs. Execution order is *not* saved.
> +        * Each bit in the buffer represents every fourth byte of the text segment.
> +        * Since a call instruction is at least four bytes on every supported
> +        * architecture, storing just every fourth byte is sufficient.
> +        */
> +       KCOV_UNIQUE_PC = 2,
>  };
>
>  /*
> diff --git a/kernel/kcov.c b/kernel/kcov.c
> index 6b8368be89c8..8f00ba6e672a 100644
> --- a/kernel/kcov.c
> +++ b/kernel/kcov.c
> @@ -24,6 +24,7 @@
>  #include <linux/refcount.h>
>  #include <linux/log2.h>
>  #include <asm/setup.h>
> +#include <asm/sections.h>
>
>  #define kcov_debug(fmt, ...) pr_debug("%s: " fmt, __func__, ##__VA_ARGS__)
>
> @@ -171,7 +172,7 @@ static notrace bool check_kcov_mode(enum kcov_mode needed_mode, struct task_stru
>          * kcov_start().
>          */
>         barrier();
> -       return mode == needed_mode;
> +       return (mode & needed_mode) && !(mode & KCOV_IN_CTXSW);

I see this produces an additional check and branch:

void foo1(unsigned mode) {
  if ((mode & 10) && !(mode & (1<<30)))
    foo();
}

   0: 40 f6 c7 0a          test   $0xa,%dil
   4: 74 0f                je     15 <foo1+0x15>
   6: 81 e7 00 00 00 40    and    $0x40000000,%edi
   c: 75 07                jne    15 <foo1+0x15>
   e: 31 c0                xor    %eax,%eax
  10: e9 00 00 00 00        jmpq   15 <foo1+0x15>

I think we could make KCOV_IN_CTXSW sign bit and then express the check as:

void foo2(unsigned mode) {
  if (((int)(mode & 0x8000000a)) > 0)
    foo();
}

0000000000000020 <foo2>:
  20: 81 e7 0a 00 00 80    and    $0x8000000a,%edi
  26: 7f 08                jg     30 <foo2+0x10>
  28: c3                    retq




>  }
>
>  static notrace unsigned long canonicalize_ip(unsigned long ip)
> @@ -191,18 +192,26 @@ void notrace __sanitizer_cov_trace_pc(void)
>         struct task_struct *t;
>         unsigned long *area;
>         unsigned long ip = canonicalize_ip(_RET_IP_);
> -       unsigned long pos;
> +       unsigned long pos, idx;
>
>         t = current;
> -       if (!check_kcov_mode(KCOV_MODE_TRACE_PC, t))
> +       if (!check_kcov_mode(KCOV_MODE_TRACE_PC | KCOV_MODE_UNIQUE_PC, t))
>                 return;
>
>         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);
> +       if (likely(t->kcov_mode == KCOV_MODE_TRACE_PC)) {

Does this introduce an additional real of t->kcov_mode?
If yes, please reuse the value read in check_kcov_mode.


> +               /* 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);
> +               }
> +       } else {
> +               idx = (ip - canonicalize_ip((unsigned long)&_stext)) / 4;
> +               pos = idx % BITS_PER_LONG;
> +               idx /= BITS_PER_LONG;
> +               if (likely(idx < t->kcov_size))
> +                       WRITE_ONCE(area[idx], READ_ONCE(area[idx]) | 1L << pos);
>         }
>  }
>  EXPORT_SYMBOL(__sanitizer_cov_trace_pc);
> @@ -474,6 +483,7 @@ static int kcov_mmap(struct file *filep, struct vm_area_struct *vma)
>                 goto exit;
>         }
>         if (!kcov->area) {
> +               kcov_debug("mmap(): Allocating 0x%lx bytes\n", size);
>                 kcov->area = area;
>                 vma->vm_flags |= VM_DONTEXPAND;
>                 spin_unlock_irqrestore(&kcov->lock, flags);
> @@ -515,6 +525,8 @@ static int kcov_get_mode(unsigned long arg)
>  {
>         if (arg == KCOV_TRACE_PC)
>                 return KCOV_MODE_TRACE_PC;
> +       else if (arg == KCOV_UNIQUE_PC)
> +               return KCOV_MODE_UNIQUE_PC;

As far as I understand, users can first do KCOV_INIT_UNIQUE and then
enable KCOV_TRACE_PC, or vice versa.
It looks somewhat strange. Is it intentional? It's not possible to
specify buffer size for KCOV_INIT_UNIQUE, so most likely the buffer
will be either too large or too small for a trace.




>         else if (arg == KCOV_TRACE_CMP)
>  #ifdef CONFIG_KCOV_ENABLE_COMPARISONS
>                 return KCOV_MODE_TRACE_CMP;
> @@ -562,12 +574,13 @@ static int kcov_ioctl_locked(struct kcov *kcov, unsigned int cmd,
>  {
>         struct task_struct *t;
>         unsigned long size, unused;
> -       int mode, i;
> +       int mode, i, text_size, ret = 0;
>         struct kcov_remote_arg *remote_arg;
>         struct kcov_remote *remote;
>         unsigned long flags;
>
>         switch (cmd) {
> +       case KCOV_INIT_UNIQUE:

I think nowadays you need some annotation like fallthrough here.

>         case KCOV_INIT_TRACE:
>                 /*
>                  * Enable kcov in trace mode and setup buffer size.
> @@ -581,11 +594,39 @@ static int kcov_ioctl_locked(struct kcov *kcov, unsigned int cmd,
>                  * that must not overflow.
>                  */
>                 size = arg;
> -               if (size < 2 || size > INT_MAX / sizeof(unsigned long))
> -                       return -EINVAL;
> -               kcov->size = size;
> +               if (cmd == KCOV_INIT_UNIQUE) {
> +                       if (size != 0)
> +                               return -EINVAL;
> +                       text_size = (canonicalize_ip((unsigned long)&_etext) - canonicalize_ip((unsigned long)&_stext));
> +                       /**
> +                        * A call instr is at least four bytes on every supported architecture.
> +                        * Hence, just every fourth instruction can potentially be a call.
> +                        */
> +                       text_size /= 4;

Strictly saying, we need to round up text_size to 4 before dividing by
4. Otherwise we potentially don't cover up to the last 3 bytes.


> +                       /*
> +                        * Round up size of text segment to multiple of BITS_PER_LONG.
> +                        * Otherwise, we cannot track
> +                        * the last (text_size % BITS_PER_LONG) addresses.
> +                        */
> +                       text_size = roundup(text_size, BITS_PER_LONG);
> +                       /* Get the amount of bytes needed */
> +                       text_size = text_size / 8;
> +                       /* mmap() requires size to be a multiple of PAGE_SIZE */
> +                       text_size = roundup(text_size, PAGE_SIZE);
> +                       /* Get the cover size (= amount of longs stored) */

s/longs/bytes/

> +                       ret = text_size;
> +                       kcov->size = text_size / sizeof(unsigned long);
> +                       kcov_debug("text size = 0x%lx, roundup = 0x%x, kcov->size = 0x%x\n",
> +                                       ((unsigned long)&_etext) - ((unsigned long)&_stext),
> +                                       text_size,
> +                                       kcov->size);
> +               } else {
> +                       if (size < 2 || size > INT_MAX / sizeof(unsigned long))
> +                               return -EINVAL;
> +                       kcov->size = size;
> +               }
>                 kcov->mode = KCOV_MODE_INIT;
> -               return 0;
> +               return ret;
>         case KCOV_ENABLE:
>                 /*
>                  * Enable coverage for the current task.
> --
> 2.30.0
>

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

* Re: [PATCH] KCOV: Introduced tracing unique covered PCs
  2021-02-12 12:54 ` Dmitry Vyukov
@ 2021-03-14 21:29   ` Alexander Lochmann
  2021-03-15  8:02     ` Dmitry Vyukov
  0 siblings, 1 reply; 13+ messages in thread
From: Alexander Lochmann @ 2021-03-14 21:29 UTC (permalink / raw)
  To: Dmitry Vyukov
  Cc: Andrey Konovalov, Jonathan Corbet, Andrew Morton, Wei Yongjun,
	Maciej Grochowski, kasan-dev, open list:DOCUMENTATION, LKML,
	syzkaller



On 12.02.21 13:54, Dmitry Vyukov wrote:
> 
> I think we could make KCOV_IN_CTXSW sign bit and then express the check as:
> 
> void foo2(unsigned mode) {
>   if (((int)(mode & 0x8000000a)) > 0)
>     foo();
> }
> 
> 0000000000000020 <foo2>:
>   20: 81 e7 0a 00 00 80    and    $0x8000000a,%edi
>   26: 7f 08                jg     30 <foo2+0x10>
>   28: c3                    retq
> 
So ((int)(mode & (KCOV_IN_CTXSW | needed_mode))) > 0?
> 
> 
> 
>>  }
>>
>>  static notrace unsigned long canonicalize_ip(unsigned long ip)
>> @@ -191,18 +192,26 @@ void notrace __sanitizer_cov_trace_pc(void)
>>         struct task_struct *t;
>>         unsigned long *area;
>>         unsigned long ip = canonicalize_ip(_RET_IP_);
>> -       unsigned long pos;
>> +       unsigned long pos, idx;
>>
>>         t = current;
>> -       if (!check_kcov_mode(KCOV_MODE_TRACE_PC, t))
>> +       if (!check_kcov_mode(KCOV_MODE_TRACE_PC | KCOV_MODE_UNIQUE_PC, t))
>>                 return;
>>
>>         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);
>> +       if (likely(t->kcov_mode == KCOV_MODE_TRACE_PC)) {
> 
> Does this introduce an additional real of t->kcov_mode?
> If yes, please reuse the value read in check_kcov_mode.
Okay. How do I get that value from check_kcov_mode() to the caller?
Shall I add an additional parameter to check_kcov_mode()?
> 
> 
>> +               /* 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);
>> +               }
>> +       } else {
>> +               idx = (ip - canonicalize_ip((unsigned long)&_stext)) / 4;
>> +               pos = idx % BITS_PER_LONG;
>> +               idx /= BITS_PER_LONG;
>> +               if (likely(idx < t->kcov_size))
>> +                       WRITE_ONCE(area[idx], READ_ONCE(area[idx]) | 1L << pos);
>>         }
>>  }
>>  EXPORT_SYMBOL(__sanitizer_cov_trace_pc);
>> @@ -474,6 +483,7 @@ static int kcov_mmap(struct file *filep, struct vm_area_struct *vma)
>>                 goto exit;
>>         }
>>         if (!kcov->area) {
>> +               kcov_debug("mmap(): Allocating 0x%lx bytes\n", size);
>>                 kcov->area = area;
>>                 vma->vm_flags |= VM_DONTEXPAND;
>>                 spin_unlock_irqrestore(&kcov->lock, flags);
>> @@ -515,6 +525,8 @@ static int kcov_get_mode(unsigned long arg)
>>  {
>>         if (arg == KCOV_TRACE_PC)
>>                 return KCOV_MODE_TRACE_PC;
>> +       else if (arg == KCOV_UNIQUE_PC)
>> +               return KCOV_MODE_UNIQUE_PC;
> 
> As far as I understand, users can first do KCOV_INIT_UNIQUE and then
> enable KCOV_TRACE_PC, or vice versa.
> It looks somewhat strange. Is it intentional? 
I'll fix that.
It's not possible to
> specify buffer size for KCOV_INIT_UNIQUE, so most likely the buffer
> will be either too large or too small for a trace.
No, the buffer will be calculated by KCOV_INIT_UNIQUE based on the size
of the text segment.

- Alex

-- 
Alexander Lochmann                PGP key: 0xBC3EF6FD
Heiliger Weg 72                   phone:  +49.231.28053964
D-44141 Dortmund                  mobile: +49.151.15738323

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

* Re: [PATCH] KCOV: Introduced tracing unique covered PCs
  2021-03-14 21:29   ` Alexander Lochmann
@ 2021-03-15  8:02     ` Dmitry Vyukov
  2021-03-15 21:43       ` Alexander Lochmann
  2021-03-17 21:10       ` Alexander Lochmann
  0 siblings, 2 replies; 13+ messages in thread
From: Dmitry Vyukov @ 2021-03-15  8:02 UTC (permalink / raw)
  To: Alexander Lochmann
  Cc: Andrey Konovalov, Jonathan Corbet, Andrew Morton, Wei Yongjun,
	Maciej Grochowski, kasan-dev, open list:DOCUMENTATION, LKML,
	syzkaller

On Sun, Mar 14, 2021 at 10:29 PM Alexander Lochmann
<info@alexander-lochmann.de> wrote:
> On 12.02.21 13:54, Dmitry Vyukov wrote:
> >
> > I think we could make KCOV_IN_CTXSW sign bit and then express the check as:
> >
> > void foo2(unsigned mode) {
> >   if (((int)(mode & 0x8000000a)) > 0)
> >     foo();
> > }
> >
> > 0000000000000020 <foo2>:
> >   20: 81 e7 0a 00 00 80    and    $0x8000000a,%edi
> >   26: 7f 08                jg     30 <foo2+0x10>
> >   28: c3                    retq
> >
> So ((int)(mode & (KCOV_IN_CTXSW | needed_mode))) > 0?

Frankly I lost all context now. If it results in optimal code, then, yes.

> >>  }
> >>
> >>  static notrace unsigned long canonicalize_ip(unsigned long ip)
> >> @@ -191,18 +192,26 @@ void notrace __sanitizer_cov_trace_pc(void)
> >>         struct task_struct *t;
> >>         unsigned long *area;
> >>         unsigned long ip = canonicalize_ip(_RET_IP_);
> >> -       unsigned long pos;
> >> +       unsigned long pos, idx;
> >>
> >>         t = current;
> >> -       if (!check_kcov_mode(KCOV_MODE_TRACE_PC, t))
> >> +       if (!check_kcov_mode(KCOV_MODE_TRACE_PC | KCOV_MODE_UNIQUE_PC, t))
> >>                 return;
> >>
> >>         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);
> >> +       if (likely(t->kcov_mode == KCOV_MODE_TRACE_PC)) {
> >
> > Does this introduce an additional real of t->kcov_mode?
> > If yes, please reuse the value read in check_kcov_mode.
> Okay. How do I get that value from check_kcov_mode() to the caller?
> Shall I add an additional parameter to check_kcov_mode()?

Yes, I would try to add an additional pointer parameter for mode. I
think after inlining the compiler should be able to regestrize it.

> >> +               /* 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);
> >> +               }
> >> +       } else {
> >> +               idx = (ip - canonicalize_ip((unsigned long)&_stext)) / 4;
> >> +               pos = idx % BITS_PER_LONG;
> >> +               idx /= BITS_PER_LONG;
> >> +               if (likely(idx < t->kcov_size))
> >> +                       WRITE_ONCE(area[idx], READ_ONCE(area[idx]) | 1L << pos);
> >>         }
> >>  }
> >>  EXPORT_SYMBOL(__sanitizer_cov_trace_pc);
> >> @@ -474,6 +483,7 @@ static int kcov_mmap(struct file *filep, struct vm_area_struct *vma)
> >>                 goto exit;
> >>         }
> >>         if (!kcov->area) {
> >> +               kcov_debug("mmap(): Allocating 0x%lx bytes\n", size);
> >>                 kcov->area = area;
> >>                 vma->vm_flags |= VM_DONTEXPAND;
> >>                 spin_unlock_irqrestore(&kcov->lock, flags);
> >> @@ -515,6 +525,8 @@ static int kcov_get_mode(unsigned long arg)
> >>  {
> >>         if (arg == KCOV_TRACE_PC)
> >>                 return KCOV_MODE_TRACE_PC;
> >> +       else if (arg == KCOV_UNIQUE_PC)
> >> +               return KCOV_MODE_UNIQUE_PC;
> >
> > As far as I understand, users can first do KCOV_INIT_UNIQUE and then
> > enable KCOV_TRACE_PC, or vice versa.
> > It looks somewhat strange. Is it intentional?
> I'll fix that.
> It's not possible to
> > specify buffer size for KCOV_INIT_UNIQUE, so most likely the buffer
> > will be either too large or too small for a trace.
> No, the buffer will be calculated by KCOV_INIT_UNIQUE based on the size
> of the text segment.

Yes, which will be either too large or too small for KCOV_TRACE_PC
enabled later.


> - Alex
>
> --
> Alexander Lochmann                PGP key: 0xBC3EF6FD
> Heiliger Weg 72                   phone:  +49.231.28053964
> D-44141 Dortmund                  mobile: +49.151.15738323

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

* Re: [PATCH] KCOV: Introduced tracing unique covered PCs
  2021-03-15  8:02     ` Dmitry Vyukov
@ 2021-03-15 21:43       ` Alexander Lochmann
  2021-03-16  6:36         ` Dmitry Vyukov
  2021-03-17 21:10       ` Alexander Lochmann
  1 sibling, 1 reply; 13+ messages in thread
From: Alexander Lochmann @ 2021-03-15 21:43 UTC (permalink / raw)
  To: Dmitry Vyukov
  Cc: Andrey Konovalov, Jonathan Corbet, Andrew Morton, Wei Yongjun,
	Maciej Grochowski, kasan-dev, open list:DOCUMENTATION, LKML,
	syzkaller



On 15.03.21 09:02, Dmitry Vyukov wrote:
>>>>  static notrace unsigned long canonicalize_ip(unsigned long ip)
>>>> @@ -191,18 +192,26 @@ void notrace __sanitizer_cov_trace_pc(void)
>>>>         struct task_struct *t;
>>>>         unsigned long *area;
>>>>         unsigned long ip = canonicalize_ip(_RET_IP_);
>>>> -       unsigned long pos;
>>>> +       unsigned long pos, idx;
>>>>
>>>>         t = current;
>>>> -       if (!check_kcov_mode(KCOV_MODE_TRACE_PC, t))
>>>> +       if (!check_kcov_mode(KCOV_MODE_TRACE_PC | KCOV_MODE_UNIQUE_PC, t))
>>>>                 return;
>>>>
>>>>         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);
>>>> +       if (likely(t->kcov_mode == KCOV_MODE_TRACE_PC)) {
>>>
>>> Does this introduce an additional real of t->kcov_mode?
>>> If yes, please reuse the value read in check_kcov_mode.
>> Okay. How do I get that value from check_kcov_mode() to the caller?
>> Shall I add an additional parameter to check_kcov_mode()?
> 
> Yes, I would try to add an additional pointer parameter for mode. I
> think after inlining the compiler should be able to regestrize it.
> 
Should kcov->mode be written directly to that ptr?
Otherwise, it must be written to the already present variable mode, and
than copied to the ptr (if not NULL).

- Alex

-- 
Alexander Lochmann                PGP key: 0xBC3EF6FD
Heiliger Weg 72                   phone:  +49.231.28053964
D-44141 Dortmund                  mobile: +49.151.15738323

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

* Re: [PATCH] KCOV: Introduced tracing unique covered PCs
  2021-03-15 21:43       ` Alexander Lochmann
@ 2021-03-16  6:36         ` Dmitry Vyukov
  0 siblings, 0 replies; 13+ messages in thread
From: Dmitry Vyukov @ 2021-03-16  6:36 UTC (permalink / raw)
  To: Alexander Lochmann
  Cc: Andrey Konovalov, Jonathan Corbet, Andrew Morton, Wei Yongjun,
	Maciej Grochowski, kasan-dev, open list:DOCUMENTATION, LKML,
	syzkaller

On Mon, Mar 15, 2021 at 10:43 PM Alexander Lochmann
<info@alexander-lochmann.de> wrote:
> On 15.03.21 09:02, Dmitry Vyukov wrote:
> >>>>  static notrace unsigned long canonicalize_ip(unsigned long ip)
> >>>> @@ -191,18 +192,26 @@ void notrace __sanitizer_cov_trace_pc(void)
> >>>>         struct task_struct *t;
> >>>>         unsigned long *area;
> >>>>         unsigned long ip = canonicalize_ip(_RET_IP_);
> >>>> -       unsigned long pos;
> >>>> +       unsigned long pos, idx;
> >>>>
> >>>>         t = current;
> >>>> -       if (!check_kcov_mode(KCOV_MODE_TRACE_PC, t))
> >>>> +       if (!check_kcov_mode(KCOV_MODE_TRACE_PC | KCOV_MODE_UNIQUE_PC, t))
> >>>>                 return;
> >>>>
> >>>>         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);
> >>>> +       if (likely(t->kcov_mode == KCOV_MODE_TRACE_PC)) {
> >>>
> >>> Does this introduce an additional real of t->kcov_mode?
> >>> If yes, please reuse the value read in check_kcov_mode.
> >> Okay. How do I get that value from check_kcov_mode() to the caller?
> >> Shall I add an additional parameter to check_kcov_mode()?
> >
> > Yes, I would try to add an additional pointer parameter for mode. I
> > think after inlining the compiler should be able to regestrize it.
> >
> Should kcov->mode be written directly to that ptr?
> Otherwise, it must be written to the already present variable mode, and
> than copied to the ptr (if not NULL).

I would expect that after inlining it won't make difference in
generated code. Is so, both options are fine. Whatever leads to a
cleaner code.

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

* Re: [PATCH] KCOV: Introduced tracing unique covered PCs
  2021-03-15  8:02     ` Dmitry Vyukov
  2021-03-15 21:43       ` Alexander Lochmann
@ 2021-03-17 21:10       ` Alexander Lochmann
  2021-03-18  7:17         ` Dmitry Vyukov
  1 sibling, 1 reply; 13+ messages in thread
From: Alexander Lochmann @ 2021-03-17 21:10 UTC (permalink / raw)
  To: Dmitry Vyukov
  Cc: Andrey Konovalov, Jonathan Corbet, Andrew Morton, Wei Yongjun,
	Maciej Grochowski, kasan-dev, open list:DOCUMENTATION, LKML,
	syzkaller



On 15.03.21 09:02, Dmitry Vyukov wrote:
>>> Does this introduce an additional real of t->kcov_mode?
>>> If yes, please reuse the value read in check_kcov_mode.
>> Okay. How do I get that value from check_kcov_mode() to the caller?
>> Shall I add an additional parameter to check_kcov_mode()?
> 
> Yes, I would try to add an additional pointer parameter for mode. I
> think after inlining the compiler should be able to regestrize it.
First, I'll go for the extra argument. However, the compiler doesn't
seem to inline check_kcov_mode(). Can I enforce inlining?
I'm using GCC 9.3 on Debian Testing.

- Alex

-- 
Alexander Lochmann                PGP key: 0xBC3EF6FD
Heiliger Weg 72                   phone:  +49.231.28053964
D-44141 Dortmund                  mobile: +49.151.15738323

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

* Re: [PATCH] KCOV: Introduced tracing unique covered PCs
  2021-03-17 21:10       ` Alexander Lochmann
@ 2021-03-18  7:17         ` Dmitry Vyukov
  2021-03-21 18:43           ` [PATCH] Introduced new tracing mode KCOV_MODE_UNIQUE Alexander Lochmann
  0 siblings, 1 reply; 13+ messages in thread
From: Dmitry Vyukov @ 2021-03-18  7:17 UTC (permalink / raw)
  To: Alexander Lochmann
  Cc: Andrey Konovalov, Jonathan Corbet, Andrew Morton, Wei Yongjun,
	Maciej Grochowski, kasan-dev, open list:DOCUMENTATION, LKML,
	syzkaller

On Wed, Mar 17, 2021 at 10:10 PM Alexander Lochmann
<info@alexander-lochmann.de> wrote:
> On 15.03.21 09:02, Dmitry Vyukov wrote:
> >>> Does this introduce an additional real of t->kcov_mode?
> >>> If yes, please reuse the value read in check_kcov_mode.
> >> Okay. How do I get that value from check_kcov_mode() to the caller?
> >> Shall I add an additional parameter to check_kcov_mode()?
> >
> > Yes, I would try to add an additional pointer parameter for mode. I
> > think after inlining the compiler should be able to regestrize it.
> First, I'll go for the extra argument. However, the compiler doesn't
> seem to inline check_kcov_mode(). Can I enforce inlining?
> I'm using GCC 9.3 on Debian Testing.

That's very strange and wrong. Maybe you use something like
CONFIG_CC_OPTIMIZE_FOR_SIZE=y?

With gcc-10 I am getting:

ffffffff81529ba0 <__sanitizer_cov_trace_pc>:
ffffffff81529ba0:       65 8b 05 59 53 af 7e    mov
%gs:0x7eaf5359(%rip),%eax        # 1ef00 <__preempt_count>
ffffffff81529ba7:       89 c1                   mov    %eax,%ecx
ffffffff81529ba9:       48 8b 34 24             mov    (%rsp),%rsi
ffffffff81529bad:       81 e1 00 01 00 00       and    $0x100,%ecx
ffffffff81529bb3:       65 48 8b 14 25 40 ef    mov    %gs:0x1ef40,%rdx
ffffffff81529bba:       01 00
ffffffff81529bbc:       a9 00 01 ff 00          test   $0xff0100,%eax
ffffffff81529bc1:       74 0e                   je
ffffffff81529bd1 <__sanitizer_cov_trace_pc+0x31>
ffffffff81529bc3:       85 c9                   test   %ecx,%ecx
ffffffff81529bc5:       74 35                   je
ffffffff81529bfc <__sanitizer_cov_trace_pc+0x5c>
ffffffff81529bc7:       8b 82 d4 14 00 00       mov    0x14d4(%rdx),%eax
ffffffff81529bcd:       85 c0                   test   %eax,%eax
ffffffff81529bcf:       74 2b                   je
ffffffff81529bfc <__sanitizer_cov_trace_pc+0x5c>
ffffffff81529bd1:       8b 82 b0 14 00 00       mov    0x14b0(%rdx),%eax
ffffffff81529bd7:       83 f8 02                cmp    $0x2,%eax
ffffffff81529bda:       75 20                   jne
ffffffff81529bfc <__sanitizer_cov_trace_pc+0x5c>
ffffffff81529bdc:       48 8b 8a b8 14 00 00    mov    0x14b8(%rdx),%rcx
ffffffff81529be3:       8b 92 b4 14 00 00       mov    0x14b4(%rdx),%edx
ffffffff81529be9:       48 8b 01                mov    (%rcx),%rax
ffffffff81529bec:       48 83 c0 01             add    $0x1,%rax
ffffffff81529bf0:       48 39 c2                cmp    %rax,%rdx
ffffffff81529bf3:       76 07                   jbe
ffffffff81529bfc <__sanitizer_cov_trace_pc+0x5c>
ffffffff81529bf5:       48 89 34 c1             mov    %rsi,(%rcx,%rax,8)
ffffffff81529bf9:       48 89 01                mov    %rax,(%rcx)
ffffffff81529bfc:       c3                      retq

Oh, wait gcc-9 indeed does not inline:

0000000000000070 <__sanitizer_cov_trace_pc>:
      70:       65 48 8b 0c 25 00 00    mov    %gs:0x0,%rcx
      77:       00 00
      79:       bf 02 00 00 00          mov    $0x2,%edi
      7e:       48 89 ce                mov    %rcx,%rsi
      81:       4c 8b 04 24             mov    (%rsp),%r8
      85:       e8 76 ff ff ff          callq  0 <check_kcov_mode>
      8a:       84 c0                   test   %al,%al
      8c:       74 20                   je     ae
<__sanitizer_cov_trace_pc+0x3e>
      8e:       48 8b 91 b8 14 00 00    mov    0x14b8(%rcx),%rdx
      95:       8b 89 b4 14 00 00       mov    0x14b4(%rcx),%ecx
      9b:       48 8b 02                mov    (%rdx),%rax
      9e:       48 83 c0 01             add    $0x1,%rax
      a2:       48 39 c1                cmp    %rax,%rcx
      a5:       76 07                   jbe    ae
<__sanitizer_cov_trace_pc+0x3e>
      a7:       4c 89 04 c2             mov    %r8,(%rdx,%rax,8)
      ab:       48 89 02                mov    %rax,(%rdx)
      ae:       c3                      retq

This looks like a bug in gcc-8/9. gcc-6 inlines again as well as
clang-11/12 inline.

Please add __always_inline for check_kcov_mode.

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

* [PATCH] Introduced new tracing mode KCOV_MODE_UNIQUE.
  2021-03-18  7:17         ` Dmitry Vyukov
@ 2021-03-21 18:43           ` Alexander Lochmann
  2021-03-22 12:17             ` Miguel Ojeda
  2021-03-23  7:23             ` Dmitry Vyukov
  0 siblings, 2 replies; 13+ messages in thread
From: Alexander Lochmann @ 2021-03-21 18:43 UTC (permalink / raw)
  Cc: Alexander Lochmann, Dmitry Vyukov, Andrey Konovalov,
	Jonathan Corbet, Miguel Ojeda, Randy Dunlap, Andrew Klychkov,
	Greg Kroah-Hartman, Andrew Morton, Aleksandr Nogikh,
	Jakub Kicinski, Wei Yongjun, Maciej Grochowski, kasan-dev,
	linux-doc, linux-kernel

It simply stores the executed PCs.
The execution order is discarded.
Each bit in the shared buffer represents every fourth
byte of the text segment.
Since a call instruction on every supported
architecture is at least four bytes, it is safe
to just store every fourth byte of the text segment.
In contrast to KCOV_MODE_TRACE_PC, the shared buffer
cannot overflow. Thus, all executed PCs are recorded.

Signed-off-by: Alexander Lochmann <info@alexander-lochmann.de>
---
 Documentation/dev-tools/kcov.rst | 80 +++++++++++++++++++++++++++
 include/linux/kcov.h             | 12 ++--
 include/uapi/linux/kcov.h        | 10 ++++
 kernel/kcov.c                    | 94 ++++++++++++++++++++++++--------
 4 files changed, 169 insertions(+), 27 deletions(-)

diff --git a/Documentation/dev-tools/kcov.rst b/Documentation/dev-tools/kcov.rst
index d2c4c27e1702..e105ffe6b6e3 100644
--- a/Documentation/dev-tools/kcov.rst
+++ b/Documentation/dev-tools/kcov.rst
@@ -127,6 +127,86 @@ 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).
 
+If someone is interested in a set of executed PCs, and does not care about
+execution order, he or she can advise KCOV to do so:
+
+.. code-block:: c
+
+    #include <stdio.h>
+    #include <stddef.h>
+    #include <stdint.h>
+    #include <stdlib.h>
+    #include <sys/types.h>
+    #include <sys/stat.h>
+    #include <sys/ioctl.h>
+    #include <sys/mman.h>
+    #include <unistd.h>
+    #include <fcntl.h>
+
+    #define KCOV_INIT_TRACE			_IOR('c', 1, unsigned long)
+    #define KCOV_INIT_UNIQUE                _IOR('c', 2, unsigned long)
+    #define KCOV_ENABLE			_IO('c', 100)
+    #define KCOV_DISABLE			_IO('c', 101)
+
+    #define BITS_PER_LONG 64
+    #define KCOV_TRACE_PC  0
+    #define KCOV_TRACE_CMP 1
+    #define KCOV_UNIQUE_PC 2
+    /*
+     * Determine start of text segment via 'nm vmlinux | grep _stext | cut -d " " -f1',
+     * and fill in.
+     */
+    #define STEXT_START 0xffffffff81000000
+
+
+
+    int main(int argc, char **argv)
+    {
+	int fd;
+	unsigned long *cover, n, i;
+
+	/* A single fd descriptor allows coverage collection on a single
+	 * thread.
+	 */
+	fd = open("/sys/kernel/debug/kcov", O_RDWR);
+	if (fd == -1)
+		perror("open"), exit(1);
+	/* Setup trace mode and trace size. */
+	if ((n = ioctl(fd, KCOV_INIT_UNIQUE, 0)) < 0)
+		perror("ioctl"), exit(1);
+	/* Mmap buffer shared between kernel- and user-space. */
+	cover = (unsigned long*)mmap(NULL, n,
+				     PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
+	if ((void*)cover == MAP_FAILED)
+		perror("mmap"), exit(1);
+	/* Enable coverage collection on the current thread. */
+	if (ioctl(fd, KCOV_ENABLE, KCOV_UNIQUE_PC))
+		perror("ioctl"), exit(1);
+	/* That's the target syscal call. */
+	read(-1, NULL, 0);
+	/* Disable coverage collection for the current thread. After this call
+	 * coverage can be enabled for a different thread.
+	 */
+	if (ioctl(fd, KCOV_DISABLE, 0))
+		perror("ioctl"), exit(1);
+        /* Convert byte size into element size */
+        n /= sizeof(unsigned long);
+        /* Print executed PCs in sorted order */
+        for (i = 0; i < n; i++) {
+            for (int j = 0; j < BITS_PER_LONG; j++) {
+                if (cover[i] & (1L << j)) {
+                    printf("0x%jx\n", (uintmax_t)(STEXT_START + (i * BITS_PER_LONG + j) * 4));
+                }
+            }
+        }
+	/* Free resources. */
+	if (munmap(cover, n * sizeof(unsigned long)))
+		perror("munmap"), exit(1);
+	if (close(fd))
+		perror("close"), exit(1);
+	return 0;
+    }
+
 Comparison operands collection
 ------------------------------
 
diff --git a/include/linux/kcov.h b/include/linux/kcov.h
index 4e3037dc1204..d72dd73388d1 100644
--- a/include/linux/kcov.h
+++ b/include/linux/kcov.h
@@ -12,17 +12,21 @@ 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,
+	KCOV_MODE_INIT_TRACE = 1,
+	/* KCOV was initialized, but recording of unique PCs hasn't been chosen yet. */
+	KCOV_MODE_INIT_UNQIUE = 2,
 	/*
 	 * Tracing coverage collection mode.
 	 * Covered PCs are collected in a per-task buffer.
 	 */
-	KCOV_MODE_TRACE_PC = 2,
+	KCOV_MODE_TRACE_PC = 4,
 	/* Collecting comparison operands mode. */
-	KCOV_MODE_TRACE_CMP = 3,
+	KCOV_MODE_TRACE_CMP = 8,
+	/* Collecting unique covered PCs. Execution order is not saved. */
+	KCOV_MODE_UNIQUE_PC = 16,
 };
 
-#define KCOV_IN_CTXSW	(1 << 30)
+#define KCOV_IN_CTXSW	(1 << 31)
 
 void kcov_task_init(struct task_struct *t);
 void kcov_task_exit(struct task_struct *t);
diff --git a/include/uapi/linux/kcov.h b/include/uapi/linux/kcov.h
index 1d0350e44ae3..5b99b6d1a1ac 100644
--- a/include/uapi/linux/kcov.h
+++ b/include/uapi/linux/kcov.h
@@ -19,6 +19,7 @@ struct kcov_remote_arg {
 #define KCOV_REMOTE_MAX_HANDLES		0x100
 
 #define KCOV_INIT_TRACE			_IOR('c', 1, unsigned long)
+#define KCOV_INIT_UNIQUE		_IOR('c', 2, unsigned long)
 #define KCOV_ENABLE			_IO('c', 100)
 #define KCOV_DISABLE			_IO('c', 101)
 #define KCOV_REMOTE_ENABLE		_IOW('c', 102, struct kcov_remote_arg)
@@ -35,6 +36,15 @@ enum {
 	KCOV_TRACE_PC = 0,
 	/* Collecting comparison operands mode. */
 	KCOV_TRACE_CMP = 1,
+	/*
+	 * Unique coverage collection mode.
+	 * Unique covered PCs are collected in a per-task buffer.
+	 * De-duplicates the collected PCs. Execution order is *not* saved.
+	 * Each bit in the buffer represents every fourth byte of the text segment.
+	 * Since a call instruction is at least four bytes on every supported
+	 * architecture, storing just every fourth byte is sufficient.
+	 */
+	KCOV_UNIQUE_PC = 2,
 };
 
 /*
diff --git a/kernel/kcov.c b/kernel/kcov.c
index 80bfe71bbe13..1f727043146a 100644
--- a/kernel/kcov.c
+++ b/kernel/kcov.c
@@ -24,6 +24,7 @@
 #include <linux/refcount.h>
 #include <linux/log2.h>
 #include <asm/setup.h>
+#include <asm/sections.h>
 
 #define kcov_debug(fmt, ...) pr_debug("%s: " fmt, __func__, ##__VA_ARGS__)
 
@@ -151,10 +152,8 @@ static void kcov_remote_area_put(struct kcov_remote_area *area,
 	list_add(&area->list, &kcov_remote_areas);
 }
 
-static notrace bool check_kcov_mode(enum kcov_mode needed_mode, struct task_struct *t)
+static __always_inline notrace bool check_kcov_mode(enum kcov_mode needed_mode, struct task_struct *t, unsigned int *mode)
 {
-	unsigned int mode;
-
 	/*
 	 * We are interested in code coverage as a function of a syscall inputs,
 	 * so we ignore code executed in interrupts, unless we are in a remote
@@ -162,7 +161,7 @@ static notrace bool check_kcov_mode(enum kcov_mode needed_mode, struct task_stru
 	 */
 	if (!in_task() && !(in_serving_softirq() && t->kcov_softirq))
 		return false;
-	mode = READ_ONCE(t->kcov_mode);
+	*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()).
@@ -171,7 +170,7 @@ static notrace bool check_kcov_mode(enum kcov_mode needed_mode, struct task_stru
 	 * kcov_start().
 	 */
 	barrier();
-	return mode == needed_mode;
+	return ((int)(*mode & (KCOV_IN_CTXSW | needed_mode))) > 0;
 }
 
 static notrace unsigned long canonicalize_ip(unsigned long ip)
@@ -191,18 +190,27 @@ void notrace __sanitizer_cov_trace_pc(void)
 	struct task_struct *t;
 	unsigned long *area;
 	unsigned long ip = canonicalize_ip(_RET_IP_);
-	unsigned long pos;
+	unsigned long pos, idx;
+	unsigned int mode;
 
 	t = current;
-	if (!check_kcov_mode(KCOV_MODE_TRACE_PC, t))
+	if (!check_kcov_mode(KCOV_MODE_TRACE_PC | KCOV_MODE_UNIQUE_PC, t, &mode))
 		return;
 
 	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);
+	if (likely(mode == KCOV_MODE_TRACE_PC)) {
+		/* 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);
+		}
+	} else {
+		idx = (ip - canonicalize_ip((unsigned long)&_stext)) / 4;
+		pos = idx % BITS_PER_LONG;
+		idx /= BITS_PER_LONG;
+		if (likely(idx < t->kcov_size))
+			WRITE_ONCE(area[idx], READ_ONCE(area[idx]) | 1L << pos);
 	}
 }
 EXPORT_SYMBOL(__sanitizer_cov_trace_pc);
@@ -213,9 +221,10 @@ static void notrace 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;
+	unsigned int mode;
 
 	t = current;
-	if (!check_kcov_mode(KCOV_MODE_TRACE_CMP, t))
+	if (!check_kcov_mode(KCOV_MODE_TRACE_CMP, t, &mode))
 		return;
 
 	ip = canonicalize_ip(ip);
@@ -362,7 +371,7 @@ void kcov_task_init(struct task_struct *t)
 static void kcov_reset(struct kcov *kcov)
 {
 	kcov->t = NULL;
-	kcov->mode = KCOV_MODE_INIT;
+	kcov->mode = KCOV_MODE_INIT_TRACE;
 	kcov->remote = false;
 	kcov->remote_size = 0;
 	kcov->sequence++;
@@ -468,12 +477,13 @@ static int kcov_mmap(struct file *filep, struct vm_area_struct *vma)
 
 	spin_lock_irqsave(&kcov->lock, flags);
 	size = kcov->size * sizeof(unsigned long);
-	if (kcov->mode != KCOV_MODE_INIT || vma->vm_pgoff != 0 ||
+	if (kcov->mode & ~(KCOV_INIT_TRACE | KCOV_INIT_UNIQUE) || vma->vm_pgoff != 0 ||
 	    vma->vm_end - vma->vm_start != size) {
 		res = -EINVAL;
 		goto exit;
 	}
 	if (!kcov->area) {
+		kcov_debug("mmap(): Allocating 0x%lx bytes\n", size);
 		kcov->area = area;
 		vma->vm_flags |= VM_DONTEXPAND;
 		spin_unlock_irqrestore(&kcov->lock, flags);
@@ -515,6 +525,8 @@ static int kcov_get_mode(unsigned long arg)
 {
 	if (arg == KCOV_TRACE_PC)
 		return KCOV_MODE_TRACE_PC;
+	else if (arg == KCOV_UNIQUE_PC)
+		return KCOV_MODE_UNIQUE_PC;
 	else if (arg == KCOV_TRACE_CMP)
 #ifdef CONFIG_KCOV_ENABLE_COMPARISONS
 		return KCOV_MODE_TRACE_CMP;
@@ -562,12 +574,14 @@ static int kcov_ioctl_locked(struct kcov *kcov, unsigned int cmd,
 {
 	struct task_struct *t;
 	unsigned long size, unused;
-	int mode, i;
+	int mode, i, text_size, ret = 0;
 	struct kcov_remote_arg *remote_arg;
 	struct kcov_remote *remote;
 	unsigned long flags;
 
 	switch (cmd) {
+	case KCOV_INIT_UNIQUE:
+		/* fallthrough here */
 	case KCOV_INIT_TRACE:
 		/*
 		 * Enable kcov in trace mode and setup buffer size.
@@ -581,11 +595,41 @@ static int kcov_ioctl_locked(struct kcov *kcov, unsigned int cmd,
 		 * that must not overflow.
 		 */
 		size = arg;
-		if (size < 2 || size > INT_MAX / sizeof(unsigned long))
-			return -EINVAL;
-		kcov->size = size;
-		kcov->mode = KCOV_MODE_INIT;
-		return 0;
+		if (cmd == KCOV_INIT_UNIQUE) {
+			if (size != 0)
+				return -EINVAL;
+			text_size = (canonicalize_ip((unsigned long)&_etext) - canonicalize_ip((unsigned long)&_stext));
+			/**
+			 * A call instr is at least four bytes on every supported architecture.
+			 * Hence, just every fourth instruction can potentially be a call.
+			 */
+			text_size = roundup(text_size, 4);
+			text_size /= 4;
+			/*
+			 * Round up size of text segment to multiple of BITS_PER_LONG.
+			 * Otherwise, we cannot track
+			 * the last (text_size % BITS_PER_LONG) addresses.
+			 */
+			text_size = roundup(text_size, BITS_PER_LONG);
+			/* Get the amount of bytes needed */
+			text_size = text_size / 8;
+			/* mmap() requires size to be a multiple of PAGE_SIZE */
+			text_size = roundup(text_size, PAGE_SIZE);
+			/* Get the cover size (= amount of bytes stored) */
+			ret = text_size;
+			kcov->size = text_size / sizeof(unsigned long);
+			kcov_debug("text size = 0x%lx, roundup = 0x%x, kcov->size = 0x%x\n",
+					((unsigned long)&_etext) - ((unsigned long)&_stext),
+					text_size,
+					kcov->size);
+			kcov->mode = KCOV_INIT_UNIQUE;
+		} else {
+			if (size < 2 || size > INT_MAX / sizeof(unsigned long))
+				return -EINVAL;
+			kcov->size = size;
+			kcov->mode = KCOV_INIT_TRACE;
+		}
+		return ret;
 	case KCOV_ENABLE:
 		/*
 		 * Enable coverage for the current task.
@@ -594,7 +638,7 @@ 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.
 		 */
-		if (kcov->mode != KCOV_MODE_INIT || !kcov->area)
+		if (!kcov->area)
 			return -EINVAL;
 		t = current;
 		if (kcov->t != NULL || t->kcov != NULL)
@@ -602,6 +646,10 @@ static int kcov_ioctl_locked(struct kcov *kcov, unsigned int cmd,
 		mode = kcov_get_mode(arg);
 		if (mode < 0)
 			return mode;
+		if (kcov->mode == KCOV_INIT_TRACE && mode == KCOV_MODE_UNIQUE_PC)
+			return -EINVAL;
+		if (kcov->mode == KCOV_INIT_UNIQUE && (mode & (KCOV_MODE_TRACE_PC | KCOV_MODE_TRACE_CMP)))
+			return -EINVAL;
 		kcov_fault_in_area(kcov);
 		kcov->mode = mode;
 		kcov_start(t, kcov, kcov->size, kcov->area, kcov->mode,
@@ -622,7 +670,7 @@ static int kcov_ioctl_locked(struct kcov *kcov, unsigned int cmd,
 		kcov_put(kcov);
 		return 0;
 	case KCOV_REMOTE_ENABLE:
-		if (kcov->mode != KCOV_MODE_INIT || !kcov->area)
+		if (kcov->mode != KCOV_MODE_INIT_TRACE || !kcov->area)
 			return -EINVAL;
 		t = current;
 		if (kcov->t != NULL || t->kcov != NULL)
-- 
2.30.2


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

* Re: [PATCH] Introduced new tracing mode KCOV_MODE_UNIQUE.
  2021-03-21 18:43           ` [PATCH] Introduced new tracing mode KCOV_MODE_UNIQUE Alexander Lochmann
@ 2021-03-22 12:17             ` Miguel Ojeda
  2021-03-22 21:14               ` Alexander Lochmann
  2021-03-23  7:23             ` Dmitry Vyukov
  1 sibling, 1 reply; 13+ messages in thread
From: Miguel Ojeda @ 2021-03-22 12:17 UTC (permalink / raw)
  To: Alexander Lochmann
  Cc: Dmitry Vyukov, Andrey Konovalov, Jonathan Corbet, Miguel Ojeda,
	Randy Dunlap, Andrew Klychkov, Greg Kroah-Hartman, Andrew Morton,
	Aleksandr Nogikh, Jakub Kicinski, Wei Yongjun, Maciej Grochowski,
	kasan-dev, Linux Doc Mailing List, linux-kernel

Hi Alexander,

On Sun, Mar 21, 2021 at 8:14 PM Alexander Lochmann
<info@alexander-lochmann.de> wrote:
>
> diff --git a/Documentation/dev-tools/kcov.rst b/Documentation/dev-tools/kcov.rst
> index d2c4c27e1702..e105ffe6b6e3 100644
> --- a/Documentation/dev-tools/kcov.rst
> +++ b/Documentation/dev-tools/kcov.rst
> @@ -127,6 +127,86 @@ 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).
>
> +If someone is interested in a set of executed PCs, and does not care about
> +execution order, he or she can advise KCOV to do so:

Please mention explicitly that KCOV_INIT_UNIQUE should be used for
that, i.e. readers of the example shouldn't need to read every line to
figure it out.

> +    #define KCOV_INIT_TRACE                    _IOR('c', 1, unsigned long)

Trace is not used in the example.

> +       /* KCOV was initialized, but recording of unique PCs hasn't been chosen yet. */
> +       KCOV_MODE_INIT_UNQIUE = 2,

Typo? It isn't used?

PS: not sure why I was Cc'd, but I hope that helps.

Cheers,
Miguel

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

* Re: [PATCH] Introduced new tracing mode KCOV_MODE_UNIQUE.
  2021-03-22 12:17             ` Miguel Ojeda
@ 2021-03-22 21:14               ` Alexander Lochmann
  0 siblings, 0 replies; 13+ messages in thread
From: Alexander Lochmann @ 2021-03-22 21:14 UTC (permalink / raw)
  To: Miguel Ojeda
  Cc: Dmitry Vyukov, Andrey Konovalov, Jonathan Corbet, Miguel Ojeda,
	Randy Dunlap, Andrew Klychkov, Greg Kroah-Hartman, Andrew Morton,
	Aleksandr Nogikh, Jakub Kicinski, Wei Yongjun, Maciej Grochowski,
	kasan-dev, Linux Doc Mailing List, linux-kernel



On 22.03.21 13:17, Miguel Ojeda wrote:
> Hi Alexander,
> 
> On Sun, Mar 21, 2021 at 8:14 PM Alexander Lochmann
> <info@alexander-lochmann.de> wrote:
>>
>> diff --git a/Documentation/dev-tools/kcov.rst b/Documentation/dev-tools/kcov.rst
>> index d2c4c27e1702..e105ffe6b6e3 100644
>> --- a/Documentation/dev-tools/kcov.rst
>> +++ b/Documentation/dev-tools/kcov.rst
>> @@ -127,6 +127,86 @@ 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).
>>
>> +If someone is interested in a set of executed PCs, and does not care about
>> +execution order, he or she can advise KCOV to do so:
> 
> Please mention explicitly that KCOV_INIT_UNIQUE should be used for
> that, i.e. readers of the example shouldn't need to read every line to
> figure it out.
> 
>> +    #define KCOV_INIT_TRACE                    _IOR('c', 1, unsigned long)
> 
> Trace is not used in the example.
> 
>> +       /* KCOV was initialized, but recording of unique PCs hasn't been chosen yet. */
>> +       KCOV_MODE_INIT_UNQIUE = 2,
> 
> Typo? It isn't used?
It is a typo. It should be used...
> 
> PS: not sure why I was Cc'd, but I hope that helps.
Thx for your feedback. get_maintainer.pl told me to include you in Cc.

Cheers,
Alex
> 
> Cheers,
> Miguel
> 

-- 
Alexander Lochmann                PGP key: 0xBC3EF6FD
Heiliger Weg 72                   phone:  +49.231.28053964
D-44141 Dortmund                  mobile: +49.151.15738323

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

* Re: [PATCH] Introduced new tracing mode KCOV_MODE_UNIQUE.
  2021-03-21 18:43           ` [PATCH] Introduced new tracing mode KCOV_MODE_UNIQUE Alexander Lochmann
  2021-03-22 12:17             ` Miguel Ojeda
@ 2021-03-23  7:23             ` Dmitry Vyukov
  2021-03-23  8:19               ` Alexander Lochmann
  1 sibling, 1 reply; 13+ messages in thread
From: Dmitry Vyukov @ 2021-03-23  7:23 UTC (permalink / raw)
  To: Alexander Lochmann
  Cc: Andrey Konovalov, Jonathan Corbet, Miguel Ojeda, Randy Dunlap,
	Andrew Klychkov, Greg Kroah-Hartman, Andrew Morton,
	Aleksandr Nogikh, Jakub Kicinski, Wei Yongjun, Maciej Grochowski,
	kasan-dev, open list:DOCUMENTATION, LKML, Gustavo A. R. Silva

On Sun, Mar 21, 2021 at 7:44 PM Alexander Lochmann
<info@alexander-lochmann.de> wrote:
>
> It simply stores the executed PCs.
> The execution order is discarded.
> Each bit in the shared buffer represents every fourth
> byte of the text segment.
> Since a call instruction on every supported
> architecture is at least four bytes, it is safe
> to just store every fourth byte of the text segment.
> In contrast to KCOV_MODE_TRACE_PC, the shared buffer
> cannot overflow. Thus, all executed PCs are recorded.
>
> Signed-off-by: Alexander Lochmann <info@alexander-lochmann.de>
> ---
>  Documentation/dev-tools/kcov.rst | 80 +++++++++++++++++++++++++++
>  include/linux/kcov.h             | 12 ++--
>  include/uapi/linux/kcov.h        | 10 ++++
>  kernel/kcov.c                    | 94 ++++++++++++++++++++++++--------
>  4 files changed, 169 insertions(+), 27 deletions(-)
>
> diff --git a/Documentation/dev-tools/kcov.rst b/Documentation/dev-tools/kcov.rst
> index d2c4c27e1702..e105ffe6b6e3 100644
> --- a/Documentation/dev-tools/kcov.rst
> +++ b/Documentation/dev-tools/kcov.rst
> @@ -127,6 +127,86 @@ 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).
>
> +If someone is interested in a set of executed PCs, and does not care about
> +execution order, he or she can advise KCOV to do so:
> +
> +.. code-block:: c
> +
> +    #include <stdio.h>
> +    #include <stddef.h>
> +    #include <stdint.h>
> +    #include <stdlib.h>
> +    #include <sys/types.h>
> +    #include <sys/stat.h>
> +    #include <sys/ioctl.h>
> +    #include <sys/mman.h>
> +    #include <unistd.h>
> +    #include <fcntl.h>
> +
> +    #define KCOV_INIT_TRACE                    _IOR('c', 1, unsigned long)
> +    #define KCOV_INIT_UNIQUE                _IOR('c', 2, unsigned long)
> +    #define KCOV_ENABLE                        _IO('c', 100)
> +    #define KCOV_DISABLE                       _IO('c', 101)
> +
> +    #define BITS_PER_LONG 64
> +    #define KCOV_TRACE_PC  0
> +    #define KCOV_TRACE_CMP 1
> +    #define KCOV_UNIQUE_PC 2
> +    /*
> +     * Determine start of text segment via 'nm vmlinux | grep _stext | cut -d " " -f1',
> +     * and fill in.
> +     */
> +    #define STEXT_START 0xffffffff81000000
> +
> +
> +
> +    int main(int argc, char **argv)
> +    {
> +       int fd;
> +       unsigned long *cover, n, i;
> +
> +       /* A single fd descriptor allows coverage collection on a single
> +        * thread.
> +        */
> +       fd = open("/sys/kernel/debug/kcov", O_RDWR);
> +       if (fd == -1)
> +               perror("open"), exit(1);
> +       /* Setup trace mode and trace size. */
> +       if ((n = ioctl(fd, KCOV_INIT_UNIQUE, 0)) < 0)
> +               perror("ioctl"), exit(1);
> +       /* Mmap buffer shared between kernel- and user-space. */
> +       cover = (unsigned long*)mmap(NULL, n,
> +                                    PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
> +       if ((void*)cover == MAP_FAILED)
> +               perror("mmap"), exit(1);
> +       /* Enable coverage collection on the current thread. */
> +       if (ioctl(fd, KCOV_ENABLE, KCOV_UNIQUE_PC))
> +               perror("ioctl"), exit(1);
> +       /* That's the target syscal call. */
> +       read(-1, NULL, 0);
> +       /* Disable coverage collection for the current thread. After this call
> +        * coverage can be enabled for a different thread.
> +        */
> +       if (ioctl(fd, KCOV_DISABLE, 0))
> +               perror("ioctl"), exit(1);
> +        /* Convert byte size into element size */
> +        n /= sizeof(unsigned long);
> +        /* Print executed PCs in sorted order */
> +        for (i = 0; i < n; i++) {
> +            for (int j = 0; j < BITS_PER_LONG; j++) {
> +                if (cover[i] & (1L << j)) {
> +                    printf("0x%jx\n", (uintmax_t)(STEXT_START + (i * BITS_PER_LONG + j) * 4));
> +                }
> +            }
> +        }
> +       /* Free resources. */
> +       if (munmap(cover, n * sizeof(unsigned long)))
> +               perror("munmap"), exit(1);
> +       if (close(fd))
> +               perror("close"), exit(1);
> +       return 0;
> +    }
> +
>  Comparison operands collection
>  ------------------------------
>
> diff --git a/include/linux/kcov.h b/include/linux/kcov.h
> index 4e3037dc1204..d72dd73388d1 100644
> --- a/include/linux/kcov.h
> +++ b/include/linux/kcov.h
> @@ -12,17 +12,21 @@ 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,
> +       KCOV_MODE_INIT_TRACE = 1,
> +       /* KCOV was initialized, but recording of unique PCs hasn't been chosen yet. */
> +       KCOV_MODE_INIT_UNQIUE = 2,
>         /*
>          * Tracing coverage collection mode.
>          * Covered PCs are collected in a per-task buffer.
>          */
> -       KCOV_MODE_TRACE_PC = 2,
> +       KCOV_MODE_TRACE_PC = 4,
>         /* Collecting comparison operands mode. */
> -       KCOV_MODE_TRACE_CMP = 3,
> +       KCOV_MODE_TRACE_CMP = 8,
> +       /* Collecting unique covered PCs. Execution order is not saved. */
> +       KCOV_MODE_UNIQUE_PC = 16,
>  };
>
> -#define KCOV_IN_CTXSW  (1 << 30)
> +#define KCOV_IN_CTXSW  (1 << 31)
>
>  void kcov_task_init(struct task_struct *t);
>  void kcov_task_exit(struct task_struct *t);
> diff --git a/include/uapi/linux/kcov.h b/include/uapi/linux/kcov.h
> index 1d0350e44ae3..5b99b6d1a1ac 100644
> --- a/include/uapi/linux/kcov.h
> +++ b/include/uapi/linux/kcov.h
> @@ -19,6 +19,7 @@ struct kcov_remote_arg {
>  #define KCOV_REMOTE_MAX_HANDLES                0x100
>
>  #define KCOV_INIT_TRACE                        _IOR('c', 1, unsigned long)
> +#define KCOV_INIT_UNIQUE               _IOR('c', 2, unsigned long)
>  #define KCOV_ENABLE                    _IO('c', 100)
>  #define KCOV_DISABLE                   _IO('c', 101)
>  #define KCOV_REMOTE_ENABLE             _IOW('c', 102, struct kcov_remote_arg)
> @@ -35,6 +36,15 @@ enum {
>         KCOV_TRACE_PC = 0,
>         /* Collecting comparison operands mode. */
>         KCOV_TRACE_CMP = 1,
> +       /*
> +        * Unique coverage collection mode.
> +        * Unique covered PCs are collected in a per-task buffer.
> +        * De-duplicates the collected PCs. Execution order is *not* saved.
> +        * Each bit in the buffer represents every fourth byte of the text segment.
> +        * Since a call instruction is at least four bytes on every supported
> +        * architecture, storing just every fourth byte is sufficient.
> +        */
> +       KCOV_UNIQUE_PC = 2,
>  };
>
>  /*
> diff --git a/kernel/kcov.c b/kernel/kcov.c
> index 80bfe71bbe13..1f727043146a 100644
> --- a/kernel/kcov.c
> +++ b/kernel/kcov.c
> @@ -24,6 +24,7 @@
>  #include <linux/refcount.h>
>  #include <linux/log2.h>
>  #include <asm/setup.h>
> +#include <asm/sections.h>

Is this for __always_inline?
__always_inline is defined in include/linux/compiler_types.h.


>
>  #define kcov_debug(fmt, ...) pr_debug("%s: " fmt, __func__, ##__VA_ARGS__)
>
> @@ -151,10 +152,8 @@ static void kcov_remote_area_put(struct kcov_remote_area *area,
>         list_add(&area->list, &kcov_remote_areas);
>  }
>
> -static notrace bool check_kcov_mode(enum kcov_mode needed_mode, struct task_struct *t)
> +static __always_inline notrace bool check_kcov_mode(enum kcov_mode needed_mode, struct task_struct *t, unsigned int *mode)
>  {
> -       unsigned int mode;
> -
>         /*
>          * We are interested in code coverage as a function of a syscall inputs,
>          * so we ignore code executed in interrupts, unless we are in a remote
> @@ -162,7 +161,7 @@ static notrace bool check_kcov_mode(enum kcov_mode needed_mode, struct task_stru
>          */
>         if (!in_task() && !(in_serving_softirq() && t->kcov_softirq))
>                 return false;
> -       mode = READ_ONCE(t->kcov_mode);
> +       *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()).
> @@ -171,7 +170,7 @@ static notrace bool check_kcov_mode(enum kcov_mode needed_mode, struct task_stru
>          * kcov_start().
>          */
>         barrier();
> -       return mode == needed_mode;
> +       return ((int)(*mode & (KCOV_IN_CTXSW | needed_mode))) > 0;

This logic and the rest of the patch looks good to me.

Thanks

>  }
>
>  static notrace unsigned long canonicalize_ip(unsigned long ip)
> @@ -191,18 +190,27 @@ void notrace __sanitizer_cov_trace_pc(void)
>         struct task_struct *t;
>         unsigned long *area;
>         unsigned long ip = canonicalize_ip(_RET_IP_);
> -       unsigned long pos;
> +       unsigned long pos, idx;
> +       unsigned int mode;
>
>         t = current;
> -       if (!check_kcov_mode(KCOV_MODE_TRACE_PC, t))
> +       if (!check_kcov_mode(KCOV_MODE_TRACE_PC | KCOV_MODE_UNIQUE_PC, t, &mode))
>                 return;
>
>         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);
> +       if (likely(mode == KCOV_MODE_TRACE_PC)) {
> +               /* 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);
> +               }
> +       } else {
> +               idx = (ip - canonicalize_ip((unsigned long)&_stext)) / 4;
> +               pos = idx % BITS_PER_LONG;
> +               idx /= BITS_PER_LONG;
> +               if (likely(idx < t->kcov_size))
> +                       WRITE_ONCE(area[idx], READ_ONCE(area[idx]) | 1L << pos);
>         }
>  }
>  EXPORT_SYMBOL(__sanitizer_cov_trace_pc);
> @@ -213,9 +221,10 @@ static void notrace 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;
> +       unsigned int mode;
>
>         t = current;
> -       if (!check_kcov_mode(KCOV_MODE_TRACE_CMP, t))
> +       if (!check_kcov_mode(KCOV_MODE_TRACE_CMP, t, &mode))
>                 return;
>
>         ip = canonicalize_ip(ip);
> @@ -362,7 +371,7 @@ void kcov_task_init(struct task_struct *t)
>  static void kcov_reset(struct kcov *kcov)
>  {
>         kcov->t = NULL;
> -       kcov->mode = KCOV_MODE_INIT;
> +       kcov->mode = KCOV_MODE_INIT_TRACE;
>         kcov->remote = false;
>         kcov->remote_size = 0;
>         kcov->sequence++;
> @@ -468,12 +477,13 @@ static int kcov_mmap(struct file *filep, struct vm_area_struct *vma)
>
>         spin_lock_irqsave(&kcov->lock, flags);
>         size = kcov->size * sizeof(unsigned long);
> -       if (kcov->mode != KCOV_MODE_INIT || vma->vm_pgoff != 0 ||
> +       if (kcov->mode & ~(KCOV_INIT_TRACE | KCOV_INIT_UNIQUE) || vma->vm_pgoff != 0 ||
>             vma->vm_end - vma->vm_start != size) {
>                 res = -EINVAL;
>                 goto exit;
>         }
>         if (!kcov->area) {
> +               kcov_debug("mmap(): Allocating 0x%lx bytes\n", size);
>                 kcov->area = area;
>                 vma->vm_flags |= VM_DONTEXPAND;
>                 spin_unlock_irqrestore(&kcov->lock, flags);
> @@ -515,6 +525,8 @@ static int kcov_get_mode(unsigned long arg)
>  {
>         if (arg == KCOV_TRACE_PC)
>                 return KCOV_MODE_TRACE_PC;
> +       else if (arg == KCOV_UNIQUE_PC)
> +               return KCOV_MODE_UNIQUE_PC;
>         else if (arg == KCOV_TRACE_CMP)
>  #ifdef CONFIG_KCOV_ENABLE_COMPARISONS
>                 return KCOV_MODE_TRACE_CMP;
> @@ -562,12 +574,14 @@ static int kcov_ioctl_locked(struct kcov *kcov, unsigned int cmd,
>  {
>         struct task_struct *t;
>         unsigned long size, unused;
> -       int mode, i;
> +       int mode, i, text_size, ret = 0;
>         struct kcov_remote_arg *remote_arg;
>         struct kcov_remote *remote;
>         unsigned long flags;
>
>         switch (cmd) {
> +       case KCOV_INIT_UNIQUE:
> +               /* fallthrough here */

Looking at "git log --grep fallthrough", it seems that the modern way
to say this is to use the fallthrough keyword.

Please run checkpatch, it shows a bunch of other warnings as well:

git diff HEAD^ | scripts/checkpatch.pl -


>         case KCOV_INIT_TRACE:
>                 /*
>                  * Enable kcov in trace mode and setup buffer size.
> @@ -581,11 +595,41 @@ static int kcov_ioctl_locked(struct kcov *kcov, unsigned int cmd,
>                  * that must not overflow.
>                  */
>                 size = arg;
> -               if (size < 2 || size > INT_MAX / sizeof(unsigned long))
> -                       return -EINVAL;
> -               kcov->size = size;
> -               kcov->mode = KCOV_MODE_INIT;
> -               return 0;
> +               if (cmd == KCOV_INIT_UNIQUE) {
> +                       if (size != 0)
> +                               return -EINVAL;
> +                       text_size = (canonicalize_ip((unsigned long)&_etext) - canonicalize_ip((unsigned long)&_stext));
> +                       /**
> +                        * A call instr is at least four bytes on every supported architecture.
> +                        * Hence, just every fourth instruction can potentially be a call.
> +                        */
> +                       text_size = roundup(text_size, 4);
> +                       text_size /= 4;
> +                       /*
> +                        * Round up size of text segment to multiple of BITS_PER_LONG.
> +                        * Otherwise, we cannot track
> +                        * the last (text_size % BITS_PER_LONG) addresses.
> +                        */
> +                       text_size = roundup(text_size, BITS_PER_LONG);
> +                       /* Get the amount of bytes needed */
> +                       text_size = text_size / 8;
> +                       /* mmap() requires size to be a multiple of PAGE_SIZE */
> +                       text_size = roundup(text_size, PAGE_SIZE);
> +                       /* Get the cover size (= amount of bytes stored) */
> +                       ret = text_size;
> +                       kcov->size = text_size / sizeof(unsigned long);
> +                       kcov_debug("text size = 0x%lx, roundup = 0x%x, kcov->size = 0x%x\n",
> +                                       ((unsigned long)&_etext) - ((unsigned long)&_stext),
> +                                       text_size,
> +                                       kcov->size);
> +                       kcov->mode = KCOV_INIT_UNIQUE;
> +               } else {
> +                       if (size < 2 || size > INT_MAX / sizeof(unsigned long))
> +                               return -EINVAL;
> +                       kcov->size = size;
> +                       kcov->mode = KCOV_INIT_TRACE;
> +               }
> +               return ret;
>         case KCOV_ENABLE:
>                 /*
>                  * Enable coverage for the current task.
> @@ -594,7 +638,7 @@ 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.
>                  */
> -               if (kcov->mode != KCOV_MODE_INIT || !kcov->area)
> +               if (!kcov->area)
>                         return -EINVAL;
>                 t = current;
>                 if (kcov->t != NULL || t->kcov != NULL)
> @@ -602,6 +646,10 @@ static int kcov_ioctl_locked(struct kcov *kcov, unsigned int cmd,
>                 mode = kcov_get_mode(arg);
>                 if (mode < 0)
>                         return mode;
> +               if (kcov->mode == KCOV_INIT_TRACE && mode == KCOV_MODE_UNIQUE_PC)
> +                       return -EINVAL;
> +               if (kcov->mode == KCOV_INIT_UNIQUE && (mode & (KCOV_MODE_TRACE_PC | KCOV_MODE_TRACE_CMP)))
> +                       return -EINVAL;
>                 kcov_fault_in_area(kcov);
>                 kcov->mode = mode;
>                 kcov_start(t, kcov, kcov->size, kcov->area, kcov->mode,
> @@ -622,7 +670,7 @@ static int kcov_ioctl_locked(struct kcov *kcov, unsigned int cmd,
>                 kcov_put(kcov);
>                 return 0;
>         case KCOV_REMOTE_ENABLE:
> -               if (kcov->mode != KCOV_MODE_INIT || !kcov->area)
> +               if (kcov->mode != KCOV_MODE_INIT_TRACE || !kcov->area)
>                         return -EINVAL;
>                 t = current;
>                 if (kcov->t != NULL || t->kcov != NULL)
> --
> 2.30.2
>

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

* Re: [PATCH] Introduced new tracing mode KCOV_MODE_UNIQUE.
  2021-03-23  7:23             ` Dmitry Vyukov
@ 2021-03-23  8:19               ` Alexander Lochmann
  0 siblings, 0 replies; 13+ messages in thread
From: Alexander Lochmann @ 2021-03-23  8:19 UTC (permalink / raw)
  To: Dmitry Vyukov
  Cc: Andrey Konovalov, Jonathan Corbet, Miguel Ojeda, Randy Dunlap,
	Andrew Klychkov, Greg Kroah-Hartman, Andrew Morton,
	Aleksandr Nogikh, Jakub Kicinski, Wei Yongjun, Maciej Grochowski,
	kasan-dev, open list:DOCUMENTATION, LKML, Gustavo A. R. Silva



On 23.03.21 08:23, Dmitry Vyukov wrote:
>> diff --git a/kernel/kcov.c b/kernel/kcov.c
>> index 80bfe71bbe13..1f727043146a 100644
>> --- a/kernel/kcov.c
>> +++ b/kernel/kcov.c
>> @@ -24,6 +24,7 @@
>>  #include <linux/refcount.h>
>>  #include <linux/log2.h>
>>  #include <asm/setup.h>
>> +#include <asm/sections.h>
> 
> Is this for __always_inline?
> __always_inline is defined in include/linux/compiler_types.h.
> 
This is for the symbols marking start and end of the text segment
(_stext/_etext).
> 
>>
>>  #define kcov_debug(fmt, ...) pr_debug("%s: " fmt, __func__, ##__VA_ARGS__)
>>
>> @@ -151,10 +152,8 @@ static void kcov_remote_area_put(struct kcov_remote_area *area,
>>         list_add(&area->list, &kcov_remote_areas);
>>  }
>>
>> -static notrace bool check_kcov_mode(enum kcov_mode needed_mode, struct task_struct *t)
>> +static __always_inline notrace bool check_kcov_mode(enum kcov_mode needed_mode, struct task_struct *t, unsigned int *mode)
>>  {
>> -       unsigned int mode;
>> -
>>         /*
>>          * We are interested in code coverage as a function of a syscall inputs,
>>          * so we ignore code executed in interrupts, unless we are in a remote
>> @@ -162,7 +161,7 @@ static notrace bool check_kcov_mode(enum kcov_mode needed_mode, struct task_stru
>>          */
>>         if (!in_task() && !(in_serving_softirq() && t->kcov_softirq))
>>                 return false;
>> -       mode = READ_ONCE(t->kcov_mode);
>> +       *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()).
>> @@ -171,7 +170,7 @@ static notrace bool check_kcov_mode(enum kcov_mode needed_mode, struct task_stru
>>          * kcov_start().
>>          */
>>         barrier();
>> -       return mode == needed_mode;
>> +       return ((int)(*mode & (KCOV_IN_CTXSW | needed_mode))) > 0;
> 
> This logic and the rest of the patch looks good to me.
> 
> Thanks
Thx.
> 
>>  }
>>
>>  static notrace unsigned long canonicalize_ip(unsigned long ip)
>> @@ -191,18 +190,27 @@ void notrace __sanitizer_cov_trace_pc(void)
>>         struct task_struct *t;
>>         unsigned long *area;
>>         unsigned long ip = canonicalize_ip(_RET_IP_);
>> -       unsigned long pos;
>> +       unsigned long pos, idx;
>> +       unsigned int mode;
>>
>>         t = current;
>> -       if (!check_kcov_mode(KCOV_MODE_TRACE_PC, t))
>> +       if (!check_kcov_mode(KCOV_MODE_TRACE_PC | KCOV_MODE_UNIQUE_PC, t, &mode))
>>                 return;
>>
>>         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);
>> +       if (likely(mode == KCOV_MODE_TRACE_PC)) {
>> +               /* 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);
>> +               }
>> +       } else {
>> +               idx = (ip - canonicalize_ip((unsigned long)&_stext)) / 4;
>> +               pos = idx % BITS_PER_LONG;
>> +               idx /= BITS_PER_LONG;
>> +               if (likely(idx < t->kcov_size))
>> +                       WRITE_ONCE(area[idx], READ_ONCE(area[idx]) | 1L << pos);
>>         }
>>  }
>>  EXPORT_SYMBOL(__sanitizer_cov_trace_pc);
>> @@ -213,9 +221,10 @@ static void notrace 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;
>> +       unsigned int mode;
>>
>>         t = current;
>> -       if (!check_kcov_mode(KCOV_MODE_TRACE_CMP, t))
>> +       if (!check_kcov_mode(KCOV_MODE_TRACE_CMP, t, &mode))
>>                 return;
>>
>>         ip = canonicalize_ip(ip);
>> @@ -362,7 +371,7 @@ void kcov_task_init(struct task_struct *t)
>>  static void kcov_reset(struct kcov *kcov)
>>  {
>>         kcov->t = NULL;
>> -       kcov->mode = KCOV_MODE_INIT;
>> +       kcov->mode = KCOV_MODE_INIT_TRACE;
>>         kcov->remote = false;
>>         kcov->remote_size = 0;
>>         kcov->sequence++;
>> @@ -468,12 +477,13 @@ static int kcov_mmap(struct file *filep, struct vm_area_struct *vma)
>>
>>         spin_lock_irqsave(&kcov->lock, flags);
>>         size = kcov->size * sizeof(unsigned long);
>> -       if (kcov->mode != KCOV_MODE_INIT || vma->vm_pgoff != 0 ||
>> +       if (kcov->mode & ~(KCOV_INIT_TRACE | KCOV_INIT_UNIQUE) || vma->vm_pgoff != 0 ||
>>             vma->vm_end - vma->vm_start != size) {
>>                 res = -EINVAL;
>>                 goto exit;
>>         }
>>         if (!kcov->area) {
>> +               kcov_debug("mmap(): Allocating 0x%lx bytes\n", size);
>>                 kcov->area = area;
>>                 vma->vm_flags |= VM_DONTEXPAND;
>>                 spin_unlock_irqrestore(&kcov->lock, flags);
>> @@ -515,6 +525,8 @@ static int kcov_get_mode(unsigned long arg)
>>  {
>>         if (arg == KCOV_TRACE_PC)
>>                 return KCOV_MODE_TRACE_PC;
>> +       else if (arg == KCOV_UNIQUE_PC)
>> +               return KCOV_MODE_UNIQUE_PC;
>>         else if (arg == KCOV_TRACE_CMP)
>>  #ifdef CONFIG_KCOV_ENABLE_COMPARISONS
>>                 return KCOV_MODE_TRACE_CMP;
>> @@ -562,12 +574,14 @@ static int kcov_ioctl_locked(struct kcov *kcov, unsigned int cmd,
>>  {
>>         struct task_struct *t;
>>         unsigned long size, unused;
>> -       int mode, i;
>> +       int mode, i, text_size, ret = 0;
>>         struct kcov_remote_arg *remote_arg;
>>         struct kcov_remote *remote;
>>         unsigned long flags;
>>
>>         switch (cmd) {
>> +       case KCOV_INIT_UNIQUE:
>> +               /* fallthrough here */
> 
> Looking at "git log --grep fallthrough", it seems that the modern way
> to say this is to use the fallthrough keyword.
> 
> Please run checkpatch, it shows a bunch of other warnings as well:
> 
> git diff HEAD^ | scripts/checkpatch.pl -
Yeah. I'll do that.

-- 
Alexander Lochmann                PGP key: 0xBC3EF6FD
Heiliger Weg 72                   phone:  +49.231.28053964
D-44141 Dortmund                  mobile: +49.151.15738323

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

end of thread, other threads:[~2021-03-23  8:29 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-11  8:07 [PATCH] KCOV: Introduced tracing unique covered PCs Alexander Lochmann
2021-02-12 12:54 ` Dmitry Vyukov
2021-03-14 21:29   ` Alexander Lochmann
2021-03-15  8:02     ` Dmitry Vyukov
2021-03-15 21:43       ` Alexander Lochmann
2021-03-16  6:36         ` Dmitry Vyukov
2021-03-17 21:10       ` Alexander Lochmann
2021-03-18  7:17         ` Dmitry Vyukov
2021-03-21 18:43           ` [PATCH] Introduced new tracing mode KCOV_MODE_UNIQUE Alexander Lochmann
2021-03-22 12:17             ` Miguel Ojeda
2021-03-22 21:14               ` Alexander Lochmann
2021-03-23  7:23             ` Dmitry Vyukov
2021-03-23  8:19               ` Alexander Lochmann

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.