MIME-Version: 1.0
References: <ebf96ea600050f00ed567e80505ae8f242633640.1666113393.git.andreyknvl@google.com>
 <CAMn1gO7Ve4-d6vP4jvASQsTZ2maHsMF6gKHL3RXSuD9N3tAOfQ@mail.gmail.com> <CANpmjNNvGL--j-20UxqX_WjeXGiAcjfDAQpfds+Orajz0ZeBsg@mail.gmail.com>
In-Reply-To: <CANpmjNNvGL--j-20UxqX_WjeXGiAcjfDAQpfds+Orajz0ZeBsg@mail.gmail.com>
From:   Peter Collingbourne <pcc@google.com>
Date:   Tue, 14 Feb 2023 18:55:52 -0800
Message-ID: <CAMn1gO6reT+MTmogLOrOVoNqzLH+fKmQ2JRAGy-tDOTLx-fpyw@mail.gmail.com>
Subject: Re: [PATCH v3 1/3] kasan: switch kunit tests to console tracepoints
To:     Marco Elver <elver@google.com>
Cc:     andrey.konovalov@linux.dev,
        Andrew Morton <akpm@linux-foundation.org>,
        Andrey Konovalov <andreyknvl@gmail.com>,
        Alexander Potapenko <glider@google.com>,
        Dmitry Vyukov <dvyukov@google.com>,
        Andrey Ryabinin <ryabinin.a.a@gmail.com>,
        kasan-dev@googlegroups.com, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org,
        Andrey Konovalov <andreyknvl@google.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, Feb 13, 2023 at 10:08 PM Marco Elver <elver@google.com> wrote:
>
> On Tue, 14 Feb 2023 at 02:21, Peter Collingbourne <pcc@google.com> wrote:
> >
> > On Tue, Oct 18, 2022 at 10:17 AM <andrey.konovalov@linux.dev> wrote:
> > >
> > > From: Andrey Konovalov <andreyknvl@google.com>
> > >
> > > Switch KUnit-compatible KASAN tests from using per-task KUnit resources
> > > to console tracepoints.
> > >
> > > This allows for two things:
> > >
> > > 1. Migrating tests that trigger a KASAN report in the context of a task
> > >    other than current to KUnit framework.
> > >    This is implemented in the patches that follow.
> > >
> > > 2. Parsing and matching the contents of KASAN reports.
> > >    This is not yet implemented.
> > >
> > > Reviewed-by: Marco Elver <elver@google.com>
> > > Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
> > >
> > > ---
> > >
> > > Changed v2->v3:
> > > - Rebased onto 6.1-rc1
> > >
> > > Changes v1->v2:
> > > - Remove kunit_kasan_status struct definition.
> > > ---
> > >  lib/Kconfig.kasan     |  2 +-
> > >  mm/kasan/kasan.h      |  8 ----
> > >  mm/kasan/kasan_test.c | 85 +++++++++++++++++++++++++++++++------------
> > >  mm/kasan/report.c     | 31 ----------------
> > >  4 files changed, 63 insertions(+), 63 deletions(-)
> > >
> > > diff --git a/lib/Kconfig.kasan b/lib/Kconfig.kasan
> > > index ca09b1cf8ee9..ba5b27962c34 100644
> > > --- a/lib/Kconfig.kasan
> > > +++ b/lib/Kconfig.kasan
> > > @@ -181,7 +181,7 @@ config KASAN_VMALLOC
> > >
> > >  config KASAN_KUNIT_TEST
> > >         tristate "KUnit-compatible tests of KASAN bug detection capabilities" if !KUNIT_ALL_TESTS
> > > -       depends on KASAN && KUNIT
> > > +       depends on KASAN && KUNIT && TRACEPOINTS
> >
> > My build script for a KASAN-enabled kernel does something like:
> >
> > make defconfig
> > scripts/config -e CONFIG_KUNIT -e CONFIG_KASAN -e CONFIG_KASAN_HW_TAGS
> > -e CONFIG_KASAN_KUNIT_TEST
> > yes '' | make syncconfig
> >
> > and after this change, the unit tests are no longer built. Should this
> > use "select TRACING" instead?
>
> I think we shouldn't select TRACING, which should only be selected by
> tracers. You'd need CONFIG_FTRACE=y.

Doesn't CONFIG_FTRACE=y mean "function tracing", i.e. function
entry/exit tracing using compiler instrumentation? As far as I can
tell, the KASAN tests do not make use of this feature. They only use
the kernel tracepoint infrastructure to trace the "console" tracepoint
defined in include/trace/events/printk.h, which is not associated with
function entry/exit.

I have yet to find any evidence that TRACING ought to only be selected
by tracers. As far as I can tell, TRACING appears to be the minimal
config required in order for it to be possible to trace pre-defined
(i.e. defined with TRACE_EVENT) tracepoints, which is all that KASAN
needs. (I also tried selecting TRACEPOINTS, but this led to a number
of link failures.) If select TRACING is only used by tracers, it could
just mean that only tracers are making use of this functionality
inside the kernel. From that perspective the KASAN tests can
themselves be considered a "tracer" (albeit a very specialized one).

If I locally revert the change to lib/Kconfig.kasan and add the
TRACING select, the KASAN tests pass when using my kernel build
script, which suggests that TRACING is all that is needed.

> Since FTRACE is rather big, we probably also shouldn't implicitly
> select it. Instead, at least when using kunit.py tool, we could add a
> mm/kasan/.kunitconfig like:
>
> CONFIG_KUNIT=y
> CONFIG_KASAN=y
> CONFIG_KASAN_KUNIT_TEST=y
> # Additional dependencies.
> CONFIG_FTRACE=y
>
> Which mirrors the KFENCE mm/kfence/.kunitconfig. But that doesn't help
> if you want to run it with something other than KUnit tool.

In any case, I'm not sure I'm in favor of adding yet another config
that folks need to know to enable in order to avoid silently disabling
the unit tests. Many developers will maintain their own scripts for
kernel development if the existing ones do not meet their needs. It's
possible that kunit.py will work out for me now (when I looked at it
before, it was useless for me because it only supported UML, but it
looks like it supports QEMU now), but there's no guarantee that it
will, so I might stick with my scripts for a while.

Peter