Re: [syzbot] upstream test error: KASAN: invalid-access Read in __entry_tramp_text_end

On Wed, Sep 29, 2021 at 09:39:47AM +0200, Peter Zijlstra wrote:

On Tue, Sep 28, 2021 at 06:36:37PM -0700, Josh Poimboeuf wrote:

quoted

On Tue, Sep 28, 2021 at 11:35:43AM +0100, Mark Rutland wrote:

quoted

quoted

In the other x86 thread Josh Poimboeuf suggested to use asm goto to a
cold part of the function instead of .fixup:
https://lore.kernel.org/lkml/20210927234543.6waods7rraxseind@treble/ (local)
This sounds like a more reliable solution that will cause less
maintenance burden. Would it work for arm64 as well?

Maybe we can use that when CC_HAS_ASM_GOTO_OUTPUT is avaiable, but in
general we can't rely on asm goto supporting output arguments (and IIRC
GCC doesn't support that at all), so we'd still have to support the
current fixup scheme.

gcc-11 has it

Neat. Worth looking at for future, then.

quoted

Even without CC_HAS_ASM_GOTO_OUTPUT it should still be possible to hack
something together if you split the original insn asm and the extable
asm into separate statements, like:

diff --git a/arch/x86/include/asm/msr.h b/arch/x86/include/asm/msr.h
index 6b52182e178a..8f62469f2027 100644
--- a/arch/x86/include/asm/msr.h
+++ b/arch/x86/include/asm/msr.h

@@ -137,20 +139,21 @@ static inline unsigned long long native_read_msr_safe(unsigned int msr,
 {
 	DECLARE_ARGS(val, low, high);
 
+	*err = 0;
+	asm volatile("417: rdmsr\n"
+		     : EAX_EDX_RET(val, low, high)
+		     : "c" (msr));
+	asm_volatile_goto(_ASM_EXTABLE(417b, %l[Efault]) :::: Efault);

That's terrible :-) Could probably do with a comment, but might just
work..

The compiler is well within its rights to spill/restore/copy/shuffle
registers or modify memory between the two asm blocks (which it's liable
to do that when optimizing this after a few layers of inlining), and
skipping that would cause all sorts of undefined behaviour.

It's akin to trying to do an asm goto without the compiler supporting
asm goto.

This would probably happen to work in the common case, but it'd cause
nightmarish bugs in others...

Thanks,
Mark.

quoted

+
+done:
 	if (tracepoint_enabled(read_msr))
 		do_trace_read_msr(msr, EAX_EDX_VAL(val, low, high), *err);
 	return EAX_EDX_VAL(val, low, high);
+
+Efault:
+	*err = -EIO;
+	ZERO_ARGS(val, low, high);
+	goto done;
 }
 
 /* Can be uninlined because referenced by paravirt */

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel