Re: [GIT pull] x86/urgent for v6.16-rc7

[tglx <tglx@linutronix.de>]

On Sun, Jul 20 2025 at 11:35, Linus Torvalds wrote:
> On Sun, 20 Jul 2025 at 05:05, Thomas Gleixner <tglx@linutronix.de> wrote:
>>
>> A single fix for a GCC wreckage, which emits a KCSAN instrumentation call
>> in __sev_es_nmi_complete() despite the function being annotated with
>> 'noinstr'. As all functions in that source file are noinstr, exclude the
>> whole file from KCSAN in the Makefile to cure it.
>
> Hmm. I'm not entirely sure if this counts as a gcc bug.
>
> In particular, look at the *declaration* of __sev_es_nmi_complete() in
> <asm/sev.h>, and note the complete lack of 'noinstr':
>
>     extern void __sev_es_nmi_complete(void);
>
> and I wonder if it might be the case that gcc might pick up the lack
> of 'noinstr' from the declaration, even if it's there in the
> definition..
>
> The fix for this obviously ends up working and is fine regardless, but
> it's _possible_ that this is self-inflicted pain rather than an
> outright compiler bug.

I agree. See below.

> Because having a declaration and a definition that doesn't match
> sounds like a bad idea in the first place.

I disagree here. When the compiler evaluates the function body it must
have evaluated noinstr on the definition, no?

Unfortunately the data provided in the change log does not tell what was
actually instrumented inside of that function. I just reproduced it
locally.

The problem are the invocations of ghcb_set_sw_*(), which are
implemented through a macro maze, but end up being marked
__always_inline all the way down. __always_inline is supposed to
guarantee that the noinstr (or other) constraints of the calling
function are preserved.

What makes these ghcb_set_sw_*() inlines so special?

They are defined by DEFINE_GHCB_ACCESSORS(field) and end up with this:

	static __always_inline void ghcb_set_##field(struct ghcb *ghcb, u64 value) \
	{									\
		__set_bit(GHCB_BITMAP_IDX(field),				\
			  (unsigned long *)&ghcb->save.valid_bitmap);		\
		ghcb->save.field = value;					\
	}

__set_bit() resolves to:

static __always_inline void ___set_bit(unsigned long nr, volatile unsigned long *addr)
{
	instrument_write(addr + BIT_WORD(nr), sizeof(long));
	arch___set_bit(nr, addr);
}

instrument_write() resolves to

static __always_inline void instrument_write(const volatile void *v, size_t size)
{
	kasan_check_write(v, size);
	kcsan_check_write(v, size);
}

and kcsan_check_write() is:

#define __kcsan_check_write(ptr, size)                                         \
	__kcsan_check_access(ptr, size, KCSAN_ACCESS_WRITE)

__kcsan_check_access() is a function provided by the kernel. So GCC just
does as told and emits a function call.

If I replace the __set_bit() in ghcb_set_##field() with arch___set_bit()
the problem goes away.

Excluding the whole file from KCSAN switches to the non-instrumented
version of __set_bit() which directly resolves to arch___set_bit().

Thanks,

        tglx