Message-ID: <DETBVMG30SW8.WBM5TRGF59YZ@google.com>
Date: Tue, 09 Dec 2025 02:25:17 +0000
From: Brendan Jackman <jackmanb@...gle.com>
To: Marco Elver <elver@...gle.com>, 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 Dec 9, 2025 at 12:52 AM UTC, Marco Elver wrote:
> 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.

Sorry I dropped an important word here, I meant to say other noinstr
_files_. I.e. anything else similar to SEV's noinstr.c that is doing
noinstr at the file level.

>> 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.

Cool, then yeah I think I will do both unless anyone shows up to object
to that. Both things ultimately make sense on their own merit and even
if you only need one or the other to make the error go away, I don't
think that actually makes them "redundant".

>> > 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.

Ack, thanks for all the input here! I'll probably wait until after LPC
to do a v2.

Powered by blists - more mailing lists