Re: [PATCH v4 2/2] UML: add support for KASAN under x86_64

[Dmitry Vyukov <dvyukov@google.com>]

On Fri, 1 Jul 2022 at 12:04, Vincent Whitchurch
<vincent.whitchurch@axis.com> wrote:
> > <vincent.whitchurch@axis.com> wrote:
> > > On Fri, Jul 01, 2022 at 11:08:27AM +0200, David Gow wrote:
> > > > On Thu, Jun 30, 2022 at 9:29 PM Andrey Konovalov <andreyknvl@gmail.com> wrote:
> > > > > Stack trace collection code might trigger KASAN splats when walking
> > > > > stack frames, but this can be resolved by using unchecked accesses.
> > > > > The main reason to disable instrumentation here is for performance
> > > > > reasons, see the upcoming patch for arm64 [1] for some details.
> > > > >
> > > > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git/commit/?id=802b91118d11
> > > >
> > > > Ah -- that does it! Using READ_ONCE_NOCHECK() in dump_trace() gets rid
> > > > of the nasty recursive KASAN failures we were getting in the tests.
> > > >
> > > > I'll send out v5 with those files instrumented again.
> > >
> > > Hmm, do we really want that?  In the patch Andrey linked to above he
> > > removed the READ_ONCE_NOCHECK() and added the KASAN_SANITIZE on the
> > > corresponding files for arm64, just like it's already the case in this
> > > patch for UML.
> >
> > Personally, I'm okay with the performance overhead so far: in my tests
> > with a collection of ~350 KUnit tests, the total difference in runtime
> > was about ~.2 seconds, and was within the margin of error caused by
> > fluctuations in the compilation time.
> >
> > As an example, without the stacktrace code instrumented:
> > [17:36:50] Testing complete. Passed: 364, Failed: 0, Crashed: 0,
> > Skipped: 47, Errors: 0
> > [17:36:50] Elapsed time: 15.114s total, 0.003s configuring, 8.518s
> > building, 6.433s running
> >
> > versus with it instrumented:
> > [17:35:40] Testing complete. Passed: 364, Failed: 0, Crashed: 0,
> > Skipped: 47, Errors: 0
> > [17:35:40] Elapsed time: 15.497s total, 0.003s configuring, 8.691s
> > building, 6.640s running
>
> OK, good to know.
>
> > That being said, I'm okay with disabling it again and adding a comment
> > if it's slow enough in some other usecase to cause problems (or even
> > just be annoying). That could either be done in a v6 of this patchset,
> > or a follow-up patch, depending on what people would prefer. But I'd
> > not have a problem with leaving it instrumented for now.
>
> I don't have any strong opinion either way either, so you don't have to
> change it back on my account.  Thanks.

I would consider using READ_ONCE_NOCHECK() by default. And then
switching to KASAN_SANITIZE:=n only if there is a real reason for
that. Disabling instrumentation of any part of the kernel makes things
faster, but at the same time we are losing checking coverage.
For arm it was done for a very specific reason related to performance.
While UML can be considered more test-oriented rather than
production-oriented.