Message-ID: <439baec2-dba8-4dab-abb5-faa14fbda943@gmail.com>
Date: Tue, 18 Mar 2025 21:13:01 +0100
From: Christian Schrefl <chrisi.schrefl@...il.com>
To: Alice Ryhl <aliceryhl@...gle.com>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 Alexander Viro <viro@...iv.linux.org.uk>, Arnd Bergmann <arnd@...db.de>,
 Miguel Ojeda <ojeda@...nel.org>
Cc: Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
 Björn Roy Baron <bjorn3_gh@...tonmail.com>,
 Benno Lossin <benno.lossin@...ton.me>,
 Andreas Hindborg <a.hindborg@...nel.org>, Trevor Gross <tmgross@...ch.edu>,
 Danilo Krummrich <dakr@...nel.org>, Matthew Maurer <mmaurer@...gle.com>,
 Lee Jones <lee@...nel.org>, linux-kernel@...r.kernel.org,
 rust-for-linux@...r.kernel.org
Subject: Re: [PATCH 2/5] rust: iov: add iov_iter abstractions for ITER_DEST

Hi Alice

On 11.03.25 3:25 PM, Alice Ryhl wrote:
> This adds abstractions for the iov_iter type in the case where
> data_source is ITER_DEST. This will make Rust implementations of
> fops->read_iter possible.
> 
> This series only has support for using existing IO vectors created by C
> code. Additional abstractions will be needed to support the creation of
> IO vectors in Rust code.
> 
> These abstractions make the assumption that `struct iov_iter` does not
> have internal self-references, which implies that it is valid to move it
> between different local variables, and that you can make a copy of it to
> get two IO vectors into the same buffers.
> 
> Signed-off-by: Alice Ryhl <aliceryhl@...gle.com>
> ---
>  rust/kernel/iov.rs | 140 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 139 insertions(+), 1 deletion(-)
> 
> diff --git a/rust/kernel/iov.rs b/rust/kernel/iov.rs
> index 4498f65e1f65bd964909810c020db3a9f8fae389..dc32c27c5c76d059562fd7c6b9d4b178a8ea7c81 100644
> --- a/rust/kernel/iov.rs
> +++ b/rust/kernel/iov.rs
> @@ -7,7 +7,12 @@
>  //! C headers: [`include/linux/iov_iter.h`](srctree/include/linux/iov_iter.h),
>  //! [`include/linux/uio.h`](srctree/include/linux/uio.h)
>  
> -use crate::{bindings, prelude::*, types::Opaque};
> +use crate::{
> +    alloc::{Allocator, Flags},
> +    bindings,
> +    prelude::*,
> +    types::Opaque,
> +};
>  use core::{marker::PhantomData, mem::MaybeUninit, slice};
>  
>  const ITER_SOURCE: bool = bindings::ITER_SOURCE != 0;
> @@ -168,3 +173,136 @@ fn clone(&self) -> IovIterSource<'data> {
>          unsafe { core::ptr::read(self) }
>      }
>  }
> +
> +/// An IO vector that acts as a destination for data.
> +///
> +/// # Invariants
> +///
> +/// Must hold a valid `struct iov_iter` with `data_source` set to `ITER_DEST`. The buffers
> +/// referenced by the IO vector must be valid for writing for the duration of `'data`.
> +///
> +/// Note that if the IO vector is backed by a userspace pointer, it is always considered valid for
> +/// writing.
> +#[repr(transparent)]
> +pub struct IovIterDest<'data> {
> +    iov: Opaque<bindings::iov_iter>,
> +    /// Represent to the type system that this value contains a pointer to writable data it does
> +    /// not own.
> +    _source: PhantomData<&'data mut [u8]>,
> +}

It might be a bit nicer to add a (private) struct 'IovIter' that implements the common operations.
Then 'IovIterDest' and 'IovIterSource' could store that struct and forward the implementations to 
it.
But I'm not sure if that's really much better.

> +
> +// SAFETY: This struct is essentially just a fancy `std::io::Cursor<&mut [u8]>`, and that type is
> +// safe to send across thread boundaries.
> +unsafe impl<'data> Send for IovIterDest<'data> {}
> +// SAFETY: This struct is essentially just a fancy `std::io::Cursor<&mut [u8]>`, and that type is
> +// safe to share across thread boundaries.
> +unsafe impl<'data> Sync for IovIterDest<'data> {}
> +
> +impl<'data> IovIterDest<'data> {
> +    /// Obtain an `IovIterDest` from a raw pointer.
> +    ///
> +    /// # Safety
> +    ///
> +    /// * For the duration of `'iov`, the `struct iov_iter` must remain valid and must not be
> +    ///   accessed except through the returned reference.
> +    /// * For the duration of `'data`, the buffers backing this IO vector must be valid for
> +    ///   writing.
> +    #[track_caller]
> +    #[inline]
> +    pub unsafe fn from_raw<'iov>(ptr: *mut bindings::iov_iter) -> &'iov mut IovIterDest<'data> {
> +        // SAFETY: The caller ensures that `ptr` is valid.
> +        let data_source = unsafe { (*ptr).data_source };
> +        assert_eq!(data_source, ITER_DEST);
> +
> +        // SAFETY: The caller ensures the struct invariants for the right durations.
> +        unsafe { &mut *ptr.cast::<IovIterDest<'data>>() }
> +    }
> +
> +    /// Access this as a raw `struct iov_iter`.
> +    #[inline]
> +    pub fn as_raw(&mut self) -> *mut bindings::iov_iter {
> +        self.iov.get()
> +    }
> +
> +    /// Returns the number of bytes available in this IO vector.
> +    ///
> +    /// Note that this may overestimate the number of bytes. For example, reading from userspace
> +    /// memory could fail with EFAULT, which will be treated as the end of the IO vector.
> +    #[inline]
> +    pub fn len(&self) -> usize {
> +        // SAFETY: It is safe to access the `count` field.
> +        unsafe {
> +            (*self.iov.get())
> +                .__bindgen_anon_1
> +                .__bindgen_anon_1
> +                .as_ref()
> +                .count
> +        }
> +    }
> +
> +    /// Returns whether there are any bytes left in this IO vector.
> +    ///
> +    /// This may return `true` even if there are no more bytes available. For example, reading from
> +    /// userspace memory could fail with EFAULT, which will be treated as the end of the IO vector.
> +    #[inline]
> +    pub fn is_empty(&self) -> bool {
> +        self.len() == 0
> +    }
> +
> +    /// Advance this IO vector by `bytes` bytes.
> +    ///
> +    /// If `bytes` is larger than the size of this IO vector, it is advanced to the end.
> +    #[inline]
> +    pub fn advance(&mut self, bytes: usize) {
> +        // SAFETY: `self.iov` is a valid IO vector.
> +        unsafe { bindings::iov_iter_advance(self.as_raw(), bytes) };
> +    }
> +
> +    /// Advance this IO vector backwards by `bytes` bytes.
> +    ///
> +    /// # Safety
> +    ///
> +    /// The IO vector must not be reverted to before its beginning.
> +    #[inline]
> +    pub unsafe fn revert(&mut self, bytes: usize) {
> +        // SAFETY: `self.iov` is a valid IO vector, and `bytes` is in bounds.
> +        unsafe { bindings::iov_iter_revert(self.as_raw(), bytes) };
> +    }
> +
> +    /// Write data to this IO vector.
> +    ///
> +    /// Returns the number of bytes that were written. If this is shorter than the provided slice,
> +    /// then no more bytes can be written.
> +    #[inline]
> +    pub fn copy_to_iter(&mut self, input: &[u8]) -> usize {
> +        // SAFETY: `input` is valid for `input.len()` bytes.
> +        unsafe { bindings::_copy_to_iter(input.as_ptr().cast(), input.len(), self.as_raw()) }
> +    }
> +
> +    /// Utility for implementing `read_iter` given the full contents of the file.
> +    ///
> +    /// The full contents of the file being read from is represented by `contents`. This call will
> +    /// write the appropriate sub-slice of `contents` and update the file position in `ppos` so
> +    /// that the file will appear to contain `contents` even if takes multiple reads to read the
> +    /// entire file.
> +    #[inline]
> +    pub fn simple_read_from_buffer(&mut self, ppos: &mut i64, contents: &[u8]) -> Result<usize> {
> +        if *ppos < 0 {
> +            return Err(EINVAL);
> +        }
> +        let Ok(pos) = usize::try_from(*ppos) else {
> +            return Ok(0);
> +        };
> +        if pos >= contents.len() {
> +            return Ok(0);
> +        }
> +
> +        // BOUNDS: We just checked that `pos < contents.len()` above.
> +        let num_written = self.copy_to_iter(&contents[pos..]);
> +
> +        // OVERFLOW: pos+num_written <= contents.len() <= isize::MAX <= i64::MAX
> +        *ppos = (pos + num_written) as i64;
> +
> +        Ok(num_written)
> +    }
> +}


Cheers
Christian

Powered by blists - more mailing lists