Message-ID: <CACT4Y+bHjfkrZ4jWZf+j6FKnj2GQv1t94Y9LJm4=WFyb8C0J8w@mail.gmail.com>
Date:	Thu, 14 Jan 2016 15:30:34 +0100
From:	Dmitry Vyukov <dvyukov@...gle.com>
To:	syzkaller <syzkaller@...glegroups.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	David Drysdale <drysdale@...gle.com>,
	Kees Cook <keescook@...gle.com>,
	Quentin Casasnovas <quentin.casasnovas@...cle.com>,
	Sasha Levin <sasha.levin@...cle.com>,
	Vegard Nossum <vegard.nossum@...cle.com>,
	LKML <linux-kernel@...r.kernel.org>,
	Eric Dumazet <edumazet@...gle.com>,
	Tavis Ormandy <taviso@...gle.com>,
	Bjorn Helgaas <bhelgaas@...gle.com>,
	Kostya Serebryany <kcc@...gle.com>,
	Alexander Potapenko <glider@...gle.com>
Subject: Re: [PATCH v2] kernel: add kcov code coverage

On Thu, Jan 14, 2016 at 11:50 AM, Andrey Ryabinin
<ryabinin.a.a@...il.com> wrote:
> 2016-01-13 15:48 GMT+03:00 Dmitry Vyukov <dvyukov@...gle.com>:
>
>> +       /* Read number of PCs collected. */
>> +       n = __atomic_load_n(&cover[0], __ATOMIC_RELAXED);
>> +       /* PCs are shorten to uint32_t, so we need to restore the upper part. */
>> +       for (i = 0; i < n; i++)
>> +               printf("0xffffffff%0lx\n", (unsigned long)cover[i + 1]);

Thanks for the review!
Mailed v3 with fixes.
Comments inline.

> This works only for x86-64.
> Probably there is no simple way to make this arch-independent with
> 32-bit values.

We probably could add an ioctl that returns base of the stripped PCs.
Not done in v3.

>> diff --git a/include/linux/sched.h b/include/linux/sched.h
>> index 61aa9bb..9e9e9f6 100644
>> --- a/include/linux/sched.h
>> +++ b/include/linux/sched.h
>> @@ -1807,6 +1807,16 @@ struct task_struct {
>>         /* bitmask and counter of trace recursion */
>>         unsigned long trace_recursion;
>>  #endif /* CONFIG_TRACING */
>> +#ifdef CONFIG_KCOV
>> +       /* Coverage collection mode enabled for this task (0 if disabled). */
>> +       int             kcov_mode;
>> +       /* Size of the kcov_area. */
>> +       unsigned long   kcov_size;
>
> Could be just 'unsigned'

Done

>> +       /* Buffer for coverage collection. */
>> +       void            *kcov_area;
>
> So, these fields above are duplicates the same fields from kcov struct.
> Consider embedding kcov struct (since it's relatively small) into task_struct.

It would be strange to copy spinlock and refcounter. Also if
additional fields added to kcov struct, most likely they won't need to
be copied to task struct. Also I did not want to pull my headers into
sched.h.
How strong are you about this? I would leave it as is.


>> +       /* kcov desciptor wired with this task or NULL. */
>> +       void            *kcov;
>
> struct kcov *kcov;

Done

>> +#endif
>>  #ifdef CONFIG_MEMCG
>>         struct mem_cgroup *memcg_in_oom;
>>         gfp_t memcg_oom_gfp_mask;
>
>
>
>> diff --git a/kernel/kcov/Makefile b/kernel/kcov/Makefile
>> new file mode 100644
>> index 0000000..88892b7
>> --- /dev/null
>> +++ b/kernel/kcov/Makefile
>> @@ -0,0 +1,5 @@
>> +KCOV_INSTRUMENT := n
>> +KASAN_SANITIZE := n
>> +
>> +obj-y := kcov.o
>> +
>
> New directory for just one file seems like overkill.

Agree. Done.
Initially I just copied gcov structure. But gcov is beefier.

>> diff --git a/kernel/kcov/kcov.c b/kernel/kcov/kcov.c
>> new file mode 100644
>> index 0000000..05ec361
>> --- /dev/null
>> +++ b/kernel/kcov/kcov.c
>
>> +
>> +enum kcov_mode {
>> +       /* Tracing coverage collection mode.
>> +        * Covered PCs are collected in a per-task buffer.
>> +        */
>
> Multi-line comment format is following:
> /*
>  * <text>
>  * <text>
>  */

Done

>> +       kcov_mode_trace = 1,
>> +};
>> +
>> +/* kcov descriptor (one per opened debugfs file). */
>> +struct kcov {
>> +       /* Reference counter. We keep one for:
>> +        *  - opened file descriptor
>> +        *  - mmapped region (including copies after fork)
>> +        *  - task with enabled coverage (we can't unwire it from another task)
>> +        */
>> +       atomic_t                rc;
>> +       /* The lock protects state transitions of the descriptor:
>
> Just say what fields are protected by this lock. Comment bellow looks
> unrelated to the lock itself,
> I think it should be somewhere else.

Done


>> +        *  - initial state after open()
>> +        *  - then there must be a single ioctl(KCOV_INIT_TRACE) call
>> +        *  - then, mmap() call (several calls are allowed but not useful)
>> +        *  - then, repeated enable/disable for a task (only one task a time
>> +        *    allowed
>> +        */
>> +       spinlock_t              lock;
>> +       enum kcov_mode          mode;
>> +       unsigned long           size;
>> +       void                    *area;
>> +       struct task_struct      *t;
>> +};
>> +
>> +/* Entry point from instrumented code.
>> + * This is called once per basic-block/edge.
>> + */
>> +void __sanitizer_cov_trace_pc(void)
>> +{
>> +       struct task_struct *t;
>> +       enum kcov_mode mode;
>> +
>> +       t = current;
>> +       /* We are interested in code coverage as a function of a syscall inputs,
>> +        * so we ignore code executed in interrupts.
>> +        */
>> +       if (!t || in_interrupt())
>> +               return;
>> +       mode = READ_ONCE(t->kcov_mode);
>> +       if (mode == kcov_mode_trace) {
>> +               u32 *area;
>> +               u32 pos;
>> +
>> +               /* 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();
>> +               area = t->kcov_area;
>> +               /* The first u32 is number of subsequent PCs. */
>> +               pos = READ_ONCE(area[0]) + 1;
>> +               if (likely(pos < t->kcov_size)) {
>> +                       area[pos] = (u32)_RET_IP_;
>> +                       WRITE_ONCE(area[0], pos);
>
> Note that this works only for cache-coherent architectures.
> For incoherent arches you'll need to flush_dcache_page() somewhere.
> Perhaps it could be done on exit to userspace, since flushing here is
> certainly an overkill.

I can say that I understand the problem. Does it have to do with the
fact that the buffer is shared between kernel and user-space?
Current code is OK from the plain multi-threading side, as user must
not read buffer concurrently with writing (that would not yield
anything useful).
We could add an ioctl that does the flush. But I would prefer if it is
done when we port kcov to such an arch. Does arm64 require the flush?


>> +               }
>> +       }
>> +}
>> +EXPORT_SYMBOL(__sanitizer_cov_trace_pc);
>> +
>> +static void kcov_put(struct kcov *kcov)
>> +{
>> +       if (atomic_dec_and_test(&kcov->rc)) {
>> +               vfree(kcov->area);
>> +               kfree(kcov);
>> +       }
>> +}
>> +
>> +void kcov_task_init(struct task_struct *t)
>> +{
>> +       t->kcov_mode = 0;
>> +       t->kcov_size = 0;
>> +       t->kcov_area = NULL;
>> +       t->kcov = NULL;
>> +}
>> +
>> +void kcov_task_exit(struct task_struct *t)
>> +{
>> +       struct kcov *kcov;
>> +
>> +       kcov = t->kcov;
>> +       if (kcov == NULL)
>> +               return;
>> +       spin_lock(&kcov->lock);
>> +       BUG_ON(kcov->t != t);
>
> WARN_ON please. It's not a fatal condition, so you should not kill the
> machine here..
>
> And please and reconsider other BUG_ONs too.
> Use BUG/BUG_ON iff you can't do anything but killing machine.

Done

>> +       /* Just to not leave dangling references behind. */
>> +       kcov_task_init(t);
>> +       kcov->t = NULL;
>> +       spin_unlock(&kcov->lock);
>> +       kcov_put(kcov);
>> +}
>> +
>> +static int kcov_vm_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
>> +{
>> +       struct kcov *kcov;
>> +       unsigned long off;
>> +       struct page *page;
>> +
>> +       /* Map the preallocated kcov->area. */
>> +       kcov = vma->vm_file->private_data;
>> +       off = vmf->pgoff << PAGE_SHIFT;
>> +       if (off >= kcov->size * sizeof(u32))
>> +               return -1;
>
> return VM_FAULT_SIGSEGV;

Done

>> +       page = vmalloc_to_page(kcov->area + off);
>> +       get_page(page);
>> +       vmf->page = page;
>> +       return 0;
>> +}
>> +
>> +static void kcov_unmap(struct vm_area_struct *vma)
>> +{
>> +       kcov_put(vma->vm_file->private_data);
>> +}
>> +
>> +static void kcov_map_copied(struct vm_area_struct *vma)
>> +{
>> +       struct kcov *kcov;
>> +
>> +       kcov = vma->vm_file->private_data;
>> +       atomic_inc(&kcov->rc);
>
> Add kcov_get(struct kcov *) helper. It much better pairs with kcov_put().

Done

>> +}
>> +
>> +static const struct vm_operations_struct kcov_vm_ops = {
>> +       .fault = kcov_vm_fault,
>> +       .close = kcov_unmap,
>> +       /* Called on fork()/clone() when the mapping is copied. */
>> +       .open  = kcov_map_copied,
>> +};
>> +
>> +static int kcov_mmap(struct file *filep, struct vm_area_struct *vma)
>> +{
>> +       int res = 0;
>> +       void *area;
>> +       struct kcov *kcov = vma->vm_file->private_data;
>> +
>> +       /* Can't call vmalloc_user() under a spinlock. */
>
> This comment doesn't bring any valuable information.

Removed

>> +       area = vmalloc_user(vma->vm_end - vma->vm_start);
>> +       if (!area)
>> +               return -ENOMEM;
>> +
>> +       spin_lock(&kcov->lock);
>> +       if (kcov->mode == 0 || vma->vm_pgoff != 0 ||
>> +           vma->vm_end - vma->vm_start != kcov->size * sizeof(u32)) {
>> +               res = -EINVAL;
>> +               goto exit;
>> +       }
>> +       if (!kcov->area) {
>> +               kcov->area = area;
>> +               area = NULL;
>> +       }
>> +       /* The file drops a reference on close, but the file
>> +        * descriptor can be closed with the mmaping still alive so we keep
>> +        * a reference for those.  This is put in kcov_unmap().
>> +        */
>> +       atomic_inc(&kcov->rc);
>> +       vma->vm_ops = &kcov_vm_ops;
>> +exit:
>> +       spin_unlock(&kcov->lock);
>> +       vfree(area);
>> +       return res;
>> +}
>> +
>> +static int kcov_open(struct inode *inode, struct file *filep)
>> +{
>> +       struct kcov *kcov;
>> +
>> +       kcov = kzalloc(sizeof(*kcov), GFP_KERNEL);
>> +       if (!kcov)
>> +               return -ENOMEM;
>> +       atomic_set(&kcov->rc, 1);
>> +       spin_lock_init(&kcov->lock);
>> +       filep->private_data = kcov;
>> +       return nonseekable_open(inode, filep);
>> +}
>> +
>> +static int kcov_close(struct inode *inode, struct file *filep)
>> +{
>> +       kcov_put(filep->private_data);
>> +       return 0;
>> +}
>> +
>> +static int kcov_ioctl_locked(struct kcov *kcov, unsigned int cmd,
>> +                            unsigned long arg)
>> +{
>> +       struct task_struct *t;
>> +
>> +       switch (cmd) {
>> +       case KCOV_INIT_TRACE:
>> +               /* Enable kcov in trace mode and setup buffer size.
>> +                * Must happen before anything else.
>> +                */
>> +               if (arg < 256 || arg > (128<<20) || arg & (arg - 1))
>
> Use constants/defines with some meaningful name.
> And for the last condition: !is_power_of_2(arg).
> BTW why it has to be power of 2?

Removed the magical constants.
Power of 2 is left-over from a previous approach.

Powered by blists - more mailing lists