On Sat, Mar 27, 2021 at 3:56 PM Andrey Konovalov <andreyk...@gmail.com> wrote:
>
> On Fri, Mar 26, 2021 at 9:52 PM Alexander Lochmann
> <i...@alexander-lochmann.de> 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?
KCOV adds call __sanitizer_cov_trace_pc per coverage point. So besides
the instructions in the original code, we also always have this call.
> [...]
>
> > -#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!
>
> --
> You received this message because you are subscribed to the Google Groups
> "kasan-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to kasan-dev+unsubscr...@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/kasan-dev/CA%2BfCnZcTi%3DQLGC_LCdhs%2BfMrxkqX66kXEuM5ewOmjVjifKzUrw%40mail.gmail.com.
