Received-SPF: pass (google.com: domain of linux-kernel+bounces-209425-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <cover.1717507310.git.dvyukov@google.com> <80972769ad2ebebc7ab0c39bd48f31ce080f0394.1717507310.git.dvyukov@google.com>
 <CANpmjNO4PX94DSVn_ndjsNqzr3Dw5Lc_nVUEu4oc6RK-iny16g@mail.gmail.com>
 <CACT4Y+bT+D41NopXN4rFbxqgC--mqYL=xCQOje0jDssjncKY+A@mail.gmail.com> <CANpmjNNPbS4V7Pf3SgUCAKX8xKwbMqWdz+h2cirAAd7XdNmJqA@mail.gmail.com>
In-Reply-To: <CANpmjNNPbS4V7Pf3SgUCAKX8xKwbMqWdz+h2cirAAd7XdNmJqA@mail.gmail.com>
From: Dmitry Vyukov <dvyukov@google.com>
Date: Tue, 11 Jun 2024 09:52:26 +0200
Message-ID: <CACT4Y+bWa_hPhU4RcVhqkH+-S+-R=FFDS3WV2Fa-RK5besUCFg@mail.gmail.com>
Subject: Re: [PATCH 2/4] kcov: add interrupt handling self test
To: Marco Elver <elver@google.com>
Cc: tglx@linutronix.de, mingo@redhat.com, bp@alien8.de, 
	dave.hansen@linux.intel.com, x86@kernel.org, linux-kernel@vger.kernel.org, 
	syzkaller@googlegroups.com, glider@google.com, nogikh@google.com, 
	tarasmadan@google.com
Content-Type: text/plain; charset="UTF-8"

On Wed, 5 Jun 2024 at 11:33, Marco Elver <elver@google.com> wrote:
>
> On Wed, 5 Jun 2024 at 11:18, Dmitry Vyukov <dvyukov@google.com> wrote:
> >
> > On Wed, 5 Jun 2024 at 11:10, Marco Elver <elver@google.com> wrote:
> > >
> > > > Add a boot self test that can catch sprious coverage from interrupts.
> > > > The coverage callback filters out interrupt code, but only after the
> > > > handler updates preempt count. Some code periodically leaks out
> > > > of that section and leads to spurious coverage.
> > > > Add a best-effort (but simple) test that is likely to catch such bugs.
> > > > If the test is enabled on CI systems that use KCOV, they should catch
> > > > any issues fast.
> > > >
> > > > Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
> > > > Cc: x86@kernel.org
> > > > Cc: linux-kernel@vger.kernel.org
> > > > Cc: syzkaller@googlegroups.com
> > > >
> > > > ---
> > > >
> > > > In my local testing w/o the previous fix,
> > > > it immidiatly produced the following splat:
> > > >
> > > > kcov: running selftest
> > > > BUG: TASK stack guard page was hit at ffffc90000147ff8
> > > > Oops: stack guard page: 0000 [#1] PREEMPT SMP KASAN PTI
> > > > ...
> > > >  kvm_set_cpu_l1tf_flush_l1d+0x5/0x20
> > > >  sysvec_call_function+0x15/0xb0
> > > >  asm_sysvec_call_function+0x1a/0x20
> > > >  kcov_init+0xe4/0x130
> > > >  do_one_initcall+0xbc/0x470
> > > >  kernel_init_freeable+0x4fc/0x930
> > > >  kernel_init+0x1c/0x2b0
> > > > ---
> > > >  kernel/kcov.c     | 28 ++++++++++++++++++++++++++++
> > > >  lib/Kconfig.debug |  9 +++++++++
> > > >  2 files changed, 37 insertions(+)
> > > >
> > > > diff --git a/kernel/kcov.c b/kernel/kcov.c
> > > > index c3124f6d5536..04136f80042f 100644
> > > > --- a/kernel/kcov.c
> > > > +++ b/kernel/kcov.c
> > > > @@ -1057,6 +1057,30 @@ u64 kcov_common_handle(void)
> > > >  }
> > > >  EXPORT_SYMBOL(kcov_common_handle);
> > > >
> > > > +#ifdef CONFIG_KCOV_TEST
> > > > +static void __init selftest(void)
> > > > +{
> > > > +       volatile int i;
> > > > +
> > > > +       pr_err("running self test\n");
> > > > +       /*
> > > > +        * Test that interrupts don't produce spurious coverage.
> > > > +        * The coverage callback filters out interrupt code, but only
> > > > +        * after the handler updates preempt count. Some code periodically
> > > > +        * leaks out of that section and leads to spurious coverage.
> > > > +        * It's hard to call the actual interrupt handler directly,
> > > > +        * so we just loop here for ~400 ms waiting for a timer interrupt.
> > >
> > > Where do the 400 ms come from? I only see that it loops a long time,
> > > but that the timing is entirely dependent on how fast the CPU executes
> > > the loop.
> > >
> > > > +        * We set kcov_mode to enable tracing, but don't setup the area,
> > > > +        * so any attempt to trace will crash.
> > > > +        */
> > > > +       current->kcov_mode = KCOV_MODE_TRACE_PC;
> > > > +       for (i = 0; i < (1 << 28); i++)
> > > > +               ;
> > >
> > > Can't you check jiffies, and e.g. check that actual ~100-500ms have elapsed?
> > >
> > > timeout = jiffies + msecs_to_jiffies(300);
> > > while (!time_after(jiffies, timeout)) {
> > >   cpu_relax();
> > > }
> >
> > We can't call any functions. If anything is instrumented, the kernel crashes.
> >
> > But just reading jiffies should be fine, so we can do:
> >
> > unsigned long start = jiffies;
> > while ((jiffies - start) * MSEC_PER_SEC / HZ < 500)
> > ;
>
> I'm quite sure that those helpers are macros, but who knows if that
> will ever change.
>
> The above open-coded version looks reasonable.

Sent v2 with fixes, PTAL.
https://lore.kernel.org/all/cover.1718092070.git.dvyukov@google.com/T/#t