Re: [PATCH v3 1/3] kasan: switch kunit tests to console tracepoints

[Marco Elver]

+Cc tracing maintainers

On Wed, 15 Feb 2023 at 03:56, Peter Collingbourne <pcc@xxxxxxxxxx> wrote:
>
> On Mon, Feb 13, 2023 at 10:08 PM Marco Elver <elver@xxxxxxxxxx> wrote:
> >
> > On Tue, 14 Feb 2023 at 02:21, Peter Collingbourne <pcc@xxxxxxxxxx> wrote:
> > >
> > > On Tue, Oct 18, 2022 at 10:17 AM <andrey.konovalov@xxxxxxxxx> wrote:
> > > >
> > > > From: Andrey Konovalov <andreyknvl@xxxxxxxxxx>
> > > >
> > > > 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@xxxxxxxxxx>
> > > > Signed-off-by: Andrey Konovalov <andreyknvl@xxxxxxxxxx>
> > > >
> > > > ---
> > > >
> > > > 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.

Yes, you are right, and it's something I've wondered how to do better
as well. Let's try to consult tracing maintainers on what the right
approach is.

> 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