Re: [PATCH v2 3/8] rust: uaccess: add UserSliceWriter::write_slice_partial()

[Alice Ryhl <aliceryhl@google.com>]

On Tue, Oct 21, 2025 at 04:34:49PM +0200, Danilo Krummrich wrote:
> On Tue Oct 21, 2025 at 4:18 PM CEST, Alice Ryhl wrote:
> > On Tue, Oct 21, 2025 at 04:14:22PM +0200, Danilo Krummrich wrote:
> >> On Tue Oct 21, 2025 at 4:00 PM CEST, Alice Ryhl wrote:
> >> > On Tue, Oct 21, 2025 at 12:26:15AM +0200, Danilo Krummrich wrote:
> >> >> The existing write_slice() method is a wrapper around copy_to_user() and
> >> >> expects the user buffer to be larger than the source buffer.
> >> >> 
> >> >> However, userspace may split up reads in multiple partial operations
> >> >> providing an offset into the source buffer and a smaller user buffer.
> >> >> 
> >> >> In order to support this common case, provide a helper for partial
> >> >> writes.
> >> >> 
> >> >> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> >> >>  rust/kernel/uaccess.rs | 24 ++++++++++++++++++++++++
> >> >>  1 file changed, 24 insertions(+)
> >> >> 
> >> >> diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs
> >> >> index 2061a7e10c65..40d47e94b54f 100644
> >> >> --- a/rust/kernel/uaccess.rs
> >> >> +++ b/rust/kernel/uaccess.rs
> >> >> @@ -463,6 +463,30 @@ pub fn write_slice(&mut self, data: &[u8]) -> Result {
> >> >>          Ok(())
> >> >>      }
> >> >>  
> >> >> +    /// Writes raw data to this user pointer from a kernel buffer partially.
> >> >> +    ///
> >> >> +    /// This is the same as [`Self::write_slice`] but considers the given `offset` into `data` and
> >> >> +    /// truncates the write to the boundaries of `self` and `data`.
> >> >> +    ///
> >> >> +    /// On success, returns the number of bytes written.
> >> >> +    pub fn write_slice_partial(&mut self, data: &[u8], offset: file::Offset) -> Result<usize> {
> >> >
> >> > I think for the current function signature, it's kind of weird to take a
> >> > file::Offset parameter
> >> >
> >> > On one hand, it is described like a generic function for writing a
> >> > partial slice, and if that's what it is, then I would argue it should
> >> > take usize because it's an offset into the slice.
> >> >
> >> > On another hand, I think what you're actually trying to do is implement
> >> > the simple_[read_from|write_to]_buffer utilities for user slices, but
> >> > it's only a "partial" version of those utilities. The full utility takes
> >> > a `&mut loff_t` so that it can also perform the required modification to
> >> > the offset.
> >> 
> >> Originally, it was intended to be the latter. And, in fact, earlier code (that
> >> did not git the mailing list) had a &mut file::Offset argument (was &mut i64
> >> back then).
> >> 
> >> However, for the version I sent to the list I chose the former because I
> >> considered it to be more flexible.
> >> 
> >> Now, in v2, it's indeed a bit mixed up. I think what we should do is to have
> >> both
> >> 
> >> 	fn write_slice_partial(&mut self, data: &[u8], offset: usize) -> Result<usize>
> >> 
> >> and
> >> 
> >> 	fn write_slice_???(&mut self, data: &[u8], offset: &mut file::Offset) -> Result<usize>
> >> 
> >> which can forward to write_slice_partial() and update the buffer.
> >
> > SGTM.
> >
> >> Any name suggestions?
> >
> > I would suggest keeping the name of the equivalent C method:
> > simple_read_from_buffer/simple_write_to_buffer
> 
> Hm..that's an option, but UserSliceWriter corresponds to
> simple_read_from_buffer() and UserSliceReader corresponds to
> simple_write_to_buffer().
> 
> I think having UserSliceWriter::simple_read_from_buffer() while we have
> UserSliceWriter::write_slice() is confusing. But swapping the semantics of
> simple_read_from_buffer() and simple_write_to_buffer() is even more confusing.
> 
> So, I think using the existing names is not a great fit.
> 
> Maybe something like write_file_slice() or write_slice_file()? The former could
> be read as "slice of files" which would be misleading though.

It's tricky. Perhaps if you make them standalone functions, then using
the simple_read_from_buffer naming is less confusing? Then it's just
kernel::uaccess::simple_read_from_buffer() and it takes a
UserSliceWriter.

Alice