Re: [PATCH 1/2] kcov: size of arena is now given in bytes.

[Dmitry Vyukov]

On Wed, Nov 16, 2016 at 10:27 PM, Quentin Casasnovas
<quentin.casasnovas@xxxxxxxxxx> 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 <dvyukov@xxxxxxxxxx>

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 <dvyukov@xxxxxxxxxx>
> Cc: Michal Zalewski <lcamtuf@xxxxxxxxx>
> Cc: Kees Cook <keescook@xxxxxxxxxxxx>
> Signed-off-by: Quentin Casasnovas <quentin.casasnovas@xxxxxxxxxx>
> Signed-off-by: Vegard Nossum <vegard.nossum@xxxxxxxxxx>
> ---
>  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
>