[PATCH v4] kernel: add kcov code coverage

[Dmitry Vyukov]

On Mon, Jan 18, 2016 at 11:55 PM, Kirill A. Shutemov
<kirill at shutemov.name> wrote:
> On Mon, Jan 18, 2016 at 08:25:16PM +0100, Dmitry Vyukov wrote:
>> +enum kcov_mode {
>> +     /*
>> +      * Tracing coverage collection mode.
>> +      * Covered PCs are collected in a per-task buffer.
>> +      */
>> +     kcov_mode_trace = 1,
>
> We usually use upper case for items in enum.

Done in v5

>> +};
>> +
>> +/*
>> + * kcov descriptor (one per opened debugfs file).
>> + * State transitions of the descriptor:
>> + *  - 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)
>> + */
>> +struct kcov {
>> +     /*
>> +      * Reference counter. We keep one for:
>> +      *  - opened file descriptor
>> +      *  - mmapped region (including copies after fork)
>
> Outdated.

Removed in v5

>> +      *  - task with enabled coverage (we can't unwire it from another task)
>> +      */
>> +     atomic_t                rc;
>> +     /* The lock protects mode, size, area and t. */
>> +     spinlock_t              lock;
>> +     enum kcov_mode          mode;
>> +     unsigned                size;
>> +     void                    *area;
>> +     struct task_struct      *t;
>> +};
>
> ...l
>> +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;
>> +     unsigned long size, off;
>> +     struct page *page;
>> +
>> +     area = vmalloc_user(vma->vm_end - vma->vm_start);
>> +     if (!area)
>> +             return -ENOMEM;
>> +
>> +     spin_lock(&kcov->lock);
>> +     size = kcov->size * sizeof(u32);
>> +     if (kcov->mode == 0 || vma->vm_pgoff != 0 ||
>> +         vma->vm_end - vma->vm_start != size) {
>> +             res = -EINVAL;
>> +             goto exit;
>> +     }
>> +     if (!kcov->area) {
>> +             kcov->area = area;
>> +             vma->vm_flags |= VM_DONTEXPAND;
>> +             spin_unlock(&kcov->lock);
>> +             for (off = 0; off < size; off += PAGE_SIZE) {
>> +                     page = vmalloc_to_page(kcov->area + off);
>> +                     vm_insert_page(vma, vma->vm_start + off, page);
>
> At least WARN_ONCE() for non-zero return code, meaning something is
> horribly wrong.

Done in v5

>> +             }
>> +             return 0;
>> +     }
>> +exit:
>> +     spin_unlock(&kcov->lock);
>> +     vfree(area);
>> +     return res;
>> +}
>
> ....
>
>> +static long kcov_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
>> +{
>> +     struct kcov *kcov = filep->private_data;
>> +     struct task_struct *t;
>> +     struct kcov_init_trace init;
>> +
>> +     switch (cmd) {
>> +     case KCOV_INIT_TRACE:
>> +             if (copy_from_user(&init, (void __user *)arg, sizeof(init)))
>> +                     return -EFAULT;
>> +             /*
>> +              * Size must be at least 2 to hold current position and one PC.
>> +              */
>> +             if (init.flags != 0 || init.size < 2 || init.size > INT_MAX)
>> +                     return -EINVAL;
>> +             /*
>> +              * Enable kcov in trace mode and setup buffer size.
>> +              * Must happen before anything else.
>> +              */
>> +             spin_lock(&kcov->lock);
>> +             if (kcov->mode != 0) {
>> +                     spin_unlock(&kcov->lock);
>> +                     return -EBUSY;
>> +             }
>> +             kcov->mode = kcov_mode_trace;
>> +             kcov->size = init.size;
>> +             init.version = 1;
>> +             init.pc_size = sizeof(u32);
>> +             init.pc_base = ((1ull << 32) - 1) << 32;
>> +             spin_unlock(&kcov->lock);
>> +             if (copy_to_user((void __user *)arg, &init, sizeof(init)))
>> +                     return -EFAULT;
>> +             return 0;
>> +     case KCOV_ENABLE:
>
> Looks like you've ignored my comment on arg validation for enable/disable.
> Why?

Sorry. Not intentional. Just missed it. Done in v5.

>> +static int __init kcov_init(void)
>> +{
>> +     if (!debugfs_create_file("kcov", 0666, NULL, NULL, &kcov_fops)) {
>
> Why 0666? May be 0600?.

The idea is that it can be useful to fuzz-test under a normal user as well.
This file is also guarded by debugfs mount permissions. Usually it is mounted
as 0700, so normal users can't get access to it.
However if one wants to fuzz-test under a normal user, he could mount
debugfs as 0777 and get access to this file.

I am not very strong about this, though. If you say, I will change it to 0600.
I open this file as root at the moment. And we can change it back
if/when we better understand a potential use case.


>> +             pr_err("init failed\n");
>
> Again, you've ignored the comment.

Sorry. Not intentional. Just missed it. Done in v5.


Thanks for review again!