MIME-Version: 1.0
References: <1637130234-57238-1-git-send-email-quic_jiangenj@quicinc.com>
In-Reply-To: <1637130234-57238-1-git-send-email-quic_jiangenj@quicinc.com>
From:   Dmitry Vyukov <dvyukov@google.com>
Date:   Thu, 18 Nov 2021 15:00:06 +0100
Message-ID: <CACT4Y+YwNawV9H7uFMVSCA5WB-Dkyu9TX+rMM3FR6gNGkKFPqw@mail.gmail.com>
Subject: Re: [PATCH] kcov: add KCOV_PC_RANGE to limit pc range
To:     Joey Jiao <quic_jiangenj@quicinc.com>
Cc:     andreyknvl@gmail.com, kasan-dev@googlegroups.com,
        LKML <linux-kernel@vger.kernel.org>,
        Alexander Lochmann <info@alexander-lochmann.de>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

,On Wed, 17 Nov 2021 at 07:24, Joey Jiao <quic_jiangenj@quicinc.com> wrote:
>
> Sometimes we only interested in the pcs within some range,
> while there are cases these pcs are dropped by kernel due
> to `pos >= t->kcov_size`, and by increasing the map area
> size doesn't help.
>
> To avoid disabling KCOV for these not intereseted pcs during
> build time, adding this new KCOV_PC_RANGE cmd.

Hi Joey,

How do you use this? I am concerned that a single range of PCs is too
restrictive. I can only see how this can work for single module
(continuous in memory) or a single function. But for anything else
(something in the main kernel, or several modules), it won't work as
PCs are not continuous.

Maybe we should use a compressed bitmap of interesting PCs? It allows
to support all cases and we already have it in syz-executor, then
syz-executor could simply pass the bitmap to the kernel rather than
post-filter.
It's also overlaps with the KCOV_MODE_UNIQUE mode that +Alexander proposed here:
https://groups.google.com/g/kasan-dev/c/oVz3ZSWaK1Q/m/9ASztdzCAAAJ
It would be reasonable if kernel uses the same bitmap format for these
2 features.


> An example usage is to use together syzkaller's cov filter.
>
> Change-Id: I954f6efe1bca604f5ce31f8f2b6f689e34a2981d
> Signed-off-by: Joey Jiao <quic_jiangenj@quicinc.com>
> ---
>  Documentation/dev-tools/kcov.rst | 10 ++++++++++
>  include/uapi/linux/kcov.h        |  7 +++++++
>  kernel/kcov.c                    | 18 ++++++++++++++++++
>  3 files changed, 35 insertions(+)
>
> diff --git a/Documentation/dev-tools/kcov.rst b/Documentation/dev-tools/kcov.rst
> index d83c9ab..fbcd422 100644
> --- a/Documentation/dev-tools/kcov.rst
> +++ b/Documentation/dev-tools/kcov.rst
> @@ -52,9 +52,15 @@ program using kcov:
>      #include <fcntl.h>
>      #include <linux/types.h>
>
> +    struct kcov_pc_range {
> +      uint32 start;
> +      uint32 end;
> +    };
> +
>      #define KCOV_INIT_TRACE                    _IOR('c', 1, unsigned long)
>      #define KCOV_ENABLE                        _IO('c', 100)
>      #define KCOV_DISABLE                       _IO('c', 101)
> +    #define KCOV_TRACE_RANGE                   _IOW('c', 103, struct kcov_pc_range)
>      #define COVER_SIZE                 (64<<10)
>
>      #define KCOV_TRACE_PC  0
> @@ -64,6 +70,8 @@ program using kcov:
>      {
>         int fd;
>         unsigned long *cover, n, i;
> +        /* Change start and/or end to your interested pc range. */
> +        struct kcov_pc_range pc_range = {.start = 0, .end = (uint32)(~((uint32)0))};
>
>         /* A single fd descriptor allows coverage collection on a single
>          * thread.
> @@ -79,6 +87,8 @@ program using kcov:
>                                      PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
>         if ((void*)cover == MAP_FAILED)
>                 perror("mmap"), exit(1);
> +        if (ioctl(fd, KCOV_PC_RANGE, pc_range))
> +               dprintf(2, "ignore KCOV_PC_RANGE error.\n");
>         /* Enable coverage collection on the current thread. */
>         if (ioctl(fd, KCOV_ENABLE, KCOV_TRACE_PC))
>                 perror("ioctl"), exit(1);
> diff --git a/include/uapi/linux/kcov.h b/include/uapi/linux/kcov.h
> index 1d0350e..353ff0a 100644
> --- a/include/uapi/linux/kcov.h
> +++ b/include/uapi/linux/kcov.h
> @@ -16,12 +16,19 @@ struct kcov_remote_arg {
>         __aligned_u64   handles[0];
>  };
>
> +#define PC_RANGE_MASK ((__u32)(~((u32) 0)))
> +struct kcov_pc_range {
> +       __u32           start;          /* start pc & 0xFFFFFFFF */
> +       __u32           end;            /* end pc & 0xFFFFFFFF */
> +};
> +
>  #define KCOV_REMOTE_MAX_HANDLES                0x100
>
>  #define KCOV_INIT_TRACE                        _IOR('c', 1, unsigned long)
>  #define KCOV_ENABLE                    _IO('c', 100)
>  #define KCOV_DISABLE                   _IO('c', 101)
>  #define KCOV_REMOTE_ENABLE             _IOW('c', 102, struct kcov_remote_arg)
> +#define KCOV_PC_RANGE                  _IOW('c', 103, struct kcov_pc_range)
>
>  enum {
>         /*
> diff --git a/kernel/kcov.c b/kernel/kcov.c
> index 36ca640..59550450 100644
> --- a/kernel/kcov.c
> +++ b/kernel/kcov.c
> @@ -36,6 +36,7 @@
>   *  - 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, optional to set trace pc range
>   *  - then, ioctl(KCOV_ENABLE, arg), where arg is
>   *     KCOV_TRACE_PC - to trace only the PCs
>   *     or
> @@ -69,6 +70,8 @@ struct kcov {
>          * kcov_remote_stop(), see the comment there.
>          */
>         int                     sequence;
> +       /* u32 Trace PC range from start to end. */
> +       struct kcov_pc_range    pc_range;
>  };
>
>  struct kcov_remote_area {
> @@ -192,6 +195,7 @@ static notrace unsigned long canonicalize_ip(unsigned long ip)
>  void notrace __sanitizer_cov_trace_pc(void)
>  {
>         struct task_struct *t;
> +       struct kcov_pc_range pc_range;
>         unsigned long *area;
>         unsigned long ip = canonicalize_ip(_RET_IP_);
>         unsigned long pos;
> @@ -199,6 +203,11 @@ void notrace __sanitizer_cov_trace_pc(void)
>         t = current;
>         if (!check_kcov_mode(KCOV_MODE_TRACE_PC, t))
>                 return;
> +       pc_range = t->kcov->pc_range;
> +       if (pc_range.start < pc_range.end &&
> +               ((ip & PC_RANGE_MASK) < pc_range.start ||
> +               (ip & PC_RANGE_MASK) > pc_range.end))
> +               return;
>
>         area = t->kcov_area;
>         /* The first 64-bit word is the number of subsequent PCs. */
> @@ -568,6 +577,7 @@ static int kcov_ioctl_locked(struct kcov *kcov, unsigned int cmd,
>         int mode, i;
>         struct kcov_remote_arg *remote_arg;
>         struct kcov_remote *remote;
> +       struct kcov_pc_range *pc_range;
>         unsigned long flags;
>
>         switch (cmd) {
> @@ -589,6 +599,14 @@ static int kcov_ioctl_locked(struct kcov *kcov, unsigned int cmd,
>                 kcov->size = size;
>                 kcov->mode = KCOV_MODE_INIT;
>                 return 0;
> +       case KCOV_PC_RANGE:
> +               /* Limit trace pc range. */
> +               pc_range = (struct kcov_pc_range *)arg;
> +               if (copy_from_user(&kcov->pc_range, pc_range, sizeof(kcov->pc_range)))
> +                       return -EINVAL;
> +               if (kcov->pc_range.start >= kcov->pc_range.end)
> +                       return -EINVAL;
> +               return 0;
>         case KCOV_ENABLE:
>                 /*
>                  * Enable coverage for the current task.
> --
> 2.7.4
>