Re: [PATCH v3 1/4] rust: uaccess: add userspace pointers

[Boqun Feng]

On Mon, Mar 18, 2024 at 09:10:07PM +0100, Alice Ryhl wrote:
> On Mon, Mar 18, 2024 at 8:33 PM Boqun Feng <boqun.feng@xxxxxxxxx> wrote:
> >
> > On Mon, Mar 18, 2024 at 08:12:27PM +0100, Alice Ryhl wrote:
> > > On Mon, Mar 18, 2024 at 7:59 PM Boqun Feng <boqun.feng@xxxxxxxxx> wrote:
> > > >
> > > > On Mon, Mar 11, 2024 at 10:47:13AM +0000, Alice Ryhl wrote:
> > > > > +
> > > > > +    /// Reads raw data from the user slice into a raw kernel buffer.
> > > > > +    ///
> > > > > +    /// Fails with `EFAULT` if the read encounters a page fault.
> > > > > +    ///
> > > > > +    /// # Safety
> > > > > +    ///
> > > > > +    /// The `out` pointer must be valid for writing `len` bytes.
> > > > > +    pub unsafe fn read_raw(&mut self, out: *mut u8, len: usize) -> Result {
> > > >
> > > > I don't think we want to promote the pub usage of this unsafe function,
> > > > right? We can provide a safe version:
> > > >
> > > >         pub fn read_slice(&mut self, to: &[u8]) -> Result
> > > >
> > > > and all users can just use the safe version (with the help of
> > > > slice::from_raw_parts_mut() if necessary).
> > >
> > > Personally, I think having the function be unsafe is plenty discouragement.
> > >
> > > Also, this method would need an &mut [u8], which opens the can of
> > > worms related to uninitialized memory. The _raw version of this method
> >
> > make it a `&mut [MayUninit<u8>]` then? If that works, then _raw version
> > is not more powerful therefore no need to pub it.
> 
> Nobody actually has a need for that. Also, it doesn't even remove the

I want to use read_slice() to replace read_raw(), and avoid even
pub(crate) for read_raw().

> need for unsafe code in the caller, since the caller still needs to
> assert that the call has initialized the memory.
> 

If we have the read_slice():

    pub fn read_slice(&mut self, to: &mut [MayUninit<u8>]) -> Result

then the read_all() function can be implemented as:

    pub fn read_all(mut self, buf: &mut Vec<u8>) -> Result {
        let len = self.length;
        buf.try_reserve(len)?;

	// Append `len` bytes in the `buf`.
        self.read_slice(&mut buf.spare_capacity_mut()[0..len])?;

	// SAFETY: Since the call to `read_slice` was successful, so the
	// next `len` bytes of the vector have been initialized.
        unsafe { buf.set_len(buf.len() + len) };
        Ok(())
    }	

one unsafe block has been removed, and yes, you're right, there is still
need of unsafe here, since the caller still needs to assert the memory
has been initialized. However, to me, it's still an improvement, since
one unsafe block gets removed because we get away from reasoning based
on raw pointers and length.


And yes, for the worst case, we still have the same amount of unsafe
code. For example in `Page::copy_from_user_slice`, if read_slice() is
used, we still need to:

    let mut s = unsafe { slice::from_raw_part_mut(dst.cast::<MayUninit<u8>>(), len) };
    reader.read_slice(&mut s);

i.e. move the unsafe part from `reader` to the construction of a
"writable slice". However, it's still better, since contructing a slice
is quite common in Rust so it's easy to check the safety requirement.


I generally think replacing a pointer+length pair with a slice is
better.

Regards,
Boqun

> > > is strictly more powerful.
> > >
> > > I don't think I actually use it directly in Binder, so I can make it
> > > private if you think that's important. It needs to be pub(crate),
> >
> > I might be too picky, but avoiding pub unsafe functions if not necessary
> > could help us reduce unnecessary unsafe code ;-)
> >
> > Regards,
> > Boqun
> >
> > > though, since it is used in `Page`.
> > >
> > > Alice