Re: [PATCH v9 2/6] rust: uaccess: add write_dma() for copying from DMA buffers to userspace

[Alice Ryhl <aliceryhl@google.com>]

On Mon, Mar 16, 2026 at 12:57:32AM -0500, Timur Tabi wrote:
> Add UserSliceWriter::write_dma() to copy data from a CoherentAllocation<u8>
> to userspace. This provides a safe interface for copying DMA buffer
> contents to userspace without requiring callers to work with raw pointers.
> 
> Because write_dma() and write_slice() have common code, factor that code
> out into a helper function, write_raw().
> 
> The method handles bounds checking and offset calculation internally,
> wrapping the unsafe copy_to_user() call.
> 
> Signed-off-by: Timur Tabi <ttabi@nvidia.com>
> Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>

> +    /// # Safety
> +    ///
> +    /// The caller must ensure that `ptr` points to a valid slice of `len` bytes (i.e., it is
> +    /// valid for reads of `len` bytes).

I don't know how I feel about this 'valid slice' thing. The memory might
be dma memory after all ... just say it's valid for reads of `len`
bytes.

After all, in below SAFETY: comments, you also argue that it's valid for
reads, not that it points at a valid slice.

> +    unsafe fn write_raw(&mut self, ptr: *const u8, len: usize) -> Result {
>          if len > self.length {
>              return Err(EFAULT);
>          }
> -        // SAFETY: `data_ptr` points into an immutable slice of length `len`, so we may read
> -        // that many bytes from it.
> -        let res = unsafe { bindings::copy_to_user(self.ptr.as_mut_ptr(), data_ptr, len) };
> +        // SAFETY:
> +        // - `self.ptr` is a userspace pointer, and `len <= self.length` is checked above to
> +        //   ensure we don't exceed the caller-specified bounds.
> +        // - `ptr` is valid for reading `len` bytes as required by this function's safety contract.
> +        // - `copy_to_user` validates the userspace address at runtime and returns non-zero on
> +        //   failure (e.g., bad address or unmapped memory).
> +        let res =
> +            unsafe { bindings::copy_to_user(self.ptr.as_mut_ptr(), ptr.cast::<c_void>(), len) };

Points 1 and 3 in this bulleted list do not seem to address any actual
safety requirements of `copy_to_user`. Yes, the function validates
userspace addresses at runtime, and this is part of why its
implementation is safe, but it's not a precondition on the caller.
There's no way to call `copy_to_user` where it would not validate the
userspace address at runtime.

> +    /// Writes raw data to this user pointer from a DMA coherent allocation.
> +    ///
> +    /// # Arguments
> +    ///
> +    /// * `data` - The DMA coherent allocation to copy from.
> +    /// * `offset` - The byte offset into `data` to start copying from.
> +    /// * `count` - The number of bytes to copy.

This is not the usual Rust style for documentation. We would generally
just write it in words:

	Copies `count` bytes from `alloc` starting from `offset` into
	this userspace slice.

Yes, argument lists are *occassionally* used, but it's rare.

> +    /// # Errors
> +    ///
> +    /// Returns [`EOVERFLOW`] if `offset + count` overflows.
> +    /// Returns [`ERANGE`] if `offset + count` exceeds the size of `data`, or `count` exceeds
> +    ///     the size of the user-space buffer.
> +    /// Returns [`EFAULT`] if the write happens on a bad address, or if the write goes out of
> +    ///     bounds of this [`UserSliceWriter`].

Using a list to format the possible errors seems sensible to me. But I
don't think you intended for this to be rendered like this in the final
docs:

	Returns `EOVERFLOW` if `offset + count` overflows. Returns
	`ERANGE` if `offset + count` exceeds the size of `data`, or
	`count` exceeds the size of the user-space buffer. Returns
	`EFAULT` if the write happens on a bad address, or if the
	write goes out of bounds of this `UserSliceWriter`.

Please use a markdown list to format this, and check the generated html
docs.

Alice