[PATCH] rust: generalize userSliceReader to support any Vec

[PATCH] rust: generalize userSliceReader to support any Vec

[Filipe Xavier]

The UserSliceReader::read_all function is currently restricted to use
only Vec with the kmalloc allocator.
However, there is no reason for this limitation.
This patch generalizes the function to accept any Vec regardless of the
allocator used.

Link: https://github.com/Rust-for-Linux/linux/issues/1136
Signed-off-by: Filipe Xavier <felipeaggger@gmail.com>
---
 rust/kernel/uaccess.rs | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs
index cc044924867b89f6116c7f9ec216d8cea2b3f8d7..719b0a48ff5550acc19f2e607c0a09f818145def 100644
--- a/rust/kernel/uaccess.rs
+++ b/rust/kernel/uaccess.rs
@@ -5,7 +5,7 @@
 //! C header: [`include/linux/uaccess.h`](srctree/include/linux/uaccess.h)
 
 use crate::{
-    alloc::Flags,
+    alloc::{Allocator, Flags},
     bindings,
     error::Result,
     ffi::c_void,
@@ -127,7 +127,7 @@ pub fn new(ptr: UserPtr, length: usize) -> Self {
     /// Reads the entirety of the user slice, appending it to the end of the provided buffer.
     ///
     /// Fails with [`EFAULT`] if the read happens on a bad address.
-    pub fn read_all(self, buf: &mut KVec<u8>, flags: Flags) -> Result {
+    pub fn read_all<A: Allocator>(self, buf: &mut Vec<u8, A>, flags: Flags) -> Result {
         self.reader().read_all(buf, flags)
     }
 
@@ -281,7 +281,7 @@ pub fn read<T: FromBytes>(&mut self) -> Result<T> {
     /// Reads the entirety of the user slice, appending it to the end of the provided buffer.
     ///
     /// Fails with [`EFAULT`] if the read happens on a bad address.
-    pub fn read_all(mut self, buf: &mut KVec<u8>, flags: Flags) -> Result {
+    pub fn read_all<A: Allocator>(mut self, buf: &mut Vec<u8, A>, flags: Flags) -> Result {
         let len = self.length;
         buf.reserve(len, flags)?;
 

---
base-commit: 0c5928deada15a8d075516e6e0d9ee19011bb000
change-id: 20241222-gen-userslice-readall-alloc-8049c668e65e

Best regards,
-- 
Filipe Xavier <felipeaggger@gmail.com>

Re: [PATCH] rust: generalize userSliceReader to support any Vec

[Boqun Feng]

On Sun, Dec 22, 2024 at 01:37:02PM -0300, Filipe Xavier wrote:
> The UserSliceReader::read_all function is currently restricted to use
> only Vec with the kmalloc allocator.
> However, there is no reason for this limitation.
> This patch generalizes the function to accept any Vec regardless of the
> allocator used.
> 

Interesting idea, however...

> Link: https://github.com/Rust-for-Linux/linux/issues/1136
> Signed-off-by: Filipe Xavier <felipeaggger@gmail.com>
> ---
>  rust/kernel/uaccess.rs | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs
> index cc044924867b89f6116c7f9ec216d8cea2b3f8d7..719b0a48ff5550acc19f2e607c0a09f818145def 100644
> --- a/rust/kernel/uaccess.rs
> +++ b/rust/kernel/uaccess.rs
> @@ -5,7 +5,7 @@
>  //! C header: [`include/linux/uaccess.h`](srctree/include/linux/uaccess.h)
>  
>  use crate::{
> -    alloc::Flags,
> +    alloc::{Allocator, Flags},
>      bindings,
>      error::Result,
>      ffi::c_void,
> @@ -127,7 +127,7 @@ pub fn new(ptr: UserPtr, length: usize) -> Self {
>      /// Reads the entirety of the user slice, appending it to the end of the provided buffer.
>      ///
>      /// Fails with [`EFAULT`] if the read happens on a bad address.
> -    pub fn read_all(self, buf: &mut KVec<u8>, flags: Flags) -> Result {
> +    pub fn read_all<A: Allocator>(self, buf: &mut Vec<u8, A>, flags: Flags) -> Result {
>          self.reader().read_all(buf, flags)
>      }
>  
> @@ -281,7 +281,7 @@ pub fn read<T: FromBytes>(&mut self) -> Result<T> {
>      /// Reads the entirety of the user slice, appending it to the end of the provided buffer.
>      ///
>      /// Fails with [`EFAULT`] if the read happens on a bad address.
> -    pub fn read_all(mut self, buf: &mut KVec<u8>, flags: Flags) -> Result {
> +    pub fn read_all<A: Allocator>(mut self, buf: &mut Vec<u8, A>, flags: Flags) -> Result {

... what if the copy hit a page fault because the target memory (e.g. a
VVec) is not mapped? Would copy_from_user() handle this? Or would it
return a different error code than `EFAULT`?

Do you have a usage of usig this functionality?

Regards,
Boqun

>          let len = self.length;
>          buf.reserve(len, flags)?;
>  
> 
> ---
> base-commit: 0c5928deada15a8d075516e6e0d9ee19011bb000
> change-id: 20241222-gen-userslice-readall-alloc-8049c668e65e
> 
> Best regards,
> -- 
> Filipe Xavier <felipeaggger@gmail.com>
> 

Re: [PATCH] rust: generalize userSliceReader to support any Vec

[Alice Ryhl]

On 12/22/24 6:20 PM, Boqun Feng wrote:
> On Sun, Dec 22, 2024 at 01:37:02PM -0300, Filipe Xavier wrote:
>> The UserSliceReader::read_all function is currently restricted to use
>> only Vec with the kmalloc allocator.
>> However, there is no reason for this limitation.
>> This patch generalizes the function to accept any Vec regardless of the
>> allocator used.
>>
> 
> Interesting idea, however...
> 
>> Link: https://github.com/Rust-for-Linux/linux/issues/1136
>> Signed-off-by: Filipe Xavier <felipeaggger@gmail.com>
>> ---
>>   rust/kernel/uaccess.rs | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs
>> index cc044924867b89f6116c7f9ec216d8cea2b3f8d7..719b0a48ff5550acc19f2e607c0a09f818145def 100644
>> --- a/rust/kernel/uaccess.rs
>> +++ b/rust/kernel/uaccess.rs
>> @@ -5,7 +5,7 @@
>>   //! C header: [`include/linux/uaccess.h`](srctree/include/linux/uaccess.h)
>>   
>>   use crate::{
>> -    alloc::Flags,
>> +    alloc::{Allocator, Flags},
>>       bindings,
>>       error::Result,
>>       ffi::c_void,
>> @@ -127,7 +127,7 @@ pub fn new(ptr: UserPtr, length: usize) -> Self {
>>       /// Reads the entirety of the user slice, appending it to the end of the provided buffer.
>>       ///
>>       /// Fails with [`EFAULT`] if the read happens on a bad address.
>> -    pub fn read_all(self, buf: &mut KVec<u8>, flags: Flags) -> Result {
>> +    pub fn read_all<A: Allocator>(self, buf: &mut Vec<u8, A>, flags: Flags) -> Result {
>>           self.reader().read_all(buf, flags)
>>       }
>>   
>> @@ -281,7 +281,7 @@ pub fn read<T: FromBytes>(&mut self) -> Result<T> {
>>       /// Reads the entirety of the user slice, appending it to the end of the provided buffer.
>>       ///
>>       /// Fails with [`EFAULT`] if the read happens on a bad address.
>> -    pub fn read_all(mut self, buf: &mut KVec<u8>, flags: Flags) -> Result {
>> +    pub fn read_all<A: Allocator>(mut self, buf: &mut Vec<u8, A>, flags: Flags) -> Result {
> 
> ... what if the copy hit a page fault because the target memory (e.g. a
> VVec) is not mapped? Would copy_from_user() handle this? Or would it
> return a different error code than `EFAULT`?
How could the memory not be mapped? A vector lets us obtain a &[u8] to 
the memory, which isn't valid if it's not mapped.

Alice

Re: [PATCH] rust: generalize userSliceReader to support any Vec

[Boqun Feng]

On Mon, Dec 30, 2024 at 02:21:39PM +0100, Alice Ryhl wrote:
> On 12/22/24 6:20 PM, Boqun Feng wrote:
> > On Sun, Dec 22, 2024 at 01:37:02PM -0300, Filipe Xavier wrote:
> > > The UserSliceReader::read_all function is currently restricted to use
> > > only Vec with the kmalloc allocator.
> > > However, there is no reason for this limitation.
> > > This patch generalizes the function to accept any Vec regardless of the
> > > allocator used.
> > > 
> > 
> > Interesting idea, however...
> > 
> > > Link: https://github.com/Rust-for-Linux/linux/issues/1136
> > > Signed-off-by: Filipe Xavier <felipeaggger@gmail.com>
> > > ---
> > >   rust/kernel/uaccess.rs | 6 +++---
> > >   1 file changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs
> > > index cc044924867b89f6116c7f9ec216d8cea2b3f8d7..719b0a48ff5550acc19f2e607c0a09f818145def 100644
> > > --- a/rust/kernel/uaccess.rs
> > > +++ b/rust/kernel/uaccess.rs
> > > @@ -5,7 +5,7 @@
> > >   //! C header: [`include/linux/uaccess.h`](srctree/include/linux/uaccess.h)
> > >   use crate::{
> > > -    alloc::Flags,
> > > +    alloc::{Allocator, Flags},
> > >       bindings,
> > >       error::Result,
> > >       ffi::c_void,
> > > @@ -127,7 +127,7 @@ pub fn new(ptr: UserPtr, length: usize) -> Self {
> > >       /// Reads the entirety of the user slice, appending it to the end of the provided buffer.
> > >       ///
> > >       /// Fails with [`EFAULT`] if the read happens on a bad address.
> > > -    pub fn read_all(self, buf: &mut KVec<u8>, flags: Flags) -> Result {
> > > +    pub fn read_all<A: Allocator>(self, buf: &mut Vec<u8, A>, flags: Flags) -> Result {
> > >           self.reader().read_all(buf, flags)
> > >       }
> > > @@ -281,7 +281,7 @@ pub fn read<T: FromBytes>(&mut self) -> Result<T> {
> > >       /// Reads the entirety of the user slice, appending it to the end of the provided buffer.
> > >       ///
> > >       /// Fails with [`EFAULT`] if the read happens on a bad address.
> > > -    pub fn read_all(mut self, buf: &mut KVec<u8>, flags: Flags) -> Result {
> > > +    pub fn read_all<A: Allocator>(mut self, buf: &mut Vec<u8, A>, flags: Flags) -> Result {
> > 
> > ... what if the copy hit a page fault because the target memory (e.g. a
> > VVec) is not mapped? Would copy_from_user() handle this? Or would it
> > return a different error code than `EFAULT`?
> How could the memory not be mapped? A vector lets us obtain a &[u8] to the
> memory, which isn't valid if it's not mapped.
> 

I mistakenly thought that vmalloc()'d memory can be swapped out, which
is not the case. So you're right the memory must be mapped.

But the usage question still exists: do you have an usage case for
reading userspace memory into a `VVec`?

Regards,
Boqun

> Alice

Re: [PATCH] rust: generalize userSliceReader to support any Vec

[Alice Ryhl]

On Mon, Dec 30, 2024 at 5:23 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> On Mon, Dec 30, 2024 at 02:21:39PM +0100, Alice Ryhl wrote:
> > On 12/22/24 6:20 PM, Boqun Feng wrote:
> > > On Sun, Dec 22, 2024 at 01:37:02PM -0300, Filipe Xavier wrote:
> > > > The UserSliceReader::read_all function is currently restricted to use
> > > > only Vec with the kmalloc allocator.
> > > > However, there is no reason for this limitation.
> > > > This patch generalizes the function to accept any Vec regardless of the
> > > > allocator used.
> > > >
> > >
> > > Interesting idea, however...
> > >
> > > > Link: https://github.com/Rust-for-Linux/linux/issues/1136
> > > > Signed-off-by: Filipe Xavier <felipeaggger@gmail.com>
> > > > ---
> > > >   rust/kernel/uaccess.rs | 6 +++---
> > > >   1 file changed, 3 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs
> > > > index cc044924867b89f6116c7f9ec216d8cea2b3f8d7..719b0a48ff5550acc19f2e607c0a09f818145def 100644
> > > > --- a/rust/kernel/uaccess.rs
> > > > +++ b/rust/kernel/uaccess.rs
> > > > @@ -5,7 +5,7 @@
> > > >   //! C header: [`include/linux/uaccess.h`](srctree/include/linux/uaccess.h)
> > > >   use crate::{
> > > > -    alloc::Flags,
> > > > +    alloc::{Allocator, Flags},
> > > >       bindings,
> > > >       error::Result,
> > > >       ffi::c_void,
> > > > @@ -127,7 +127,7 @@ pub fn new(ptr: UserPtr, length: usize) -> Self {
> > > >       /// Reads the entirety of the user slice, appending it to the end of the provided buffer.
> > > >       ///
> > > >       /// Fails with [`EFAULT`] if the read happens on a bad address.
> > > > -    pub fn read_all(self, buf: &mut KVec<u8>, flags: Flags) -> Result {
> > > > +    pub fn read_all<A: Allocator>(self, buf: &mut Vec<u8, A>, flags: Flags) -> Result {
> > > >           self.reader().read_all(buf, flags)
> > > >       }
> > > > @@ -281,7 +281,7 @@ pub fn read<T: FromBytes>(&mut self) -> Result<T> {
> > > >       /// Reads the entirety of the user slice, appending it to the end of the provided buffer.
> > > >       ///
> > > >       /// Fails with [`EFAULT`] if the read happens on a bad address.
> > > > -    pub fn read_all(mut self, buf: &mut KVec<u8>, flags: Flags) -> Result {
> > > > +    pub fn read_all<A: Allocator>(mut self, buf: &mut Vec<u8, A>, flags: Flags) -> Result {
> > >
> > > ... what if the copy hit a page fault because the target memory (e.g. a
> > > VVec) is not mapped? Would copy_from_user() handle this? Or would it
> > > return a different error code than `EFAULT`?
> > How could the memory not be mapped? A vector lets us obtain a &[u8] to the
> > memory, which isn't valid if it's not mapped.
> >
>
> I mistakenly thought that vmalloc()'d memory can be swapped out, which
> is not the case. So you're right the memory must be mapped.
>
> But the usage question still exists: do you have an usage case for
> reading userspace memory into a `VVec`?

I believe there's a use-case for a KVVec in Binder to avoid maximum
sizes for a certain array.

Alice

Re: [PATCH] rust: generalize userSliceReader to support any Vec

[Boqun Feng]

On Mon, Jan 06, 2025 at 02:34:13PM +0100, Alice Ryhl wrote:
[...]
> > > > ... what if the copy hit a page fault because the target memory (e.g. a
> > > > VVec) is not mapped? Would copy_from_user() handle this? Or would it
> > > > return a different error code than `EFAULT`?
> > > How could the memory not be mapped? A vector lets us obtain a &[u8] to the
> > > memory, which isn't valid if it's not mapped.
> > >
> >
> > I mistakenly thought that vmalloc()'d memory can be swapped out, which
> > is not the case. So you're right the memory must be mapped.
> >
> > But the usage question still exists: do you have an usage case for
> > reading userspace memory into a `VVec`?
> 
> I believe there's a use-case for a KVVec in Binder to avoid maximum
> sizes for a certain array.
> 

I see, thank you. `KVVec` makes a lot of senses here. I would still
suggest putting the details about the potential usage in the commit
message. Besides, Filipe, could you Cc memory management people next
time? Thanks!

Regards,
Boqun

> Alice