Re: [PATCH 3/3] rust: add abstraction for `struct page`

[Trevor Gross]

On Fri, Jan 26, 2024 at 1:28 PM Boqun Feng <boqun.feng@xxxxxxxxx> wrote:
>
> On Fri, Jan 26, 2024 at 01:33:46PM +0100, Alice Ryhl wrote:
> > On Fri, Jan 26, 2024 at 1:47 AM Boqun Feng <boqun.feng@xxxxxxxxx> wrote:
> > >
> > > On Wed, Jan 24, 2024 at 11:20:23AM +0000, Alice Ryhl wrote:
> > > > [...]
> > > > +    /// Maps the page and writes into it from the given buffer.
> > > > +    ///
> > > > +    /// # Safety
> > > > +    ///
> > > > +    /// Callers must ensure that `src` is valid for reading `len` bytes.
> > > > +    pub unsafe fn write(&self, src: *const u8, offset: usize, len: usize) -> Result {
> > >
> > > Use a slice like type as `src` maybe? Then the function can be safe:
> > >
> > >         pub fn write<S: AsRef<[u8]>>(&self, src: S, offset: usize) -> Result
> > >
> > > Besides, since `Page` impl `Sync`, shouldn't this `write` and the
> > > `fill_zero` be a `&mut self` function? Or make them both `unsafe`
> > > because of potential race and add some safety requirement?
> >
> > Ideally, we don't want data races with these methods to be UB. They
>
> I understand that, but in the current code, you can write:
>
>         CPU 0                   CPU 1
>         =====                   =====
>
>         page.write(src1, 0, 8);
>                                 page.write(src2, 0, 8);
>
> and it's a data race at kernel end. So my question is more how we can
> prevent the UB ;-)

Hm. Would the following work?

    // Change existing functions to work with references, meaning they need an
    // exclusive &mut self
    pub fn with_page_mapped<T>(
        &mut self,
        f: impl FnOnce(&mut [u8; PAGE_SIZE]) -> T
    ) -> T

    pub fn with_pointer_into_page<T>(
        &mut self,
        off: usize,
        len: usize,
        f: impl FnOnce(&mut [u8]) -> Result<T>,
    ) -> Result<T>

    // writing methods now take &mut self
    pub fn write(&mut self ...)
    pub fn fill_zero(&mut self ...)
    pub fn copy_into_page(&mut self ...)

    // Add two new functions that take &self, but return shared access
    pub fn with_page_mapped_raw<T>(
        &self,
        f: impl FnOnce(&UnsafeCell<[u8; PAGE_SIZE]>) -> T
    ) -> T

    pub fn with_pointer_into_page_raw<T>(
        &self,
        off: usize,
        len: usize,
        f: impl FnOnce(&[UnsafeCell<u8>]) -> Result<T>,
    ) -> Result<T>

This would mean that anyone who can obey rust's mutability rules can
use a page without any safety or race conditions to worry about, much
better for usability.

But if you do need to allow the data to be shared and racy, such as
the userspace example, the `_raw` methods allow for that and you can
`.get()` a `*mut u8` from the UnsafeCell. This moves the interior
mutability only to the mapped data rather than the Page itself, which
I think is more accurate anyway.

Leveraging UnsafeCell would also make some things with UserSlicePtr
more clear too.

- Trevor

> Regards,
> Boqun
>
> > could be mapped into the address space of a userspace process.
> >
> > Alice
>