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

["Kirill A. Shutemov" <kirill@shutemov.name>]

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.

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

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