Received-SPF: pass (google.com: domain of linux-kernel+bounces-202055-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>
In-Reply-To: <80972769ad2ebebc7ab0c39bd48f31ce080f0394.1717507310.git.dvyukov@google.com>
From: Marco Elver <elver@google.com>
Date: Wed, 5 Jun 2024 11:09:40 +0200
Message-ID: <CANpmjNO4PX94DSVn_ndjsNqzr3Dw5Lc_nVUEu4oc6RK-iny16g@mail.gmail.com>
Subject: Re: [PATCH 2/4] kcov: add interrupt handling self test
To: Dmitry Vyukov <dvyukov@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 Tue, 4 Jun 2024 at 15:45, Dmitry Vyukov <dvyukov@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();
}

> +       current->kcov_mode = 0;
> +       pr_err("done running self test\n");
> +}
> +#endif
> +
>  static int __init kcov_init(void)
>  {
>         int cpu;
> @@ -1076,6 +1100,10 @@ static int __init kcov_init(void)
>          */
>         debugfs_create_file_unsafe("kcov", 0600, NULL, NULL, &kcov_fops);
>
> +#ifdef CONFIG_KCOV_TEST
> +       selftest();
> +#endif
> +
>         return 0;
>  }
>
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 59b6765d86b8..79836a15b6cb 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -2171,6 +2171,15 @@ config KCOV_IRQ_AREA_SIZE
>           soft interrupts. This specifies the size of those areas in the
>           number of unsigned long words.
>
> +config KCOV_TEST

s/TEST/SELFTEST/

It may be confused with a longer standalone test.

> +       bool "Test CONFIG_KCOV feature"

Maybe "Perform short selftests on boot" (similar to CONFIG_KCSAN_SELFTEST).

> +       depends on KCOV
> +       help
> +         Sanity check for KCOV coverage collection.
> +         Runs built-in self test on boot to detect some common issues.
> +
> +         If unsure, say N.
> +
>  menuconfig RUNTIME_TESTING_MENU
>         bool "Runtime Testing"
>         default y
> --
> 2.45.1.467.gbab1589fc0-goog
>