By exposing this variable as a config option, conditional compilation in Rust code may rely on it to determine whether it should use a volatile read or call a C helper function to perform a READ_ONCE operation. This config option is also added on alpha for consistency, even if Rust does not support alpha right now. Signed-off-by: Alice Ryhl --- arch/Kconfig | 11 +++++++++++ arch/alpha/Kconfig | 1 + arch/alpha/include/asm/rwonce.h | 4 ++-- arch/arm64/Kconfig | 1 + arch/arm64/include/asm/rwonce.h | 4 ++-- 5 files changed, 17 insertions(+), 4 deletions(-) diff --git a/arch/Kconfig b/arch/Kconfig index 31220f512b16d5cfbc259935c2d3675b60c1e25c..683176bb09e50e31f398bb92678283e5de66b282 100644 --- a/arch/Kconfig +++ b/arch/Kconfig @@ -229,6 +229,17 @@ config HAVE_EFFICIENT_UNALIGNED_ACCESS See Documentation/core-api/unaligned-memory-access.rst for more information on the topic of unaligned memory accesses. +config ARCH_USE_CUSTOM_READ_ONCE + bool + help + Some architectures reorder address-dependent volatile loads, + which means that the default implementation of READ_ONCE that + relies on a volatile load is not appropriate. + + This symbol should be selected by an architecture if it + redefines READ_ONCE to use a different implementation than a + volatile load. + config ARCH_USE_BUILTIN_BSWAP bool help diff --git a/arch/alpha/Kconfig b/arch/alpha/Kconfig index 80367f2cf821ceb4fc29485b7b21b37d5c310765..1d5d48153ba0087554221e9412a6af0c672d3f5c 100644 --- a/arch/alpha/Kconfig +++ b/arch/alpha/Kconfig @@ -11,6 +11,7 @@ config ALPHA select ARCH_NO_PREEMPT select ARCH_NO_SG_CHAIN select ARCH_USE_CMPXCHG_LOCKREF + select ARCH_USE_CUSTOM_READ_ONCE if SMP select FORCE_PCI select PCI_DOMAINS if PCI select PCI_SYSCALL if PCI diff --git a/arch/alpha/include/asm/rwonce.h b/arch/alpha/include/asm/rwonce.h index 35542bcf92b3a883df353784bcb2d243475ccd91..c9f21aa0764625b24e8957923926d09a2eb97e7c 100644 --- a/arch/alpha/include/asm/rwonce.h +++ b/arch/alpha/include/asm/rwonce.h @@ -5,7 +5,7 @@ #ifndef __ASM_RWONCE_H #define __ASM_RWONCE_H -#ifdef CONFIG_SMP +#ifdef CONFIG_ARCH_USE_CUSTOM_READ_ONCE #include @@ -28,7 +28,7 @@ (typeof(x))__x; \ }) -#endif /* CONFIG_SMP */ +#endif /* CONFIG_ARCH_USE_CUSTOM_READ_ONCE */ #include diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index 93173f0a09c7deb07b46ab4f16a1a0e4320dfbf1..cd16053c8302479458a05c23ba9cfb73ee50232c 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -89,6 +89,7 @@ config ARM64 select ARCH_KEEP_MEMBLOCK select ARCH_MHP_MEMMAP_ON_MEMORY_ENABLE select ARCH_USE_CMPXCHG_LOCKREF + select ARCH_USE_CUSTOM_READ_ONCE if LTO select ARCH_USE_GNU_PROPERTY select ARCH_USE_MEMTEST select ARCH_USE_QUEUED_RWLOCKS diff --git a/arch/arm64/include/asm/rwonce.h b/arch/arm64/include/asm/rwonce.h index 78beceec10cda47b319db29d9f79d2a5df35e92d..5da6b2d6a12399a520f7a3310014de723baa278a 100644 --- a/arch/arm64/include/asm/rwonce.h +++ b/arch/arm64/include/asm/rwonce.h @@ -5,7 +5,7 @@ #ifndef __ASM_RWONCE_H #define __ASM_RWONCE_H -#if defined(CONFIG_LTO) && !defined(__ASSEMBLER__) +#if defined(CONFIG_ARCH_USE_CUSTOM_READ_ONCE) && !defined(__ASSEMBLER__) #include #include @@ -62,7 +62,7 @@ }) #endif /* !BUILD_VDSO */ -#endif /* CONFIG_LTO && !__ASSEMBLER__ */ +#endif /* CONFIG_ARCH_USE_CUSTOM_READ_ONCE && !__ASSEMBLER__ */ #include -- 2.52.0.351.gbe84eed79e-goog There are currently a few places in the kernel where we use volatile reads when we really should be using `READ_ONCE`. To make it possible to replace these with proper `READ_ONCE` calls, introduce a Rust version of `READ_ONCE`. I've written the code to use Rust's volatile ops directly when possible. This results in a small amount of code duplication, but I think it makes sense for READ_ONCE and WRITE_ONCE to be implemented in pure Rust when possible. Otherwise they would unconditionally be a function call unless you have a system where you can perform cross-language inlining. I considered these functions in the bindings crate instead of kernel crate. I actually think it would make a lot of sense. But it implies some annoying complications on old compilers since the #![feature()] invocations in kernel/lib.rs do not apply in the bindings crate. For now, we do not support using READ_ONCE on compound types even if they have the right size. This can be added later. This fails checkpatch due to a misordered MAINTAINERS entry, but this is a pre-existing problem. Signed-off-by: Alice Ryhl --- MAINTAINERS | 2 + rust/helpers/helpers.c | 1 + rust/helpers/rwonce.c | 34 ++++++++ rust/kernel/sync.rs | 2 + rust/kernel/sync/rwonce.rs | 188 +++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 227 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index 12f49de7fe036c2439c00f9f4c67b2219d72a4c3..1d0cae158fe2cc7d99b6a64c11176b635e2d14e4 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -4117,9 +4117,11 @@ F: arch/*/include/asm/atomic*.h F: include/*/atomic*.h F: include/linux/refcount.h F: scripts/atomic/ +F: rust/helpers/rwonce.c F: rust/kernel/sync/atomic.rs F: rust/kernel/sync/atomic/ F: rust/kernel/sync/refcount.rs +F: rust/kernel/sync/rwonce.rs ATTO EXPRESSSAS SAS/SATA RAID SCSI DRIVER M: Bradley Grove diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c index 79c72762ad9c4b473971e6210c9577860d2e2b08..28b79ca7844fb744e5ad128238824921c055ec82 100644 --- a/rust/helpers/helpers.c +++ b/rust/helpers/helpers.c @@ -48,6 +48,7 @@ #include "rcu.c" #include "refcount.c" #include "regulator.c" +#include "rwonce.c" #include "scatterlist.c" #include "security.c" #include "signal.c" diff --git a/rust/helpers/rwonce.c b/rust/helpers/rwonce.c new file mode 100644 index 0000000000000000000000000000000000000000..55c621678cd632e728cb925b6a4a2e34e2fc4884 --- /dev/null +++ b/rust/helpers/rwonce.c @@ -0,0 +1,34 @@ +// SPDX-License-Identifier: GPL-2.0 + +/* + * Copyright (C) 2025 Google LLC. + */ + +#ifdef CONFIG_ARCH_USE_CUSTOM_READ_ONCE + +__rust_helper u8 rust_helper_read_once_1(const u8 *ptr) +{ + return READ_ONCE(*ptr); +} + +__rust_helper u16 rust_helper_read_once_2(const u16 *ptr) +{ + return READ_ONCE(*ptr); +} + +__rust_helper u32 rust_helper_read_once_4(const u32 *ptr) +{ + return READ_ONCE(*ptr); +} + +__rust_helper u64 rust_helper_read_once_8(const u64 *ptr) +{ + return READ_ONCE(*ptr); +} + +__rust_helper void *rust_helper_read_once_ptr(void * const *ptr) +{ + return READ_ONCE(*ptr); +} + +#endif diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs index 5df87e2bd212e192b8a67644bd99f05b9d4afd75..a5bf7bdc3fa8a044786eafae39fe8844aeeef057 100644 --- a/rust/kernel/sync.rs +++ b/rust/kernel/sync.rs @@ -20,6 +20,7 @@ pub mod poll; pub mod rcu; mod refcount; +pub mod rwonce; mod set_once; pub use arc::{Arc, ArcBorrow, UniqueArc}; @@ -30,6 +31,7 @@ pub use lock::spinlock::{new_spinlock, SpinLock, SpinLockGuard}; pub use locked_by::LockedBy; pub use refcount::Refcount; +pub use rwonce::{READ_ONCE, WRITE_ONCE}; pub use set_once::SetOnce; /// Represents a lockdep class. It's a wrapper around C's `lock_class_key`. diff --git a/rust/kernel/sync/rwonce.rs b/rust/kernel/sync/rwonce.rs new file mode 100644 index 0000000000000000000000000000000000000000..a1660e43c9ef94011812d1816713cf031a73de1d --- /dev/null +++ b/rust/kernel/sync/rwonce.rs @@ -0,0 +1,188 @@ +// SPDX-License-Identifier: GPL-2.0 + +// Copyright (C) 2025 Google LLC. + +//! Rust version of the raw `READ_ONCE`/`WRITE_ONCE` functions. +//! +//! C header: [`include/asm-generic/rwonce.h`](srctree/include/asm-generic/rwonce.h) + +/// Read the pointer once. +/// +/// # Safety +/// +/// It must be safe to `READ_ONCE` the `ptr` with this type. +#[inline(always)] +#[must_use] +#[track_caller] +#[expect(non_snake_case)] +pub unsafe fn READ_ONCE(ptr: *const T) -> T { + // SAFETY: It's safe to read `ptr` once with this type. + unsafe { T::read_once(ptr) } +} + +/// Write the pointer once. +/// +/// # Safety +/// +/// It must be safe to `WRITE_ONCE` the `ptr` with this type. +#[inline(always)] +#[track_caller] +#[expect(non_snake_case)] +pub unsafe fn WRITE_ONCE(ptr: *mut T, val: T) { + // SAFETY: It's safe to write `ptr` once with this type. + unsafe { T::write_once(ptr, val) }; +} + +/// This module contains the generic implementations. +#[expect(clippy::undocumented_unsafe_blocks)] +#[expect(clippy::missing_safety_doc)] +mod rwonce_generic_impl { + use core::ffi::c_void; + #[allow(unused_imports)] + use core::ptr::{read_volatile, write_volatile}; + + #[inline(always)] + #[track_caller] + #[cfg(not(CONFIG_ARCH_USE_CUSTOM_READ_ONCE))] + pub(super) unsafe fn read_once_1(ptr: *const u8) -> u8 { + unsafe { read_volatile::(ptr) } + } + + #[inline(always)] + #[track_caller] + #[cfg(not(CONFIG_ARCH_USE_CUSTOM_READ_ONCE))] + pub(super) unsafe fn read_once_2(ptr: *const u16) -> u16 { + unsafe { read_volatile::(ptr) } + } + + #[inline(always)] + #[track_caller] + #[cfg(not(CONFIG_ARCH_USE_CUSTOM_READ_ONCE))] + pub(super) unsafe fn read_once_4(ptr: *const u32) -> u32 { + unsafe { read_volatile::(ptr) } + } + + #[inline(always)] + #[track_caller] + #[cfg(not(CONFIG_ARCH_USE_CUSTOM_READ_ONCE))] + pub(super) unsafe fn read_once_8(ptr: *const u64) -> u64 { + unsafe { read_volatile::(ptr) } + } + + #[inline(always)] + #[track_caller] + #[cfg(not(CONFIG_ARCH_USE_CUSTOM_READ_ONCE))] + pub(super) unsafe fn read_once_ptr(ptr: *const *mut c_void) -> *mut c_void { + unsafe { read_volatile::<*mut c_void>(ptr) } + } + + #[inline(always)] + #[track_caller] + pub(super) unsafe fn write_once_1(ptr: *mut u8, val: u8) { + unsafe { write_volatile::(ptr, val) } + } + + #[inline(always)] + #[track_caller] + pub(super) unsafe fn write_once_2(ptr: *mut u16, val: u16) { + unsafe { write_volatile::(ptr, val) } + } + + #[inline(always)] + #[track_caller] + pub(super) unsafe fn write_once_4(ptr: *mut u32, val: u32) { + unsafe { write_volatile::(ptr, val) } + } + + #[inline(always)] + #[track_caller] + pub(super) unsafe fn write_once_8(ptr: *mut u64, val: u64) { + unsafe { write_volatile::(ptr, val) } + } + + #[inline(always)] + #[track_caller] + pub(super) unsafe fn write_once_ptr(ptr: *mut *mut c_void, val: *mut c_void) { + unsafe { write_volatile::<*mut c_void>(ptr, val) } + } +} +use rwonce_generic_impl::*; + +#[cfg(CONFIG_ARCH_USE_CUSTOM_READ_ONCE)] +use bindings::{read_once_1, read_once_2, read_once_4, read_once_8, read_once_ptr}; + +/// Rust trait for types that may be used with `READ_ONCE`/`WRITE_ONCE`. +/// +/// This serves a similar purpose to the `compiletime_assert_rwonce_type` macro in the C header. +pub trait RwOnceType { + /// The `READ_ONCE` for this type. + /// + /// # Safety + /// + /// It must be safe to `READ_ONCE` the `ptr` with this type. + unsafe fn read_once(ptr: *const Self) -> Self; + + /// The `WRITE_ONCE` for this type. + /// + /// # Safety + /// + /// It must be safe to `WRITE_ONCE` the `ptr` with this type. + unsafe fn write_once(ptr: *mut Self, val: Self); +} + +macro_rules! impl_rw_once_type { + ($($t:ty, $read:ident, $write:ident $(, <$u:ident>)?;)*) => {$( + #[allow(unknown_lints, reason = "unnecessary_transmutes is unknown prior to MSRV 1.88.0")] + #[allow(unnecessary_transmutes)] + #[allow(clippy::missing_transmute_annotations)] + #[allow(clippy::useless_transmute)] + impl$(<$u>)? RwOnceType for $t { + #[inline(always)] + #[track_caller] + unsafe fn read_once(ptr: *const Self) -> Self { + // SAFETY: The caller ensures we can `READ_ONCE`. + // + // Note that `transmute` fails to compile if the two types are of different sizes. + unsafe { core::mem::transmute($read(ptr.cast())) } + } + + #[inline(always)] + #[track_caller] + unsafe fn write_once(ptr: *mut Self, val: Self) { + // SAFETY: The caller ensures we can `WRITE_ONCE`. + unsafe { $write(ptr.cast(), core::mem::transmute(val)) }; + } + } + )*} +} + +// These macros determine which types may be used with rwonce, and which helper function should be +// used if so. +// +// Note that `core::mem::transmute` fails the build if the source and target type have different +// sizes, so picking the wrong helper should lead to a build error. + +impl_rw_once_type! { + u8, read_once_1, write_once_1; + i8, read_once_1, write_once_1; + u16, read_once_2, write_once_2; + i16, read_once_2, write_once_2; + u32, read_once_4, write_once_4; + i32, read_once_4, write_once_4; + u64, read_once_8, write_once_8; + i64, read_once_8, write_once_8; + *mut T, read_once_ptr, write_once_ptr, ; + *const T, read_once_ptr, write_once_ptr, ; +} + +#[cfg(target_pointer_width = "32")] +impl_rw_once_type! { + usize, read_once_4, write_once_4; + isize, read_once_4, write_once_4; +} + +#[cfg(target_pointer_width = "64")] +impl_rw_once_type! { + usize, read_once_8, write_once_8; + isize, read_once_8, write_once_8; +} -- 2.52.0.351.gbe84eed79e-goog Normally it is undefined behavior for a bool to take any value other than 0 or 1. However, in the case of READ_ONCE(some_bool) is used, this UB seems dangerous and unnecessary. I can easily imagine some Rust code that looks like this: if READ_ONCE(&raw const (*my_c_struct).my_bool_field) { ... } And by making an analogy to what the equivalent C code is, anyone writing this probably just meant to treat any non-zero value as true. For WRITE_ONCE no special logic is required. Signed-off-by: Alice Ryhl --- rust/kernel/sync/rwonce.rs | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/rust/kernel/sync/rwonce.rs b/rust/kernel/sync/rwonce.rs index a1660e43c9ef94011812d1816713cf031a73de1d..73477f53131926996614df573b2d50fff98e624f 100644 --- a/rust/kernel/sync/rwonce.rs +++ b/rust/kernel/sync/rwonce.rs @@ -163,6 +163,7 @@ unsafe fn write_once(ptr: *mut Self, val: Self) { // sizes, so picking the wrong helper should lead to a build error. impl_rw_once_type! { + bool, read_once_bool, write_once_1; u8, read_once_1, write_once_1; i8, read_once_1, write_once_1; u16, read_once_2, write_once_2; @@ -186,3 +187,21 @@ unsafe fn write_once(ptr: *mut Self, val: Self) { usize, read_once_8, write_once_8; isize, read_once_8, write_once_8; } + +/// Read an integer as a boolean once. +/// +/// Returns `true` if the value behind the pointer is non-zero. Otherwise returns `false`. +/// +/// # Safety +/// +/// It must be safe to `READ_ONCE` the `ptr` with type `u8`. +#[inline(always)] +#[track_caller] +unsafe fn read_once_bool(ptr: *const bool) -> bool { + // Implement `read_once_bool` in terms of `read_once_1`. The arch-specific logic is inside + // of `read_once_1`. + // + // SAFETY: It is safe to `READ_ONCE` the `ptr` with type `u8`. + let byte = unsafe { read_once_1(ptr.cast::()) }; + byte != 0u8 +} -- 2.52.0.351.gbe84eed79e-goog Using `READ_ONCE` is the correct way to read the `node.expires` field. Signed-off-by: Alice Ryhl --- rust/kernel/time/hrtimer.rs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs index 856d2d929a00892dc8eaec63cebdf547817953d3..e2b7a26f8aade972356c3eb5f6489bcda3e2e849 100644 --- a/rust/kernel/time/hrtimer.rs +++ b/rust/kernel/time/hrtimer.rs @@ -239,11 +239,9 @@ pub fn expires(&self) -> HrTimerInstant // - Timers cannot have negative ktime_t values as their expiration time. // - There's no actual locking here, a racy read is fine and expected unsafe { - Instant::from_ktime( - // This `read_volatile` is intended to correspond to a READ_ONCE call. - // FIXME(read_once): Replace with `read_once` when available on the Rust side. - core::ptr::read_volatile(&raw const ((*c_timer_ptr).node.expires)), - ) + Instant::from_ktime(kernel::sync::READ_ONCE( + &raw const (*c_timer_ptr).node.expires, + )) } } } -- 2.52.0.351.gbe84eed79e-goog Using `READ_ONCE` is the correct way to read the `f_flags` field. Signed-off-by: Alice Ryhl --- rust/kernel/fs/file.rs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/rust/kernel/fs/file.rs b/rust/kernel/fs/file.rs index 23ee689bd2400565223181645157d832a836589f..6b07f08e7012f512e53743266096ce0076d29e1c 100644 --- a/rust/kernel/fs/file.rs +++ b/rust/kernel/fs/file.rs @@ -335,12 +335,8 @@ pub fn cred(&self) -> &Credential { /// The flags are a combination of the constants in [`flags`]. #[inline] pub fn flags(&self) -> u32 { - // This `read_volatile` is intended to correspond to a READ_ONCE call. - // - // SAFETY: The file is valid because the shared reference guarantees a nonzero refcount. - // - // FIXME(read_once): Replace with `read_once` when available on the Rust side. - unsafe { core::ptr::addr_of!((*self.as_ptr()).f_flags).read_volatile() } + // SAFETY: The `f_flags` field of `struct file` is readable with `READ_ONCE`. + unsafe { kernel::sync::READ_ONCE(&raw const (*self.as_ptr()).f_flags) } } } -- 2.52.0.351.gbe84eed79e-goog