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 09:42:44PM +0000, Timur Tabi wrote:
> On Mon, 2026-03-16 at 20:42 +0000, Alice Ryhl wrote:
> > 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.
> 
> Shouldn't the safety comment also say that `len` is valid for writes to self.ptr, not just reads for
> `ptr`?  I know there is a "if len > self.length" check.  Does that check obviate the need for a
> safety comment that references self.ptr?

You do not need the caller to do anything special about the userspace
part of the write. That's just safe because 'copy_to_user' either
succeeds safely or returns an error. Only reading from `ptr` is
dangerous here.

> Maybe I should rename `ptr`, to avoid confusion with self.ptr.  Would be okay to rename it to
> `from`, since it's copying from that kernel address?

Yes, that is a good suggestion.

> > 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.
> 
> Fair enough, but I'm struggling to come up with a safety comment that doesn't just repeat what's in
> the function comment:
> 
>     /// The caller must ensure that `ptr` is valid for reads of `len` bytes.
> 
> (and maybe also valid for writes to self.ptr).
> 
> It seems to me that we're just extending the safety conditions copy_to_user() directly to
> write_raw().  That is, the reason write_raw() is unsafe is because copy_to_user() is unsafe. 
> Therefore, shouldn't the safety comment for copy_to_user just be:
> 
> // SAFETY: Caller guarantees `ptr` is valid for `len` bytes (see this function's safety contract).

Yes, that's a good safety comment for copy_to_user(). I think most
commonly it is worded like this: "By this methods safety requirements,
the caller guarantees that `ptr` is valid for reading `len` bytes."

Alice