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

Danilo Krummrich posted 8 patches 3 months, 2 weeks ago
There is a newer version of this series
[PATCH v2 3/8] rust: uaccess: add UserSliceWriter::write_slice_partial()
Posted by Danilo Krummrich 3 months, 2 weeks ago
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> {
+        if offset < 0 {
+            return Err(EINVAL);
+        }
+
+        let Ok(offset) = usize::try_from(offset) else {
+            return Ok(0);
+        };
+
+        let end = offset
+            .checked_add(self.len())
+            .unwrap_or(data.len())
+            .min(data.len());
+
+        data.get(offset..end)
+            .map_or(Ok(0), |src| self.write_slice(src).map(|()| src.len()))
+    }
+
     /// Writes the provided Rust value to this userspace pointer.
     ///
     /// Fails with [`EFAULT`] if the write happens on a bad address, or if the write goes out of
-- 
2.51.0
Re: [PATCH v2 3/8] rust: uaccess: add UserSliceWriter::write_slice_partial()
Posted by Alice Ryhl 3 months, 2 weeks ago
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.

Alice
Re: [PATCH v2 3/8] rust: uaccess: add UserSliceWriter::write_slice_partial()
Posted by Danilo Krummrich 3 months, 2 weeks ago
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.

Any name suggestions?
Re: [PATCH v2 3/8] rust: uaccess: add UserSliceWriter::write_slice_partial()
Posted by Alice Ryhl 3 months, 2 weeks ago
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

Alice
Re: [PATCH v2 3/8] rust: uaccess: add UserSliceWriter::write_slice_partial()
Posted by Danilo Krummrich 3 months, 2 weeks ago
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.
Re: [PATCH v2 3/8] rust: uaccess: add UserSliceWriter::write_slice_partial()
Posted by Alice Ryhl 3 months, 2 weeks ago
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
Re: [PATCH v2 3/8] rust: uaccess: add UserSliceWriter::write_slice_partial()
Posted by Danilo Krummrich 3 months, 2 weeks ago
On 10/22/25 10:22 AM, Alice Ryhl wrote:
> 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.

If we want to preserve the name, then that's probably the best option.

However, given that write_slice() and read_slice() represent
copy_{to,from}_user(), it also seems reasonable to give it a name that matches
the UserSlice{Writer,Reader} naming scheme. Also because the implementation
utilizes UserSlice{Writer,Reader} rather than doing the corresponding FFI call.