Message-ID: <CACT4Y+acaVDk0X13AoxTWahR-xoA8CZ=rCio6roesQ7pLvy-cw@mail.gmail.com>
Date:	Fri, 15 Jan 2016 15:21:24 +0100
From:	Dmitry Vyukov <dvyukov@...gle.com>
To:	"Kirill A. Shutemov" <kirill@...temov.name>
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>,
	syzkaller <syzkaller@...glegroups.com>,
	Kostya Serebryany <kcc@...gle.com>,
	Alexander Potapenko <glider@...gle.com>,
	Andrey Ryabinin <ryabinin.a.a@...il.com>
Subject: Re: [PATCH v3] kernel: add kcov code coverage

On Fri, Jan 15, 2016 at 1:42 PM, Kirill A. Shutemov
<kirill@...temov.name> wrote:
> On Thu, Jan 14, 2016 at 03:22:21PM +0100, Dmitry Vyukov wrote:
>> +#include <stdio.h>
>> +#include <stddef.h>
>> +#include <stdint.h>
>> +#include <sys/types.h>
>> +#include <sys/stat.h>
>> +#include <sys/ioctl.h>
>> +#include <sys/mman.h>
>> +#include <fcntl.h>
>> +
>> +#define KCOV_INIT_TRACE                      _IOR('c', 1, unsigned long)
>> +#define KCOV_ENABLE                  _IO('c', 100)
>> +#define KCOV_DISABLE                 _IO('c', 101)
>> +#define COVER_SIZE                   (64<<10)
>> +
>> +int main(int argc, char **argv)
>> +{
>> +     int fd;
>> +     uint32_t *cover, n, i;
>> +
>> +     /* A single fd descriptor allows coverage collection on a single
>> +      * thread.
>> +      */
>> +     fd = open("/sys/kernel/debug/kcov", O_RDWR);
>> +     if (fd == -1)
>> +             perror("open");
>> +     /* Setup trace mode and trace size. */
>> +     if (ioctl(fd, KCOV_INIT_TRACE, COVER_SIZE))
>> +             perror("ioctl");
>> +     /* Mmap buffer shared between kernel- and user-space. */
>> +     cover = (uint32_t*)mmap(NULL, COVER_SIZE * sizeof(uint32_t),
>> +                             PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
>> +     if ((void*)cover == MAP_FAILED)
>> +             perror("mmap");
>> +     /* Enable coverage collection on the current thread. */
>> +     if (ioctl(fd, KCOV_ENABLE, 0))
>> +             perror("ioctl");
>> +     /* Reset coverage from the tail of the ioctl() call. */
>> +     __atomic_store_n(&cover[0], 0, __ATOMIC_RELAXED);
>> +     /* That's the target syscal call. */
>> +     read(-1, NULL, 0);
>> +     /* 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. */
>
> Why do we do this in the first place? I don't think it's very portable.
>
> I would rather trancate upper bits on 32-bit system.


Well, it is truncated to save time and space.
On hot path we fill the buffer, sort, deduplicate, compare with
another one. Plus I need to remember tens of thousands of such traces
in memory.

Any suggestions on how to keep compressed PCs for x86_64 and arm64
while supporting archs where PCs don't fir into 32 bits?


>> +     for (i = 0; i < n; i++)
>> +             printf("0xffffffff%0lx\n", (unsigned long)cover[i + 1]);
>> +     /* Disable coverage collection for the current thread. After this call
>> +      * coverage can be enabled for a different thread.
>> +      */
>> +     if (ioctl(fd, KCOV_DISABLE, 0))
>> +             perror("ioctl");
>> +     /* Free resources. */
>> +     if (munmap(cover, COVER_SIZE * sizeof(uint32_t)))
>> +             perror("munmap");
>> +     if (close(fd))
>> +             perror("close");
>> +     return 0;
>> +}
>
> ....
>
>> +static void kcov_unmap(struct vm_area_struct *vma)
>> +{
>> +     kcov_put(vma->vm_file->private_data);
>
> Can be dropped, if I'm right about mmap counterpart.

Great!
Will drop all this in a next version.

>> +}
>> +
>> +static void kcov_map_copied(struct vm_area_struct *vma)
>> +{
>> +     struct kcov *kcov;
>> +
>> +     kcov = vma->vm_file->private_data;
>> +     kcov_get(kcov);
>
> As with kcov_mmap(), I think this is redundant.
>
>> +}
>> +
>> +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;
>> +
>> +     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().
>
> No, this is other way around: mmap takes file reference (see get_file() in
> mmap_region()). So kcov_close() cannot happen, until last mmap gone.
> I think this kcov_get() is redundant.
>
>> +      */
>> +     kcov_get(kcov);
>> +     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.
>> +              * Size must be at least 2 to hold current position and one PC.
>> +              */
>> +             if (arg < 2 || arg > INT_MAX)
>> +                     return -EINVAL;
>> +             if (kcov->mode != 0)
>> +                     return -EBUSY;
>> +             kcov->mode = kcov_mode_trace;
>> +             kcov->size = arg;
>> +             return 0;
>> +     case KCOV_ENABLE:
>> +             /*
>> +              * Enable coverage for the current task.
>> +              * At this point user must have been enabled trace mode,
>> +              * and mmapped the file. Coverage collection is disabled only
>> +              * at task exit or voluntary by KCOV_DISABLE. After that it can
>> +              * be enabled for another task.
>> +              */
>> +             if (kcov->mode == 0 || kcov->area == NULL)
>> +                     return -EINVAL;
>> +             if (kcov->t != NULL)
>> +                     return -EBUSY;
>> +             t = current;
>> +             /* Cache in task struct for performance. */
>> +             t->kcov_size = kcov->size;
>> +             t->kcov_area = kcov->area;
>> +             /* See comment in __sanitizer_cov_trace_pc(). */
>> +             barrier();
>> +             WRITE_ONCE(t->kcov_mode, kcov->mode);
>> +             t->kcov = kcov;
>> +             kcov->t = t;
>> +             /* This is put either in kcov_task_exit() or in KCOV_DISABLE. */
>> +             kcov_get(kcov);
>> +             return 0;
>> +     case KCOV_DISABLE:
>> +             /* Disable coverage for the current task. */
>> +             if (current->kcov != kcov)
>> +                     return -EINVAL;
>> +             t = current;
>> +             if (WARN_ON(kcov->t != t))
>> +                     return -EINVAL;
>> +             kcov_task_init(t);
>> +             kcov->t = NULL;
>> +             kcov_put(kcov);
>> +             return 0;
>
> You probably want to verify that arg is 0 for enable/disable just in case
> if you would want to pass addtional information in the future.
>
>> +     default:
>> +             return -EINVAL;
>> +     }
>> +}
>> +
>> +static long kcov_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
>> +{
>> +     struct kcov *kcov;
>> +     int res;
>> +
>> +     kcov = filep->private_data;
>> +     spin_lock(&kcov->lock);
>> +     res = kcov_ioctl_locked(kcov, cmd, arg);
>> +     spin_unlock(&kcov->lock);
>> +     return res;
>> +}
>> +
>> +static const struct file_operations kcov_fops = {
>> +     .open           = kcov_open,
>> +     .unlocked_ioctl = kcov_ioctl,
>> +     .mmap           = kcov_mmap,
>> +     .release        = kcov_close,
>> +};
>> +
>> +static int __init kcov_init(void)
>> +{
>> +     if (!debugfs_create_file("kcov", 0666, NULL, NULL, &kcov_fops)) {
>> +             pr_err("init failed\n");
>
> I guess you should be more specific about which init failed.
>
>> +             return 1;
>> +     }
>> +     return 0;
>> +}
>> +
>> +device_initcall(kcov_init);
> --
>  Kirill A. Shutemov

Powered by blists - more mailing lists