Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752975AbcKQHy7 (ORCPT ); Thu, 17 Nov 2016 02:54:59 -0500 Received: from mail-qt0-f179.google.com ([209.85.216.179]:34100 "EHLO mail-qt0-f179.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751100AbcKQHy5 (ORCPT ); Thu, 17 Nov 2016 02:54:57 -0500 MIME-Version: 1.0 In-Reply-To: <20161116212746.29047-2-quentin.casasnovas@oracle.com> References: <20161116212746.29047-1-quentin.casasnovas@oracle.com> <20161116212746.29047-2-quentin.casasnovas@oracle.com> From: Dmitry Vyukov Date: Thu, 17 Nov 2016 08:54:36 +0100 Message-ID: Subject: Re: [PATCH 1/2] kcov: size of arena is now given in bytes. To: Quentin Casasnovas Cc: LKML , Vegard Nossum , Michal Zalewski , Kees Cook , syzkaller Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3366 Lines: 83 On Wed, Nov 16, 2016 at 10:27 PM, Quentin Casasnovas wrote: > We'll introduce a different mode of tracing a-la AFL fixed table and Dmitry > suggests that the code would be simpler with the size expressed in bytes as > opposed unsigned longs. > > We only change the kcov::size field, which will be shared between different > modes, but leave the task_struct::kcov_size field expressed in unsigned > long in order to save an unecessary bitshift/division in the hot path when > using KCOV_MODE_TRACE. Thanks! Reviewed-and-tested-by: Dmitry Vyukov I've tested that KCOV_MODE_TRACE works for me with these two patches applied. > but leave the task_struct::kcov_size field expressed in unsigned long The only purpose of the cache in task struct is to make coverage fast path fast, so this looks good to me. > Cc: Dmitry Vyukov > Cc: Michal Zalewski > Cc: Kees Cook > Signed-off-by: Quentin Casasnovas > Signed-off-by: Vegard Nossum > --- > kernel/kcov.c | 10 ++++------ > 1 file changed, 4 insertions(+), 6 deletions(-) > > diff --git a/kernel/kcov.c b/kernel/kcov.c > index 30e6d05aa5a9..c2aa93851f93 100644 > --- a/kernel/kcov.c > +++ b/kernel/kcov.c > @@ -32,7 +32,7 @@ struct kcov { > /* The lock protects mode, size, area and t. */ > spinlock_t lock; > enum kcov_mode mode; > - /* Size of arena (in long's for KCOV_MODE_TRACE). */ > + /* Size of arena in bytes. */ > unsigned size; > /* Coverage buffer shared with user space. */ > void *area; > @@ -140,7 +140,7 @@ static int kcov_mmap(struct file *filep, struct vm_area_struct *vma) > return -ENOMEM; > > spin_lock(&kcov->lock); > - size = kcov->size * sizeof(unsigned long); > + size = kcov->size; > if (kcov->mode == KCOV_MODE_DISABLED || vma->vm_pgoff != 0 || > vma->vm_end - vma->vm_start != size) { > res = -EINVAL; > @@ -198,13 +198,11 @@ static int kcov_ioctl_locked(struct kcov *kcov, unsigned int cmd, > return -EBUSY; > /* > * Size must be at least 2 to hold current position and one PC. > - * Later we allocate size * sizeof(unsigned long) memory, > - * that must not overflow. > */ > size = arg; > if (size < 2 || size > INT_MAX / sizeof(unsigned long)) > return -EINVAL; > - kcov->size = size; > + kcov->size = size * sizeof(unsigned long); > kcov->mode = KCOV_MODE_TRACE; > return 0; > case KCOV_ENABLE: > @@ -223,7 +221,7 @@ static int kcov_ioctl_locked(struct kcov *kcov, unsigned int cmd, > return -EBUSY; > t = current; > /* Cache in task struct for performance. */ > - t->kcov_size = kcov->size; > + t->kcov_size = kcov->size / sizeof(unsigned long); > t->kcov_area = kcov->area; > /* See comment in __sanitizer_cov_trace_pc(). */ > barrier(); > -- > 2.10.2 >