rust/kernel/lib.rs | 2 + rust/kernel/transmute.rs | 87 +++++++++++++++++++++++++++++++++++++--- 2 files changed, 84 insertions(+), 5 deletions(-)
Add implementation and documentation for FromBytes trait.
Add new feature block in order to allow using ToBytes
and bound to from_bytes_mut function. I'm adding this feature
because is possible create a value with disallowed bit pattern
and as_byte_mut could create such value by mutating the array and
accessing the original value. So adding ToBytes this can be avoided.
Link: https://github.com/Rust-for-Linux/linux/issues/1119
Signed-off-by: Christian dos Santos de Lima <christiansantoslima21@gmail.com>
---
Changes in v2:
- Rollback the implementation for the macro in the repository and add implementation of functions in trait
Changes in v3:
- Fix grammar errors
- Remove repeated tests
- Fix alignment errors errors
- Fix tests not building
- Link to v2: https://lore.kernel.org/rust-for-linux/20241012193657.290cc79c@eugeo/T/#t
---
rust/kernel/lib.rs | 2 +
rust/kernel/transmute.rs | 87 +++++++++++++++++++++++++++++++++++++---
2 files changed, 84 insertions(+), 5 deletions(-)
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index dc37aef6a008..5215f5744e12 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -18,6 +18,8 @@
#![feature(lint_reasons)]
#![feature(new_uninit)]
#![feature(unsize)]
+#![feature(portable_simd)]
+#![feature(trivial_bounds)]
// Ensure conditional compilation based on the kernel configuration works;
// otherwise we may silently break things like initcall handling.
diff --git a/rust/kernel/transmute.rs b/rust/kernel/transmute.rs
index 1c7d43771a37..a4b462e07639 100644
--- a/rust/kernel/transmute.rs
+++ b/rust/kernel/transmute.rs
@@ -2,6 +2,7 @@
//! Traits for transmuting types.
+use core::simd::ToBytes;
/// Types for which any bit pattern is valid.
///
/// Not all types are valid for all values. For example, a `bool` must be either zero or one, so
@@ -9,15 +10,60 @@
///
/// It's okay for the type to have padding, as initializing those bytes has no effect.
///
+/// # Example
+///
+/// This example is how to use the FromBytes trait
+/// ```
+/// use kernel::transmute::FromBytes;
+/// // Initialize a slice of bytes
+/// let foo = &[1, 2, 3, 4];
+///
+/// //Use the function implemented by trait in integer type
+/// unsafe {
+/// let result = u32::from_bytes(foo);
+/// assert_eq!(*result, 0x4030201);
+/// }
+/// ```
/// # Safety
///
/// All bit-patterns must be valid for this type. This type must not have interior mutability.
-pub unsafe trait FromBytes {}
+pub unsafe trait FromBytes {
+ /// Get an imutable slice of bytes and converts to a reference to Self
+ unsafe fn from_bytes(slice_of_bytes: &[u8]) -> &Self;
+ /// Get a mutable slice of bytes and converts to a reference to Self
+ ///
+ /// # Safety
+ ///
+ /// Bound ToBytes in order to avoid use with disallowed bit patterns
+ unsafe fn from_bytes_mut(slice_of_bytes: &mut [u8]) -> &mut Self
+ where
+ Self: ToBytes;
+}
+// Get a reference of slice of bytes and converts into a reference of integer or a slice with a defined size
macro_rules! impl_frombytes {
($($({$($generics:tt)*})? $t:ty, )*) => {
// SAFETY: Safety comments written in the macro invocation.
- $(unsafe impl$($($generics)*)? FromBytes for $t {})*
+ $(unsafe impl$($($generics)*)? FromBytes for $t {
+ unsafe fn from_bytes(slice_of_bytes: &[u8]) -> &Self
+ {
+ unsafe {
+ let slice_ptr = slice_of_bytes.as_ptr() as *const Self;
+ &*slice_ptr
+ }
+ }
+
+ unsafe fn from_bytes_mut(slice_of_bytes: &mut [u8]) -> &mut Self
+ where
+ Self: ToBytes,
+ {
+ unsafe {
+ let slice_ptr = slice_of_bytes.as_mut_ptr() as *mut Self;
+ &mut *slice_ptr
+ }
+
+ }
+ })*
};
}
@@ -28,10 +74,43 @@ macro_rules! impl_frombytes {
// SAFETY: If all bit patterns are acceptable for individual values in an array, then all bit
// patterns are also acceptable for arrays of that type.
- {<T: FromBytes>} [T],
{<T: FromBytes, const N: usize>} [T; N],
}
+/// Get a reference of slice of bytes and converts into a reference of an array of integers
+///
+/// Types for which any bit pattern is valid.
+///
+/// Not all types are valid for all values. For example, a `bool` must be either zero or one, so
+/// reading arbitrary bytes into something that contains a `bool` is not okay.
+///
+/// It's okay for the type to have padding, as initializing those bytes has no effect.
+///
+// SAFETY: If all bit patterns are acceptable for individual values in an array, then all bit
+// patterns are also acceptable for arrays of that type.
+unsafe impl<T: FromBytes> FromBytes for [T] {
+ unsafe fn from_bytes(slice_of_bytes: &[u8]) -> &Self {
+ // Safety: Guarantee that all values are initialized
+ unsafe {
+ let slice_ptr = slice_of_bytes.as_ptr() as *const T;
+ let slice_len = slice_of_bytes.len() / core::mem::size_of::<T>();
+ core::slice::from_raw_parts(slice_ptr, slice_len)
+ }
+ }
+
+ unsafe fn from_bytes_mut(slice_of_bytes: &mut [u8]) -> &mut Self
+ where
+ Self: ToBytes,
+ {
+ // Safety: Guarantee that all values are initialized
+ unsafe {
+ let slice_ptr = slice_of_bytes.as_mut_ptr() as *mut T;
+ let slice_len = slice_of_bytes.len() / core::mem::size_of::<T>();
+ core::slice::from_raw_parts_mut(slice_ptr, slice_len)
+ }
+ }
+}
+
/// Types that can be viewed as an immutable slice of initialized bytes.
///
/// If a struct implements this trait, then it is okay to copy it byte-for-byte to userspace. This
@@ -48,7 +127,6 @@ macro_rules! impl_frombytes {
/// Values of this type may not contain any uninitialized bytes. This type must not have interior
/// mutability.
pub unsafe trait AsBytes {}
-
macro_rules! impl_asbytes {
($($({$($generics:tt)*})? $t:ty, )*) => {
// SAFETY: Safety comments written in the macro invocation.
@@ -63,7 +141,6 @@ macro_rules! impl_asbytes {
bool,
char,
str,
-
// SAFETY: If individual values in an array have no uninitialized portions, then the array
// itself does not have any uninitialized portions either.
{<T: AsBytes>} [T],
--
2.47.0+cc Abdiel
Hi Christian,
> On 9 Nov 2024, at 02:54, Christian dos Santos de Lima <christiansantoslima21@gmail.com> wrote:
>
> Add implementation and documentation for FromBytes trait.
>
> Add new feature block in order to allow using ToBytes
> and bound to from_bytes_mut function. I'm adding this feature
> because is possible create a value with disallowed bit pattern
> and as_byte_mut could create such value by mutating the array and
> accessing the original value. So adding ToBytes this can be avoided.
I’ve read this a couple of times. It’s hard to understand what you’re trying to say.
There are some grammar errors, but the even the key idea is hard to understand. IOW,
I had to go on Github to get the extra context needed to understand this part below:
> possible create a value with disallowed bit pattern
> and as_byte_mut could create such value by mutating the array and
> accessing the original value. So adding ToBytes this can be avoided.
Also, core::simd::ToBytes? Maybe Bjorn meant a new kernel::transmute::ToBytes trait?
Christian, are you aware of Abdiel’s dma_alloc_coherent patch [0]? How is this patch
an improvement over the simpler code below, for example?
> + fn cpu_buf(&self) -> &[T]
> + {
> + // SAFETY: The pointer is valid due to type invariant on `CoherentAllocation` and
> + // is valid for reads for `self.count * size_of::<T>` bytes.
> + unsafe { core::slice::from_raw_parts(self.cpu_addr, self.count) }
> + }
> +
> + fn cpu_buf_mut(&mut self) -> &mut [T]
> + {
> + // SAFETY: The pointer is valid due to type invariant on `CoherentAllocation` and
> + // is valid for reads for `self.count * size_of::<T>` bytes.
> + unsafe { core::slice::from_raw_parts_mut(self.cpu_addr, self.count) }
> + }
I believe that most users would have a pointer to memory allocated on the C side, as you can
see from Abdiel’s excerpt above. Without a user, it’s hard to know why we need this, specially
given the simd::ToBytes bound.
Just my 2c.
— Daniel
[0] https://lore.kernel.org/rust-for-linux/20241105104726.3111058-3-abdiel.janulgue@gmail.com/
Hi, Daniel! I didn't know. Thank you in advance, I'll improve in v4.
Best regards,
Christian
On Sat, Nov 9, 2024 at 8:33 PM Daniel Almeida
<daniel.almeida@collabora.com> wrote:
>
> +cc Abdiel
>
> Hi Christian,
>
>
> > On 9 Nov 2024, at 02:54, Christian dos Santos de Lima <christiansantoslima21@gmail.com> wrote:
> >
> > Add implementation and documentation for FromBytes trait.
> >
> > Add new feature block in order to allow using ToBytes
> > and bound to from_bytes_mut function. I'm adding this feature
> > because is possible create a value with disallowed bit pattern
> > and as_byte_mut could create such value by mutating the array and
> > accessing the original value. So adding ToBytes this can be avoided.
>
> I’ve read this a couple of times. It’s hard to understand what you’re trying to say.
>
> There are some grammar errors, but the even the key idea is hard to understand. IOW,
> I had to go on Github to get the extra context needed to understand this part below:
>
> > possible create a value with disallowed bit pattern
> > and as_byte_mut could create such value by mutating the array and
> > accessing the original value. So adding ToBytes this can be avoided.
>
>
> Also, core::simd::ToBytes? Maybe Bjorn meant a new kernel::transmute::ToBytes trait?
>
> Christian, are you aware of Abdiel’s dma_alloc_coherent patch [0]? How is this patch
> an improvement over the simpler code below, for example?
>
> > + fn cpu_buf(&self) -> &[T]
> > + {
> > + // SAFETY: The pointer is valid due to type invariant on `CoherentAllocation` and
> > + // is valid for reads for `self.count * size_of::<T>` bytes.
> > + unsafe { core::slice::from_raw_parts(self.cpu_addr, self.count) }
> > + }
> > +
> > + fn cpu_buf_mut(&mut self) -> &mut [T]
> > + {
> > + // SAFETY: The pointer is valid due to type invariant on `CoherentAllocation` and
> > + // is valid for reads for `self.count * size_of::<T>` bytes.
> > + unsafe { core::slice::from_raw_parts_mut(self.cpu_addr, self.count) }
> > + }
>
> I believe that most users would have a pointer to memory allocated on the C side, as you can
> see from Abdiel’s excerpt above. Without a user, it’s hard to know why we need this, specially
> given the simd::ToBytes bound.
>
> Just my 2c.
>
> — Daniel
>
>
> [0] https://lore.kernel.org/rust-for-linux/20241105104726.3111058-3-abdiel.janulgue@gmail.com/
On Sat, Nov 9, 2024 at 6:54 AM Christian dos Santos de Lima
<christiansantoslima21@gmail.com> wrote:
>
> Add implementation and documentation for FromBytes trait.
Thanks for the patch! Some comments below...
The title does not need to be duplicated in the message.
More importantly, the commit message should explain what you are
changing and why.
In particular, "Add implementation and documentation for FromBytes
trait." seems wrong, because we already have the implementation and
the documentation for the `FromBytes` trait.
> Add new feature block in order to allow using ToBytes
> and bound to from_bytes_mut function. I'm adding this feature
> because is possible create a value with disallowed bit pattern
> and as_byte_mut could create such value by mutating the array and
> accessing the original value. So adding ToBytes this can be avoided.
I understand that you want to add those two features, but you need to
justify more precisely why they are needed, e.g. is there a way to do
it without them? If yes, why cannot use that alternative way? e.g. is
it too complicated? Or is it that it cannot be achieved without the
feature at all? Please try to be explicit and mention both features by
name.
Also, you should mention their status if you know about it, since we
should avoid adding new features, given that we are trying to get
Linux into stable Rust:
https://rust-for-linux.com/unstable-features#usage-in-the-kernel
> //! Traits for transmuting types.
>
> +use core::simd::ToBytes;
> /// Types for which any bit pattern is valid.
Please try to be consistent with the rest of the code in its style,
e.g. newline here, whitespace elsewhere, safety comments, use
Markdown, etc. (but we can discuss that in a later version).
> +pub unsafe trait FromBytes {
> + /// Get an imutable slice of bytes and converts to a reference to Self
Typo (`scripts/checkpatch.pl` has spell checking capabilities).
More importantly, this `unsafe fn` does not have a `# Safety` section.
> + /// # Safety
> + ///
> + /// Bound ToBytes in order to avoid use with disallowed bit patterns
This `# Safety` section does not explain the safety preconditions; and
the bound is anyway in the signature already, i.e. this section is not
about explaining how you implemented something, but for users to learn
how to use it properly.
> +// Get a reference of slice of bytes and converts into a reference of integer or a slice with a defined size
This seems misplaced?
> +/// Get a reference of slice of bytes and converts into a reference of an array of integers
> +///
> +/// Types for which any bit pattern is valid.
> +///
> +/// Not all types are valid for all values. For example, a `bool` must be either zero or one, so
> +/// reading arbitrary bytes into something that contains a `bool` is not okay.
> +///
> +/// It's okay for the type to have padding, as initializing those bytes has no effect.
> +///
This also seems misplaced, and apparently is a mixture of the
`FromBytes` docs (?).
> +unsafe impl<T: FromBytes> FromBytes for [T] {
> + unsafe fn from_bytes(slice_of_bytes: &[u8]) -> &Self {
> + // Safety: Guarantee that all values are initialized
Please see how other safety comments are written: we need to justify
the preconditions of the unsafe operations within the block. So, for
instance, indeed, the values need to be initialized to call
`from_raw_parts_mut`, but why they are so?
Some operations here do not need to be in the block -- we try to minimize them.
> pub unsafe trait AsBytes {}
> -
> macro_rules! impl_asbytes {
> ($($({$($generics:tt)*})? $t:ty, )*) => {
> // SAFETY: Safety comments written in the macro invocation.
> @@ -63,7 +141,6 @@ macro_rules! impl_asbytes {
> bool,
> char,
> str,
> -
> // SAFETY: If individual values in an array have no uninitialized portions, then the array
Please avoid spurious/unrelated changes -- these should not be in the patch.
Thanks!
Cheers,
Miguel
© 2016 - 2024 Red Hat, Inc.