Re: [PATCH v4] kernel: add kcov code coverage

From: Kirill A. Shutemov
Date: Mon Jan 18 2016 - 17:56:16 EST


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.

> +};
> +
> +/*
> + * 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.

> + * - 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.

> + }
> + 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?

....

> +static int __init kcov_init(void)
> +{
> + if (!debugfs_create_file("kcov", 0666, NULL, NULL, &kcov_fops)) {

Why 0666? May be 0600?.

> + pr_err("init failed\n");

Again, you've ignored the comment.

> + return 1;
> + }
> + return 0;
> +}
> +
> +device_initcall(kcov_init);
--
Kirill A. Shutemov