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>

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,

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.

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.

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

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) }
	}

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

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.

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

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?

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>