[PATCH] uaccess: rust: add strncpy_from_user

[PATCH] uaccess: rust: add strncpy_from_user

[Alice Ryhl]

This is needed for ioctls that operate on a user-provided string.

It is somewhat unfortunate that strncpy_from_user does not nul-terminate
the string when the end of `buf` is reached. This implies that we can't
return a &CStr from the function, since the buffer may not always be
nul-terminated.

That said, we could add more convenient helpers on top that add a NUL
byte in that case.

This method isn't defined on UserSliceReader because it complicates the
semantics. The UserSliceReader type also has its own maximum length, so
we would have to limit the read by that length too.

Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
 rust/kernel/uaccess.rs | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs
index 80a9782b1c6e98ed6eae308ade8551afa7adc188..1bd82045e81ea887008e30241bd6de27f096b639 100644
--- a/rust/kernel/uaccess.rs
+++ b/rust/kernel/uaccess.rs
@@ -369,3 +369,30 @@ pub fn write<T: AsBytes>(&mut self, value: &T) -> Result {
         Ok(())
     }
 }
+
+/// Reads a nul-terminated string into `buf` and returns the length.
+///
+/// Fails with [`EFAULT`] if the read happens on a bad address. If the end of `buf` is reached,
+/// then the buffer will not be nul-terminated.
+#[inline]
+pub fn strncpy_from_user(ptr: UserPtr, buf: &mut [u8]) -> Result<usize> {
+    // CAST: Slice lengths are guaranteed to be `<= isize::MAX`.
+    let len = buf.len() as isize;
+
+    // SAFETY: `buf` is valid for writing `buf.len()` bytes.
+    let res = unsafe {
+        bindings::strncpy_from_user(
+            buf.as_mut_ptr(),
+            ptr as *const u8,
+            len,
+        )
+    };
+
+    if res < 0 {
+        Err(Error::from_errno(res as i32))
+    } else {
+        #[cfg(CONFIG_RUST_OVERFLOW_CHECKS)]
+        assert!(res <= len);
+        Ok(res as usize)
+    }
+}

---
base-commit: 9c32cda43eb78f78c73aee4aa344b777714e259b
change-id: 20250424-strncpy-from-user-1f2d06b0cdde

Best regards,
-- 
Alice Ryhl <aliceryhl@google.com>

Re: [PATCH] uaccess: rust: add strncpy_from_user

[Boqun Feng]

On Thu, Apr 24, 2025 at 03:17:48PM +0000, Alice Ryhl wrote:
> This is needed for ioctls that operate on a user-provided string.
> 
> It is somewhat unfortunate that strncpy_from_user does not nul-terminate
> the string when the end of `buf` is reached. This implies that we can't
> return a &CStr from the function, since the buffer may not always be
> nul-terminated.
> 
> That said, we could add more convenient helpers on top that add a NUL
> byte in that case.
> 
> This method isn't defined on UserSliceReader because it complicates the
> semantics. The UserSliceReader type also has its own maximum length, so
> we would have to limit the read by that length too.
> 
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
>  rust/kernel/uaccess.rs | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs
> index 80a9782b1c6e98ed6eae308ade8551afa7adc188..1bd82045e81ea887008e30241bd6de27f096b639 100644
> --- a/rust/kernel/uaccess.rs
> +++ b/rust/kernel/uaccess.rs
> @@ -369,3 +369,30 @@ pub fn write<T: AsBytes>(&mut self, value: &T) -> Result {
>          Ok(())
>      }
>  }
> +
> +/// Reads a nul-terminated string into `buf` and returns the length.
> +///
> +/// Fails with [`EFAULT`] if the read happens on a bad address. If the end of `buf` is reached,
> +/// then the buffer will not be nul-terminated.
> +#[inline]
> +pub fn strncpy_from_user(ptr: UserPtr, buf: &mut [u8]) -> Result<usize> {

Sorry maybe there is an email I'm missing, but could you provide more
context of the usage?

First the function name is a bit weird, because the 'n' in "strncpy"
means the parameters should have an 'n' (i.e. length) in it, but there
is none in the Rust version. Also, we don't need to replicate the
semantics of C here, we could just do a strncpy_from_user(..., ..., len
- 1), where `len` is the len of the `buf`, and then we would always have
a nul-terminated string. But maybe I'm missing something from the usage
side, so this is not doable?

Regards,
Boqun

> +    // CAST: Slice lengths are guaranteed to be `<= isize::MAX`.
> +    let len = buf.len() as isize;
> +
> +    // SAFETY: `buf` is valid for writing `buf.len()` bytes.
> +    let res = unsafe {
> +        bindings::strncpy_from_user(
> +            buf.as_mut_ptr(),
> +            ptr as *const u8,
> +            len,
> +        )
> +    };
> +
> +    if res < 0 {
> +        Err(Error::from_errno(res as i32))
> +    } else {
> +        #[cfg(CONFIG_RUST_OVERFLOW_CHECKS)]
> +        assert!(res <= len);
> +        Ok(res as usize)
> +    }
> +}
> 
> ---
> base-commit: 9c32cda43eb78f78c73aee4aa344b777714e259b
> change-id: 20250424-strncpy-from-user-1f2d06b0cdde
> 
> Best regards,
> -- 
> Alice Ryhl <aliceryhl@google.com>
> 

Re: [PATCH] uaccess: rust: add strncpy_from_user

[Danilo Krummrich]

On Thu, Apr 24, 2025 at 03:17:48PM +0000, Alice Ryhl wrote:
> 
> +/// Reads a nul-terminated string into `buf` and returns the length.
> +///
> +/// Fails with [`EFAULT`] if the read happens on a bad address. If the end of `buf` is reached,
> +/// then the buffer will not be nul-terminated.
> +#[inline]
> +pub fn strncpy_from_user(ptr: UserPtr, buf: &mut [u8]) -> Result<usize> {

Should probably be named strcpy_from_user() instead.

> +    // CAST: Slice lengths are guaranteed to be `<= isize::MAX`.
> +    let len = buf.len() as isize;
> +
> +    // SAFETY: `buf` is valid for writing `buf.len()` bytes.
> +    let res = unsafe {
> +        bindings::strncpy_from_user(
> +            buf.as_mut_ptr(),
> +            ptr as *const u8,

kernel::ffi::c_char should always match u8, but should we use the FFI type
regardless?

Re: [PATCH] uaccess: rust: add strncpy_from_user

[Miguel Ojeda]

On Thu, Apr 24, 2025 at 5:18 PM Alice Ryhl <aliceryhl@google.com> wrote:
>
> +    if res < 0 {
> +        Err(Error::from_errno(res as i32))
> +    } else {
> +        #[cfg(CONFIG_RUST_OVERFLOW_CHECKS)]
> +        assert!(res <= len);
> +        Ok(res as usize)
> +    }

What about:

    if res < 0 {
        return Err(...);
    }

    overflow_assert!(res <= len);
    Ok(res as usize)

That follows a bit better what is usually done on the C side, in using
early returns (especially for error paths) and in avoiding local
`#ifdef`s.

Of course, we can leave this `overflow_assert!` to a different patch
later on with this code as an example use case, or a good first issue
etc. It also allows to document it etc. Happy to send it or create the
issue.

(I wrote that instead of `assert_overflow!` because it follows the
`{static,debug}_assert!` patterns, i.e. it changes more the "kind" of
assert rather than asserting a particular thing, like `_eq!` or
`_same_type!`).

Thanks!

Cheers,
Miguel

Re: [PATCH] uaccess: rust: add strncpy_from_user

[Alice Ryhl]

On Thu, Apr 24, 2025 at 08:57:13AM -0700, Boqun Feng wrote:
> On Thu, Apr 24, 2025 at 03:17:48PM +0000, Alice Ryhl wrote:
> > This is needed for ioctls that operate on a user-provided string.
> > 
> > It is somewhat unfortunate that strncpy_from_user does not nul-terminate
> > the string when the end of `buf` is reached. This implies that we can't
> > return a &CStr from the function, since the buffer may not always be
> > nul-terminated.
> > 
> > That said, we could add more convenient helpers on top that add a NUL
> > byte in that case.
> > 
> > This method isn't defined on UserSliceReader because it complicates the
> > semantics. The UserSliceReader type also has its own maximum length, so
> > we would have to limit the read by that length too.
> > 
> > Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> > ---
> >  rust/kernel/uaccess.rs | 27 +++++++++++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> > 
> > diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs
> > index 80a9782b1c6e98ed6eae308ade8551afa7adc188..1bd82045e81ea887008e30241bd6de27f096b639 100644
> > --- a/rust/kernel/uaccess.rs
> > +++ b/rust/kernel/uaccess.rs
> > @@ -369,3 +369,30 @@ pub fn write<T: AsBytes>(&mut self, value: &T) -> Result {
> >          Ok(())
> >      }
> >  }
> > +
> > +/// Reads a nul-terminated string into `buf` and returns the length.
> > +///
> > +/// Fails with [`EFAULT`] if the read happens on a bad address. If the end of `buf` is reached,
> > +/// then the buffer will not be nul-terminated.
> > +#[inline]
> > +pub fn strncpy_from_user(ptr: UserPtr, buf: &mut [u8]) -> Result<usize> {
> 
> Sorry maybe there is an email I'm missing, but could you provide more
> context of the usage?
> 
> First the function name is a bit weird, because the 'n' in "strncpy"
> means the parameters should have an 'n' (i.e. length) in it, but there
> is none in the Rust version.

There is a length! It's the length of `buf`. It's pretty normal that C
methods with a pointer and length become a Rust method with a slice.

The distinction between strcpy and strncpy in my eyes is that strcpy
reads until you find a NUL byte, whereas strncpy reads until you find a
NUL byte *or* you read a user-specified number of bytes. This method is
in the latter category.

> Also, we don't need to replicate the
> semantics of C here, we could just do a strncpy_from_user(..., ..., len
> - 1), where `len` is the len of the `buf`, and then we would always have
> a nul-terminated string. But maybe I'm missing something from the usage
> side, so this is not doable?

Well, I guess that probably does make sense. I'll update this to always
add a nul-terminator.

Alice

Re: [PATCH] uaccess: rust: add strncpy_from_user

[Alice Ryhl]

On Thu, Apr 24, 2025 at 06:32:08PM +0200, Danilo Krummrich wrote:
> On Thu, Apr 24, 2025 at 03:17:48PM +0000, Alice Ryhl wrote:
> > 
> > +/// Reads a nul-terminated string into `buf` and returns the length.
> > +///
> > +/// Fails with [`EFAULT`] if the read happens on a bad address. If the end of `buf` is reached,
> > +/// then the buffer will not be nul-terminated.
> > +#[inline]
> > +pub fn strncpy_from_user(ptr: UserPtr, buf: &mut [u8]) -> Result<usize> {
> 
> Should probably be named strcpy_from_user() instead.

See my reply to Boqun.

> > +    // CAST: Slice lengths are guaranteed to be `<= isize::MAX`.
> > +    let len = buf.len() as isize;
> > +
> > +    // SAFETY: `buf` is valid for writing `buf.len()` bytes.
> > +    let res = unsafe {
> > +        bindings::strncpy_from_user(
> > +            buf.as_mut_ptr(),
> > +            ptr as *const u8,
> 
> kernel::ffi::c_char should always match u8, but should we use the FFI type
> regardless?

Hmm. Should we? I don't mind changing it, but I guess this could be an
interesting discussion point.

Alice

Re: [PATCH] uaccess: rust: add strncpy_from_user

[Alice Ryhl]

On Thu, Apr 24, 2025 at 06:38:52PM +0200, Miguel Ojeda wrote:
> On Thu, Apr 24, 2025 at 5:18 PM Alice Ryhl <aliceryhl@google.com> wrote:
> >
> > +    if res < 0 {
> > +        Err(Error::from_errno(res as i32))
> > +    } else {
> > +        #[cfg(CONFIG_RUST_OVERFLOW_CHECKS)]
> > +        assert!(res <= len);
> > +        Ok(res as usize)
> > +    }
> 
> What about:
> 
>     if res < 0 {
>         return Err(...);
>     }
> 
>     overflow_assert!(res <= len);
>     Ok(res as usize)
> 
> That follows a bit better what is usually done on the C side, in using
> early returns (especially for error paths) and in avoiding local
> `#ifdef`s.

Sure, that looks good to me.

> Of course, we can leave this `overflow_assert!` to a different patch
> later on with this code as an example use case, or a good first issue
> etc. It also allows to document it etc. Happy to send it or create the
> issue.
> 
> (I wrote that instead of `assert_overflow!` because it follows the
> `{static,debug}_assert!` patterns, i.e. it changes more the "kind" of
> assert rather than asserting a particular thing, like `_eq!` or
> `_same_type!`).

Sounds like a good good-first-issue.

Alice

Re: [PATCH] uaccess: rust: add strncpy_from_user

[Boqun Feng]

On Fri, Apr 25, 2025 at 09:43:30AM +0000, Alice Ryhl wrote:
> On Thu, Apr 24, 2025 at 08:57:13AM -0700, Boqun Feng wrote:
> > On Thu, Apr 24, 2025 at 03:17:48PM +0000, Alice Ryhl wrote:
> > > This is needed for ioctls that operate on a user-provided string.
> > > 
> > > It is somewhat unfortunate that strncpy_from_user does not nul-terminate
> > > the string when the end of `buf` is reached. This implies that we can't
> > > return a &CStr from the function, since the buffer may not always be
> > > nul-terminated.
> > > 
> > > That said, we could add more convenient helpers on top that add a NUL
> > > byte in that case.
> > > 
> > > This method isn't defined on UserSliceReader because it complicates the
> > > semantics. The UserSliceReader type also has its own maximum length, so
> > > we would have to limit the read by that length too.
> > > 
> > > Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> > > ---
> > >  rust/kernel/uaccess.rs | 27 +++++++++++++++++++++++++++
> > >  1 file changed, 27 insertions(+)
> > > 
> > > diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs
> > > index 80a9782b1c6e98ed6eae308ade8551afa7adc188..1bd82045e81ea887008e30241bd6de27f096b639 100644
> > > --- a/rust/kernel/uaccess.rs
> > > +++ b/rust/kernel/uaccess.rs
> > > @@ -369,3 +369,30 @@ pub fn write<T: AsBytes>(&mut self, value: &T) -> Result {
> > >          Ok(())
> > >      }
> > >  }
> > > +
> > > +/// Reads a nul-terminated string into `buf` and returns the length.
> > > +///
> > > +/// Fails with [`EFAULT`] if the read happens on a bad address. If the end of `buf` is reached,
> > > +/// then the buffer will not be nul-terminated.
> > > +#[inline]
> > > +pub fn strncpy_from_user(ptr: UserPtr, buf: &mut [u8]) -> Result<usize> {
> > 
> > Sorry maybe there is an email I'm missing, but could you provide more
> > context of the usage?
> > 
> > First the function name is a bit weird, because the 'n' in "strncpy"
> > means the parameters should have an 'n' (i.e. length) in it, but there
> > is none in the Rust version.
> 
> There is a length! It's the length of `buf`. It's pretty normal that C
> methods with a pointer and length become a Rust method with a slice.
> 

That's exactly the point, no need to reuse a name from C if we have
something better.

> The distinction between strcpy and strncpy in my eyes is that strcpy
> reads until you find a NUL byte, whereas strncpy reads until you find a
> NUL byte *or* you read a user-specified number of bytes. This method is
> in the latter category.
> 

Then copy_from_user_until_nul()? Or cstrcpy_from_user()? We should have
a bit consistent naming on Rust side regardless how C names things IMO.

Regards,
Boqun

> > Also, we don't need to replicate the
> > semantics of C here, we could just do a strncpy_from_user(..., ..., len
> > - 1), where `len` is the len of the `buf`, and then we would always have
> > a nul-terminated string. But maybe I'm missing something from the usage
> > side, so this is not doable?
> 
> Well, I guess that probably does make sense. I'll update this to always
> add a nul-terminator.
> 
> Alice

Re: [PATCH] uaccess: rust: add strncpy_from_user

[Greg Kroah-Hartman]

On Fri, Apr 25, 2025 at 06:39:25AM -0700, Boqun Feng wrote:
> On Fri, Apr 25, 2025 at 09:43:30AM +0000, Alice Ryhl wrote:
> > On Thu, Apr 24, 2025 at 08:57:13AM -0700, Boqun Feng wrote:
> > > On Thu, Apr 24, 2025 at 03:17:48PM +0000, Alice Ryhl wrote:
> > > > This is needed for ioctls that operate on a user-provided string.
> > > > 
> > > > It is somewhat unfortunate that strncpy_from_user does not nul-terminate
> > > > the string when the end of `buf` is reached. This implies that we can't
> > > > return a &CStr from the function, since the buffer may not always be
> > > > nul-terminated.
> > > > 
> > > > That said, we could add more convenient helpers on top that add a NUL
> > > > byte in that case.
> > > > 
> > > > This method isn't defined on UserSliceReader because it complicates the
> > > > semantics. The UserSliceReader type also has its own maximum length, so
> > > > we would have to limit the read by that length too.
> > > > 
> > > > Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> > > > ---
> > > >  rust/kernel/uaccess.rs | 27 +++++++++++++++++++++++++++
> > > >  1 file changed, 27 insertions(+)
> > > > 
> > > > diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs
> > > > index 80a9782b1c6e98ed6eae308ade8551afa7adc188..1bd82045e81ea887008e30241bd6de27f096b639 100644
> > > > --- a/rust/kernel/uaccess.rs
> > > > +++ b/rust/kernel/uaccess.rs
> > > > @@ -369,3 +369,30 @@ pub fn write<T: AsBytes>(&mut self, value: &T) -> Result {
> > > >          Ok(())
> > > >      }
> > > >  }
> > > > +
> > > > +/// Reads a nul-terminated string into `buf` and returns the length.
> > > > +///
> > > > +/// Fails with [`EFAULT`] if the read happens on a bad address. If the end of `buf` is reached,
> > > > +/// then the buffer will not be nul-terminated.
> > > > +#[inline]
> > > > +pub fn strncpy_from_user(ptr: UserPtr, buf: &mut [u8]) -> Result<usize> {
> > > 
> > > Sorry maybe there is an email I'm missing, but could you provide more
> > > context of the usage?
> > > 
> > > First the function name is a bit weird, because the 'n' in "strncpy"
> > > means the parameters should have an 'n' (i.e. length) in it, but there
> > > is none in the Rust version.
> > 
> > There is a length! It's the length of `buf`. It's pretty normal that C
> > methods with a pointer and length become a Rust method with a slice.
> > 
> 
> That's exactly the point, no need to reuse a name from C if we have
> something better.

Up to point, us kernel developers are used to the C names, so keep it
close if at all possible, ESPECIALLY for just links/wrappers of C
functions like this one is.

> > The distinction between strcpy and strncpy in my eyes is that strcpy
> > reads until you find a NUL byte, whereas strncpy reads until you find a
> > NUL byte *or* you read a user-specified number of bytes. This method is
> > in the latter category.
> > 
> 
> Then copy_from_user_until_nul()? Or cstrcpy_from_user()? We should have
> a bit consistent naming on Rust side regardless how C names things IMO.

You need to specify a max length, otherwise that's just going to confuse
us all.  strncpy_from_user() is the function we are used to using for
copying up to N number of bytes from userspace where a 0 termination
stops the copy if N isn't reached.  So I vote highly for the original
name here please.

thanks,

greg k-h

Re: [PATCH] uaccess: rust: add strncpy_from_user

[Danilo Krummrich]

On Fri, Apr 25, 2025 at 09:44:06AM +0000, Alice Ryhl wrote:
> On Thu, Apr 24, 2025 at 06:32:08PM +0200, Danilo Krummrich wrote:
> > On Thu, Apr 24, 2025 at 03:17:48PM +0000, Alice Ryhl wrote:
> > > 
> > > +/// Reads a nul-terminated string into `buf` and returns the length.
> > > +///
> > > +/// Fails with [`EFAULT`] if the read happens on a bad address. If the end of `buf` is reached,
> > > +/// then the buffer will not be nul-terminated.
> > > +#[inline]
> > > +pub fn strncpy_from_user(ptr: UserPtr, buf: &mut [u8]) -> Result<usize> {
> > 
> > Should probably be named strcpy_from_user() instead.
> 
> See my reply to Boqun.
> 
> > > +    // CAST: Slice lengths are guaranteed to be `<= isize::MAX`.
> > > +    let len = buf.len() as isize;
> > > +
> > > +    // SAFETY: `buf` is valid for writing `buf.len()` bytes.
> > > +    let res = unsafe {
> > > +        bindings::strncpy_from_user(
> > > +            buf.as_mut_ptr(),
> > > +            ptr as *const u8,
> > 
> > kernel::ffi::c_char should always match u8, but should we use the FFI type
> > regardless?
> 
> Hmm. Should we? I don't mind changing it, but I guess this could be an
> interesting discussion point.

I think we should stick to the FFI type, even if we know the types are
guaranteed to match. But I also won't object with how it is.

Re: [PATCH] uaccess: rust: add strncpy_from_user

[Boqun Feng]

On Fri, Apr 25, 2025 at 03:52:16PM +0200, Greg Kroah-Hartman wrote:
> On Fri, Apr 25, 2025 at 06:39:25AM -0700, Boqun Feng wrote:
> > On Fri, Apr 25, 2025 at 09:43:30AM +0000, Alice Ryhl wrote:
> > > On Thu, Apr 24, 2025 at 08:57:13AM -0700, Boqun Feng wrote:
> > > > On Thu, Apr 24, 2025 at 03:17:48PM +0000, Alice Ryhl wrote:
> > > > > This is needed for ioctls that operate on a user-provided string.
> > > > > 
> > > > > It is somewhat unfortunate that strncpy_from_user does not nul-terminate
> > > > > the string when the end of `buf` is reached. This implies that we can't
> > > > > return a &CStr from the function, since the buffer may not always be
> > > > > nul-terminated.
> > > > > 
> > > > > That said, we could add more convenient helpers on top that add a NUL
> > > > > byte in that case.
> > > > > 
> > > > > This method isn't defined on UserSliceReader because it complicates the
> > > > > semantics. The UserSliceReader type also has its own maximum length, so
> > > > > we would have to limit the read by that length too.
> > > > > 
> > > > > Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> > > > > ---
> > > > >  rust/kernel/uaccess.rs | 27 +++++++++++++++++++++++++++
> > > > >  1 file changed, 27 insertions(+)
> > > > > 
> > > > > diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs
> > > > > index 80a9782b1c6e98ed6eae308ade8551afa7adc188..1bd82045e81ea887008e30241bd6de27f096b639 100644
> > > > > --- a/rust/kernel/uaccess.rs
> > > > > +++ b/rust/kernel/uaccess.rs
> > > > > @@ -369,3 +369,30 @@ pub fn write<T: AsBytes>(&mut self, value: &T) -> Result {
> > > > >          Ok(())
> > > > >      }
> > > > >  }
> > > > > +
> > > > > +/// Reads a nul-terminated string into `buf` and returns the length.
> > > > > +///
> > > > > +/// Fails with [`EFAULT`] if the read happens on a bad address. If the end of `buf` is reached,
> > > > > +/// then the buffer will not be nul-terminated.
> > > > > +#[inline]
> > > > > +pub fn strncpy_from_user(ptr: UserPtr, buf: &mut [u8]) -> Result<usize> {
> > > > 
> > > > Sorry maybe there is an email I'm missing, but could you provide more
> > > > context of the usage?
> > > > 
> > > > First the function name is a bit weird, because the 'n' in "strncpy"
> > > > means the parameters should have an 'n' (i.e. length) in it, but there
> > > > is none in the Rust version.
> > > 
> > > There is a length! It's the length of `buf`. It's pretty normal that C
> > > methods with a pointer and length become a Rust method with a slice.
> > > 
> > 
> > That's exactly the point, no need to reuse a name from C if we have
> > something better.
> 
> Up to point, us kernel developers are used to the C names, so keep it
> close if at all possible, ESPECIALLY for just links/wrappers of C
> functions like this one is.
> 

Well, see my other suggestion about always putting a NUL at the end.
Then it's going to be a different function than what strncpy() does.

And I also asked for the usage there, because IMO, there's no point of
replicating a strncpy() in Rust, we should design a better API, rather
than mimic what C does.

> > > The distinction between strcpy and strncpy in my eyes is that strcpy
> > > reads until you find a NUL byte, whereas strncpy reads until you find a
> > > NUL byte *or* you read a user-specified number of bytes. This method is
> > > in the latter category.
> > > 
> > 
> > Then copy_from_user_until_nul()? Or cstrcpy_from_user()? We should have
> > a bit consistent naming on Rust side regardless how C names things IMO.
> 
> You need to specify a max length, otherwise that's just going to confuse
> us all.  strncpy_from_user() is the function we are used to using for
> copying up to N number of bytes from userspace where a 0 termination
> stops the copy if N isn't reached.  So I vote highly for the original
> name here please.
> 

Have you read the Rust the function signature? There is no parameter for
the max length, the max length is implied in the `buf` slice. Plus we
should really consider what the usage is, for example, wouldn't it be
ideal that we provide a buffer that has an extra byte so that the
copy result is always NUL terminated? I randomly checked a few users of
C strncpy_from_user() (alloc_name() in mm/memfd.c, mtrr_write() in
arch/x86/kernel/cpu/mtrr/if.c), they all do the same: providing the
extra byte (i.e. buf size is > n). So it seems preferable to me that we
provide a function doing that instead of just replicating
strncpy_from_user() semantics here.

(You're the one that keeps telling us to focus on usages, and I think
that's a good perspective ;-))

Regards,
Boqun

> thanks,
> 
> greg k-h

Re: [PATCH] uaccess: rust: add strncpy_from_user

[Greg Kroah-Hartman]

On Fri, Apr 25, 2025 at 07:35:41AM -0700, Boqun Feng wrote:
> On Fri, Apr 25, 2025 at 03:52:16PM +0200, Greg Kroah-Hartman wrote:
> > On Fri, Apr 25, 2025 at 06:39:25AM -0700, Boqun Feng wrote:
> > > On Fri, Apr 25, 2025 at 09:43:30AM +0000, Alice Ryhl wrote:
> > > > On Thu, Apr 24, 2025 at 08:57:13AM -0700, Boqun Feng wrote:
> > > > > On Thu, Apr 24, 2025 at 03:17:48PM +0000, Alice Ryhl wrote:
> > > > > > This is needed for ioctls that operate on a user-provided string.
> > > > > > 
> > > > > > It is somewhat unfortunate that strncpy_from_user does not nul-terminate
> > > > > > the string when the end of `buf` is reached. This implies that we can't
> > > > > > return a &CStr from the function, since the buffer may not always be
> > > > > > nul-terminated.
> > > > > > 
> > > > > > That said, we could add more convenient helpers on top that add a NUL
> > > > > > byte in that case.
> > > > > > 
> > > > > > This method isn't defined on UserSliceReader because it complicates the
> > > > > > semantics. The UserSliceReader type also has its own maximum length, so
> > > > > > we would have to limit the read by that length too.
> > > > > > 
> > > > > > Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> > > > > > ---
> > > > > >  rust/kernel/uaccess.rs | 27 +++++++++++++++++++++++++++
> > > > > >  1 file changed, 27 insertions(+)
> > > > > > 
> > > > > > diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs
> > > > > > index 80a9782b1c6e98ed6eae308ade8551afa7adc188..1bd82045e81ea887008e30241bd6de27f096b639 100644
> > > > > > --- a/rust/kernel/uaccess.rs
> > > > > > +++ b/rust/kernel/uaccess.rs
> > > > > > @@ -369,3 +369,30 @@ pub fn write<T: AsBytes>(&mut self, value: &T) -> Result {
> > > > > >          Ok(())
> > > > > >      }
> > > > > >  }
> > > > > > +
> > > > > > +/// Reads a nul-terminated string into `buf` and returns the length.
> > > > > > +///
> > > > > > +/// Fails with [`EFAULT`] if the read happens on a bad address. If the end of `buf` is reached,
> > > > > > +/// then the buffer will not be nul-terminated.
> > > > > > +#[inline]
> > > > > > +pub fn strncpy_from_user(ptr: UserPtr, buf: &mut [u8]) -> Result<usize> {
> > > > > 
> > > > > Sorry maybe there is an email I'm missing, but could you provide more
> > > > > context of the usage?
> > > > > 
> > > > > First the function name is a bit weird, because the 'n' in "strncpy"
> > > > > means the parameters should have an 'n' (i.e. length) in it, but there
> > > > > is none in the Rust version.
> > > > 
> > > > There is a length! It's the length of `buf`. It's pretty normal that C
> > > > methods with a pointer and length become a Rust method with a slice.
> > > > 
> > > 
> > > That's exactly the point, no need to reuse a name from C if we have
> > > something better.
> > 
> > Up to point, us kernel developers are used to the C names, so keep it
> > close if at all possible, ESPECIALLY for just links/wrappers of C
> > functions like this one is.
> > 
> 
> Well, see my other suggestion about always putting a NUL at the end.
> Then it's going to be a different function than what strncpy() does.

Ah, I missed that, and yes, we should do that.

> And I also asked for the usage there, because IMO, there's no point of
> replicating a strncpy() in Rust, we should design a better API, rather
> than mimic what C does.

Fair enough.  But as this is going to be getting into the "let's make
string copies correct", please add Kees Cook to the discussion as he's
doing a lot of work here on the C side of this.

> > You need to specify a max length, otherwise that's just going to confuse
> > us all.  strncpy_from_user() is the function we are used to using for
> > copying up to N number of bytes from userspace where a 0 termination
> > stops the copy if N isn't reached.  So I vote highly for the original
> > name here please.
> > 
> 
> Have you read the Rust the function signature? There is no parameter for
> the max length, the max length is implied in the `buf` slice. Plus we
> should really consider what the usage is, for example, wouldn't it be
> ideal that we provide a buffer that has an extra byte so that the
> copy result is always NUL terminated? I randomly checked a few users of
> C strncpy_from_user() (alloc_name() in mm/memfd.c, mtrr_write() in
> arch/x86/kernel/cpu/mtrr/if.c), they all do the same: providing the
> extra byte (i.e. buf size is > n). So it seems preferable to me that we
> provide a function doing that instead of just replicating
> strncpy_from_user() semantics here.
> 
> (You're the one that keeps telling us to focus on usages, and I think
> that's a good perspective ;-))

Ok, fair enough, I had missed the "always put a NULL at the end" which
is a good idea.  And yeah, "implying" the size by the size of the buffer
passed in makes sense in one way, the function signature then does look
odd, I missed that.

So yes, a better name might be good, but again, let's try to sync with
the C side where we can.  We now have C string functions that have an
"implicit" length already in them that is discovered by the compiler at
build time, so following their model is good.

thanks,

greg k-h