Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210326205135.6098-1-info@alexander-lochmann.de> <CA+fCnZcTi=QLGC_LCdhs+fMrxkqX66kXEuM5ewOmjVjifKzUrw@mail.gmail.com>
In-Reply-To: <CA+fCnZcTi=QLGC_LCdhs+fMrxkqX66kXEuM5ewOmjVjifKzUrw@mail.gmail.com>
From:   Dmitry Vyukov <dvyukov@google.com>
Date:   Fri, 16 Apr 2021 10:42:32 +0200
Message-ID: <CACT4Y+Y_PfAhjV26xYf8wcEv0MYygC14c_92hBN8gqOACK7Oow@mail.gmail.com>
Subject: Re: [PATCHv3] Introduced new tracing mode KCOV_MODE_UNIQUE.
To:     Andrey Konovalov <andreyknvl@gmail.com>
Cc:     Alexander Lochmann <info@alexander-lochmann.de>,
        Andrey Konovalov <andreyknvl@google.com>,
        Jonathan Corbet <corbet@lwn.net>,
        Randy Dunlap <rdunlap@infradead.org>,
        Andrew Klychkov <andrew.a.klychkov@gmail.com>,
        Miguel Ojeda <ojeda@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        Jakub Kicinski <kuba@kernel.org>,
        Aleksandr Nogikh <nogikh@google.com>,
        Wei Yongjun <weiyongjun1@huawei.com>,
        Maciej Grochowski <maciej.grochowski@pm.me>,
        kasan-dev <kasan-dev@googlegroups.com>,
        "open list:DOCUMENTATION" <linux-doc@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Sat, Mar 27, 2021 at 3:56 PM Andrey Konovalov <andreyknvl@gmail.com> wrote:
>
> On Fri, Mar 26, 2021 at 9:52 PM Alexander Lochmann
> <info@alexander-lochmann.de> wrote:
> >
>
> Hi Alexander,
>
> > It simply stores the executed PCs.
> > The execution order is discarded.
> > Each bit in the shared buffer represents every fourth
> > byte of the text segment.
> > Since a call instruction on every supported
> > architecture is at least four bytes, it is safe
> > to just store every fourth byte of the text segment.
>
> What about jumps?

KCOV adds call __sanitizer_cov_trace_pc per coverage point. So besides
the instructions in the original code, we also always have this call.


> [...]
>
> > -#define KCOV_IN_CTXSW  (1 << 30)
> > +#define KCOV_IN_CTXSW  (1 << 31)
>
> This change needs to be mentioned and explained in the changelog.
>
> > -static notrace bool check_kcov_mode(enum kcov_mode needed_mode, struct task_struct *t)
> > +static __always_inline notrace bool check_kcov_mode(enum kcov_mode needed_mode,
> > +                                                   struct task_struct *t,
> > +                                                   unsigned int *mode)
> >  {
> > -       unsigned int mode;
> > -
> >         /*
> >          * We are interested in code coverage as a function of a syscall inputs,
> >          * so we ignore code executed in interrupts, unless we are in a remote
> > @@ -162,7 +163,7 @@ static notrace bool check_kcov_mode(enum kcov_mode needed_mode, struct task_stru
> >          */
> >         if (!in_task() && !(in_serving_softirq() && t->kcov_softirq))
> >                 return false;
> > -       mode = READ_ONCE(t->kcov_mode);
> > +       *mode = READ_ONCE(t->kcov_mode);
> >         /*
> >          * There is some code that runs in interrupts but for which
> >          * in_interrupt() returns false (e.g. preempt_schedule_irq()).
> > @@ -171,7 +172,7 @@ static notrace bool check_kcov_mode(enum kcov_mode needed_mode, struct task_stru
> >          * kcov_start().
> >          */
> >         barrier();
> > -       return mode == needed_mode;
> > +       return ((int)(*mode & (KCOV_IN_CTXSW | needed_mode))) > 0;
>
> This change needs to be mentioned and explained in the changelog.
>
> [...]
>
> >  static notrace unsigned long canonicalize_ip(unsigned long ip)
> > @@ -191,18 +192,27 @@ void notrace __sanitizer_cov_trace_pc(void)
> >         struct task_struct *t;
> >         unsigned long *area;
> >         unsigned long ip = canonicalize_ip(_RET_IP_);
> > -       unsigned long pos;
> > +       unsigned long pos, idx;
> > +       unsigned int mode;
> >
> >         t = current;
> > -       if (!check_kcov_mode(KCOV_MODE_TRACE_PC, t))
> > +       if (!check_kcov_mode(KCOV_MODE_TRACE_PC | KCOV_MODE_UNIQUE_PC, t, &mode))
> >                 return;
> >
> >         area = t->kcov_area;
> > -       /* The first 64-bit word is the number of subsequent PCs. */
> > -       pos = READ_ONCE(area[0]) + 1;
> > -       if (likely(pos < t->kcov_size)) {
> > -               area[pos] = ip;
> > -               WRITE_ONCE(area[0], pos);
> > +       if (likely(mode == KCOV_MODE_TRACE_PC)) {
> > +               /* The first 64-bit word is the number of subsequent PCs. */
> > +               pos = READ_ONCE(area[0]) + 1;
> > +               if (likely(pos < t->kcov_size)) {
> > +                       area[pos] = ip;
> > +                       WRITE_ONCE(area[0], pos);
> > +               }
> > +       } else {
> > +               idx = (ip - canonicalize_ip((unsigned long)&_stext)) / 4;
> > +               pos = idx % BITS_PER_LONG;
> > +               idx /= BITS_PER_LONG;
> > +               if (likely(idx < t->kcov_size))
> > +                       WRITE_ONCE(area[idx], READ_ONCE(area[idx]) | 1L << pos);
>
> This is confusing: for KCOV_MODE_TRACE_PC, pos is used to index area,
> and for else, idx is used to index area. You should swap idx and pos.
>
> [...]
>
> > @@ -213,9 +223,10 @@ static void notrace write_comp_data(u64 type, u64 arg1, u64 arg2, u64 ip)
> >         struct task_struct *t;
> >         u64 *area;
> >         u64 count, start_index, end_pos, max_pos;
> > +       unsigned int mode;
> >
> >         t = current;
> > -       if (!check_kcov_mode(KCOV_MODE_TRACE_CMP, t))
> > +       if (!check_kcov_mode(KCOV_MODE_TRACE_CMP, t, &mode))
> >                 return;
>
> mode isn't used here, right? No need for it then.
>
> > @@ -562,12 +576,14 @@ static int kcov_ioctl_locked(struct kcov *kcov, unsigned int cmd,
> >  {
> >         struct task_struct *t;
> >         unsigned long size, unused;
> > -       int mode, i;
> > +       int mode, i, text_size, ret = 0;
> >         struct kcov_remote_arg *remote_arg;
> >         struct kcov_remote *remote;
> >         unsigned long flags;
> >
> >         switch (cmd) {
> > +       case KCOV_INIT_UNIQUE:
> > +               fallthrough;
> >         case KCOV_INIT_TRACE:
> >                 /*
> >                  * Enable kcov in trace mode and setup buffer size.
> > @@ -581,11 +597,42 @@ static int kcov_ioctl_locked(struct kcov *kcov, unsigned int cmd,
> >                  * that must not overflow.
> >                  */
> >                 size = arg;
> > -               if (size < 2 || size > INT_MAX / sizeof(unsigned long))
> > -                       return -EINVAL;
> > -               kcov->size = size;
> > -               kcov->mode = KCOV_MODE_INIT;
> > -               return 0;
> > +               if (cmd == KCOV_INIT_UNIQUE) {
>
> Let's put this code under KCOV_INIT_UNIQUE in the switch. This
> internal if only saves duplicating two lines of code, which isn't
> worth it.
>
> > +                       if (size != 0)
> > +                               return -EINVAL;
> > +                       text_size = (canonicalize_ip((unsigned long)&_etext)
> > +                                    - canonicalize_ip((unsigned long)&_stext));
> > +                       /**
> > +                        * A call instr is at least four bytes on every supported architecture.
> > +                        * Hence, just every fourth instruction can potentially be a call.
> > +                        */
> > +                       text_size = roundup(text_size, 4);
> > +                       text_size /= 4;
> > +                       /*
> > +                        * Round up size of text segment to multiple of BITS_PER_LONG.
> > +                        * Otherwise, we cannot track
> > +                        * the last (text_size % BITS_PER_LONG) addresses.
> > +                        */
> > +                       text_size = roundup(text_size, BITS_PER_LONG);
> > +                       /* Get the amount of bytes needed */
> > +                       text_size = text_size / 8;
> > +                       /* mmap() requires size to be a multiple of PAGE_SIZE */
> > +                       text_size = roundup(text_size, PAGE_SIZE);
> > +                       /* Get the cover size (= amount of bytes stored) */
> > +                       ret = text_size;
> > +                       kcov->size = text_size / sizeof(unsigned long);
> > +                       kcov_debug("text size = 0x%lx, roundup = 0x%x, kcov->size = 0x%x\n",
> > +                                       ((unsigned long)&_etext) - ((unsigned long)&_stext),
> > +                                       text_size,
> > +                                       kcov->size);
> > +                       kcov->mode = KCOV_MODE_INIT_UNIQUE;
> > +               } else {
> > +                       if (size < 2 || size > INT_MAX / sizeof(unsigned long))
> > +                               return -EINVAL;
> > +                       kcov->size = size;
> > +                       kcov->mode = KCOV_MODE_INIT_TRACE;
> > +               }
> > +               return ret;
>
> Thanks!
>
> --
> You received this message because you are subscribed to the Google Groups "kasan-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kasan-dev+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/CA%2BfCnZcTi%3DQLGC_LCdhs%2BfMrxkqX66kXEuM5ewOmjVjifKzUrw%40mail.gmail.com.