Message-ID: <CANpmjNOpC2kGhfM8k=Y8VfLL0wSTkiOdkfU05tt1xTr+FuMjOQ@mail.gmail.com>
Date: Tue, 9 Dec 2025 01:52:28 +0100
From: Marco Elver <elver@...gle.com>
To: Brendan Jackman <jackmanb@...gle.com>
Cc: Andrey Ryabinin <ryabinin.a.a@...il.com>, Alexander Potapenko <glider@...gle.com>, 
	Andrey Konovalov <andreyknvl@...il.com>, Dmitry Vyukov <dvyukov@...gle.com>, 
	Vincenzo Frascino <vincenzo.frascino@....com>, Ard Biesheuvel <ardb@...nel.org>, kasan-dev@...glegroups.com, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 0/2] Noinstr fixes for K[CA]SAN with GCOV

On Tue, 9 Dec 2025 at 01:05, Brendan Jackman <jackmanb@...gle.com> wrote:
>
> On Mon Dec 8, 2025 at 11:12 AM UTC, Marco Elver wrote:
> > On Mon, 8 Dec 2025 at 10:37, Marco Elver <elver@...gle.com> wrote:
> >>
> >> On Mon, 8 Dec 2025 at 02:35, Brendan Jackman <jackmanb@...gle.com> wrote:
> >> >
> >> > Details:
> >> >
> >> >  - ❯❯  clang --version
> >> >    Debian clang version 19.1.7 (3+build5)
> >> >    Target: x86_64-pc-linux-gnu
> >> >    Thread model: posix
> >> >    InstalledDir: /usr/lib/llvm-19/bin
> >> >
> >> >  - Kernel config:
> >> >
> >> >    https://gist.githubusercontent.com/bjackman/bbfdf4ec2e1dfd0e18657174f0537e2c/raw/a88dcc6567d14c69445e7928a7d5dfc23ca9f619/gistfile0.txt
> >> >
> >> > Note I also get this error:
> >> >
> >> > vmlinux.o: warning: objtool: set_ftrace_ops_ro+0x3b: relocation to !ENDBR: machine_kexec_prepare+0x810
> >> >
> >> > That one's a total mystery to me. I guess it's better to "fix" the SEV
> >> > one independently rather than waiting until I know how to fix them both.
> >> >
> >> > Note I also mentioned other similar errors in [0]. Those errors don't
> >> > exist in Linus' master and I didn't note down where I saw them. Either
> >> > they have since been fixed, or I observed them in Google's internal
> >> > codebase where they were instroduced downstream.
> >> >
> >> > This is a successor to [1] but I haven't called it a v2 because it's a
> >> > totally different solution. Thanks to Ard for the guidance and
> >> > corrections.
> >> >
> >> > [0] https://lore.kernel.org/all/DERNCQGNRITE.139O331ACPKZ9@google.com/
> >> >
> >> > [1] https://lore.kernel.org/all/20251117-b4-sev-gcov-objtool-v1-1-54f7790d54df@google.com/
> >>
> >> Why is [1] not the right solution?
> >> The problem is we have lots of "inline" functions, and any one of them
> >> could cause problems in future.
> >
> > Perhaps I should qualify: lots of *small* inline functions, including
> > those stubs.
> >
> >> I don't mind turning "inline" into "__always_inline", but it seems
> >> we're playing whack-a-mole here, and just disabling GCOV entirely
> >> would make this noinstr.c file more robust.
> >
> > To elaborate: `UBSAN_SANITIZE_noinstr.o := n` and
> > `K{A,C}SAN_SANITIZE_noinstr.o := n` is already set on this file.
> > Perhaps adding __always_inline to the stub functions here will be
> > enough today, but might no longer be in future.
>
> Well you can also see it the other way around: disabling GCOV_PROFILE
> might be enough today, but as soon as some other noinstr disables
> __SANITIZE_ADDRESS__ and expects to be able to call instrumented
> helpers, that code will be broken too.

This itself is a contradiction: a `noinstr` function should not call
instrumented helpers. Normally this all works due to the compiler's
function attributes working as intended for the compiler-inserted
instrumentation, but for explicitly inserted instrumentation it's
obviously not. In otherwise instrumented files with few (not all)
`noinstr` functions, making the stub functions `__always_inline` will
not work, because the preprocessor is applied globally not per
function. In the past, I recall the underlying implementation being
used of e.g. the bitops (arch_foo... or __foo) in `noinstr` functions
to solve that.

The other hammer we have is K[ACM]SAN_SANITIZE_foo.o := n,
GCOV_PROFILE_foo.o := n, and KCOV_INSTRUMENT_foo.o := n.

> I don't think we can avoid whack-a-mole here. In fact I think the whole
> noinstr thing is an inevitable game of whack-a-mole unless we can get a
> static anlyzer to find violations at the source level. I suspect there
> are loads of violations in the tree that only show up in objtool if you
> build in weird configs on a full moon.
>
> One argument in favour of `GCOV_PROFILE_noinstr.o := n` would be: "this
> is non-instrumentable code, the issue here is that it is getting
> instrumented, so the fix is surely to stop instrumenting it". But, I
> don't think that's really true, the issue is not with the
> instrumentation but with the out-of-lining. Which highlights another
> point: a sufficiently annoying compiler could out-of-line these
> stub functions even without GCOV, right?

This would be a compiler bug in my book. Without instrumentation a
"static inline" function with nothing in it not being inlined is an
optimization bug. But those things get caught because it'd have made
someone's system slow.

> Still, despite my long-winded arguments I'm not gonna die on this hill,
> I would be OK with both ways.

To some extent I think doing both to reduce the chance of issues in
future might be what you want. On the other hand, avoiding the
Makefile-level opt-out will help catch more corner cases in future,
which may or may not be helpful outside this noinstr.c file.

> > If you look at
> > <linux/instrumented.h>, we also have KMSAN. The KMSAN explicit
> > instrumentation doesn't appear to be invoked on that file today, but
> > given it shouldn't, we might consider:
> >
> > KMSAN_SANITIZE_noinstr.o := n
> > GCOV_PROFILE_noinstr.o := n
>
> This would make sense to me, although as I hinted above I think it's
> sorta orthogonal and we should __always_inline the k[ca]san stubs
> regardless.
>
> > The alternative is to audit the various sanitizer stub functions, and
> > mark all these "inline" stub functions as "__always_inline". The
> > changes made in this series are sufficient for the noinstr.c case, but
> > not complete.
>
> Oh, yeah I should have  done __kcsan_{en,di}able_current() too I think.
>
> Are there other stubs you are thinking of? I think we only care about the
> !__SANITIZE_*__ stubs - we don't need this for !CONFIG_* stubs, right?
> Anything else I'm forgetting?

Initially, I think !__SANITIZE_* stubs are enough. Well, basically
anything that appears in <linux/instrumented.h>, because all those are
__always_inline, we should make the called functions also
__always_inline.

If you think that'll be enough, and the Makefile-level opt-out is
redundant as a result, let's just do that.

Thanks,
-- Marco

Powered by blists - more mailing lists