* [PATCH v3 0/2] rust: add pointer projection infrastructure and convert DMA
@ 2026-03-02 13:02 Gary Guo
2026-03-02 13:02 ` [PATCH v3 1/2] rust: add projection infrastructure Gary Guo
2026-03-02 13:02 ` [PATCH v3 2/2] rust: dma: use pointer projection infra for `dma_{read,write}` macro Gary Guo
0 siblings, 2 replies; 16+ messages in thread
From: Gary Guo @ 2026-03-02 13:02 UTC (permalink / raw)
To: Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
Danilo Krummrich
Cc: rust-for-linux
From: Gary Guo <gary@garyguo.net>
This adds a general pointer projection infrastructure which is intended to
serve as the basis for generic I/O projections. The idea is that we will
have a unified way of reading/writing MMIO or DMA allocation, creating
typed subviews into them, and able to read/write subfields safely.
This is the first step towards that direction, where an infra for
projecting arbitrary pointers are added. This takes care to not assume
pointers are part of a valid allocation, so it can be used to project I/O
pointers, too. `dma_read!` and `dma_write!` is converted to use this infra,
as they currently have soundness issues [1] and users have already
encountered limitation of it [2].
Link: https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/Soundness.20of.20.60dma_read!.28.29.60/with/573645610 [1]
Link: https://lore.kernel.org/rust-for-linux/DGE9DBH7X8CO.2EES6EX8UA5ZF@kernel.org/T/#mc17b9c0309ac10346b3d6c6bd8461488e7c73161 [2]
---
Changes since v2:
- Fix off-by-one issue (Aditya)
- Fix comment (Aditya)
- Link to v2: https://lore.kernel.org/rust-for-linux/20260226154656.3241736-1-gary@kernel.org/
Changes since v1:
- Squashed patch 2,3,4 so that all users are converted in single commit
(Danilo)
- I discovered an additional soundness bug in dma_read/write macros
where it can create misaligned access. This is addressed in this new
version.
- Updated sample driver to have a function that does check instead use a
budget-try block (Benno)
- Improved diagnostics when index projection is used on types that are
not slices.
- Link to v1: https://lore.kernel.org/rust-for-linux/20260214053344.1994776-1-gary@garyguo.net/
---
Gary Guo (2):
rust: add projection infrastructure
rust: dma: use pointer projection infra for `dma_{read,write}` macro
drivers/gpu/nova-core/gsp.rs | 14 +-
drivers/gpu/nova-core/gsp/boot.rs | 2 +-
drivers/gpu/nova-core/gsp/cmdq.rs | 10 +-
rust/kernel/dma.rs | 114 ++++++------
rust/kernel/lib.rs | 5 +
rust/kernel/projection.rs | 284 ++++++++++++++++++++++++++++++
samples/rust/rust_dma.rs | 30 ++--
scripts/Makefile.build | 4 +-
8 files changed, 373 insertions(+), 90 deletions(-)
create mode 100644 rust/kernel/projection.rs
base-commit: 877552aa875839314afad7154b5a561889e87ea9
--
2.51.2
^ permalink raw reply [flat|nested] 16+ messages in thread* [PATCH v3 1/2] rust: add projection infrastructure 2026-03-02 13:02 [PATCH v3 0/2] rust: add pointer projection infrastructure and convert DMA Gary Guo @ 2026-03-02 13:02 ` Gary Guo 2026-03-02 14:38 ` Benno Lossin 2026-03-02 13:02 ` [PATCH v3 2/2] rust: dma: use pointer projection infra for `dma_{read,write}` macro Gary Guo 1 sibling, 1 reply; 16+ messages in thread From: Gary Guo @ 2026-03-02 13:02 UTC (permalink / raw) To: Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross, Danilo Krummrich, Nathan Chancellor, Nicolas Schier Cc: rust-for-linux, Aditya Rajan, linux-kernel, linux-kbuild From: Gary Guo <gary@garyguo.net> Add a generic infrastructure for performing field and index projections on raw pointers. This will form the basis of performing I/O projections. Pointers manipulations are intentionally using the safe wrapping variants instead of the unsafe variants, as the latter requires pointers to be inside an allocation which is not necessarily true for I/O pointers. This projection macro protects against rogue `Deref` implementation, which can causes the projected pointer to be outside the bounds of starting pointer. This is extremely unlikely and Rust has a lint to catch this, but is unsoundness regardless. The protection works by inducing type inference ambiguity when `Deref` is implemented. This projection macro also stops projecting into unaligned fields (i.e. fields of `#[repr(packed)]` structs), as misaligned pointers require special handling. This is implemented by attempting to create reference to projected field inside a `if false` block. Despite being unreachable, Rust still checks that they're not unaligned fields. The projection macro supports both fallible and infallible index projections. These are described in detail inside the documentation. Reviewed-by: Aditya Rajan <adi.dev.github@gmail.com> Signed-off-by: Gary Guo <gary@garyguo.net> --- rust/kernel/lib.rs | 5 + rust/kernel/projection.rs | 284 ++++++++++++++++++++++++++++++++++++++ scripts/Makefile.build | 4 +- 3 files changed, 292 insertions(+), 1 deletion(-) create mode 100644 rust/kernel/projection.rs diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs index 3da92f18f4ee..50866b481bdb 100644 --- a/rust/kernel/lib.rs +++ b/rust/kernel/lib.rs @@ -20,6 +20,7 @@ #![feature(generic_nonzero)] #![feature(inline_const)] #![feature(pointer_is_aligned)] +#![feature(slice_ptr_len)] // // Stable since Rust 1.80.0. #![feature(slice_flatten)] @@ -37,6 +38,9 @@ #![feature(const_ptr_write)] #![feature(const_refs_to_cell)] // +// Stable since Rust 1.84.0. +#![feature(strict_provenance)] +// // Expected to become stable. #![feature(arbitrary_self_types)] // @@ -130,6 +134,7 @@ pub mod prelude; pub mod print; pub mod processor; +pub mod projection; pub mod ptr; #[cfg(CONFIG_RUST_PWM_ABSTRACTIONS)] pub mod pwm; diff --git a/rust/kernel/projection.rs b/rust/kernel/projection.rs new file mode 100644 index 000000000000..b9e2b174615c --- /dev/null +++ b/rust/kernel/projection.rs @@ -0,0 +1,284 @@ +// SPDX-License-Identifier: GPL-2.0 + +//! Infrastructure for handling projections. + +use core::{ + mem::MaybeUninit, + ops::Deref, // +}; + +use crate::prelude::*; + +/// Error raised when a projection is attempted on array or slices out of bounds. +pub struct OutOfBound; + +impl From<OutOfBound> for Error { + #[inline(always)] + fn from(_: OutOfBound) -> Self { + ERANGE + } +} + +/// A helper trait to perform index projection. +/// +/// This is similar to `core::slice::SliceIndex`, but operate on raw pointers safely and fallibly. +/// +/// # Safety +/// +/// `get` must return a pointer in bounds of the provided pointer. +#[diagnostic::on_unimplemented(message = "`{Self}` cannot be used to index `{T}`")] +#[doc(hidden)] +pub unsafe trait ProjectIndex<T: ?Sized>: Sized { + type Output: ?Sized; + + /// Returns an index-projected pointer, if in bounds. + fn get(self, slice: *mut T) -> Option<*mut Self::Output>; + + /// Returns an index-projected pointer; fail the build if it cannot be proved to be in bounds. + #[inline(always)] + fn index(self, slice: *mut T) -> *mut Self::Output { + Self::get(self, slice).unwrap_or_else(|| build_error!()) + } +} + +// Forward array impl to slice impl. +// SAFETY: `get` returned pointers are in bounds. +unsafe impl<T, I, const N: usize> ProjectIndex<[T; N]> for I +where + I: ProjectIndex<[T]>, +{ + type Output = <I as ProjectIndex<[T]>>::Output; + + #[inline(always)] + fn get(self, slice: *mut [T; N]) -> Option<*mut Self::Output> { + <I as ProjectIndex<[T]>>::get(self, slice) + } + + #[inline(always)] + fn index(self, slice: *mut [T; N]) -> *mut Self::Output { + <I as ProjectIndex<[T]>>::index(self, slice) + } +} + +// SAFETY: `get` returned pointers are in bounds. +unsafe impl<T> ProjectIndex<[T]> for usize { + type Output = T; + + #[inline(always)] + fn get(self, slice: *mut [T]) -> Option<*mut T> { + if self >= slice.len() { + None + } else { + Some(slice.cast::<T>().wrapping_add(self)) + } + } +} + +// SAFETY: `get` returned pointers are in bounds. +unsafe impl<T> ProjectIndex<[T]> for core::ops::Range<usize> { + type Output = [T]; + + #[inline(always)] + fn get(self, slice: *mut [T]) -> Option<*mut [T]> { + let new_len = self.end.checked_sub(self.start)?; + if self.end > slice.len() { + return None; + } + Some(core::ptr::slice_from_raw_parts_mut( + slice.cast::<T>().wrapping_add(self.start), + new_len, + )) + } +} + +// SAFETY: `get` returned pointers are in bounds. +unsafe impl<T> ProjectIndex<[T]> for core::ops::RangeTo<usize> { + type Output = [T]; + + #[inline(always)] + fn get(self, slice: *mut [T]) -> Option<*mut [T]> { + (0..self.end).get(slice) + } +} + +// SAFETY: `get` returned pointers are in bounds. +unsafe impl<T> ProjectIndex<[T]> for core::ops::RangeFrom<usize> { + type Output = [T]; + + #[inline(always)] + fn get(self, slice: *mut [T]) -> Option<*mut [T]> { + (self.start..slice.len()).get(slice) + } +} + +// SAFETY: `get` returned pointers are in bounds. +unsafe impl<T> ProjectIndex<[T]> for core::ops::RangeFull { + type Output = [T]; + + #[inline(always)] + fn get(self, slice: *mut [T]) -> Option<*mut [T]> { + Some(slice) + } +} + +/// A helper trait to perform field projection. +/// +/// This trait has a `DEREF` generic parameter so it can be implemented twice for types that +/// implement `Deref`. This will cause an ambiguity error and thus block `Deref` types being used +/// as base of projection, as they can inject unsoundness. +/// +/// # Safety +/// +/// `proj` should invoke `f` with valid allocation, as documentation described. +#[doc(hidden)] +pub unsafe trait ProjectField<const DEREF: bool> { + /// Project a pointer to a type to a pointer of a field. + /// + /// `f` is always invoked with valid allocation so it can safely obtain raw pointers to fields + /// using `&raw mut`. + /// + /// This is needed because `base` might not point to a valid allocation, while `&raw mut` + /// requires pointers to be in bounds of a valid allocation. + /// + /// # Safety + /// + /// `f` must returns a pointer in bounds of the provided pointer. + unsafe fn proj<F>(base: *mut Self, f: impl FnOnce(*mut Self) -> *mut F) -> *mut F; +} + +// NOTE: in theory, this API should work for `T: ?Sized` and `F: ?Sized`, too. However we cannot +// currently support that as we need to obtain a valid allocation that `&raw const` can operate on. +// SAFETY: `proj` invokes `f` with valid allocation. +unsafe impl<T> ProjectField<false> for T { + #[inline(always)] + unsafe fn proj<F>(base: *mut Self, f: impl FnOnce(*mut Self) -> *mut F) -> *mut F { + // Create a valid allocation to start projection, as `base` is not necessarily so. The + // memory is never actually used so it will be optimized out, so it should work even for + // very large `T` (`memoffset` crate also relies on this). To be extra certain, we also + // annotate `f` closure with `#[inline(always)]` in the macro. + let mut place = MaybeUninit::uninit(); + let place_base = place.as_mut_ptr(); + let field = f(place_base); + // SAFETY: `field` is in bounds from `base` per safety requirement. + let offset = unsafe { field.byte_offset_from(place_base) }; + // Use `wrapping_byte_offset` as `base` does not need to be of valid allocation. + base.wrapping_byte_offset(offset).cast() + } +} + +// SAFETY: vacuously satisfied. +unsafe impl<T: Deref> ProjectField<true> for T { + #[inline(always)] + unsafe fn proj<F>(_: *mut Self, _: impl FnOnce(*mut Self) -> *mut F) -> *mut F { + build_error!("this function is a guard against `Deref` impl and is never invoked"); + } +} + +/// Create a projection from a raw pointer. +/// +/// The projected pointer is within the memory region marked by the input pointer. There is no +/// requirement that the input raw pointer needs to be valid, so this macro may be used for +/// projecting pointers outside normal address space, e.g. I/O pointers. However, if the input +/// pointer is valid, the projected pointer is also valid. +/// +/// Supported projections include field projections and index projections. +/// It is not allowed to project into types that implement custom `Deref` or `Index`. +/// +/// The macro has basic syntax of `kernel::project_pointer!(ptr, projection)`, where `ptr` is an +/// expression that evaluates to a raw pointer which serves as the base of projection. `projection` +/// can be a projection expression of form `.field` (normally identifer, or numeral in case of +/// tuple structs) or of form `[index]`. +/// +/// If mutable pointer is needed, the macro input can be prefixed with `mut` keyword, i.e. +/// `kernel::project_pointer!(mut ptr, projection)`. By default, a const pointer is created. +/// +/// `project_pointer!` macro can perform both fallible indexing and build-time checked indexing. +/// `[index]` form performs build-time bounds checking; if compiler fails to prove `[index]` is in +/// bounds, compilation will fail. `[index]?` can be used to perform runtime bounds checking; +/// `OutOfBound` error is raised via `?` if the index is out of bounds. +/// +/// # Examples +/// +/// Field projections are performed with `.field_name`: +/// ``` +/// struct MyStruct { field: u32, } +/// let ptr: *const MyStruct = core::ptr::dangling(); +/// let field_ptr: *const u32 = kernel::project_pointer!(ptr, .field); +/// +/// struct MyTupleStruct(u32, u32); +/// +/// fn proj(ptr: *const MyTupleStruct) { +/// let field_ptr: *const u32 = kernel::project_pointer!(ptr, .1); +/// } +/// ``` +/// +/// Index projections are performed with `[index]`: +/// ``` +/// fn proj(ptr: *const [u8; 32]) -> Result { +/// let field_ptr: *const u8 = kernel::project_pointer!(ptr, [1]); +/// // This will fail the build. +/// // kernel::project_pointer!(ptr, [128]); +/// // This will raise an `OutOfBound` error (which is convertable to `ERANGE`). +/// kernel::project_pointer!(ptr, [128]?); +/// Ok(()) +/// } +/// ``` +/// +/// If you need to match on the error instead of propagate, put the invocation inside a closure: +/// ``` +/// let ptr: *const [u8; 32] = core::ptr::dangling(); +/// let field_ptr: Result<*const u8> = (|| -> Result<_> { +/// Ok(kernel::project_pointer!(ptr, [128]?)) +/// })(); +/// assert!(field_ptr.is_err()); +/// ``` +/// +/// For mutable pointers, put `mut` as the first token in macro invocation. +/// ``` +/// let ptr: *mut [(u8, u16); 32] = core::ptr::dangling_mut(); +/// let field_ptr: *mut u16 = kernel::project_pointer!(mut ptr, [1].1); +/// ``` +#[macro_export] +macro_rules! project_pointer { + (@gen $ptr:ident, ) => {}; + // Field projection. `$field` needs to be `tt` to support tuple index like `.0`. + (@gen $ptr:ident, .$field:tt $($rest:tt)*) => { + // SAFETY: the provided closure always return in bounds pointer. + let $ptr = unsafe { + $crate::projection::ProjectField::proj($ptr, #[inline(always)] |ptr| { + // Check unaligned field. Not all users (e.g. DMA) can handle unaligned + // projections. + if false { + let _ = &(*ptr).$field; + } + // SAFETY: `$field` is in bounds, and no implicit `Deref` is possible (if the + // type implements `Deref`, Rust cannot infer the generic parameter `DEREF`). + &raw mut (*ptr).$field + }) + }; + $crate::project_pointer!(@gen $ptr, $($rest)*) + }; + // Fallible index projection. + (@gen $ptr:ident, [$index:expr]? $($rest:tt)*) => { + let $ptr = $crate::projection::ProjectIndex::get($index, $ptr) + .ok_or($crate::projection::OutOfBound)?; + $crate::project_pointer!(@gen $ptr, $($rest)*) + }; + // Build-time checked index projection. + (@gen $ptr:ident, [$index:expr] $($rest:tt)*) => { + let $ptr = $crate::projection::ProjectIndex::index($index, $ptr); + $crate::project_pointer!(@gen $ptr, $($rest)*) + }; + (mut $ptr:expr, $($proj:tt)*) => {{ + let ptr: *mut _ = $ptr; + $crate::project_pointer!(@gen ptr, $($proj)*); + ptr + }}; + ($ptr:expr, $($proj:tt)*) => {{ + let ptr = <*const _>::cast_mut($ptr); + // We currently always project using mutable pointer, as it is not decided whether `&raw + // const` allows the resulting pointer to be mutated (see documentation of `addr_of!`). + $crate::project_pointer!(@gen ptr, $($proj)*); + ptr.cast_const() + }}; +} diff --git a/scripts/Makefile.build b/scripts/Makefile.build index 32e209bc7985..3652b85be545 100644 --- a/scripts/Makefile.build +++ b/scripts/Makefile.build @@ -310,16 +310,18 @@ $(obj)/%.lst: $(obj)/%.c FORCE # The features in this list are the ones allowed for non-`rust/` code. # +# - Stable since Rust 1.79.0: `feature(slice_ptr_len)`. # - Stable since Rust 1.81.0: `feature(lint_reasons)`. # - Stable since Rust 1.82.0: `feature(asm_const)`, # `feature(offset_of_nested)`, `feature(raw_ref_op)`. +# - Stable since Rust 1.84.0: `feature(strict_provenance)`. # - Stable since Rust 1.87.0: `feature(asm_goto)`. # - Expected to become stable: `feature(arbitrary_self_types)`. # - To be determined: `feature(used_with_arg)`. # # Please see https://github.com/Rust-for-Linux/linux/issues/2 for details on # the unstable features in use. -rust_allowed_features := asm_const,asm_goto,arbitrary_self_types,lint_reasons,offset_of_nested,raw_ref_op,used_with_arg +rust_allowed_features := asm_const,asm_goto,arbitrary_self_types,lint_reasons,offset_of_nested,raw_ref_op,slice_ptr_len,strict_provenance,used_with_arg # `--out-dir` is required to avoid temporaries being created by `rustc` in the # current working directory, which may be not accessible in the out-of-tree -- 2.51.2 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/2] rust: add projection infrastructure 2026-03-02 13:02 ` [PATCH v3 1/2] rust: add projection infrastructure Gary Guo @ 2026-03-02 14:38 ` Benno Lossin 2026-03-02 14:48 ` Danilo Krummrich 2026-03-02 14:49 ` Gary Guo 0 siblings, 2 replies; 16+ messages in thread From: Benno Lossin @ 2026-03-02 14:38 UTC (permalink / raw) To: Gary Guo, Miguel Ojeda, Boqun Feng, Björn Roy Baron, Andreas Hindborg, Alice Ryhl, Trevor Gross, Danilo Krummrich, Nathan Chancellor, Nicolas Schier Cc: rust-for-linux, Aditya Rajan, linux-kernel, linux-kbuild On Mon Mar 2, 2026 at 2:02 PM CET, Gary Guo wrote: > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs > index 3da92f18f4ee..50866b481bdb 100644 > --- a/rust/kernel/lib.rs > +++ b/rust/kernel/lib.rs > @@ -20,6 +20,7 @@ > #![feature(generic_nonzero)] > #![feature(inline_const)] > #![feature(pointer_is_aligned)] > +#![feature(slice_ptr_len)] This is missing a stability comment (stable since 1.79). > // > // Stable since Rust 1.80.0. > #![feature(slice_flatten)] ... > +/// A helper trait to perform index projection. > +/// > +/// This is similar to `core::slice::SliceIndex`, but operate on raw pointers safely and fallibly. > +/// > +/// # Safety > +/// > +/// `get` must return a pointer in bounds of the provided pointer. This only makes sense when the provided pointer already points at an allocation. But since the functions of this trait aren't `unsafe`, it must be sound to pass `ptr::null` to them. I first thought that we might be able to just use `mem::size_of_val_raw` [1] to give an upper and lower bound on the address of the returned pointer, but that is unsafe and cannot be called with an arbitrary pointer. Interestingly, `ptr::metadata` [2] can be called safely & with any pointer; I would expect them to be very similar (except of course for extern types). [1]: https://doc.rust-lang.org/std/mem/fn.size_of_val_raw.html [2]: https://doc.rust-lang.org/std/ptr/fn.metadata.html A pretty expensive solution would be to add a sealed trait `Indexable` that we implement for all things that `T` is allowed to be; and then we provide a safe function in that trait to query the maximum offset the `get` function is allowed to make. Alternatively, we could use something like this: The implementation of `get` must: - return a pointer obtained by offsetting the input pointer. - ensure that when the input pointer points at a valid value of type `T`, the offset must not be greater than [`mem::size_of_val_raw`] of the input pointer. Or something simpler that says "if the input pointer is valid, then `get` must return a valid output pointer"? > +#[diagnostic::on_unimplemented(message = "`{Self}` cannot be used to index `{T}`")] > +#[doc(hidden)] > +pub unsafe trait ProjectIndex<T: ?Sized>: Sized { > + type Output: ?Sized; > + > + /// Returns an index-projected pointer, if in bounds. > + fn get(self, slice: *mut T) -> Option<*mut Self::Output>; How about we name this `try_index` instead of the general `get`? > + > + /// Returns an index-projected pointer; fail the build if it cannot be proved to be in bounds. > + #[inline(always)] > + fn index(self, slice: *mut T) -> *mut Self::Output { > + Self::get(self, slice).unwrap_or_else(|| build_error!()) > + } > +} ... > +/// A helper trait to perform field projection. > +/// > +/// This trait has a `DEREF` generic parameter so it can be implemented twice for types that > +/// implement `Deref`. This will cause an ambiguity error and thus block `Deref` types being used > +/// as base of projection, as they can inject unsoundness. I think it's important to also say that the ambiguity error only happens when calling the function without specifying the `DEREF` constant. Essentially it is a load-bearing part of the macro that it does this. > +/// > +/// # Safety > +/// > +/// `proj` should invoke `f` with valid allocation, as documentation described. s/should invoke `f` with/may invoke `f` only with a/ "should" sounds like only a suggestion. If it is a requirement, then the `build_error!` impl of the `DEREF = true` case would be violating it. > +#[doc(hidden)] > +pub unsafe trait ProjectField<const DEREF: bool> { > + /// Project a pointer to a type to a pointer of a field. > + /// > + /// `f` is always invoked with valid allocation so it can safely obtain raw pointers to fields > + /// using `&raw mut`. > + /// > + /// This is needed because `base` might not point to a valid allocation, while `&raw mut` > + /// requires pointers to be in bounds of a valid allocation. > + /// > + /// # Safety > + /// > + /// `f` must returns a pointer in bounds of the provided pointer. Typo: "must returns" -> "must return" Cheers, Benno > + unsafe fn proj<F>(base: *mut Self, f: impl FnOnce(*mut Self) -> *mut F) -> *mut F; > +} ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/2] rust: add projection infrastructure 2026-03-02 14:38 ` Benno Lossin @ 2026-03-02 14:48 ` Danilo Krummrich 2026-03-02 18:49 ` Benno Lossin 2026-03-02 14:49 ` Gary Guo 1 sibling, 1 reply; 16+ messages in thread From: Danilo Krummrich @ 2026-03-02 14:48 UTC (permalink / raw) To: Benno Lossin Cc: Gary Guo, Miguel Ojeda, Boqun Feng, Björn Roy Baron, Andreas Hindborg, Alice Ryhl, Trevor Gross, Nathan Chancellor, Nicolas Schier, rust-for-linux, Aditya Rajan, linux-kernel, linux-kbuild On Mon Mar 2, 2026 at 3:38 PM CET, Benno Lossin wrote: > [1]: https://doc.rust-lang.org/std/mem/fn.size_of_val_raw.html I recently would have had a use-case for this as well, but besides the reasons you can't use it in this patch, I think it's also still unstable? ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/2] rust: add projection infrastructure 2026-03-02 14:48 ` Danilo Krummrich @ 2026-03-02 18:49 ` Benno Lossin 0 siblings, 0 replies; 16+ messages in thread From: Benno Lossin @ 2026-03-02 18:49 UTC (permalink / raw) To: Danilo Krummrich Cc: Gary Guo, Miguel Ojeda, Boqun Feng, Björn Roy Baron, Andreas Hindborg, Alice Ryhl, Trevor Gross, Nathan Chancellor, Nicolas Schier, rust-for-linux, Aditya Rajan, linux-kernel, linux-kbuild On Mon Mar 2, 2026 at 3:48 PM CET, Danilo Krummrich wrote: > On Mon Mar 2, 2026 at 3:38 PM CET, Benno Lossin wrote: >> [1]: https://doc.rust-lang.org/std/mem/fn.size_of_val_raw.html > > I recently would have had a use-case for this as well, but besides the reasons > you can't use it in this patch, I think it's also still unstable? Oh yeah that's right. Cheers, Benno ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/2] rust: add projection infrastructure 2026-03-02 14:38 ` Benno Lossin 2026-03-02 14:48 ` Danilo Krummrich @ 2026-03-02 14:49 ` Gary Guo 2026-03-02 18:49 ` Benno Lossin 1 sibling, 1 reply; 16+ messages in thread From: Gary Guo @ 2026-03-02 14:49 UTC (permalink / raw) To: Benno Lossin, Gary Guo, Miguel Ojeda, Boqun Feng, Björn Roy Baron, Andreas Hindborg, Alice Ryhl, Trevor Gross, Danilo Krummrich, Nathan Chancellor, Nicolas Schier Cc: rust-for-linux, Aditya Rajan, linux-kernel, linux-kbuild On Mon Mar 2, 2026 at 2:38 PM GMT, Benno Lossin wrote: > On Mon Mar 2, 2026 at 2:02 PM CET, Gary Guo wrote: >> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs >> index 3da92f18f4ee..50866b481bdb 100644 >> --- a/rust/kernel/lib.rs >> +++ b/rust/kernel/lib.rs >> @@ -20,6 +20,7 @@ >> #![feature(generic_nonzero)] >> #![feature(inline_const)] >> #![feature(pointer_is_aligned)] >> +#![feature(slice_ptr_len)] > > This is missing a stability comment (stable since 1.79). > >> // >> // Stable since Rust 1.80.0. >> #![feature(slice_flatten)] > ... >> +/// A helper trait to perform index projection. >> +/// >> +/// This is similar to `core::slice::SliceIndex`, but operate on raw pointers safely and fallibly. >> +/// >> +/// # Safety >> +/// >> +/// `get` must return a pointer in bounds of the provided pointer. > > This only makes sense when the provided pointer already points at an > allocation. But since the functions of this trait aren't `unsafe`, it > must be sound to pass `ptr::null` to them. The "in bounds" here is the conceptual bounds of the pointer. So, for a pointer with size `x`, the address of the returned pointer lies between `ptr .. ptr + x`. > > I first thought that we might be able to just use `mem::size_of_val_raw` > [1] to give an upper and lower bound on the address of the returned > pointer, but that is unsafe and cannot be called with an arbitrary > pointer. Interestingly, `ptr::metadata` [2] can be called safely & with > any pointer; I would expect them to be very similar (except of course > for extern types). > > [1]: https://doc.rust-lang.org/std/mem/fn.size_of_val_raw.html > [2]: https://doc.rust-lang.org/std/ptr/fn.metadata.html I have a `KnownSize` trait for this in my I/O projection series that is implemented for `T: Sized` and `[T]`, and it returns the size when given a raw pointer. > > A pretty expensive solution would be to add a sealed trait `Indexable` > that we implement for all things that `T` is allowed to be; and then we > provide a safe function in that trait to query the maximum offset the > `get` function is allowed to make. > > Alternatively, we could use something like this: > > The implementation of `get` must: > - return a pointer obtained by offsetting the input pointer. > - ensure that when the input pointer points at a valid value of type > `T`, the offset must not be greater than [`mem::size_of_val_raw`] > of the input pointer. Given that I'm not introducing `KnownSize` trait in this patch, this is why I haven't used this kind of wording. Perhaps I can just bring `KnownSize` in early and use it first for documentation purpose only? > > Or something simpler that says "if the input pointer is valid, then > `get` must return a valid output pointer"? Hmm, wouldn't this give impression that "you can do whatever you want if the input pointer is not valid"? > >> +#[diagnostic::on_unimplemented(message = "`{Self}` cannot be used to index `{T}`")] >> +#[doc(hidden)] >> +pub unsafe trait ProjectIndex<T: ?Sized>: Sized { >> + type Output: ?Sized; >> + >> + /// Returns an index-projected pointer, if in bounds. >> + fn get(self, slice: *mut T) -> Option<*mut Self::Output>; > > How about we name this `try_index` instead of the general `get`? I'm following the name on `SliceIndex`: https://doc.rust-lang.org/stable/std/slice/trait.SliceIndex.html. Best, Gary > >> + >> + /// Returns an index-projected pointer; fail the build if it cannot be proved to be in bounds. >> + #[inline(always)] >> + fn index(self, slice: *mut T) -> *mut Self::Output { >> + Self::get(self, slice).unwrap_or_else(|| build_error!()) >> + } >> +} > ... >> +/// A helper trait to perform field projection. >> +/// >> +/// This trait has a `DEREF` generic parameter so it can be implemented twice for types that >> +/// implement `Deref`. This will cause an ambiguity error and thus block `Deref` types being used >> +/// as base of projection, as they can inject unsoundness. > > I think it's important to also say that the ambiguity error only happens > when calling the function without specifying the `DEREF` constant. > Essentially it is a load-bearing part of the macro that it does this. > >> +/// >> +/// # Safety >> +/// >> +/// `proj` should invoke `f` with valid allocation, as documentation described. > > s/should invoke `f` with/may invoke `f` only with a/ > > "should" sounds like only a suggestion. If it is a requirement, then the > `build_error!` impl of the `DEREF = true` case would be violating it. > >> +#[doc(hidden)] >> +pub unsafe trait ProjectField<const DEREF: bool> { >> + /// Project a pointer to a type to a pointer of a field. >> + /// >> + /// `f` is always invoked with valid allocation so it can safely obtain raw pointers to fields >> + /// using `&raw mut`. >> + /// >> + /// This is needed because `base` might not point to a valid allocation, while `&raw mut` >> + /// requires pointers to be in bounds of a valid allocation. >> + /// >> + /// # Safety >> + /// >> + /// `f` must returns a pointer in bounds of the provided pointer. > > Typo: "must returns" -> "must return" > > Cheers, > Benno > >> + unsafe fn proj<F>(base: *mut Self, f: impl FnOnce(*mut Self) -> *mut F) -> *mut F; >> +} ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/2] rust: add projection infrastructure 2026-03-02 14:49 ` Gary Guo @ 2026-03-02 18:49 ` Benno Lossin 2026-03-02 20:14 ` Gary Guo 0 siblings, 1 reply; 16+ messages in thread From: Benno Lossin @ 2026-03-02 18:49 UTC (permalink / raw) To: Gary Guo, Miguel Ojeda, Boqun Feng, Björn Roy Baron, Andreas Hindborg, Alice Ryhl, Trevor Gross, Danilo Krummrich, Nathan Chancellor, Nicolas Schier Cc: rust-for-linux, Aditya Rajan, linux-kernel, linux-kbuild On Mon Mar 2, 2026 at 3:49 PM CET, Gary Guo wrote: > On Mon Mar 2, 2026 at 2:38 PM GMT, Benno Lossin wrote: >> On Mon Mar 2, 2026 at 2:02 PM CET, Gary Guo wrote: >>> +/// A helper trait to perform index projection. >>> +/// >>> +/// This is similar to `core::slice::SliceIndex`, but operate on raw pointers safely and fallibly. >>> +/// >>> +/// # Safety >>> +/// >>> +/// `get` must return a pointer in bounds of the provided pointer. >> >> This only makes sense when the provided pointer already points at an >> allocation. But since the functions of this trait aren't `unsafe`, it >> must be sound to pass `ptr::null` to them. > > The "in bounds" here is the conceptual bounds of the pointer. So, for a pointer > with size `x`, the address of the returned pointer lies between `ptr .. ptr + > x`. Okay, I haven't really seen that as a concept. Also, what is the size of an invalid pointer? >> I first thought that we might be able to just use `mem::size_of_val_raw` >> [1] to give an upper and lower bound on the address of the returned >> pointer, but that is unsafe and cannot be called with an arbitrary >> pointer. Interestingly, `ptr::metadata` [2] can be called safely & with >> any pointer; I would expect them to be very similar (except of course >> for extern types). >> >> [1]: https://doc.rust-lang.org/std/mem/fn.size_of_val_raw.html >> [2]: https://doc.rust-lang.org/std/ptr/fn.metadata.html > > I have a `KnownSize` trait for this in my I/O projection series that is > implemented for `T: Sized` and `[T]`, and it returns the size when given a raw > pointer. > >> >> A pretty expensive solution would be to add a sealed trait `Indexable` >> that we implement for all things that `T` is allowed to be; and then we >> provide a safe function in that trait to query the maximum offset the >> `get` function is allowed to make. >> >> Alternatively, we could use something like this: >> >> The implementation of `get` must: >> - return a pointer obtained by offsetting the input pointer. >> - ensure that when the input pointer points at a valid value of type >> `T`, the offset must not be greater than [`mem::size_of_val_raw`] >> of the input pointer. > > Given that I'm not introducing `KnownSize` trait in this patch, this is why I > haven't used this kind of wording. Perhaps I can just bring `KnownSize` in early > and use it first for documentation purpose only? That sounds great. >> Or something simpler that says "if the input pointer is valid, then >> `get` must return a valid output pointer"? > > Hmm, wouldn't this give impression that "you can do whatever you want if the > input pointer is not valid"? Yes that's true, but why is that a problem? >>> +#[diagnostic::on_unimplemented(message = "`{Self}` cannot be used to index `{T}`")] >>> +#[doc(hidden)] >>> +pub unsafe trait ProjectIndex<T: ?Sized>: Sized { >>> + type Output: ?Sized; >>> + >>> + /// Returns an index-projected pointer, if in bounds. >>> + fn get(self, slice: *mut T) -> Option<*mut Self::Output>; >> >> How about we name this `try_index` instead of the general `get`? > > I'm following the name on `SliceIndex`: > https://doc.rust-lang.org/stable/std/slice/trait.SliceIndex.html. Hmm, the methods in that trait are marked as unstable under `slice_index_methods`, which doesn't have a tracking issue, so are perma-unstable? I'll suggest the rename upstream as well. Cheers, Benno ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/2] rust: add projection infrastructure 2026-03-02 18:49 ` Benno Lossin @ 2026-03-02 20:14 ` Gary Guo 2026-03-02 22:01 ` Benno Lossin 0 siblings, 1 reply; 16+ messages in thread From: Gary Guo @ 2026-03-02 20:14 UTC (permalink / raw) To: Benno Lossin, Gary Guo, Miguel Ojeda, Boqun Feng, Björn Roy Baron, Andreas Hindborg, Alice Ryhl, Trevor Gross, Danilo Krummrich, Nathan Chancellor, Nicolas Schier Cc: rust-for-linux, Aditya Rajan, linux-kernel, linux-kbuild On Mon Mar 2, 2026 at 6:49 PM GMT, Benno Lossin wrote: > On Mon Mar 2, 2026 at 3:49 PM CET, Gary Guo wrote: >> On Mon Mar 2, 2026 at 2:38 PM GMT, Benno Lossin wrote: >>> On Mon Mar 2, 2026 at 2:02 PM CET, Gary Guo wrote: >>>> +/// A helper trait to perform index projection. >>>> +/// >>>> +/// This is similar to `core::slice::SliceIndex`, but operate on raw pointers safely and fallibly. >>>> +/// >>>> +/// # Safety >>>> +/// >>>> +/// `get` must return a pointer in bounds of the provided pointer. >>> >>> This only makes sense when the provided pointer already points at an >>> allocation. But since the functions of this trait aren't `unsafe`, it >>> must be sound to pass `ptr::null` to them. >> >> The "in bounds" here is the conceptual bounds of the pointer. So, for a pointer >> with size `x`, the address of the returned pointer lies between `ptr .. ptr + >> x`. > > Okay, I haven't really seen that as a concept. Also, what is the size of > an invalid pointer? It's `size_of::<T>()` for sized types, and `size_of::<T>() * slice.len()` for a raw slice pointer. The projection semantics is same regardless whether it's valid or not. The I/O projection work will rely on this, as many I/O impls will act on pointers that are not "valid" in Rust sense because they refer to a different address space. But they're still legit pointers with proper meaning. > >>> I first thought that we might be able to just use `mem::size_of_val_raw` >>> [1] to give an upper and lower bound on the address of the returned >>> pointer, but that is unsafe and cannot be called with an arbitrary >>> pointer. Interestingly, `ptr::metadata` [2] can be called safely & with >>> any pointer; I would expect them to be very similar (except of course >>> for extern types). >>> >>> [1]: https://doc.rust-lang.org/std/mem/fn.size_of_val_raw.html >>> [2]: https://doc.rust-lang.org/std/ptr/fn.metadata.html >> >> I have a `KnownSize` trait for this in my I/O projection series that is >> implemented for `T: Sized` and `[T]`, and it returns the size when given a raw >> pointer. >> >>> >>> A pretty expensive solution would be to add a sealed trait `Indexable` >>> that we implement for all things that `T` is allowed to be; and then we >>> provide a safe function in that trait to query the maximum offset the >>> `get` function is allowed to make. >>> >>> Alternatively, we could use something like this: >>> >>> The implementation of `get` must: >>> - return a pointer obtained by offsetting the input pointer. >>> - ensure that when the input pointer points at a valid value of type >>> `T`, the offset must not be greater than [`mem::size_of_val_raw`] >>> of the input pointer. >> >> Given that I'm not introducing `KnownSize` trait in this patch, this is why I >> haven't used this kind of wording. Perhaps I can just bring `KnownSize` in early >> and use it first for documentation purpose only? > > That sounds great. > >>> Or something simpler that says "if the input pointer is valid, then >>> `get` must return a valid output pointer"? >> >> Hmm, wouldn't this give impression that "you can do whatever you want if the >> input pointer is not valid"? > > Yes that's true, but why is that a problem? A impl that returns an arbitrary pointer when given a null pointer is not valid. I/O projection will use the ability to project on null pointers, too. An example is PCI config space code, which will project using null pointer as starting pointer. The "bounds" projected pointer must still be with in `0..KnownSize::size(ptr)`. > >>>> +#[diagnostic::on_unimplemented(message = "`{Self}` cannot be used to index `{T}`")] >>>> +#[doc(hidden)] >>>> +pub unsafe trait ProjectIndex<T: ?Sized>: Sized { >>>> + type Output: ?Sized; >>>> + >>>> + /// Returns an index-projected pointer, if in bounds. >>>> + fn get(self, slice: *mut T) -> Option<*mut Self::Output>; >>> >>> How about we name this `try_index` instead of the general `get`? >> >> I'm following the name on `SliceIndex`: >> https://doc.rust-lang.org/stable/std/slice/trait.SliceIndex.html. > > Hmm, the methods in that trait are marked as unstable under > `slice_index_methods`, which doesn't have a tracking issue, so are > perma-unstable? I'll suggest the rename upstream as well. I mean, they're named after the `slice::get` method [1] and `Index::index` method [2], so I don't really see the naming issue here. [1]: https://doc.rust-lang.org/stable/std/primitive.slice.html#method.get [2]: https://doc.rust-lang.org/stable/std/primitive.slice.html#method.index-24 Best, Gary > > Cheers, > Benno ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/2] rust: add projection infrastructure 2026-03-02 20:14 ` Gary Guo @ 2026-03-02 22:01 ` Benno Lossin 2026-03-02 22:19 ` Gary Guo 0 siblings, 1 reply; 16+ messages in thread From: Benno Lossin @ 2026-03-02 22:01 UTC (permalink / raw) To: Gary Guo, Miguel Ojeda, Boqun Feng, Björn Roy Baron, Andreas Hindborg, Alice Ryhl, Trevor Gross, Danilo Krummrich, Nathan Chancellor, Nicolas Schier Cc: rust-for-linux, Aditya Rajan, linux-kernel, linux-kbuild On Mon Mar 2, 2026 at 9:14 PM CET, Gary Guo wrote: > On Mon Mar 2, 2026 at 6:49 PM GMT, Benno Lossin wrote: >> On Mon Mar 2, 2026 at 3:49 PM CET, Gary Guo wrote: >>> On Mon Mar 2, 2026 at 2:38 PM GMT, Benno Lossin wrote: >>>> On Mon Mar 2, 2026 at 2:02 PM CET, Gary Guo wrote: >>>>> +/// A helper trait to perform index projection. >>>>> +/// >>>>> +/// This is similar to `core::slice::SliceIndex`, but operate on raw pointers safely and fallibly. >>>>> +/// >>>>> +/// # Safety >>>>> +/// >>>>> +/// `get` must return a pointer in bounds of the provided pointer. >>>> >>>> This only makes sense when the provided pointer already points at an >>>> allocation. But since the functions of this trait aren't `unsafe`, it >>>> must be sound to pass `ptr::null` to them. >>> >>> The "in bounds" here is the conceptual bounds of the pointer. So, for a pointer >>> with size `x`, the address of the returned pointer lies between `ptr .. ptr + >>> x`. >> >> Okay, I haven't really seen that as a concept. Also, what is the size of >> an invalid pointer? > > It's `size_of::<T>()` for sized types, and `size_of::<T>() * slice.len()` for a > raw slice pointer. And for `dyn Trait`? Do you have a link to somewhere? > The projection semantics is same regardless whether it's valid or not. The I/O > projection work will rely on this, as many I/O impls will act on pointers that > are not "valid" in Rust sense because they refer to a different address space. > But they're still legit pointers with proper meaning. I like the solution with `KnownSize`. The previous safety requirement was nebulous IMO. >>>> I first thought that we might be able to just use `mem::size_of_val_raw` >>>> [1] to give an upper and lower bound on the address of the returned >>>> pointer, but that is unsafe and cannot be called with an arbitrary >>>> pointer. Interestingly, `ptr::metadata` [2] can be called safely & with >>>> any pointer; I would expect them to be very similar (except of course >>>> for extern types). >>>> >>>> [1]: https://doc.rust-lang.org/std/mem/fn.size_of_val_raw.html >>>> [2]: https://doc.rust-lang.org/std/ptr/fn.metadata.html >>> >>> I have a `KnownSize` trait for this in my I/O projection series that is >>> implemented for `T: Sized` and `[T]`, and it returns the size when given a raw >>> pointer. >>> >>>> >>>> A pretty expensive solution would be to add a sealed trait `Indexable` >>>> that we implement for all things that `T` is allowed to be; and then we >>>> provide a safe function in that trait to query the maximum offset the >>>> `get` function is allowed to make. >>>> >>>> Alternatively, we could use something like this: >>>> >>>> The implementation of `get` must: >>>> - return a pointer obtained by offsetting the input pointer. >>>> - ensure that when the input pointer points at a valid value of type >>>> `T`, the offset must not be greater than [`mem::size_of_val_raw`] >>>> of the input pointer. >>> >>> Given that I'm not introducing `KnownSize` trait in this patch, this is why I >>> haven't used this kind of wording. Perhaps I can just bring `KnownSize` in early >>> and use it first for documentation purpose only? >> >> That sounds great. >> >>>> Or something simpler that says "if the input pointer is valid, then >>>> `get` must return a valid output pointer"? >>> >>> Hmm, wouldn't this give impression that "you can do whatever you want if the >>> input pointer is not valid"? >> >> Yes that's true, but why is that a problem? > > A impl that returns an arbitrary pointer when given a null pointer is not valid. > > I/O projection will use the ability to project on null pointers, too. An example > is PCI config space code, which will project using null pointer as starting > pointer. > > The "bounds" projected pointer must still be with in `0..KnownSize::size(ptr)`. I would not have assumed this to be valid with the previous comment. Cheers, Benno ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/2] rust: add projection infrastructure 2026-03-02 22:01 ` Benno Lossin @ 2026-03-02 22:19 ` Gary Guo 2026-03-03 9:14 ` Benno Lossin 0 siblings, 1 reply; 16+ messages in thread From: Gary Guo @ 2026-03-02 22:19 UTC (permalink / raw) To: Benno Lossin, Gary Guo, Miguel Ojeda, Boqun Feng, Björn Roy Baron, Andreas Hindborg, Alice Ryhl, Trevor Gross, Danilo Krummrich, Nathan Chancellor, Nicolas Schier Cc: rust-for-linux, Aditya Rajan, linux-kernel, linux-kbuild On Mon Mar 2, 2026 at 10:01 PM GMT, Benno Lossin wrote: > On Mon Mar 2, 2026 at 9:14 PM CET, Gary Guo wrote: >> On Mon Mar 2, 2026 at 6:49 PM GMT, Benno Lossin wrote: >>> On Mon Mar 2, 2026 at 3:49 PM CET, Gary Guo wrote: >>>> On Mon Mar 2, 2026 at 2:38 PM GMT, Benno Lossin wrote: >>>>> On Mon Mar 2, 2026 at 2:02 PM CET, Gary Guo wrote: >>>>>> +/// A helper trait to perform index projection. >>>>>> +/// >>>>>> +/// This is similar to `core::slice::SliceIndex`, but operate on raw pointers safely and fallibly. >>>>>> +/// >>>>>> +/// # Safety >>>>>> +/// >>>>>> +/// `get` must return a pointer in bounds of the provided pointer. >>>>> >>>>> This only makes sense when the provided pointer already points at an >>>>> allocation. But since the functions of this trait aren't `unsafe`, it >>>>> must be sound to pass `ptr::null` to them. >>>> >>>> The "in bounds" here is the conceptual bounds of the pointer. So, for a pointer >>>> with size `x`, the address of the returned pointer lies between `ptr .. ptr + >>>> x`. >>> >>> Okay, I haven't really seen that as a concept. Also, what is the size of >>> an invalid pointer? >> >> It's `size_of::<T>()` for sized types, and `size_of::<T>() * slice.len()` for a >> raw slice pointer. > > And for `dyn Trait`? > > Do you have a link to somewhere? For `dyn Trait` it would be the size in the vtable, which is always available as vtable metadata on a raw pointer is required to be valid anyway (this is something that lang team has already decided so that trait upcasting could work for raw pointers). I am basically just having `size_of_val_raw` in mind when writing this. So the current `KnownSize` comment in v4 is something that I am happy about. > >> The projection semantics is same regardless whether it's valid or not. The I/O >> projection work will rely on this, as many I/O impls will act on pointers that >> are not "valid" in Rust sense because they refer to a different address space. >> But they're still legit pointers with proper meaning. > > I like the solution with `KnownSize`. The previous safety requirement > was nebulous IMO. Best, Gary ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/2] rust: add projection infrastructure 2026-03-02 22:19 ` Gary Guo @ 2026-03-03 9:14 ` Benno Lossin 2026-03-03 10:17 ` Gary Guo 0 siblings, 1 reply; 16+ messages in thread From: Benno Lossin @ 2026-03-03 9:14 UTC (permalink / raw) To: Gary Guo, Miguel Ojeda, Boqun Feng, Björn Roy Baron, Andreas Hindborg, Alice Ryhl, Trevor Gross, Danilo Krummrich, Nathan Chancellor, Nicolas Schier Cc: rust-for-linux, Aditya Rajan, linux-kernel, linux-kbuild On Mon Mar 2, 2026 at 11:19 PM CET, Gary Guo wrote: > On Mon Mar 2, 2026 at 10:01 PM GMT, Benno Lossin wrote: >> On Mon Mar 2, 2026 at 9:14 PM CET, Gary Guo wrote: >>> On Mon Mar 2, 2026 at 6:49 PM GMT, Benno Lossin wrote: >>>> On Mon Mar 2, 2026 at 3:49 PM CET, Gary Guo wrote: >>>>> On Mon Mar 2, 2026 at 2:38 PM GMT, Benno Lossin wrote: >>>>>> On Mon Mar 2, 2026 at 2:02 PM CET, Gary Guo wrote: >>>>>>> +/// A helper trait to perform index projection. >>>>>>> +/// >>>>>>> +/// This is similar to `core::slice::SliceIndex`, but operate on raw pointers safely and fallibly. >>>>>>> +/// >>>>>>> +/// # Safety >>>>>>> +/// >>>>>>> +/// `get` must return a pointer in bounds of the provided pointer. >>>>>> >>>>>> This only makes sense when the provided pointer already points at an >>>>>> allocation. But since the functions of this trait aren't `unsafe`, it >>>>>> must be sound to pass `ptr::null` to them. >>>>> >>>>> The "in bounds" here is the conceptual bounds of the pointer. So, for a pointer >>>>> with size `x`, the address of the returned pointer lies between `ptr .. ptr + >>>>> x`. >>>> >>>> Okay, I haven't really seen that as a concept. Also, what is the size of >>>> an invalid pointer? >>> >>> It's `size_of::<T>()` for sized types, and `size_of::<T>() * slice.len()` for a >>> raw slice pointer. >> >> And for `dyn Trait`? >> >> Do you have a link to somewhere? > > For `dyn Trait` it would be the size in the vtable, which is always available as > vtable metadata on a raw pointer is required to be valid anyway (this is > something that lang team has already decided so that trait upcasting could work > for raw pointers). I really would like to see some docs of that, I didn't find anything in the reference, official docs, or nomicon. The reference does say [1] that: `dyn Trait` metadata must be a pointer to a compiler-generated vtable for Trait. (For raw pointers, this requirement remains a subject of some debate.) Do you know where it was decided? I did find this [2] UCG issue that covers it, but that doesn't seem like the decision, just the discussion. [1]: https://doc.rust-lang.org/reference/behavior-considered-undefined.html#r-undefined.validity.wide [2]: https://github.com/rust-lang/unsafe-code-guidelines/issues/516 > I am basically just having `size_of_val_raw` in mind when writing this. So the > current `KnownSize` comment in v4 is something that I am happy about. Well size_of_val_raw is `unsafe` and only valid to call in certain conditions. It asks in the case of slices that the length is an initialized integer and that the entire value must fit into `isize`. This to me just further indicates that `*mut T` has safety requirements to obtaining the size of an arbitrary pointer. In the special cases of `T: Sized` and `T == [U]`, we have safe ways of getting their size. Cheers, Benno ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/2] rust: add projection infrastructure 2026-03-03 9:14 ` Benno Lossin @ 2026-03-03 10:17 ` Gary Guo 2026-03-03 11:39 ` Alice Ryhl 0 siblings, 1 reply; 16+ messages in thread From: Gary Guo @ 2026-03-03 10:17 UTC (permalink / raw) To: Benno Lossin, Gary Guo, Miguel Ojeda, Boqun Feng, Björn Roy Baron, Andreas Hindborg, Alice Ryhl, Trevor Gross, Danilo Krummrich, Nathan Chancellor, Nicolas Schier Cc: rust-for-linux, Aditya Rajan, linux-kernel, linux-kbuild On Tue Mar 3, 2026 at 9:14 AM GMT, Benno Lossin wrote: > On Mon Mar 2, 2026 at 11:19 PM CET, Gary Guo wrote: >> On Mon Mar 2, 2026 at 10:01 PM GMT, Benno Lossin wrote: >>> On Mon Mar 2, 2026 at 9:14 PM CET, Gary Guo wrote: >>>> On Mon Mar 2, 2026 at 6:49 PM GMT, Benno Lossin wrote: >>>>> On Mon Mar 2, 2026 at 3:49 PM CET, Gary Guo wrote: >>>>>> On Mon Mar 2, 2026 at 2:38 PM GMT, Benno Lossin wrote: >>>>>>> On Mon Mar 2, 2026 at 2:02 PM CET, Gary Guo wrote: >>>>>>>> +/// A helper trait to perform index projection. >>>>>>>> +/// >>>>>>>> +/// This is similar to `core::slice::SliceIndex`, but operate on raw pointers safely and fallibly. >>>>>>>> +/// >>>>>>>> +/// # Safety >>>>>>>> +/// >>>>>>>> +/// `get` must return a pointer in bounds of the provided pointer. >>>>>>> >>>>>>> This only makes sense when the provided pointer already points at an >>>>>>> allocation. But since the functions of this trait aren't `unsafe`, it >>>>>>> must be sound to pass `ptr::null` to them. >>>>>> >>>>>> The "in bounds" here is the conceptual bounds of the pointer. So, for a pointer >>>>>> with size `x`, the address of the returned pointer lies between `ptr .. ptr + >>>>>> x`. >>>>> >>>>> Okay, I haven't really seen that as a concept. Also, what is the size of >>>>> an invalid pointer? >>>> >>>> It's `size_of::<T>()` for sized types, and `size_of::<T>() * slice.len()` for a >>>> raw slice pointer. >>> >>> And for `dyn Trait`? >>> >>> Do you have a link to somewhere? >> >> For `dyn Trait` it would be the size in the vtable, which is always available as >> vtable metadata on a raw pointer is required to be valid anyway (this is >> something that lang team has already decided so that trait upcasting could work >> for raw pointers). > > I really would like to see some docs of that, I didn't find anything in > the reference, official docs, or nomicon. The reference does say [1] > that: > > `dyn Trait` metadata must be a pointer to a compiler-generated > vtable for Trait. (For raw pointers, this requirement remains a > subject of some debate.) > > Do you know where it was decided? I did find this [2] UCG issue that > covers it, but that doesn't seem like the decision, just the discussion. The FCP is here: https://github.com/rust-lang/rust/issues/101336 The gist: for a fat raw pointer with `dyn Trait` metadata, the safety invariant requires a fully valid vtable. Validity invariant is not yet decided. > > [1]: https://doc.rust-lang.org/reference/behavior-considered-undefined.html#r-undefined.validity.wide > [2]: https://github.com/rust-lang/unsafe-code-guidelines/issues/516 > >> I am basically just having `size_of_val_raw` in mind when writing this. So the >> current `KnownSize` comment in v4 is something that I am happy about. > > Well size_of_val_raw is `unsafe` and only valid to call in certain > conditions. It asks in the case of slices that the length is an > initialized integer and that the entire value must fit into `isize`. > This to me just further indicates that `*mut T` has safety > requirements to obtaining the size of an arbitrary pointer. > > In the special cases of `T: Sized` and `T == [U]`, we have safe ways of > getting their size. Hmm, the `isize` fitting requirement is problematic indeed. It's broken code if pointer projection is used with an allocation that exceeds the limit, but I want the API to be safe, so it'll be good if the API is defined to just be wrapping and safe (it may return values that doesn't make sense, but that'll be on the user). Anyhow this is moot as we're going the `KnownSize` route. Best, Gary > > Cheers, > Benno ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/2] rust: add projection infrastructure 2026-03-03 10:17 ` Gary Guo @ 2026-03-03 11:39 ` Alice Ryhl 2026-03-03 12:21 ` Gary Guo 0 siblings, 1 reply; 16+ messages in thread From: Alice Ryhl @ 2026-03-03 11:39 UTC (permalink / raw) To: Gary Guo Cc: Benno Lossin, Miguel Ojeda, Boqun Feng, Björn Roy Baron, Andreas Hindborg, Trevor Gross, Danilo Krummrich, Nathan Chancellor, Nicolas Schier, rust-for-linux, Aditya Rajan, linux-kernel, linux-kbuild On Tue, Mar 03, 2026 at 10:17:01AM +0000, Gary Guo wrote: > On Tue Mar 3, 2026 at 9:14 AM GMT, Benno Lossin wrote: > > On Mon Mar 2, 2026 at 11:19 PM CET, Gary Guo wrote: > >> I am basically just having `size_of_val_raw` in mind when writing this. So the > >> current `KnownSize` comment in v4 is something that I am happy about. > > > > Well size_of_val_raw is `unsafe` and only valid to call in certain > > conditions. It asks in the case of slices that the length is an > > initialized integer and that the entire value must fit into `isize`. > > This to me just further indicates that `*mut T` has safety > > requirements to obtaining the size of an arbitrary pointer. > > > > In the special cases of `T: Sized` and `T == [U]`, we have safe ways of > > getting their size. > > Hmm, the `isize` fitting requirement is problematic indeed. It's broken code if > pointer projection is used with an allocation that exceeds the limit, but I want > the API to be safe, so it'll be good if the API is defined to just be wrapping > and safe (it may return values that doesn't make sense, but that'll be on the > user). > > Anyhow this is moot as we're going the `KnownSize` route. It sounds like that's no different from dyn trait vtable case. Fat pointer must have valid metadata, even if raw pointer. Alice ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/2] rust: add projection infrastructure 2026-03-03 11:39 ` Alice Ryhl @ 2026-03-03 12:21 ` Gary Guo 0 siblings, 0 replies; 16+ messages in thread From: Gary Guo @ 2026-03-03 12:21 UTC (permalink / raw) To: Alice Ryhl, Gary Guo Cc: Benno Lossin, Miguel Ojeda, Boqun Feng, Björn Roy Baron, Andreas Hindborg, Trevor Gross, Danilo Krummrich, Nathan Chancellor, Nicolas Schier, rust-for-linux, Aditya Rajan, linux-kernel, linux-kbuild On Tue Mar 3, 2026 at 11:39 AM GMT, Alice Ryhl wrote: > On Tue, Mar 03, 2026 at 10:17:01AM +0000, Gary Guo wrote: >> On Tue Mar 3, 2026 at 9:14 AM GMT, Benno Lossin wrote: >> > On Mon Mar 2, 2026 at 11:19 PM CET, Gary Guo wrote: >> >> I am basically just having `size_of_val_raw` in mind when writing this. So the >> >> current `KnownSize` comment in v4 is something that I am happy about. >> > >> > Well size_of_val_raw is `unsafe` and only valid to call in certain >> > conditions. It asks in the case of slices that the length is an >> > initialized integer and that the entire value must fit into `isize`. >> > This to me just further indicates that `*mut T` has safety >> > requirements to obtaining the size of an arbitrary pointer. >> > >> > In the special cases of `T: Sized` and `T == [U]`, we have safe ways of >> > getting their size. >> >> Hmm, the `isize` fitting requirement is problematic indeed. It's broken code if >> pointer projection is used with an allocation that exceeds the limit, but I want >> the API to be safe, so it'll be good if the API is defined to just be wrapping >> and safe (it may return values that doesn't make sense, but that'll be on the >> user). >> >> Anyhow this is moot as we're going the `KnownSize` route. > > It sounds like that's no different from dyn trait vtable case. Fat > pointer must have valid metadata, even if raw pointer. If you have a `*const dyn Trait`, then yes, it's all valid. However, the `isize` fitting requirement means that if you have `*const TypeWithUnsizedTail<dyn Trait>`, the fixed-sized prefix, when added together with the `dyn Trait` tail, may exceed isize::MAX and violate the safety requirement of `size_of_val_raw`. Best, Gary > > Alice ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v3 2/2] rust: dma: use pointer projection infra for `dma_{read,write}` macro 2026-03-02 13:02 [PATCH v3 0/2] rust: add pointer projection infrastructure and convert DMA Gary Guo 2026-03-02 13:02 ` [PATCH v3 1/2] rust: add projection infrastructure Gary Guo @ 2026-03-02 13:02 ` Gary Guo 2026-03-02 14:42 ` [PATCH v3 2/2] rust: dma: use pointer projection infra for `dma_{read, write}` macro Benno Lossin 1 sibling, 1 reply; 16+ messages in thread From: Gary Guo @ 2026-03-02 13:02 UTC (permalink / raw) To: Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross, Danilo Krummrich, Alexandre Courbot, David Airlie, Simona Vetter, Abdiel Janulgue, Daniel Almeida, Robin Murphy Cc: rust-for-linux, nouveau, dri-devel, linux-kernel, driver-core From: Gary Guo <gary@garyguo.net> Current `dma_read!`, `dma_write!` macros also use a custom `addr_of!()`-based implementation for projecting pointers, which has soundness issue as it relies on absence of `Deref` implementation on types. It also has a soundness issue where it does not protect against unaligned fields (when `#[repr(packed)]` is used) so it can generate misaligned accesses. This commit migrates them to use the general pointer projection infrastructure, which handles these cases correctly. As part of migration, the macro is updated to have an improved surface syntax. The current macro have dma_read!(a.b.c[d].e.f) to mean `a.b.c` is a DMA coherent allocation and it should project into it with `[d].e.f` and do a read, which is confusing as it makes the indexing operator integral to the macro (so it will break if you have an array of `CoherentAllocation`, for example). This also is problematic as we would like to generalize `CoherentAllocation` from just slices to arbitrary types. Make the macro expects `dma_read!(path.to.dma, .path.inside.dma)` as the canonical syntax. The index operator is no longer special and is just one type of projection (in additional to field projection). Similarly, make `dma_write!(path.to.dma, .path.inside.dma, value)` become the canonical syntax for writing. Another issue of the current macro is that it is always fallible. This makes sense with existing design of `CoherentAllocation`, but once we support fixed size arrays with `CoherentAllocation`, it is desirable to have the ability to perform infallible indexing as well, e.g. doing a `[0]` index of `[Foo; 2]` is okay and can be checked at build-time, so forcing falliblity is non-ideal. To capture this, the macro is changed to use `[idx]` as infallible projection and `[idx]?` as fallible index projection (those syntax are part of the general projection infra). A benefit of this is that while individual indexing operation may fail, the overall read/write operation is not fallible. Fixes: ad2907b4e308 ("rust: add dma coherent allocator abstraction") Signed-off-by: Gary Guo <gary@garyguo.net> --- drivers/gpu/nova-core/gsp.rs | 14 ++-- drivers/gpu/nova-core/gsp/boot.rs | 2 +- drivers/gpu/nova-core/gsp/cmdq.rs | 10 ++- rust/kernel/dma.rs | 114 +++++++++++++----------------- samples/rust/rust_dma.rs | 30 ++++---- 5 files changed, 81 insertions(+), 89 deletions(-) diff --git a/drivers/gpu/nova-core/gsp.rs b/drivers/gpu/nova-core/gsp.rs index 174feaca0a6b..25cd48514c77 100644 --- a/drivers/gpu/nova-core/gsp.rs +++ b/drivers/gpu/nova-core/gsp.rs @@ -143,14 +143,14 @@ pub(crate) fn new(pdev: &pci::Device<device::Bound>) -> impl PinInit<Self, Error // _kgspInitLibosLoggingStructures (allocates memory for buffers) // kgspSetupLibosInitArgs_IMPL (creates pLibosInitArgs[] array) dma_write!( - libos[0] = LibosMemoryRegionInitArgument::new("LOGINIT", &loginit.0) - )?; + libos, [0]?, LibosMemoryRegionInitArgument::new("LOGINIT", &loginit.0) + ); dma_write!( - libos[1] = LibosMemoryRegionInitArgument::new("LOGINTR", &logintr.0) - )?; - dma_write!(libos[2] = LibosMemoryRegionInitArgument::new("LOGRM", &logrm.0))?; - dma_write!(rmargs[0].inner = fw::GspArgumentsCached::new(cmdq))?; - dma_write!(libos[3] = LibosMemoryRegionInitArgument::new("RMARGS", rmargs))?; + libos, [1]?, LibosMemoryRegionInitArgument::new("LOGINTR", &logintr.0) + ); + dma_write!(libos, [2]?, LibosMemoryRegionInitArgument::new("LOGRM", &logrm.0)); + dma_write!(rmargs, [0]?.inner, fw::GspArgumentsCached::new(cmdq)); + dma_write!(libos, [3]?, LibosMemoryRegionInitArgument::new("RMARGS", rmargs)); }, })) }) diff --git a/drivers/gpu/nova-core/gsp/boot.rs b/drivers/gpu/nova-core/gsp/boot.rs index c56029f444cb..7f46fa5e9b50 100644 --- a/drivers/gpu/nova-core/gsp/boot.rs +++ b/drivers/gpu/nova-core/gsp/boot.rs @@ -157,7 +157,7 @@ pub(crate) fn boot( let wpr_meta = CoherentAllocation::<GspFwWprMeta>::alloc_coherent(dev, 1, GFP_KERNEL | __GFP_ZERO)?; - dma_write!(wpr_meta[0] = GspFwWprMeta::new(&gsp_fw, &fb_layout))?; + dma_write!(wpr_meta, [0]?, GspFwWprMeta::new(&gsp_fw, &fb_layout)); self.cmdq .send_command(bar, commands::SetSystemInfo::new(pdev))?; diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs index 87dbbd6d1be9..0056bfbf0a44 100644 --- a/drivers/gpu/nova-core/gsp/cmdq.rs +++ b/drivers/gpu/nova-core/gsp/cmdq.rs @@ -202,9 +202,13 @@ fn new(dev: &device::Device<device::Bound>) -> Result<Self> { let gsp_mem = CoherentAllocation::<GspMem>::alloc_coherent(dev, 1, GFP_KERNEL | __GFP_ZERO)?; - dma_write!(gsp_mem[0].ptes = PteArray::new(gsp_mem.dma_handle())?)?; - dma_write!(gsp_mem[0].cpuq.tx = MsgqTxHeader::new(MSGQ_SIZE, RX_HDR_OFF, MSGQ_NUM_PAGES))?; - dma_write!(gsp_mem[0].cpuq.rx = MsgqRxHeader::new())?; + dma_write!(gsp_mem, [0]?.ptes, PteArray::new(gsp_mem.dma_handle())?); + dma_write!( + gsp_mem, + [0]?.cpuq.tx, + MsgqTxHeader::new(MSGQ_SIZE, RX_HDR_OFF, MSGQ_NUM_PAGES) + ); + dma_write!(gsp_mem, [0]?.cpuq.rx, MsgqRxHeader::new()); Ok(Self(gsp_mem)) } diff --git a/rust/kernel/dma.rs b/rust/kernel/dma.rs index 909d56fd5118..07fa4912ea78 100644 --- a/rust/kernel/dma.rs +++ b/rust/kernel/dma.rs @@ -461,6 +461,19 @@ pub fn size(&self) -> usize { self.count * core::mem::size_of::<T>() } + /// Returns the raw pointer to the allocated region in the CPU's virtual address space. + #[inline] + pub fn as_ptr(&self) -> *const [T] { + core::ptr::slice_from_raw_parts(self.cpu_addr.as_ptr(), self.count) + } + + /// Returns the raw pointer to the allocated region in the CPU's virtual address space as + /// a mutable pointer. + #[inline] + pub fn as_mut_ptr(&self) -> *mut [T] { + core::ptr::slice_from_raw_parts_mut(self.cpu_addr.as_ptr(), self.count) + } + /// Returns the base address to the allocated region in the CPU's virtual address space. pub fn start_ptr(&self) -> *const T { self.cpu_addr.as_ptr() @@ -581,23 +594,6 @@ pub unsafe fn write(&mut self, src: &[T], offset: usize) -> Result { Ok(()) } - /// Returns a pointer to an element from the region with bounds checking. `offset` is in - /// units of `T`, not the number of bytes. - /// - /// Public but hidden since it should only be used from [`dma_read`] and [`dma_write`] macros. - #[doc(hidden)] - pub fn item_from_index(&self, offset: usize) -> Result<*mut T> { - if offset >= self.count { - return Err(EINVAL); - } - // SAFETY: - // - The pointer is valid due to type invariant on `CoherentAllocation` - // and we've just checked that the range and index is within bounds. - // - `offset` can't overflow since it is smaller than `self.count` and we've checked - // that `self.count` won't overflow early in the constructor. - Ok(unsafe { self.cpu_addr.as_ptr().add(offset) }) - } - /// Reads the value of `field` and ensures that its type is [`FromBytes`]. /// /// # Safety @@ -670,6 +666,9 @@ unsafe impl<T: AsBytes + FromBytes + Send> Send for CoherentAllocation<T> {} /// Reads a field of an item from an allocated region of structs. /// +/// The syntax is of form `kernel::dma_read!(dma, proj)` where `dma` is an expression to an +/// [`CoherentAllocation`] and `proj` is a [projection specification](kernel::project_pointer!). +/// /// # Examples /// /// ``` @@ -684,36 +683,29 @@ unsafe impl<T: AsBytes + FromBytes + Send> Send for CoherentAllocation<T> {} /// unsafe impl kernel::transmute::AsBytes for MyStruct{}; /// /// # fn test(alloc: &kernel::dma::CoherentAllocation<MyStruct>) -> Result { -/// let whole = kernel::dma_read!(alloc[2]); -/// let field = kernel::dma_read!(alloc[1].field); +/// let whole = kernel::dma_read!(alloc, [2]?); +/// let field = kernel::dma_read!(alloc, [1]?.field); /// # Ok::<(), Error>(()) } /// ``` #[macro_export] macro_rules! dma_read { - ($dma:expr, $idx: expr, $($field:tt)*) => {{ - (|| -> ::core::result::Result<_, $crate::error::Error> { - let item = $crate::dma::CoherentAllocation::item_from_index(&$dma, $idx)?; - // SAFETY: `item_from_index` ensures that `item` is always a valid pointer and can be - // dereferenced. The compiler also further validates the expression on whether `field` - // is a member of `item` when expanded by the macro. - unsafe { - let ptr_field = ::core::ptr::addr_of!((*item) $($field)*); - ::core::result::Result::Ok( - $crate::dma::CoherentAllocation::field_read(&$dma, ptr_field) - ) - } - })() + ($dma:expr, $($proj:tt)*) => {{ + let dma = &$dma; + let ptr = $crate::project_pointer!( + $crate::dma::CoherentAllocation::as_ptr(dma), $($proj)* + ); + // SAFETY: pointer created by projection is within DMA region. + unsafe { $crate::dma::CoherentAllocation::field_read(dma, ptr) } }}; - ($dma:ident [ $idx:expr ] $($field:tt)* ) => { - $crate::dma_read!($dma, $idx, $($field)*) - }; - ($($dma:ident).* [ $idx:expr ] $($field:tt)* ) => { - $crate::dma_read!($($dma).*, $idx, $($field)*) - }; } /// Writes to a field of an item from an allocated region of structs. /// +/// The syntax is of form `kernel::dma_write!(dma, proj, val)` where `dma` is an expression to an +/// [`CoherentAllocation`] and `proj` is a [projection specification](kernel::project_pointer!), +/// and `val` is the value to be written to the projected location. +/// +/// /// # Examples /// /// ``` @@ -728,37 +720,31 @@ macro_rules! dma_read { /// unsafe impl kernel::transmute::AsBytes for MyStruct{}; /// /// # fn test(alloc: &kernel::dma::CoherentAllocation<MyStruct>) -> Result { -/// kernel::dma_write!(alloc[2].member = 0xf); -/// kernel::dma_write!(alloc[1] = MyStruct { member: 0xf }); +/// kernel::dma_write!(alloc, [2]?.member, 0xf); +/// kernel::dma_write!(alloc, [1]?, MyStruct { member: 0xf }); /// # Ok::<(), Error>(()) } /// ``` #[macro_export] macro_rules! dma_write { - ($dma:ident [ $idx:expr ] $($field:tt)*) => {{ - $crate::dma_write!($dma, $idx, $($field)*) - }}; - ($($dma:ident).* [ $idx:expr ] $($field:tt)* ) => {{ - $crate::dma_write!($($dma).*, $idx, $($field)*) + (@parse [$dma:expr] [$($proj:tt)*] [, $val:expr]) => {{ + let dma = &$dma; + let ptr = $crate::project_pointer!( + mut $crate::dma::CoherentAllocation::as_mut_ptr(dma), $($proj)* + ); + let val = $val; + // SAFETY: pointer created by projection is within DMA region. + unsafe { $crate::dma::CoherentAllocation::field_write(dma, ptr, val) } }}; - ($dma:expr, $idx: expr, = $val:expr) => { - (|| -> ::core::result::Result<_, $crate::error::Error> { - let item = $crate::dma::CoherentAllocation::item_from_index(&$dma, $idx)?; - // SAFETY: `item_from_index` ensures that `item` is always a valid item. - unsafe { $crate::dma::CoherentAllocation::field_write(&$dma, item, $val) } - ::core::result::Result::Ok(()) - })() + (@parse [$dma:expr] [$($proj:tt)*] [.$field:tt $($rest:tt)*]) => { + $crate::dma_write!(@parse [$dma] [$($proj)* .$field] [$($rest)*]) + }; + (@parse [$dma:expr] [$($proj:tt)*] [[$index:expr]? $($rest:tt)*]) => { + $crate::dma_write!(@parse [$dma] [$($proj)* [$index]?] [$($rest)*]) + }; + (@parse [$dma:expr] [$($proj:tt)*] [[$index:expr] $($rest:tt)*]) => { + $crate::dma_write!(@parse [$dma] [$($proj)* [$index]] [$($rest)*]) }; - ($dma:expr, $idx: expr, $(.$field:ident)* = $val:expr) => { - (|| -> ::core::result::Result<_, $crate::error::Error> { - let item = $crate::dma::CoherentAllocation::item_from_index(&$dma, $idx)?; - // SAFETY: `item_from_index` ensures that `item` is always a valid pointer and can be - // dereferenced. The compiler also further validates the expression on whether `field` - // is a member of `item` when expanded by the macro. - unsafe { - let ptr_field = ::core::ptr::addr_of_mut!((*item) $(.$field)*); - $crate::dma::CoherentAllocation::field_write(&$dma, ptr_field, $val) - } - ::core::result::Result::Ok(()) - })() + ($dma:expr, $($rest:tt)*) => { + $crate::dma_write!(@parse [$dma] [] [$($rest)*]) }; } diff --git a/samples/rust/rust_dma.rs b/samples/rust/rust_dma.rs index 9c45851c876e..ce39b5545097 100644 --- a/samples/rust/rust_dma.rs +++ b/samples/rust/rust_dma.rs @@ -68,7 +68,7 @@ fn probe(pdev: &pci::Device<Core>, _info: &Self::IdInfo) -> impl PinInit<Self, E CoherentAllocation::alloc_coherent(pdev.as_ref(), TEST_VALUES.len(), GFP_KERNEL)?; for (i, value) in TEST_VALUES.into_iter().enumerate() { - kernel::dma_write!(ca[i] = MyStruct::new(value.0, value.1))?; + kernel::dma_write!(ca, [i]?, MyStruct::new(value.0, value.1)); } let size = 4 * page::PAGE_SIZE; @@ -85,24 +85,26 @@ fn probe(pdev: &pci::Device<Core>, _info: &Self::IdInfo) -> impl PinInit<Self, E } } +impl DmaSampleDriver { + fn check_dma(&self) -> Result { + for (i, value) in TEST_VALUES.into_iter().enumerate() { + let val0 = kernel::dma_read!(self.ca, [i]?.h); + let val1 = kernel::dma_read!(self.ca, [i]?.b); + + assert_eq!(val0, value.0); + assert_eq!(val1, value.1); + } + + Ok(()) + } +} + #[pinned_drop] impl PinnedDrop for DmaSampleDriver { fn drop(self: Pin<&mut Self>) { dev_info!(self.pdev, "Unload DMA test driver.\n"); - for (i, value) in TEST_VALUES.into_iter().enumerate() { - let val0 = kernel::dma_read!(self.ca[i].h); - let val1 = kernel::dma_read!(self.ca[i].b); - assert!(val0.is_ok()); - assert!(val1.is_ok()); - - if let Ok(val0) = val0 { - assert_eq!(val0, value.0); - } - if let Ok(val1) = val1 { - assert_eq!(val1, value.1); - } - } + assert!(self.check_dma().is_ok()); for (i, entry) in self.sgt.iter().enumerate() { dev_info!( -- 2.51.2 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v3 2/2] rust: dma: use pointer projection infra for `dma_{read, write}` macro 2026-03-02 13:02 ` [PATCH v3 2/2] rust: dma: use pointer projection infra for `dma_{read,write}` macro Gary Guo @ 2026-03-02 14:42 ` Benno Lossin 0 siblings, 0 replies; 16+ messages in thread From: Benno Lossin @ 2026-03-02 14:42 UTC (permalink / raw) To: Gary Guo, Miguel Ojeda, Boqun Feng, Björn Roy Baron, Andreas Hindborg, Alice Ryhl, Trevor Gross, Danilo Krummrich, Alexandre Courbot, David Airlie, Simona Vetter, Abdiel Janulgue, Daniel Almeida, Robin Murphy Cc: rust-for-linux, nouveau, dri-devel, linux-kernel, driver-core, dri-devel On Mon Mar 2, 2026 at 2:02 PM CET, Gary Guo wrote: > From: Gary Guo <gary@garyguo.net> > > Current `dma_read!`, `dma_write!` macros also use a custom > `addr_of!()`-based implementation for projecting pointers, which has > soundness issue as it relies on absence of `Deref` implementation on types. > It also has a soundness issue where it does not protect against unaligned > fields (when `#[repr(packed)]` is used) so it can generate misaligned > accesses. > > This commit migrates them to use the general pointer projection > infrastructure, which handles these cases correctly. > > As part of migration, the macro is updated to have an improved surface > syntax. The current macro have > > dma_read!(a.b.c[d].e.f) > > to mean `a.b.c` is a DMA coherent allocation and it should project into it > with `[d].e.f` and do a read, which is confusing as it makes the indexing > operator integral to the macro (so it will break if you have an array of > `CoherentAllocation`, for example). > > This also is problematic as we would like to generalize > `CoherentAllocation` from just slices to arbitrary types. > > Make the macro expects `dma_read!(path.to.dma, .path.inside.dma)` as the > canonical syntax. The index operator is no longer special and is just one > type of projection (in additional to field projection). Similarly, make > `dma_write!(path.to.dma, .path.inside.dma, value)` become the canonical > syntax for writing. > > Another issue of the current macro is that it is always fallible. This > makes sense with existing design of `CoherentAllocation`, but once we > support fixed size arrays with `CoherentAllocation`, it is desirable to > have the ability to perform infallible indexing as well, e.g. doing a `[0]` > index of `[Foo; 2]` is okay and can be checked at build-time, so forcing > falliblity is non-ideal. To capture this, the macro is changed to use > `[idx]` as infallible projection and `[idx]?` as fallible index projection > (those syntax are part of the general projection infra). A benefit of this > is that while individual indexing operation may fail, the overall > read/write operation is not fallible. > > Fixes: ad2907b4e308 ("rust: add dma coherent allocator abstraction") > Signed-off-by: Gary Guo <gary@garyguo.net> Reviewed-by: Benno Lossin <lossin@kernel.org> Cheers, Benno > --- > drivers/gpu/nova-core/gsp.rs | 14 ++-- > drivers/gpu/nova-core/gsp/boot.rs | 2 +- > drivers/gpu/nova-core/gsp/cmdq.rs | 10 ++- > rust/kernel/dma.rs | 114 +++++++++++++----------------- > samples/rust/rust_dma.rs | 30 ++++---- > 5 files changed, 81 insertions(+), 89 deletions(-) ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2026-03-03 12:21 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-03-02 13:02 [PATCH v3 0/2] rust: add pointer projection infrastructure and convert DMA Gary Guo
2026-03-02 13:02 ` [PATCH v3 1/2] rust: add projection infrastructure Gary Guo
2026-03-02 14:38 ` Benno Lossin
2026-03-02 14:48 ` Danilo Krummrich
2026-03-02 18:49 ` Benno Lossin
2026-03-02 14:49 ` Gary Guo
2026-03-02 18:49 ` Benno Lossin
2026-03-02 20:14 ` Gary Guo
2026-03-02 22:01 ` Benno Lossin
2026-03-02 22:19 ` Gary Guo
2026-03-03 9:14 ` Benno Lossin
2026-03-03 10:17 ` Gary Guo
2026-03-03 11:39 ` Alice Ryhl
2026-03-03 12:21 ` Gary Guo
2026-03-02 13:02 ` [PATCH v3 2/2] rust: dma: use pointer projection infra for `dma_{read,write}` macro Gary Guo
2026-03-02 14:42 ` [PATCH v3 2/2] rust: dma: use pointer projection infra for `dma_{read, write}` macro Benno Lossin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox