On Wed, Jan 24, 2024 at 6:21 AM Alice Ryhl <aliceryhl@xxxxxxxxxx> wrote:
I see this patch answers some of my naming questions from 1/3, sorry
for not reading all the way through.
> diff --git a/rust/kernel/user_ptr.rs b/rust/kernel/user_ptr.rs
> index 00aa26aa6a83..daa46abe5525 100644
> --- a/rust/kernel/user_ptr.rs
> +++ b/rust/kernel/user_ptr.rs
> @@ -11,6 +11,7 @@
> use crate::{bindings, error::code::*, error::Result};
> use alloc::vec::Vec;
> use core::ffi::{c_ulong, c_void};
> +use core::mem::{size_of, MaybeUninit};
>
> /// The maximum length of a operation using `copy_[from|to]_user`.
> ///
> @@ -151,6 +152,36 @@ pub unsafe fn read_raw(&mut self, out: *mut u8, len: usize) -> Result {
> Ok(())
> }
>
> + /// Reads a value of the specified type.
> + ///
> + /// Fails with `EFAULT` if the read encounters a page fault.
> + pub fn read<T: ReadableFromBytes>(&mut self) -> Result<T> {
I think that `T: Copy` is required here, or for Copy to be a
supertrait of ReadableBytes, since the data in the buffer is being
duplicated from a reference.
Send is probably also a reasonable bound to have .
> + if size_of::<T>() > self.1 || size_of::<T>() > MAX_USER_OP_LEN {
> + return Err(EFAULT);
> + }
> + let mut out: MaybeUninit<T> = MaybeUninit::uninit();
> + // SAFETY: The local variable `out` is valid for writing `size_of::<T>()` bytes.
> + let res = unsafe {
> + bindings::copy_from_user_unsafe_skip_check_object_size(
> + out.as_mut_ptr().cast::<c_void>(),
> + self.0,
> + size_of::<T>() as c_ulong,
As with the other patch, I think it would be more clear to use
`c_ulong::try_from(...)` rather than comparing against
`MAX_USER_OP_LEN ` and later casting. Possibly just in a helper
function.
> + )
> + };
> + if res != 0 {
> + return Err(EFAULT);
> + }
> + // Since this is not a pointer to a valid object in our program,
> + // we cannot use `add`, which has C-style rules for defined
> + // behavior.
> + self.0 = self.0.wrapping_add(size_of::<T>());
There are now methods `wrapping_byte_add` (since 1.75). Doesn't make
much of a difference since the pointer is c_void anyway, but it does
make the unit more clear.
> + self.1 -= size_of::<T>();
> +
> + // SAFETY: The read above has initialized all bytes in `out`, and since
> + // `T` implements `ReadableFromBytes`, any bit-pattern is a valid value
> + // for this type.
> + Ok(unsafe { out.assume_init() })
> + }
> +
> /// Reads all remaining data in the buffer into a vector.
> ///
> /// Fails with `EFAULT` if the read encounters a page fault.
> @@ -219,4 +250,98 @@ pub fn write_slice(&mut self, data: &[u8]) -> Result {
> // `len`, so the pointer is valid for reading `len` bytes.
> unsafe { self.write_raw(ptr, len) }
> }
> +
> + /// Writes the provided Rust value to this userspace pointer.
> + ///
> + /// Fails with `EFAULT` if the write encounters a page fault.
> + pub fn write<T: WritableToBytes>(&mut self, value: &T) -> Result {
Send + Copy are also needed here, or supertraits of WritableToBytes.
> + if size_of::<T>() > self.1 || size_of::<T>() > MAX_USER_OP_LEN {
> + return Err(EFAULT);
> + }
> + // SAFETY: The reference points to a value of type `T`, so it is valid
> + // for reading `size_of::<T>()` bytes.
> + let res = unsafe {
> + bindings::copy_to_user_unsafe_skip_check_object_size(
> + self.0,
> + (value as *const T).cast::<c_void>(),
> + size_of::<T>() as c_ulong,
> + )
> + };
> + if res != 0 {
> + return Err(EFAULT);
> + }
> + // Since this is not a pointer to a valid object in our program,
> + // we cannot use `add`, which has C-style rules for defined
> + // behavior.
> + self.0 = self.0.wrapping_add(size_of::<T>());
> + self.1 -= size_of::<T>();
> + Ok(())
> + }
> }
> +
> +/// Specifies that a type is safely readable from bytes.
> +///
> +/// 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
> +///
> +/// All bit-patterns must be valid for this type.
> +pub unsafe trait ReadableFromBytes {}
> +
> +// SAFETY: All bit patterns are acceptable values of the types below.
> +unsafe impl ReadableFromBytes for u8 {}
> +unsafe impl ReadableFromBytes for u16 {}
> +unsafe impl ReadableFromBytes for u32 {}
> +unsafe impl ReadableFromBytes for u64 {}
> +unsafe impl ReadableFromBytes for usize {}
> +unsafe impl ReadableFromBytes for i8 {}
> +unsafe impl ReadableFromBytes for i16 {}
> +unsafe impl ReadableFromBytes for i32 {}
> +unsafe impl ReadableFromBytes for i64 {}
> +unsafe impl ReadableFromBytes for isize {}
> +// 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: ReadableFromBytes> ReadableFromBytes for [T] {}
> +unsafe impl<T: ReadableFromBytes, const N: usize> ReadableFromBytes for [T; N] {}
> +
> +/// Specifies that a type is safely writable to bytes.
> +///
> +/// If a struct implements this trait, then it is okay to copy it byte-for-byte
> +/// to userspace. This means that it should not have any padding, as padding
> +/// bytes are uninitialized. Reading uninitialized memory is not just undefined
> +/// behavior, it may even lead to leaking sensitive information on the stack to
> +/// userspace.
> +///
> +/// The struct should also not hold kernel pointers, as kernel pointer addresses
> +/// are also considered sensitive. However, leaking kernel pointers is not
> +/// considered undefined behavior by Rust, so this is a correctness requirement,
> +/// but not a safety requirement.
> +///
> +/// # Safety
> +///
> +/// Values of this type may not contain any uninitialized bytes.
> +pub unsafe trait WritableToBytes {}
> +
> +// SAFETY: Instances of the following types have no uninitialized portions.
> +unsafe impl WritableToBytes for u8 {}
> +unsafe impl WritableToBytes for u16 {}
> +unsafe impl WritableToBytes for u32 {}
> +unsafe impl WritableToBytes for u64 {}
> +unsafe impl WritableToBytes for usize {}
> +unsafe impl WritableToBytes for i8 {}
> +unsafe impl WritableToBytes for i16 {}
> +unsafe impl WritableToBytes for i32 {}
> +unsafe impl WritableToBytes for i64 {}
> +unsafe impl WritableToBytes for isize {}
> +unsafe impl WritableToBytes for bool {}
> +unsafe impl WritableToBytes for char {}
> +unsafe impl WritableToBytes for str {}
> +// SAFETY: If individual values in an array have no uninitialized portions, then
> +// the the array itself does not have any uninitialized portions either.
> +unsafe impl<T: WritableToBytes> WritableToBytes for [T] {}
> +unsafe impl<T: WritableToBytes, const N: usize> WritableToBytes for [T; N] {}
>
> --
> 2.43.0.429.g432eaa2c6b-goog
>
>
These traits are probably usable in a lot of other places (e.g.
packets, GPU), so could you put them in a separate module?
The patterns here are pretty similar to what the bytemuck crate does
[1]. Since that crate is well established and open licensed, I think
it makes sense to keep their naming or possibly even vendor a portion
in.
In particular, this would likely include the traits:
- AnyBitPattern, which is roughly ReadableFromBytes here
- NoUninit, which is roughly WritableToBytes here
- Optionally Pod (plain old data), a supertrait of both AnyBitPattern
and NoUninit just used to simplify trait implementation (impl Pod and
you get the other two).
And the functions:
- from_bytes to turn &[u8] into &T for use in `read`. Needs `T: Copy`
to return an owned value, as noted above.
- bytes_of to turn &T into &[u8], for use in `write`
The derive macros would also be nice to have down the line, though
bytemuck's unfortunately relies on syn.
- Trevor
[1]: https://docs.rs/bytemuck/latest/bytemuck/
