On Fri, Mar 26, 2021 at 9:52 PM Alexander Lochmann
<info@xxxxxxxxxxxxxxxxxxxxx> 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.
> -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.
[...]
> 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.
[...]
> @@ -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.
> @@ -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!
