rust: add pointer projection infrastructure and convert DMA
Subsystems (7)
v3 cover rust: add pointer projection infrastructure and convert DMA
From: Gary Guo <[email protected]>
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].
---
Changes since v2:
- Fix off-by-one issue (Aditya)
- Fix comment (Aditya)
- Link to v2: https://lore.kernel.org/rust-for-linux/[email protected]/
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/[email protected]/
---
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
Signature · 1 line
2.51.2
PATCH v3 1/2 rust: add projection infrastructure
From: Gary Guo <[email protected]>
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.
--- 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
rust/kernel/lib.rs +5 -0
@@ -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;
rust/kernel/projection.rs new file +284 -0
284 lines not on this page · raw message
scripts/Makefile.build +3 -1
@@ -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
Signature · 1 line
2.51.2
Benno Lossin
reviewerRe: [PATCH v3 1/2] rust: add projection infrastructure
rust/kernel/lib.rs line 23
Quoted 9 lines · Gary Guo
rust/kernel/lib.rs +1
-0@@ -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).
Quoted 3 lines
rust/kernel/lib.rs +0
-0// // Stable since Rust 1.80.0. #![feature(slice_flatten)]
...
rust/kernel/lib.rs
Quoted 7 lines
rust/kernel/lib.rs +7
-0+/// 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"?
rust/kernel/lib.rs
Quoted 7 lines
rust/kernel/lib.rs +7
-0+#[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?
Quoted 7 lines
rust/kernel/lib.rs +7
-0+ +/// 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! ()) + } +}
...
rust/kernel/lib.rs
Quoted 5 lines
rust/kernel/lib.rs +5
-0+/// 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.
rust/kernel/lib.rs
Quoted 4 lines
rust/kernel/lib.rs +4
-0+/// +/// # 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.
rust/kernel/lib.rs
Quoted 13 lines
rust/kernel/lib.rs +13
-0+#[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
Quoted 2 lines
rust/kernel/lib.rs +2
-0+unsafe fn proj <F >(base: *mut Self , f:impl FnOnce (*mut Self ) -> *mut F ) -> *mut F ; +}
Danilo Krummrich
maintainerRe: [PATCH v3 1/2] rust: add projection infrastructure
Quoted 1 line · Benno Lossin
[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?
Benno Lossin
reviewerRe: [PATCH v3 1/2] rust: add projection infrastructure
Quoted 5 lines · Danilo Krummrich
Quoted 1 line · Benno Lossin
[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
Gary Guo
reviewerRe: [PATCH v3 1/2] rust: add projection infrastructure
In reply to Benno LossinQuoted 17 lines · Benno Lossin
rust/kernel/lib.rs line 23
Quoted 9 lines · Gary Guo
rust/kernel/lib.rs +1
-0@@ -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).
Quoted 3 lines
rust/kernel/lib.rs +0
-0// // Stable since Rust 1.80.0. #![feature(slice_flatten)] ...
Quoted 11 lines
rust/kernel/lib.rs
Quoted 7 lines
rust/kernel/lib.rs +7
-0+/// 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 passptr::nullto 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.
Quoted 10 lines
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.
Quoted 6 lines
A pretty expensive solution would be to add a sealed trait
Indexablethat we implement for all things thatTis allowed to be; and then we provide a safe function in that trait to query the maximum offset thegetfunction is allowed to make.
Quoted 7 lines
Alternatively, we could use something like this:
The implementation of
getmust:
- return a pointer obtained by offsetting the input pointer.
- ensure that when the input pointer points at a valid value of typeT, 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?
Quoted 3 lines
Or something simpler that says "if the input pointer is valid, then
getmust 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"?
Quoted 10 lines
rust/kernel/lib.rs
Quoted 8 lines
rust/kernel/lib.rs +7
-0+#[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_indexinstead of the generalget?
I'm following the name on SliceIndex:
https://doc.rust-lang.org/stable/std/slice/trait.SliceIndex.html.
Best,
Gary
Quoted 50 lines
Quoted 8 lines
rust/kernel/lib.rs +7
-0+ +/// 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! ()) + } +}...
rust/kernel/lib.rs
Quoted 5 lines
rust/kernel/lib.rs +5
-0+/// 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
DEREFconstant. Essentially it is a load-bearing part of the macro that it does this.rust/kernel/lib.rs
Quoted 4 lines
rust/kernel/lib.rs +4
-0+/// +/// # Safety +/// +/// `proj` should invoke `f` with valid allocation, as documentation described. s/should invoke
fwith/may invokefonly with a/"should" sounds like only a suggestion. If it is a requirement, then the
build_error!impl of theDEREF = truecase would be violating it.rust/kernel/lib.rs
Quoted 13 lines
rust/kernel/lib.rs +13
-0+#[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,
BennoQuoted 2 lines
rust/kernel/lib.rs +2
-0+unsafe fn proj <F >(base: *mut Self , f:impl FnOnce (*mut Self ) -> *mut F ) -> *mut F ; +}
Benno Lossin
reviewerRe: [PATCH v3 1/2] rust: add projection infrastructure
Quoted 14 lines · Gary Guo
Quoted 12 lines · Benno Lossin
rust/kernel/projection.rs line 28
Quoted 7 lines · Gary Guo
rust/kernel/projection.rs new file +7
-0+/// 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 passptr::nullto them.
Quoted 3 lines
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 betweenptr .. ptr + x.
Okay, I haven't really seen that as a concept. Also, what is the size of an invalid pointer?
Quoted 28 lines
Quoted 9 lines
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.htmlI have a
KnownSizetrait for this in my I/O projection series that is implemented forT: Sizedand[T], and it returns the size when given a raw pointer.Quoted 6 lines
A pretty expensive solution would be to add a sealed trait
Indexablethat we implement for all things thatTis allowed to be; and then we provide a safe function in that trait to query the maximum offset thegetfunction is allowed to make.Quoted 7 lines
Alternatively, we could use something like this:
The implementation of
getmust:
- return a pointer obtained by offsetting the input pointer.
- ensure that when the input pointer points at a valid value of typeT, the offset must not be greater than [mem::size_of_val_raw] of the input pointer.
Quoted 3 lines
Given that I'm not introducing
KnownSizetrait in this patch, this is why I haven't used this kind of wording. Perhaps I can just bringKnownSizein early and use it first for documentation purpose only?
That sounds great.
Quoted 5 lines
Quoted 2 lines
Or something simpler that says "if the input pointer is valid, then
getmust 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?
Quoted 10 lines
Quoted 9 lines
rust/kernel/projection.rs line 35
Quoted 7 lines
rust/kernel/projection.rs new file +7
-0+#[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_indexinstead of the generalget?
Quoted 2 lines
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
Gary Guo
reviewerRe: [PATCH v3 1/2] rust: add projection infrastructure
Quoted 15 lines · Benno Lossin
Quoted 14 lines · Gary Guo
Quoted 12 lines · Benno Lossin
rust/kernel/projection.rs line 28
Quoted 7 lines · Gary Guo
rust/kernel/projection.rs new file +7
-0+/// 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 passptr::nullto them.
Quoted 6 lines
Quoted 3 lines
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 betweenptr .. 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.
Quoted 35 lines
Quoted 29 lines
Quoted 10 lines
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.htmlI have a
KnownSizetrait for this in my I/O projection series that is implemented forT: Sizedand[T], and it returns the size when given a raw pointer.Quoted 6 lines
A pretty expensive solution would be to add a sealed trait
Indexablethat we implement for all things thatTis allowed to be; and then we provide a safe function in that trait to query the maximum offset thegetfunction is allowed to make.Quoted 7 lines
Alternatively, we could use something like this:
The implementation of
getmust:
- return a pointer obtained by offsetting the input pointer.
- ensure that when the input pointer points at a valid value of typeT, the offset must not be greater than [mem::size_of_val_raw] of the input pointer.Quoted 3 lines
Given that I'm not introducing
KnownSizetrait in this patch, this is why I haven't used this kind of wording. Perhaps I can just bringKnownSizein early and use it first for documentation purpose only?That sounds great.
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
Or something simpler that says "if the input pointer is valid, then
getmust 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).
Quoted 11 lines
Quoted 11 lines
Quoted 10 lines
rust/kernel/projection.rs line 35
Quoted 8 lines
rust/kernel/projection.rs new file +7
-0+#[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_indexinstead of the generalget?
Quoted 6 lines
Quoted 2 lines
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
Quoted 3 lines
Cheers,
Benno
Benno Lossin
reviewerRe: [PATCH v3 1/2] rust: add projection infrastructure
Quoted 20 lines · Gary Guo
Quoted 19 lines · Benno Lossin
Quoted 17 lines · Gary Guo
Quoted 12 lines · Benno Lossin
rust/kernel/projection.rs line 28
Quoted 7 lines · Gary Guo
rust/kernel/projection.rs new file +7
-0+/// 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 passptr::nullto 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 betweenptr .. ptr + x.
Quoted 5 lines
Quoted 2 lines
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, andsize_of::<T>() * slice.len()for a raw slice pointer.
And for dyn Trait?
Do you have a link to somewhere?
Quoted 4 lines
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.
Quoted 42 lines
Quoted 34 lines
Quoted 28 lines
Quoted 9 lines
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.htmlI have a
KnownSizetrait for this in my I/O projection series that is implemented forT: Sizedand[T], and it returns the size when given a raw pointer.Quoted 6 lines
A pretty expensive solution would be to add a sealed trait
Indexablethat we implement for all things thatTis allowed to be; and then we provide a safe function in that trait to query the maximum offset thegetfunction is allowed to make.Quoted 7 lines
Alternatively, we could use something like this:
The implementation of
getmust:
- return a pointer obtained by offsetting the input pointer.
- ensure that when the input pointer points at a valid value of typeT, the offset must not be greater than [mem::size_of_val_raw] of the input pointer.Quoted 3 lines
Given that I'm not introducing
KnownSizetrait in this patch, this is why I haven't used this kind of wording. Perhaps I can just bringKnownSizein early and use it first for documentation purpose only?That sounds great.
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
Or something simpler that says "if the input pointer is valid, then
getmust 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?
Quoted 7 lines
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
Gary Guo
reviewerRe: [PATCH v3 1/2] rust: add projection infrastructure
Quoted 24 lines · Benno Lossin
Quoted 23 lines · Gary Guo
Quoted 21 lines · Benno Lossin
Quoted 14 lines · Gary Guo
Quoted 12 lines · Benno Lossin
rust/kernel/projection.rs line 28
Quoted 7 lines · Gary Guo
rust/kernel/projection.rs new file +7
-0+/// 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 passptr::nullto them.Quoted 3 lines
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 betweenptr .. ptr + x.Okay, I haven't really seen that as a concept. Also, what is the size of an invalid pointer?
Quoted 6 lines
Quoted 2 lines
It's
size_of::<T>()for sized types, andsize_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.
Quoted 8 lines
Quoted 5 lines
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
Benno Lossin
reviewerRe: [PATCH v3 1/2] rust: add projection infrastructure
Quoted 28 lines · Gary Guo
Quoted 27 lines · Benno Lossin
Quoted 25 lines · Gary Guo
Quoted 15 lines · Benno Lossin
Quoted 14 lines · Gary Guo
Quoted 12 lines · Benno Lossin
Review
Quoted 7 lines · Gary Guo
(quoted diff) +7
-0+/// 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 passptr::nullto them.Quoted 6 lines
Quoted 3 lines
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 betweenptr .. 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, andsize_of::<T>() * slice.len()for a raw slice pointer.
Quoted 8 lines
Quoted 3 lines
And for
dyn Trait?Do you have a link to somewhere?
For
dyn Traitit 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
Quoted 2 lines
I am basically just having
size_of_val_rawin mind when writing this. So the currentKnownSizecomment 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
Gary Guo
reviewerRe: [PATCH v3 1/2] rust: add projection infrastructure
Quoted 42 lines · Benno Lossin
Quoted 28 lines · Gary Guo
Quoted 27 lines · Benno Lossin
Quoted 25 lines · Gary Guo
Quoted 15 lines · Benno Lossin
Quoted 14 lines · Gary Guo
Quoted 12 lines · Benno Lossin
Review
Quoted 7 lines · Gary Guo
(quoted diff) +7
-0+/// 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 passptr::nullto them.Quoted 6 lines
Quoted 3 lines
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 betweenptr .. 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, andsize_of::<T>() * slice.len()for a raw slice pointer.Quoted 8 lines
Quoted 3 lines
And for
dyn Trait?Do you have a link to somewhere?
For
dyn Traitit 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:
Quoted 6 lines
dyn Traitmetadata 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.
Quoted 7 lines
[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/516Quoted 2 lines
I am basically just having
size_of_val_rawin mind when writing this. So the currentKnownSizecomment in v4 is something that I am happy about.
Quoted 8 lines
Well
size_of_val_rawisunsafeand 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 intoisize. This to me just further indicates that*mut Thas safety requirements to obtaining the size of an arbitrary pointer.In the special cases of
T: SizedandT == [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
Quoted 3 lines
Cheers,
Benno
Alice Ryhl
maintainerRe: [PATCH v3 1/2] rust: add projection infrastructure
Quoted 14 lines · Gary Guo
Quoted 4 lines · Benno Lossin
Quoted 2 lines · Gary Guo
I am basically just having
size_of_val_rawin mind when writing this. So the currentKnownSizecomment in v4 is something that I am happy about.Quoted 8 lines
Well
size_of_val_rawisunsafeand 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 intoisize. This to me just further indicates that*mut Thas safety requirements to obtaining the size of an arbitrary pointer.In the special cases of
T: SizedandT == [U], we have safe ways of getting their size.
Quoted 7 lines
Hmm, the
isizefitting 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
KnownSizeroute.
It sounds like that's no different from dyn trait vtable case. Fat pointer must have valid metadata, even if raw pointer.
Alice
Gary Guo
reviewerRe: [PATCH v3 1/2] rust: add projection infrastructure
Quoted 21 lines · Alice Ryhl
Quoted 20 lines · Gary Guo
Quoted 4 lines · Benno Lossin
Quoted 2 lines · Gary Guo
I am basically just having
size_of_val_rawin mind when writing this. So the currentKnownSizecomment in v4 is something that I am happy about.Quoted 8 lines
Well
size_of_val_rawisunsafeand 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 intoisize. This to me just further indicates that*mut Thas safety requirements to obtaining the size of an arbitrary pointer.In the special cases of
T: SizedandT == [U], we have safe ways of getting their size.Hmm, the
isizefitting 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).
Quoted 4 lines
Quoted 1 line
Anyhow this is moot as we're going the
KnownSizeroute.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
Quoted 2 lines
Alice
PATCH v3 2/2 rust: dma: use pointer projection infra for `dma_{read,write}` macro
In reply to Gary GuoFrom: Gary Guo <[email protected]>
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.
--- 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(-)
drivers/gpu/nova-core/gsp.rs +7 -7
@@ -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)); }, })) })
drivers/gpu/nova-core/gsp/boot.rs +1 -1
@@ -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))?;
drivers/gpu/nova-core/gsp/cmdq.rs +7 -3
@@ -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)) }
rust/kernel/dma.rs +50 -64
158 lines not on this page · raw message
samples/rust/rust_dma.rs +16 -14
@@ -68,7 +68,7 @@ fn probe(pdev: &pci::Device<Core>, _info: &Self::IdInfo) -> impl PinInit<Self, ECoherentAllocation ::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! (
Signature · 1 line
2.51.2
Re: [PATCH v3 2/2] rust: dma: use pointer projection infra for `dma_{read, write}` macro
Quoted 42 lines · Gary Guo
From: Gary Guo <[email protected]>
Current
dma_read!,dma_write!macros also use a customaddr_of!()-based implementation for projecting pointers, which has soundness issue as it relies on absence ofDerefimplementation 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.cis a DMA coherent allocation and it should project into it with[d].e.fand 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 ofCoherentAllocation, for example).This also is problematic as we would like to generalize
CoherentAllocationfrom 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, makedma_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 withCoherentAllocation, 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.
Quoted 2 lines
Fixes ad2907b4e308 ("rust: add dma coherent allocator abstraction")Signed-off-by Gary Guo <[email protected]>
Cheers,
Benno
Quoted 7 lines
--- 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(-)