When arm64 is built with LTO, it upgrades READ_ONCE() to ldar / ldapr
(load-acquire) to avoid issues that can be caused by the compiler
optimizing away implicit address dependencies.

Unlike plain loads, these load-acquire instructions actually require an
aligned address.

For now, fix it by removing the READ_ONCE() that the buggy commit
introduced.

Fixes: ece69af2ede1 ("rwonce: handle KCSAN like KASAN in read_word_at_a_time()")
Reported-by: Nathan Chancellor <nathan@kernel.org>
Closes: https://lore.kernel.org/r/20250326203926.GA10484@ax162
Signed-off-by: Jann Horn <jannh@google.com>
---
 include/asm-generic/rwonce.h | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/include/asm-generic/rwonce.h b/include/asm-generic/rwonce.h
index e9f2b84d2338..52b969c7cef9 100644
--- a/include/asm-generic/rwonce.h
+++ b/include/asm-generic/rwonce.h
@@ -86,7 +86,12 @@ unsigned long read_word_at_a_time(const void *addr)
 	kasan_check_read(addr, 1);
 	kcsan_check_read(addr, 1);
 
-	return READ_ONCE(*(unsigned long *)addr);
+	/*
+	 * This load can race with concurrent stores to out-of-bounds memory,
+	 * but READ_ONCE() can't be used because it requires higher alignment
+	 * than plain loads in arm64 builds with LTO.
+	 */
+	return *(unsigned long *)addr;
 }
 
 #endif /* __ASSEMBLY__ */

---
base-commit: ece69af2ede103e190ffdfccd9f9ec850606ab5e
change-id: 20250326-rwaat-fix-63d7557b3d88

-- 
Jann Horn <jannh@google.com>

On Wed, Mar 26, 2025, at 22:04, Jann Horn wrote:
> When arm64 is built with LTO, it upgrades READ_ONCE() to ldar / ldapr
> (load-acquire) to avoid issues that can be caused by the compiler
> optimizing away implicit address dependencies.
>
> Unlike plain loads, these load-acquire instructions actually require an
> aligned address.
>
> For now, fix it by removing the READ_ONCE() that the buggy commit
> introduced.
>
> Fixes: ece69af2ede1 ("rwonce: handle KCSAN like KASAN in read_word_at_a_time()")
> Reported-by: Nathan Chancellor <nathan@kernel.org>
> Closes: https://lore.kernel.org/r/20250326203926.GA10484@ax162
> Signed-off-by: Jann Horn <jannh@google.com>

Thanks for the quick fix!

I've applied this on top of the asm-generic branch, but I just sent
the pull request with the regression to Linus an hour ago.

I'll try to get a new pull request out tomorrow.

      Arnd

> ---
>  include/asm-generic/rwonce.h | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/include/asm-generic/rwonce.h b/include/asm-generic/rwonce.h
> index e9f2b84d2338..52b969c7cef9 100644
> --- a/include/asm-generic/rwonce.h
> +++ b/include/asm-generic/rwonce.h
> @@ -86,7 +86,12 @@ unsigned long read_word_at_a_time(const void *addr)
>  	kasan_check_read(addr, 1);
>  	kcsan_check_read(addr, 1);
> 
> -	return READ_ONCE(*(unsigned long *)addr);
> +	/*
> +	 * This load can race with concurrent stores to out-of-bounds memory,
> +	 * but READ_ONCE() can't be used because it requires higher alignment
> +	 * than plain loads in arm64 builds with LTO.
> +	 */
> +	return *(unsigned long *)addr;
>  }
> 
>  #endif /* __ASSEMBLY__ */
>
> ---
> base-commit: ece69af2ede103e190ffdfccd9f9ec850606ab5e
> change-id: 20250326-rwaat-fix-63d7557b3d88
>
> -- 
> Jann Horn <jannh@google.com>

On Wed, 26 Mar 2025 at 14:24, Arnd Bergmann <arnd@arndb.de> wrote:
>
> I've applied this on top of the asm-generic branch, but I just sent
> the pull request with the regression to Linus an hour ago.
>
> I'll try to get a new pull request out tomorrow.

I will ignore that pull request, and wait for your updated one.

That said, this whole thing worries me. The fact that the compiler
magically makes READ_ONCE() require alignment that it normally doesn't
require seems like a bug waiting to happen somewhere else.

Because I do think that we might want READ_ONCE() on unaligned data in
general. Should said places generally use "get_unaligned()"? Sure. And
are unaligned accesses potentially non-atomic anyway because of
hardware? Also sure.

But one reason for READ_ONCE() isn't for some kind of hardware
atomicity, it is to avoid any ToCToU issues due to compilers doing bad
things.

And then this seems to be a serious issue with the whole "READ_ONCE()
now requires alignment that it didn't require before".

Put another way: I wonder what other cases may lurk around this all...

           Linus

On Wed, Mar 26, 2025 at 03:41:34PM -0700, Linus Torvalds wrote:
> That said, this whole thing worries me. The fact that the compiler
> magically makes READ_ONCE() require alignment that it normally doesn't
> require seems like a bug waiting to happen somewhere else.

For the record, I do not think it is the compiler doing this, it is the
arm64 code after commit e35123d83ee3 ("arm64: lto: Strengthen
READ_ONCE() to acquire when CONFIG_LTO=y") back in 5.11.

> Because I do think that we might want READ_ONCE() on unaligned data in
> general. Should said places generally use "get_unaligned()"? Sure. And
> are unaligned accesses potentially non-atomic anyway because of
> hardware? Also sure.
> 
> But one reason for READ_ONCE() isn't for some kind of hardware
> atomicity, it is to avoid any ToCToU issues due to compilers doing bad
> things.
> 
> And then this seems to be a serious issue with the whole "READ_ONCE()
> now requires alignment that it didn't require before".
> 
> Put another way: I wonder what other cases may lurk around this all...

That change has caused only one issue that I know of, which was fixed by
commit d3f450533bbc ("efi: tpm: Avoid READ_ONCE() for accessing the
event log"). I have not seen any since then until this point and I do
daily boots of -next with LTO enabled on both of my arm64 test machines.

Cheers,
Nathan

On Wed, 26 Mar 2025 at 15:54, Nathan Chancellor <nathan@kernel.org> wrote:
>
> > Put another way: I wonder what other cases may lurk around this all...
>
> That change has caused only one issue that I know of, which was fixed by
> commit d3f450533bbc ("efi: tpm: Avoid READ_ONCE() for accessing the
> event log"). I have not seen any since then until this point and I do
> daily boots of -next with LTO enabled on both of my arm64 test machines.

Ahh, ok. That makes me happier.

I guess unaligned READ_ONCE() code really shouldn't exist in generic
code anyway, since some architectures will fail any unaligned access.

But those architectures tend to not get a lot of testing (they are a
dying breed - good riddance), so "shouldn't exist" doesn't necessarily
equate to really not existing.

          Linus

On Wed, Mar 26, 2025 at 11:41 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Wed, 26 Mar 2025 at 14:24, Arnd Bergmann <arnd@arndb.de> wrote:
> >
> > I've applied this on top of the asm-generic branch, but I just sent
> > the pull request with the regression to Linus an hour ago.
> >
> > I'll try to get a new pull request out tomorrow.
>
> I will ignore that pull request, and wait for your updated one.
>
> That said, this whole thing worries me. The fact that the compiler
> magically makes READ_ONCE() require alignment that it normally doesn't
> require seems like a bug waiting to happen somewhere else.

To be clear, this is not the compiler's doing. The arch-specific arm64
code defines a custom version of the __READ_ONCE() macro used by
READ_ONCE() in LTO builds:

https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/include/asm/rwonce.h

/*
 * When building with LTO, there is an increased risk of the compiler
 * converting an address dependency headed by a READ_ONCE() invocation
 * into a control dependency and consequently allowing for harmful
 * reordering by the CPU.
 *
 * Ensure that such transformations are harmless by overriding the generic
 * READ_ONCE() definition with one that provides RCpc acquire semantics
 * when building with LTO.
 */