Re: [PATCH v2 1/3] kcov: support comparison operands collection

[Alexander Potapenko <glider@google.com>]

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

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

Agreed.

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



-- 
Alexander Potapenko
Software Engineer

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

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