public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH] rust: alloc: use `spare_capacity_mut` to reduce unsafe
@ 2025-03-17 11:42 Tamir Duberstein
  2025-03-17 11:50 ` Alice Ryhl
  2025-03-17 14:34 ` Benno Lossin
  0 siblings, 2 replies; 11+ messages in thread
From: Tamir Duberstein @ 2025-03-17 11:42 UTC (permalink / raw)
  To: Danilo Krummrich, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross
  Cc: rust-for-linux, linux-kernel, Tamir Duberstein

Use `spare_capacity_mut` in the implementation of `push` to reduce the
use of `unsafe`. Both methods were added in commit 2aac4cd7dae3 ("rust:
alloc: implement kernel `Vec` type").

Signed-off-by: Tamir Duberstein <tamird@gmail.com>
---
 rust/kernel/alloc/kvec.rs | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
index ae9d072741ce..d2bc3d02179e 100644
--- a/rust/kernel/alloc/kvec.rs
+++ b/rust/kernel/alloc/kvec.rs
@@ -285,15 +285,8 @@ pub fn spare_capacity_mut(&mut self) -> &mut [MaybeUninit<T>] {
     pub fn push(&mut self, v: T, flags: Flags) -> Result<(), AllocError> {
         self.reserve(1, flags)?;
 
-        // SAFETY:
-        // - `self.len` is smaller than `self.capacity` and hence, the resulting pointer is
-        //   guaranteed to be part of the same allocated object.
-        // - `self.len` can not overflow `isize`.
-        let ptr = unsafe { self.as_mut_ptr().add(self.len) };
-
-        // SAFETY:
-        // - `ptr` is properly aligned and valid for writes.
-        unsafe { core::ptr::write(ptr, v) };
+        // The call to `reserve` was successful so the spare capacity is at least 1.
+        self.spare_capacity_mut()[0].write(v);
 
         // SAFETY: We just initialised the first spare entry, so it is safe to increase the length
         // by 1. We also know that the new length is <= capacity because of the previous call to

---
base-commit: cf25bc61f8aecad9b0c45fe32697e35ea4b13378
change-id: 20250317-vec-push-use-spare-27484fd016a9

Best regards,
-- 
Tamir Duberstein <tamird@gmail.com>


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] rust: alloc: use `spare_capacity_mut` to reduce unsafe
  2025-03-17 11:42 [PATCH] rust: alloc: use `spare_capacity_mut` to reduce unsafe Tamir Duberstein
@ 2025-03-17 11:50 ` Alice Ryhl
  2025-03-17 14:34 ` Benno Lossin
  1 sibling, 0 replies; 11+ messages in thread
From: Alice Ryhl @ 2025-03-17 11:50 UTC (permalink / raw)
  To: Tamir Duberstein
  Cc: Danilo Krummrich, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg,
	Trevor Gross, rust-for-linux, linux-kernel

On Mon, Mar 17, 2025 at 07:42:28AM -0400, Tamir Duberstein wrote:
> Use `spare_capacity_mut` in the implementation of `push` to reduce the
> use of `unsafe`. Both methods were added in commit 2aac4cd7dae3 ("rust:
> alloc: implement kernel `Vec` type").
> 
> Signed-off-by: Tamir Duberstein <tamird@gmail.com>

Looks reasonable enough.

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] rust: alloc: use `spare_capacity_mut` to reduce unsafe
  2025-03-17 11:42 [PATCH] rust: alloc: use `spare_capacity_mut` to reduce unsafe Tamir Duberstein
  2025-03-17 11:50 ` Alice Ryhl
@ 2025-03-17 14:34 ` Benno Lossin
  2025-03-17 14:39   ` Tamir Duberstein
  1 sibling, 1 reply; 11+ messages in thread
From: Benno Lossin @ 2025-03-17 14:34 UTC (permalink / raw)
  To: Tamir Duberstein, Danilo Krummrich, Miguel Ojeda, Alex Gaynor,
	Boqun Feng, Gary Guo, Björn Roy Baron, Andreas Hindborg,
	Alice Ryhl, Trevor Gross
  Cc: rust-for-linux, linux-kernel

On Mon Mar 17, 2025 at 12:42 PM CET, Tamir Duberstein wrote:
> Use `spare_capacity_mut` in the implementation of `push` to reduce the
> use of `unsafe`. Both methods were added in commit 2aac4cd7dae3 ("rust:
> alloc: implement kernel `Vec` type").
>
> Signed-off-by: Tamir Duberstein <tamird@gmail.com>
> ---
>  rust/kernel/alloc/kvec.rs | 11 ++---------
>  1 file changed, 2 insertions(+), 9 deletions(-)
>
> diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
> index ae9d072741ce..d2bc3d02179e 100644
> --- a/rust/kernel/alloc/kvec.rs
> +++ b/rust/kernel/alloc/kvec.rs
> @@ -285,15 +285,8 @@ pub fn spare_capacity_mut(&mut self) -> &mut [MaybeUninit<T>] {
>      pub fn push(&mut self, v: T, flags: Flags) -> Result<(), AllocError> {
>          self.reserve(1, flags)?;
>  
> -        // SAFETY:
> -        // - `self.len` is smaller than `self.capacity` and hence, the resulting pointer is
> -        //   guaranteed to be part of the same allocated object.
> -        // - `self.len` can not overflow `isize`.
> -        let ptr = unsafe { self.as_mut_ptr().add(self.len) };
> -
> -        // SAFETY:
> -        // - `ptr` is properly aligned and valid for writes.
> -        unsafe { core::ptr::write(ptr, v) };
> +        // The call to `reserve` was successful so the spare capacity is at least 1.
> +        self.spare_capacity_mut()[0].write(v);

I think the code uses unsafe to avoid a bounds check, but I'm not 100%
sure. Danilo might remember more info.

---
Cheers,
Benno

>  
>          // SAFETY: We just initialised the first spare entry, so it is safe to increase the length
>          // by 1. We also know that the new length is <= capacity because of the previous call to
>
> ---
> base-commit: cf25bc61f8aecad9b0c45fe32697e35ea4b13378
> change-id: 20250317-vec-push-use-spare-27484fd016a9
>
> Best regards,



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] rust: alloc: use `spare_capacity_mut` to reduce unsafe
  2025-03-17 14:34 ` Benno Lossin
@ 2025-03-17 14:39   ` Tamir Duberstein
  2025-03-17 17:09     ` Danilo Krummrich
  0 siblings, 1 reply; 11+ messages in thread
From: Tamir Duberstein @ 2025-03-17 14:39 UTC (permalink / raw)
  To: Benno Lossin
  Cc: Danilo Krummrich, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Andreas Hindborg, Alice Ryhl, Trevor Gross,
	rust-for-linux, linux-kernel

On Mon, Mar 17, 2025 at 10:34 AM Benno Lossin <benno.lossin@proton.me> wrote:
>
> On Mon Mar 17, 2025 at 12:42 PM CET, Tamir Duberstein wrote:
> > Use `spare_capacity_mut` in the implementation of `push` to reduce the
> > use of `unsafe`. Both methods were added in commit 2aac4cd7dae3 ("rust:
> > alloc: implement kernel `Vec` type").
> >
> > Signed-off-by: Tamir Duberstein <tamird@gmail.com>
> > ---
> >  rust/kernel/alloc/kvec.rs | 11 ++---------
> >  1 file changed, 2 insertions(+), 9 deletions(-)
> >
> > diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
> > index ae9d072741ce..d2bc3d02179e 100644
> > --- a/rust/kernel/alloc/kvec.rs
> > +++ b/rust/kernel/alloc/kvec.rs
> > @@ -285,15 +285,8 @@ pub fn spare_capacity_mut(&mut self) -> &mut [MaybeUninit<T>] {
> >      pub fn push(&mut self, v: T, flags: Flags) -> Result<(), AllocError> {
> >          self.reserve(1, flags)?;
> >
> > -        // SAFETY:
> > -        // - `self.len` is smaller than `self.capacity` and hence, the resulting pointer is
> > -        //   guaranteed to be part of the same allocated object.
> > -        // - `self.len` can not overflow `isize`.
> > -        let ptr = unsafe { self.as_mut_ptr().add(self.len) };
> > -
> > -        // SAFETY:
> > -        // - `ptr` is properly aligned and valid for writes.
> > -        unsafe { core::ptr::write(ptr, v) };
> > +        // The call to `reserve` was successful so the spare capacity is at least 1.
> > +        self.spare_capacity_mut()[0].write(v);
>
> I think the code uses unsafe to avoid a bounds check, but I'm not 100%
> sure. Danilo might remember more info.

We could use `slice::get_unchecked_mut` here to retain the same
guarantee of no bounds check. That would still be one fewer unsafe
blocks.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] rust: alloc: use `spare_capacity_mut` to reduce unsafe
  2025-03-17 14:39   ` Tamir Duberstein
@ 2025-03-17 17:09     ` Danilo Krummrich
  2025-03-17 17:22       ` Benno Lossin
  0 siblings, 1 reply; 11+ messages in thread
From: Danilo Krummrich @ 2025-03-17 17:09 UTC (permalink / raw)
  To: Tamir Duberstein
  Cc: Benno Lossin, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Andreas Hindborg, Alice Ryhl, Trevor Gross,
	rust-for-linux, linux-kernel

On Mon, Mar 17, 2025 at 10:39:05AM -0400, Tamir Duberstein wrote:
> On Mon, Mar 17, 2025 at 10:34 AM Benno Lossin <benno.lossin@proton.me> wrote:
> >
> > On Mon Mar 17, 2025 at 12:42 PM CET, Tamir Duberstein wrote:
> > > Use `spare_capacity_mut` in the implementation of `push` to reduce the
> > > use of `unsafe`. Both methods were added in commit 2aac4cd7dae3 ("rust:
> > > alloc: implement kernel `Vec` type").
> > >
> > > Signed-off-by: Tamir Duberstein <tamird@gmail.com>
> > > ---
> > >  rust/kernel/alloc/kvec.rs | 11 ++---------
> > >  1 file changed, 2 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
> > > index ae9d072741ce..d2bc3d02179e 100644
> > > --- a/rust/kernel/alloc/kvec.rs
> > > +++ b/rust/kernel/alloc/kvec.rs
> > > @@ -285,15 +285,8 @@ pub fn spare_capacity_mut(&mut self) -> &mut [MaybeUninit<T>] {
> > >      pub fn push(&mut self, v: T, flags: Flags) -> Result<(), AllocError> {
> > >          self.reserve(1, flags)?;
> > >
> > > -        // SAFETY:
> > > -        // - `self.len` is smaller than `self.capacity` and hence, the resulting pointer is
> > > -        //   guaranteed to be part of the same allocated object.
> > > -        // - `self.len` can not overflow `isize`.
> > > -        let ptr = unsafe { self.as_mut_ptr().add(self.len) };
> > > -
> > > -        // SAFETY:
> > > -        // - `ptr` is properly aligned and valid for writes.
> > > -        unsafe { core::ptr::write(ptr, v) };
> > > +        // The call to `reserve` was successful so the spare capacity is at least 1.
> > > +        self.spare_capacity_mut()[0].write(v);
> >
> > I think the code uses unsafe to avoid a bounds check, but I'm not 100%
> > sure. Danilo might remember more info.

Yes, that was the justification to use unsafe calls instead.

(This may also justify keeping dec_len() unsafe, since otherwise it would
introduce an additional boundary check for pop().)

> 
> We could use `slice::get_unchecked_mut` here to retain the same
> guarantee of no bounds check. That would still be one fewer unsafe
> blocks.

Sounds reasonable.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] rust: alloc: use `spare_capacity_mut` to reduce unsafe
  2025-03-17 17:09     ` Danilo Krummrich
@ 2025-03-17 17:22       ` Benno Lossin
  2025-03-17 17:30         ` Danilo Krummrich
  0 siblings, 1 reply; 11+ messages in thread
From: Benno Lossin @ 2025-03-17 17:22 UTC (permalink / raw)
  To: Danilo Krummrich, Tamir Duberstein
  Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Andreas Hindborg, Alice Ryhl, Trevor Gross,
	rust-for-linux, linux-kernel

On Mon Mar 17, 2025 at 6:09 PM CET, Danilo Krummrich wrote:
> On Mon, Mar 17, 2025 at 10:39:05AM -0400, Tamir Duberstein wrote:
>> On Mon, Mar 17, 2025 at 10:34 AM Benno Lossin <benno.lossin@proton.me> wrote:
>> > On Mon Mar 17, 2025 at 12:42 PM CET, Tamir Duberstein wrote:
>> > > Use `spare_capacity_mut` in the implementation of `push` to reduce the
>> > > use of `unsafe`. Both methods were added in commit 2aac4cd7dae3 ("rust:
>> > > alloc: implement kernel `Vec` type").
>> > >
>> > > Signed-off-by: Tamir Duberstein <tamird@gmail.com>
>> > > ---
>> > >  rust/kernel/alloc/kvec.rs | 11 ++---------
>> > >  1 file changed, 2 insertions(+), 9 deletions(-)
>> > >
>> > > diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
>> > > index ae9d072741ce..d2bc3d02179e 100644
>> > > --- a/rust/kernel/alloc/kvec.rs
>> > > +++ b/rust/kernel/alloc/kvec.rs
>> > > @@ -285,15 +285,8 @@ pub fn spare_capacity_mut(&mut self) -> &mut [MaybeUninit<T>] {
>> > >      pub fn push(&mut self, v: T, flags: Flags) -> Result<(), AllocError> {
>> > >          self.reserve(1, flags)?;
>> > >
>> > > -        // SAFETY:
>> > > -        // - `self.len` is smaller than `self.capacity` and hence, the resulting pointer is
>> > > -        //   guaranteed to be part of the same allocated object.
>> > > -        // - `self.len` can not overflow `isize`.
>> > > -        let ptr = unsafe { self.as_mut_ptr().add(self.len) };
>> > > -
>> > > -        // SAFETY:
>> > > -        // - `ptr` is properly aligned and valid for writes.
>> > > -        unsafe { core::ptr::write(ptr, v) };
>> > > +        // The call to `reserve` was successful so the spare capacity is at least 1.
>> > > +        self.spare_capacity_mut()[0].write(v);
>> >
>> > I think the code uses unsafe to avoid a bounds check, but I'm not 100%
>> > sure. Danilo might remember more info.
>
> Yes, that was the justification to use unsafe calls instead.
>
> (This may also justify keeping dec_len() unsafe, since otherwise it would
> introduce an additional boundary check for pop().)

If we use saturating_sub then we don't need a bounds check (at least on
non-debug builds), right? 

>> We could use `slice::get_unchecked_mut` here to retain the same
>> guarantee of no bounds check. That would still be one fewer unsafe
>> blocks.
>
> Sounds reasonable.

+1


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] rust: alloc: use `spare_capacity_mut` to reduce unsafe
  2025-03-17 17:22       ` Benno Lossin
@ 2025-03-17 17:30         ` Danilo Krummrich
  2025-03-17 17:41           ` Benno Lossin
  0 siblings, 1 reply; 11+ messages in thread
From: Danilo Krummrich @ 2025-03-17 17:30 UTC (permalink / raw)
  To: Benno Lossin
  Cc: Tamir Duberstein, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Andreas Hindborg, Alice Ryhl, Trevor Gross,
	rust-for-linux, linux-kernel

On Mon, Mar 17, 2025 at 05:22:15PM +0000, Benno Lossin wrote:
> On Mon Mar 17, 2025 at 6:09 PM CET, Danilo Krummrich wrote:
> > On Mon, Mar 17, 2025 at 10:39:05AM -0400, Tamir Duberstein wrote:
> >> On Mon, Mar 17, 2025 at 10:34 AM Benno Lossin <benno.lossin@proton.me> wrote:
> >> > On Mon Mar 17, 2025 at 12:42 PM CET, Tamir Duberstein wrote:
> >> > > Use `spare_capacity_mut` in the implementation of `push` to reduce the
> >> > > use of `unsafe`. Both methods were added in commit 2aac4cd7dae3 ("rust:
> >> > > alloc: implement kernel `Vec` type").
> >> > >
> >> > > Signed-off-by: Tamir Duberstein <tamird@gmail.com>
> >> > > ---
> >> > >  rust/kernel/alloc/kvec.rs | 11 ++---------
> >> > >  1 file changed, 2 insertions(+), 9 deletions(-)
> >> > >
> >> > > diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
> >> > > index ae9d072741ce..d2bc3d02179e 100644
> >> > > --- a/rust/kernel/alloc/kvec.rs
> >> > > +++ b/rust/kernel/alloc/kvec.rs
> >> > > @@ -285,15 +285,8 @@ pub fn spare_capacity_mut(&mut self) -> &mut [MaybeUninit<T>] {
> >> > >      pub fn push(&mut self, v: T, flags: Flags) -> Result<(), AllocError> {
> >> > >          self.reserve(1, flags)?;
> >> > >
> >> > > -        // SAFETY:
> >> > > -        // - `self.len` is smaller than `self.capacity` and hence, the resulting pointer is
> >> > > -        //   guaranteed to be part of the same allocated object.
> >> > > -        // - `self.len` can not overflow `isize`.
> >> > > -        let ptr = unsafe { self.as_mut_ptr().add(self.len) };
> >> > > -
> >> > > -        // SAFETY:
> >> > > -        // - `ptr` is properly aligned and valid for writes.
> >> > > -        unsafe { core::ptr::write(ptr, v) };
> >> > > +        // The call to `reserve` was successful so the spare capacity is at least 1.
> >> > > +        self.spare_capacity_mut()[0].write(v);
> >> >
> >> > I think the code uses unsafe to avoid a bounds check, but I'm not 100%
> >> > sure. Danilo might remember more info.
> >
> > Yes, that was the justification to use unsafe calls instead.
> >
> > (This may also justify keeping dec_len() unsafe, since otherwise it would
> > introduce an additional boundary check for pop().)
> 
> If we use saturating_sub then we don't need a bounds check (at least on
> non-debug builds), right? 

	fn dec_len(&mut self, count: usize) -> &mut [T] {
	    self.len = self.len.saturating_sub(count);

	    // Potentially broken, since maybe `count > self.len`, hence need an
	    // additional check.
	    unsafe { slice::from_raw_parts_mut(self.as_mut_ptr().add(self.len), count) }
	}

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] rust: alloc: use `spare_capacity_mut` to reduce unsafe
  2025-03-17 17:30         ` Danilo Krummrich
@ 2025-03-17 17:41           ` Benno Lossin
  2025-03-17 17:55             ` Tamir Duberstein
  0 siblings, 1 reply; 11+ messages in thread
From: Benno Lossin @ 2025-03-17 17:41 UTC (permalink / raw)
  To: Danilo Krummrich
  Cc: Tamir Duberstein, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Andreas Hindborg, Alice Ryhl, Trevor Gross,
	rust-for-linux, linux-kernel

On Mon Mar 17, 2025 at 6:30 PM CET, Danilo Krummrich wrote:
> On Mon, Mar 17, 2025 at 05:22:15PM +0000, Benno Lossin wrote:
>> On Mon Mar 17, 2025 at 6:09 PM CET, Danilo Krummrich wrote:
>> > On Mon, Mar 17, 2025 at 10:39:05AM -0400, Tamir Duberstein wrote:
>> >> On Mon, Mar 17, 2025 at 10:34 AM Benno Lossin <benno.lossin@proton.me> wrote:
>> >> > On Mon Mar 17, 2025 at 12:42 PM CET, Tamir Duberstein wrote:
>> >> > > Use `spare_capacity_mut` in the implementation of `push` to reduce the
>> >> > > use of `unsafe`. Both methods were added in commit 2aac4cd7dae3 ("rust:
>> >> > > alloc: implement kernel `Vec` type").
>> >> > >
>> >> > > Signed-off-by: Tamir Duberstein <tamird@gmail.com>
>> >> > > ---
>> >> > >  rust/kernel/alloc/kvec.rs | 11 ++---------
>> >> > >  1 file changed, 2 insertions(+), 9 deletions(-)
>> >> > >
>> >> > > diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
>> >> > > index ae9d072741ce..d2bc3d02179e 100644
>> >> > > --- a/rust/kernel/alloc/kvec.rs
>> >> > > +++ b/rust/kernel/alloc/kvec.rs
>> >> > > @@ -285,15 +285,8 @@ pub fn spare_capacity_mut(&mut self) -> &mut [MaybeUninit<T>] {
>> >> > >      pub fn push(&mut self, v: T, flags: Flags) -> Result<(), AllocError> {
>> >> > >          self.reserve(1, flags)?;
>> >> > >
>> >> > > -        // SAFETY:
>> >> > > -        // - `self.len` is smaller than `self.capacity` and hence, the resulting pointer is
>> >> > > -        //   guaranteed to be part of the same allocated object.
>> >> > > -        // - `self.len` can not overflow `isize`.
>> >> > > -        let ptr = unsafe { self.as_mut_ptr().add(self.len) };
>> >> > > -
>> >> > > -        // SAFETY:
>> >> > > -        // - `ptr` is properly aligned and valid for writes.
>> >> > > -        unsafe { core::ptr::write(ptr, v) };
>> >> > > +        // The call to `reserve` was successful so the spare capacity is at least 1.
>> >> > > +        self.spare_capacity_mut()[0].write(v);
>> >> >
>> >> > I think the code uses unsafe to avoid a bounds check, but I'm not 100%
>> >> > sure. Danilo might remember more info.
>> >
>> > Yes, that was the justification to use unsafe calls instead.
>> >
>> > (This may also justify keeping dec_len() unsafe, since otherwise it would
>> > introduce an additional boundary check for pop().)
>> 
>> If we use saturating_sub then we don't need a bounds check (at least on
>> non-debug builds), right? 
>
> 	fn dec_len(&mut self, count: usize) -> &mut [T] {
> 	    self.len = self.len.saturating_sub(count);
>
> 	    // Potentially broken, since maybe `count > self.len`, hence need an
> 	    // additional check.
> 	    unsafe { slice::from_raw_parts_mut(self.as_mut_ptr().add(self.len), count) }
> 	}

Ah sorry, in my mental model the function returned `()`. Do we need the
return value?

---
Cheers,
Benno


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] rust: alloc: use `spare_capacity_mut` to reduce unsafe
  2025-03-17 17:41           ` Benno Lossin
@ 2025-03-17 17:55             ` Tamir Duberstein
  2025-03-18  9:22               ` Alice Ryhl
  0 siblings, 1 reply; 11+ messages in thread
From: Tamir Duberstein @ 2025-03-17 17:55 UTC (permalink / raw)
  To: Benno Lossin
  Cc: Danilo Krummrich, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Andreas Hindborg, Alice Ryhl, Trevor Gross,
	rust-for-linux, linux-kernel

On Mon, Mar 17, 2025 at 1:41 PM Benno Lossin <benno.lossin@proton.me> wrote:
>
> On Mon Mar 17, 2025 at 6:30 PM CET, Danilo Krummrich wrote:
> > On Mon, Mar 17, 2025 at 05:22:15PM +0000, Benno Lossin wrote:
> >> On Mon Mar 17, 2025 at 6:09 PM CET, Danilo Krummrich wrote:
> >> > On Mon, Mar 17, 2025 at 10:39:05AM -0400, Tamir Duberstein wrote:
> >> >> On Mon, Mar 17, 2025 at 10:34 AM Benno Lossin <benno.lossin@proton.me> wrote:
> >> >> > On Mon Mar 17, 2025 at 12:42 PM CET, Tamir Duberstein wrote:
> >> >> > > Use `spare_capacity_mut` in the implementation of `push` to reduce the
> >> >> > > use of `unsafe`. Both methods were added in commit 2aac4cd7dae3 ("rust:
> >> >> > > alloc: implement kernel `Vec` type").
> >> >> > >
> >> >> > > Signed-off-by: Tamir Duberstein <tamird@gmail.com>
> >> >> > > ---
> >> >> > >  rust/kernel/alloc/kvec.rs | 11 ++---------
> >> >> > >  1 file changed, 2 insertions(+), 9 deletions(-)
> >> >> > >
> >> >> > > diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
> >> >> > > index ae9d072741ce..d2bc3d02179e 100644
> >> >> > > --- a/rust/kernel/alloc/kvec.rs
> >> >> > > +++ b/rust/kernel/alloc/kvec.rs
> >> >> > > @@ -285,15 +285,8 @@ pub fn spare_capacity_mut(&mut self) -> &mut [MaybeUninit<T>] {
> >> >> > >      pub fn push(&mut self, v: T, flags: Flags) -> Result<(), AllocError> {
> >> >> > >          self.reserve(1, flags)?;
> >> >> > >
> >> >> > > -        // SAFETY:
> >> >> > > -        // - `self.len` is smaller than `self.capacity` and hence, the resulting pointer is
> >> >> > > -        //   guaranteed to be part of the same allocated object.
> >> >> > > -        // - `self.len` can not overflow `isize`.
> >> >> > > -        let ptr = unsafe { self.as_mut_ptr().add(self.len) };
> >> >> > > -
> >> >> > > -        // SAFETY:
> >> >> > > -        // - `ptr` is properly aligned and valid for writes.
> >> >> > > -        unsafe { core::ptr::write(ptr, v) };
> >> >> > > +        // The call to `reserve` was successful so the spare capacity is at least 1.
> >> >> > > +        self.spare_capacity_mut()[0].write(v);
> >> >> >
> >> >> > I think the code uses unsafe to avoid a bounds check, but I'm not 100%
> >> >> > sure. Danilo might remember more info.
> >> >
> >> > Yes, that was the justification to use unsafe calls instead.
> >> >
> >> > (This may also justify keeping dec_len() unsafe, since otherwise it would
> >> > introduce an additional boundary check for pop().)
> >>
> >> If we use saturating_sub then we don't need a bounds check (at least on
> >> non-debug builds), right?
> >
> >       fn dec_len(&mut self, count: usize) -> &mut [T] {
> >           self.len = self.len.saturating_sub(count);
> >
> >           // Potentially broken, since maybe `count > self.len`, hence need an
> >           // additional check.
> >           unsafe { slice::from_raw_parts_mut(self.as_mut_ptr().add(self.len), count) }
> >       }
>
> Ah sorry, in my mental model the function returned `()`. Do we need the
> return value?

The return value is the whole genesis of `dec_len`, we want to return
something to let the caller know they need to drop or copy the memory.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] rust: alloc: use `spare_capacity_mut` to reduce unsafe
  2025-03-17 17:55             ` Tamir Duberstein
@ 2025-03-18  9:22               ` Alice Ryhl
  2025-03-18 11:53                 ` Danilo Krummrich
  0 siblings, 1 reply; 11+ messages in thread
From: Alice Ryhl @ 2025-03-18  9:22 UTC (permalink / raw)
  To: Tamir Duberstein
  Cc: Benno Lossin, Danilo Krummrich, Miguel Ojeda, Alex Gaynor,
	Boqun Feng, Gary Guo, Björn Roy Baron, Andreas Hindborg,
	Trevor Gross, rust-for-linux, linux-kernel

On Mon, Mar 17, 2025 at 01:55:18PM -0400, Tamir Duberstein wrote:
> > >
> > >       fn dec_len(&mut self, count: usize) -> &mut [T] {
> > >           self.len = self.len.saturating_sub(count);
> > >
> > >           // Potentially broken, since maybe `count > self.len`, hence need an
> > >           // additional check.
> > >           unsafe { slice::from_raw_parts_mut(self.as_mut_ptr().add(self.len), count) }
> > >       }
> >
> > Ah sorry, in my mental model the function returned `()`. Do we need the
> > return value?
> 
> The return value is the whole genesis of `dec_len`, we want to return
> something to let the caller know they need to drop or copy the memory.

Hold on .. it returns &mut [T]. You're usually not allowed to take
ownership of or drop values behind a mutable reference.

Alice

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] rust: alloc: use `spare_capacity_mut` to reduce unsafe
  2025-03-18  9:22               ` Alice Ryhl
@ 2025-03-18 11:53                 ` Danilo Krummrich
  0 siblings, 0 replies; 11+ messages in thread
From: Danilo Krummrich @ 2025-03-18 11:53 UTC (permalink / raw)
  To: Alice Ryhl
  Cc: Tamir Duberstein, Benno Lossin, Miguel Ojeda, Alex Gaynor,
	Boqun Feng, Gary Guo, Björn Roy Baron, Andreas Hindborg,
	Trevor Gross, rust-for-linux, linux-kernel

On Tue, Mar 18, 2025 at 09:22:59AM +0000, Alice Ryhl wrote:
> On Mon, Mar 17, 2025 at 01:55:18PM -0400, Tamir Duberstein wrote:
> > > >
> > > >       fn dec_len(&mut self, count: usize) -> &mut [T] {
> > > >           self.len = self.len.saturating_sub(count);
> > > >
> > > >           // Potentially broken, since maybe `count > self.len`, hence need an
> > > >           // additional check.
> > > >           unsafe { slice::from_raw_parts_mut(self.as_mut_ptr().add(self.len), count) }
> > > >       }
> > >
> > > Ah sorry, in my mental model the function returned `()`. Do we need the
> > > return value?
> > 
> > The return value is the whole genesis of `dec_len`, we want to return
> > something to let the caller know they need to drop or copy the memory.
> 
> Hold on .. it returns &mut [T]. You're usually not allowed to take
> ownership of or drop values behind a mutable reference.

I think it should be fine. dec_len(), by returning this slice, indicates to the
caller what's left behind in case no action is taken.

Subsequent operations are unsafe anyways and can easily justify their validity
by saying that they only take over, what otherwise would have been left behind.

Do I miss anything?

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2025-03-18 11:53 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-03-17 11:42 [PATCH] rust: alloc: use `spare_capacity_mut` to reduce unsafe Tamir Duberstein
2025-03-17 11:50 ` Alice Ryhl
2025-03-17 14:34 ` Benno Lossin
2025-03-17 14:39   ` Tamir Duberstein
2025-03-17 17:09     ` Danilo Krummrich
2025-03-17 17:22       ` Benno Lossin
2025-03-17 17:30         ` Danilo Krummrich
2025-03-17 17:41           ` Benno Lossin
2025-03-17 17:55             ` Tamir Duberstein
2025-03-18  9:22               ` Alice Ryhl
2025-03-18 11:53                 ` Danilo Krummrich

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox