Re: [PATCHv3] Introduced new tracing mode KCOV_MODE_UNIQUE.
From: Andrey Konovalov <andreyknvl@gmail.com>
Date: 2021-03-27 14:57:33
Also in:
lkml
On Fri, Mar 26, 2021 at 9:52 PM Alexander Lochmann [off-list ref] wrote:
Hi Alexander,
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.
What about jumps? [...]
-#define KCOV_IN_CTXSW (1 << 30) +#define KCOV_IN_CTXSW (1 << 31)
This change needs to be mentioned and explained in the changelog.
quoted hunk ↗ jump to hunk
-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 +163,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 +172,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 change needs to be mentioned and explained in the changelog. [...]
quoted hunk ↗ jump to hunk
static notrace unsigned long canonicalize_ip(unsigned long ip)@@ -191,18 +192,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);
This is confusing: for KCOV_MODE_TRACE_PC, pos is used to index area, and for else, idx is used to index area. You should swap idx and pos. [...]
quoted hunk ↗ jump to hunk
@@ -213,9 +223,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;
mode isn't used here, right? No need for it then.
quoted hunk ↗ jump to hunk
@@ -562,12 +576,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; case KCOV_INIT_TRACE: /* * Enable kcov in trace mode and setup buffer size.@@ -581,11 +597,42 @@ 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) {
Let's put this code under KCOV_INIT_UNIQUE in the switch. This internal if only saves duplicating two lines of code, which isn't worth it.
+ 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_MODE_INIT_UNIQUE;
+ } else {
+ if (size < 2 || size > INT_MAX / sizeof(unsigned long))
+ return -EINVAL;
+ kcov->size = size;
+ kcov->mode = KCOV_MODE_INIT_TRACE;
+ }
+ return ret;Thanks!