From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757511AbcAOMmL (ORCPT ); Fri, 15 Jan 2016 07:42:11 -0500 Received: from mail-wm0-f65.google.com ([74.125.82.65]:35323 "EHLO mail-wm0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754616AbcAOMmJ (ORCPT ); Fri, 15 Jan 2016 07:42:09 -0500 Date: Fri, 15 Jan 2016 14:42:05 +0200 From: "Kirill A. Shutemov" To: Dmitry Vyukov Cc: akpm@linux-foundation.org, drysdale@google.com, keescook@google.com, quentin.casasnovas@oracle.com, sasha.levin@oracle.com, vegard.nossum@oracle.com, linux-kernel@vger.kernel.org, edumazet@google.com, taviso@google.com, bhelgaas@google.com, syzkaller@googlegroups.com, kcc@google.com, glider@google.com, ryabinin.a.a@gmail.com Subject: Re: [PATCH v3] kernel: add kcov code coverage Message-ID: <20160115124205.GA19780@node.shutemov.name> References: <1452781341-34178-1-git-send-email-dvyukov@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1452781341-34178-1-git-send-email-dvyukov@google.com> User-Agent: Mutt/1.5.23.1 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Jan 14, 2016 at 03:22:21PM +0100, Dmitry Vyukov wrote: > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#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