[PATCH 1/2] rust: alloc: extend safety requirements of Vec::set_len()

[PATCH 1/2] rust: alloc: extend safety requirements of Vec::set_len()

[Danilo Krummrich]

Extend the safety requirements of set_len() to consider the case when
the new length is smaller than the old length.

Fixes: 2aac4cd7dae3 ("rust: alloc: implement kernel `Vec` type")
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/kernel/alloc/kvec.rs | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
index ae9d072741ce..8540d9e2b717 100644
--- a/rust/kernel/alloc/kvec.rs
+++ b/rust/kernel/alloc/kvec.rs
@@ -189,7 +189,9 @@ pub fn len(&self) -> usize {
     ///
     /// - `new_len` must be less than or equal to [`Self::capacity`].
     /// - If `new_len` is greater than `self.len`, all elements within the interval
-    ///   [`self.len`,`new_len`) must be initialized.
+    ///   [`self.len`,`new_len`) must be initialized,
+    /// - if `new_len` is smaller than `self.len`, all elements within the interval
+    ///   [`new_len`, `self.len`) must either be dropped or copied and taken ownership of.
     #[inline]
     pub unsafe fn set_len(&mut self, new_len: usize) {
         debug_assert!(new_len <= self.capacity());

base-commit: 80e54e84911a923c40d7bee33a34c1b4be148d7a
-- 
2.48.1

[PATCH 2/2] rust: alloc: add missing invariant in Vec::set_len()

[Danilo Krummrich]

When setting a new length, we have to justify that the set length
represents the exact number of elements stored in the vector.

Fixes: 2aac4cd7dae3 ("rust: alloc: implement kernel `Vec` type")
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/kernel/alloc/kvec.rs | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
index 8540d9e2b717..0a4681ad4ce9 100644
--- a/rust/kernel/alloc/kvec.rs
+++ b/rust/kernel/alloc/kvec.rs
@@ -195,6 +195,9 @@ pub fn len(&self) -> usize {
     #[inline]
     pub unsafe fn set_len(&mut self, new_len: usize) {
         debug_assert!(new_len <= self.capacity());
+
+        // INVARIANT: By the safety requirements of this method `new_len` represents the exact
+        // number of elements stored within `self`.
         self.len = new_len;
     }
 
-- 
2.48.1

Re: [PATCH 2/2] rust: alloc: add missing invariant in Vec::set_len()

[Danilo Krummrich]

On Sat, Mar 15, 2025 at 04:43:02PM +0100, Danilo Krummrich wrote:
> When setting a new length, we have to justify that the set length
> represents the exact number of elements stored in the vector.
> 
> Fixes: 2aac4cd7dae3 ("rust: alloc: implement kernel `Vec` type")
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>

[1] probably justifies

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

[1] https://lore.kernel.org/rust-for-linux/20250311-iov-iter-v1-4-f6c9134ea824@google.com/

> ---
>  rust/kernel/alloc/kvec.rs | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
> index 8540d9e2b717..0a4681ad4ce9 100644
> --- a/rust/kernel/alloc/kvec.rs
> +++ b/rust/kernel/alloc/kvec.rs
> @@ -195,6 +195,9 @@ pub fn len(&self) -> usize {
>      #[inline]
>      pub unsafe fn set_len(&mut self, new_len: usize) {
>          debug_assert!(new_len <= self.capacity());
> +
> +        // INVARIANT: By the safety requirements of this method `new_len` represents the exact
> +        // number of elements stored within `self`.
>          self.len = new_len;
>      }
>  
> -- 
> 2.48.1
> 

Re: [PATCH 2/2] rust: alloc: add missing invariant in Vec::set_len()

[Benno Lossin]

On Sat Mar 15, 2025 at 4:43 PM CET, Danilo Krummrich wrote:
> When setting a new length, we have to justify that the set length
> represents the exact number of elements stored in the vector.
>
> Fixes: 2aac4cd7dae3 ("rust: alloc: implement kernel `Vec` type")
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>

Reviewed-by: Benno Lossin <benno.lossin@proton.me>

---
Cheers,
Benno

> ---
>  rust/kernel/alloc/kvec.rs | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
> index 8540d9e2b717..0a4681ad4ce9 100644
> --- a/rust/kernel/alloc/kvec.rs
> +++ b/rust/kernel/alloc/kvec.rs
> @@ -195,6 +195,9 @@ pub fn len(&self) -> usize {
>      #[inline]
>      pub unsafe fn set_len(&mut self, new_len: usize) {
>          debug_assert!(new_len <= self.capacity());
> +
> +        // INVARIANT: By the safety requirements of this method `new_len` represents the exact
> +        // number of elements stored within `self`.
>          self.len = new_len;
>      }
>  

Re: [PATCH 2/2] rust: alloc: add missing invariant in Vec::set_len()

[Danilo Krummrich]

On 3/15/25 4:43 PM, Danilo Krummrich wrote:
> When setting a new length, we have to justify that the set length
> represents the exact number of elements stored in the vector.
> 
> Fixes: 2aac4cd7dae3 ("rust: alloc: implement kernel `Vec` type")
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>

Applied to rust/alloc-next, thanks!

Re: [PATCH 1/2] rust: alloc: extend safety requirements of Vec::set_len()

[Tamir Duberstein]

On Sat, Mar 15, 2025 at 11:45 AM Danilo Krummrich <dakr@kernel.org> wrote:
>
> Extend the safety requirements of set_len() to consider the case when
> the new length is smaller than the old length.
>
> Fixes: 2aac4cd7dae3 ("rust: alloc: implement kernel `Vec` type")
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---
>  rust/kernel/alloc/kvec.rs | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
> index ae9d072741ce..8540d9e2b717 100644
> --- a/rust/kernel/alloc/kvec.rs
> +++ b/rust/kernel/alloc/kvec.rs
> @@ -189,7 +189,9 @@ pub fn len(&self) -> usize {
>      ///
>      /// - `new_len` must be less than or equal to [`Self::capacity`].
>      /// - If `new_len` is greater than `self.len`, all elements within the interval
> -    ///   [`self.len`,`new_len`) must be initialized.
> +    ///   [`self.len`,`new_len`) must be initialized,

Why a comma here? There's a period on the previous list item, and a
period seems more appropriate before the next item.

> +    /// - if `new_len` is smaller than `self.len`, all elements within the interval
> +    ///   [`new_len`, `self.len`) must either be dropped or copied and taken ownership of.

Nit: I'd argue that dropping is a special case of taking ownership,
and that such phrasing would be clearer.

>      #[inline]
>      pub unsafe fn set_len(&mut self, new_len: usize) {
>          debug_assert!(new_len <= self.capacity());
>
> base-commit: 80e54e84911a923c40d7bee33a34c1b4be148d7a
> --
> 2.48.1
>
>

Minor comments.

Reviewed-by: Tamir Duberstein <tamird@gmail.com>

It seems that the standard library also lacks this language, it would
be good to send them a PR.

Re: [PATCH 1/2] rust: alloc: extend safety requirements of Vec::set_len()

[Benno Lossin]

On Sat Mar 15, 2025 at 4:43 PM CET, Danilo Krummrich wrote:
> Extend the safety requirements of set_len() to consider the case when
> the new length is smaller than the old length.
>
> Fixes: 2aac4cd7dae3 ("rust: alloc: implement kernel `Vec` type")
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---
>  rust/kernel/alloc/kvec.rs | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
> index ae9d072741ce..8540d9e2b717 100644
> --- a/rust/kernel/alloc/kvec.rs
> +++ b/rust/kernel/alloc/kvec.rs
> @@ -189,7 +189,9 @@ pub fn len(&self) -> usize {
>      ///
>      /// - `new_len` must be less than or equal to [`Self::capacity`].
>      /// - If `new_len` is greater than `self.len`, all elements within the interval
> -    ///   [`self.len`,`new_len`) must be initialized.
> +    ///   [`self.len`,`new_len`) must be initialized,
> +    /// - if `new_len` is smaller than `self.len`, all elements within the interval
> +    ///   [`new_len`, `self.len`) must either be dropped or copied and taken ownership of.

What is meant by "copied"? I agree with Tamir, this can probably just be
"taken ownership of".

With that change:

Reviewed-by: Benno Lossin <benno.lossin@proton.me>

---
Cheers,
Benno

>      #[inline]
>      pub unsafe fn set_len(&mut self, new_len: usize) {
>          debug_assert!(new_len <= self.capacity());
>
> base-commit: 80e54e84911a923c40d7bee33a34c1b4be148d7a

Re: [PATCH 1/2] rust: alloc: extend safety requirements of Vec::set_len()

[Danilo Krummrich]

On Sat, Mar 15, 2025 at 05:44:29PM +0000, Benno Lossin wrote:
> On Sat Mar 15, 2025 at 4:43 PM CET, Danilo Krummrich wrote:
> > Extend the safety requirements of set_len() to consider the case when
> > the new length is smaller than the old length.
> >
> > Fixes: 2aac4cd7dae3 ("rust: alloc: implement kernel `Vec` type")
> > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> > ---
> >  rust/kernel/alloc/kvec.rs | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
> > index ae9d072741ce..8540d9e2b717 100644
> > --- a/rust/kernel/alloc/kvec.rs
> > +++ b/rust/kernel/alloc/kvec.rs
> > @@ -189,7 +189,9 @@ pub fn len(&self) -> usize {
> >      ///
> >      /// - `new_len` must be less than or equal to [`Self::capacity`].
> >      /// - If `new_len` is greater than `self.len`, all elements within the interval
> > -    ///   [`self.len`,`new_len`) must be initialized.
> > +    ///   [`self.len`,`new_len`) must be initialized,
> > +    /// - if `new_len` is smaller than `self.len`, all elements within the interval
> > +    ///   [`new_len`, `self.len`) must either be dropped or copied and taken ownership of.
> 
> What is meant by "copied"? I agree with Tamir, this can probably just be
> "taken ownership of".

If not dropped in place, the only way to take ownership of the object is to copy
the corresponding memory, i.e. the caller can not take ownership of the object
at its current memory location, since this memory is owned by the vector.

AFAIU, when we speak of ownership, we mean ownership over a value or object, but
not ownership over the underlying memory.

Hence, I think it's important to mention that it needs to be either dropped in
place or copied in order to take ownership.

Even if we agree that "drop in place" is considered as "take ownership", we need
to differentiate between the special case of taking ownership where its dropped
in place and the requirement to copy the value and then take ownership.

> 
> With that change:
> 
> Reviewed-by: Benno Lossin <benno.lossin@proton.me>
> 
> ---
> Cheers,
> Benno
> 
> >      #[inline]
> >      pub unsafe fn set_len(&mut self, new_len: usize) {
> >          debug_assert!(new_len <= self.capacity());
> >
> > base-commit: 80e54e84911a923c40d7bee33a34c1b4be148d7a
> 
> 

Re: [PATCH 1/2] rust: alloc: extend safety requirements of Vec::set_len()

[Tamir Duberstein]

On Sat, Mar 15, 2025 at 2:36 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Sat, Mar 15, 2025 at 05:44:29PM +0000, Benno Lossin wrote:
> >
> > What is meant by "copied"? I agree with Tamir, this can probably just be
> > "taken ownership of".
>
> If not dropped in place, the only way to take ownership of the object is to copy
> the corresponding memory, i.e. the caller can not take ownership of the object
> at its current memory location, since this memory is owned by the vector.

I think this is the point I was trying to make: the language semantics
are such that "taking ownership of" is equivalent to "become
responsible for the dropping of", so it doesn't make sense to write
that here, as though this requirement is specific to the semantics of
this function.

> AFAIU, when we speak of ownership, we mean ownership over a value or object, but
> not ownership over the underlying memory.
>
> Hence, I think it's important to mention that it needs to be either dropped in
> place or copied in order to take ownership.
>
> Even if we agree that "drop in place" is considered as "take ownership", we need
> to differentiate between the special case of taking ownership where its dropped
> in place and the requirement to copy the value and then take ownership.

Perhaps:

> The caller must ensure the destructors of [`new_len`, `self.len`) are run exactly once.

On Sat, Mar 15, 2025 at 12:06 PM Tamir Duberstein <tamird@gmail.com> wrote:
>
> On Sat, Mar 15, 2025 at 11:45 AM Danilo Krummrich <dakr@kernel.org> wrote:
> >
> > diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
> > index ae9d072741ce..8540d9e2b717 100644
> > --- a/rust/kernel/alloc/kvec.rs
> > +++ b/rust/kernel/alloc/kvec.rs
> > @@ -189,7 +189,9 @@ pub fn len(&self) -> usize {
> >      ///
> >      /// - `new_len` must be less than or equal to [`Self::capacity`].
> >      /// - If `new_len` is greater than `self.len`, all elements within the interval
> > -    ///   [`self.len`,`new_len`) must be initialized.
> > +    ///   [`self.len`,`new_len`) must be initialized,
>
> Why a comma here? There's a period on the previous list item, and a
> period seems more appropriate before the next item.

Did you see this one? Was it intentional?

Re: [PATCH 1/2] rust: alloc: extend safety requirements of Vec::set_len()

[Benno Lossin]

On Sun Mar 16, 2025 at 1:33 AM CET, Tamir Duberstein wrote:
> On Sat, Mar 15, 2025 at 2:36 PM Danilo Krummrich <dakr@kernel.org> wrote:
>>
>> On Sat, Mar 15, 2025 at 05:44:29PM +0000, Benno Lossin wrote:
>> >
>> > What is meant by "copied"? I agree with Tamir, this can probably just be
>> > "taken ownership of".
>>
>> If not dropped in place, the only way to take ownership of the object is to copy
>> the corresponding memory, i.e. the caller can not take ownership of the object
>> at its current memory location, since this memory is owned by the vector.
>
> I think this is the point I was trying to make: the language semantics
> are such that "taking ownership of" is equivalent to "become
> responsible for the dropping of", so it doesn't make sense to write
> that here, as though this requirement is specific to the semantics of
> this function.
>
>> AFAIU, when we speak of ownership, we mean ownership over a value or object, but
>> not ownership over the underlying memory.
>>
>> Hence, I think it's important to mention that it needs to be either dropped in
>> place or copied in order to take ownership.
>>
>> Even if we agree that "drop in place" is considered as "take ownership", we need
>> to differentiate between the special case of taking ownership where its dropped
>> in place and the requirement to copy the value and then take ownership.

I get where you're coming from, but I wouldn't call that copying a
value. Copying for me means that there would be two instances of the
same object. But since the one in `[new_len, len)` will get invalidated,
I don't think that copy makes sense.

Also why even is this a safety requirement? There is `mem::forget`, so
doing `vec.set_len(0)` can also be done by safe code:

    while let Some(val) = vec.pop() {
        forget(val);
    }

I don't think this needs to be a safety requirement.

> Perhaps:
>
>> The caller must ensure the destructors of [`new_len`, `self.len`) are run exactly once.

Hmm that is not correct though, since

    let mut vec = vec![];
    vec.push(x.clone());
    vec.pop();
    vec.push(x.clone());
    vec.pop();

Drops the memory in `[0, 1)` two times.

---
Cheers,
Benno

Re: [PATCH 1/2] rust: alloc: extend safety requirements of Vec::set_len()

[Danilo Krummrich]

On Sun, Mar 16, 2025 at 09:38:23AM +0000, Benno Lossin wrote:
> On Sun Mar 16, 2025 at 1:33 AM CET, Tamir Duberstein wrote:
> > On Sat, Mar 15, 2025 at 2:36 PM Danilo Krummrich <dakr@kernel.org> wrote:
> >>
> >> On Sat, Mar 15, 2025 at 05:44:29PM +0000, Benno Lossin wrote:
> >> >
> >> > What is meant by "copied"? I agree with Tamir, this can probably just be
> >> > "taken ownership of".
> >>
> >> If not dropped in place, the only way to take ownership of the object is to copy
> >> the corresponding memory, i.e. the caller can not take ownership of the object
> >> at its current memory location, since this memory is owned by the vector.
> >
> > I think this is the point I was trying to make: the language semantics
> > are such that "taking ownership of" is equivalent to "become
> > responsible for the dropping of", so it doesn't make sense to write
> > that here, as though this requirement is specific to the semantics of
> > this function.
> >
> >> AFAIU, when we speak of ownership, we mean ownership over a value or object, but
> >> not ownership over the underlying memory.
> >>
> >> Hence, I think it's important to mention that it needs to be either dropped in
> >> place or copied in order to take ownership.
> >>
> >> Even if we agree that "drop in place" is considered as "take ownership", we need
> >> to differentiate between the special case of taking ownership where its dropped
> >> in place and the requirement to copy the value and then take ownership.
> 
> I get where you're coming from, but I wouldn't call that copying a
> value. Copying for me means that there would be two instances of the
> same object. But since the one in `[new_len, len)` will get invalidated,
> I don't think that copy makes sense.

What I meant is "copy the memory where the value is stored".

> 
> Also why even is this a safety requirement? There is `mem::forget`, so
> doing `vec.set_len(0)` can also be done by safe code:
> 
>     while let Some(val) = vec.pop() {
>         forget(val);
>     }
> 
> I don't think this needs to be a safety requirement.

Given that mem::forget() is considered safe, you clearly have a point here.

Let's go with just "must be taken ownership of" then. Unless there's subsequent
feedback, I won't send a new version for this, since you both already gave your
(conditional) RB for this.

(I tend to forget that mem::forget() is formally safe, since it can cause all
kinds of bugs, e.g. dead lock the whole machine if called for a lock in the
kernel, etc.)

Re: [PATCH 1/2] rust: alloc: extend safety requirements of Vec::set_len()

[Tamir Duberstein]

On Sun, Mar 16, 2025 at 8:31 AM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Sun, Mar 16, 2025 at 09:38:23AM +0000, Benno Lossin wrote:
> >
> > I don't think this needs to be a safety requirement.
>
> Given that mem::forget() is considered safe, you clearly have a point here.

Yeah, good catch.

> Let's go with just "must be taken ownership of" then. Unless there's subsequent
> feedback, I won't send a new version for this, since you both already gave your
> (conditional) RB for this.

What does it mean to take ownership, if not to run the destructor?
Given Benno's insight, I think the safety text is correct as it
existed before this patch.

Re: [PATCH 1/2] rust: alloc: extend safety requirements of Vec::set_len()

[Danilo Krummrich]

On Sun, Mar 16, 2025 at 08:42:43AM -0400, Tamir Duberstein wrote:
> On Sun, Mar 16, 2025 at 8:31 AM Danilo Krummrich <dakr@kernel.org> wrote:
> 
> > Let's go with just "must be taken ownership of" then. Unless there's subsequent
> > feedback, I won't send a new version for this, since you both already gave your
> > (conditional) RB for this.
> 
> What does it mean to take ownership, if not to run the destructor?

Taking responsibility over the decision of what should happen to the value, i.e.
forget about the value, drop it right away, keep it alive at a different memory
location.

> Given Benno's insight, I think the safety text is correct as it
> existed before this patch.

Without the addition there's no requirement for the caller to take ownership,
but that's what we want here. Without the requirement it would be on set_len()
to take a decision on what should happen with the value.

Re: [PATCH 1/2] rust: alloc: extend safety requirements of Vec::set_len()

[Tamir Duberstein]

On Sun, Mar 16, 2025 at 9:01 AM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Sun, Mar 16, 2025 at 08:42:43AM -0400, Tamir Duberstein wrote:
> > On Sun, Mar 16, 2025 at 8:31 AM Danilo Krummrich <dakr@kernel.org> wrote:
> >
> > > Let's go with just "must be taken ownership of" then. Unless there's subsequent
> > > feedback, I won't send a new version for this, since you both already gave your
> > > (conditional) RB for this.
> >
> > What does it mean to take ownership, if not to run the destructor?
>
> Taking responsibility over the decision of what should happen to the value, i.e.
> forget about the value, drop it right away, keep it alive at a different memory
> location.
>
> > Given Benno's insight, I think the safety text is correct as it
> > existed before this patch.
>
> Without the addition there's no requirement for the caller to take ownership,
> but that's what we want here. Without the requirement it would be on set_len()
> to take a decision on what should happen with the value.

It isn't a safety requirement, right? I think it's fine to document,
but given that neglecting to run a destructor is safe, it doesn't seem
to affect safety.

Re: [PATCH 1/2] rust: alloc: extend safety requirements of Vec::set_len()

[Danilo Krummrich]

On Sun, Mar 16, 2025 at 09:13:50AM -0400, Tamir Duberstein wrote:
> On Sun, Mar 16, 2025 at 9:01 AM Danilo Krummrich <dakr@kernel.org> wrote:
> >
> > On Sun, Mar 16, 2025 at 08:42:43AM -0400, Tamir Duberstein wrote:
> > > On Sun, Mar 16, 2025 at 8:31 AM Danilo Krummrich <dakr@kernel.org> wrote:
> > >
> > > > Let's go with just "must be taken ownership of" then. Unless there's subsequent
> > > > feedback, I won't send a new version for this, since you both already gave your
> > > > (conditional) RB for this.
> > >
> > > What does it mean to take ownership, if not to run the destructor?
> >
> > Taking responsibility over the decision of what should happen to the value, i.e.
> > forget about the value, drop it right away, keep it alive at a different memory
> > location.
> >
> > > Given Benno's insight, I think the safety text is correct as it
> > > existed before this patch.
> >
> > Without the addition there's no requirement for the caller to take ownership,
> > but that's what we want here. Without the requirement it would be on set_len()
> > to take a decision on what should happen with the value.
> 
> It isn't a safety requirement, right? I think it's fine to document,
> but given that neglecting to run a destructor is safe, it doesn't seem
> to affect safety.

I don't think there's a rule that safety requirements of unsafe functions must
only be used to guarantee memory safety, etc.

But you're right, if set_len() guarantees a default of what happens to the
value(s) when the caller doesn't take ownership, we could avoid the safety
requirement.

But I don't want set_len() to make any guarantees about about a default, instead
I want that the caller is forced to make an explicit and documented decision
over what happens to the discarded values.

Re: [PATCH 1/2] rust: alloc: extend safety requirements of Vec::set_len()

[Benno Lossin]

On Sun Mar 16, 2025 at 2:46 PM CET, Danilo Krummrich wrote:
> On Sun, Mar 16, 2025 at 09:13:50AM -0400, Tamir Duberstein wrote:
>> On Sun, Mar 16, 2025 at 9:01 AM Danilo Krummrich <dakr@kernel.org> wrote:
>> > On Sun, Mar 16, 2025 at 08:42:43AM -0400, Tamir Duberstein wrote:
>> > > On Sun, Mar 16, 2025 at 8:31 AM Danilo Krummrich <dakr@kernel.org> wrote:
>> > >
>> > > > Let's go with just "must be taken ownership of" then. Unless there's subsequent
>> > > > feedback, I won't send a new version for this, since you both already gave your
>> > > > (conditional) RB for this.

I didn't realize the `forget` argument immediately, so I sadly have to
take my RB back.

>> > > What does it mean to take ownership, if not to run the destructor?
>> >
>> > Taking responsibility over the decision of what should happen to the value, i.e.
>> > forget about the value, drop it right away, keep it alive at a different memory
>> > location.
>> >
>> > > Given Benno's insight, I think the safety text is correct as it
>> > > existed before this patch.
>> >
>> > Without the addition there's no requirement for the caller to take ownership,
>> > but that's what we want here. Without the requirement it would be on set_len()
>> > to take a decision on what should happen with the value.
>> 
>> It isn't a safety requirement, right? I think it's fine to document,
>> but given that neglecting to run a destructor is safe, it doesn't seem
>> to affect safety.
>
> I don't think there's a rule that safety requirements of unsafe functions must
> only be used to guarantee memory safety, etc.

Well that's their only point. Safety documentation should only be
concerned with memory safety.

You can still put it in the normal documentation though.

> But you're right, if set_len() guarantees a default of what happens to the
> value(s) when the caller doesn't take ownership, we could avoid the safety
> requirement.

Yeah the default is to forget the value.

> But I don't want set_len() to make any guarantees about about a default, instead
> I want that the caller is forced to make an explicit and documented decision
> over what happens to the discarded values.

But it shouldn't be part of the safety docs.

---
Cheers,
Benno

Re: [PATCH 1/2] rust: alloc: extend safety requirements of Vec::set_len()

[Danilo Krummrich]

On Sun, Mar 16, 2025 at 05:40:29PM +0000, Benno Lossin wrote:
> On Sun Mar 16, 2025 at 2:46 PM CET, Danilo Krummrich wrote:
> > On Sun, Mar 16, 2025 at 09:13:50AM -0400, Tamir Duberstein wrote:
> >> On Sun, Mar 16, 2025 at 9:01 AM Danilo Krummrich <dakr@kernel.org> wrote:
> >> > On Sun, Mar 16, 2025 at 08:42:43AM -0400, Tamir Duberstein wrote:
> >> > > On Sun, Mar 16, 2025 at 8:31 AM Danilo Krummrich <dakr@kernel.org> wrote:
> >> > >
> >> > > > Let's go with just "must be taken ownership of" then. Unless there's subsequent
> >> > > > feedback, I won't send a new version for this, since you both already gave your
> >> > > > (conditional) RB for this.
> 
> I didn't realize the `forget` argument immediately, so I sadly have to
> take my RB back.
> 
> >> > > What does it mean to take ownership, if not to run the destructor?
> >> >
> >> > Taking responsibility over the decision of what should happen to the value, i.e.
> >> > forget about the value, drop it right away, keep it alive at a different memory
> >> > location.
> >> >
> >> > > Given Benno's insight, I think the safety text is correct as it
> >> > > existed before this patch.
> >> >
> >> > Without the addition there's no requirement for the caller to take ownership,
> >> > but that's what we want here. Without the requirement it would be on set_len()
> >> > to take a decision on what should happen with the value.
> >> 
> >> It isn't a safety requirement, right? I think it's fine to document,
> >> but given that neglecting to run a destructor is safe, it doesn't seem
> >> to affect safety.
> >
> > I don't think there's a rule that safety requirements of unsafe functions must
> > only be used to guarantee memory safety, etc.
> 
> Well that's their only point. Safety documentation should only be
> concerned with memory safety.

Oops, I wanted to say "must not only be used to *directly* guarantee memory
safety, etc.".

For instance, it is also used to justify type invariants (that exist to prevent
UB).

So, what I wanted to say is that setting self.len by iteself is never directly a
safety critical thing.

> You can still put it in the normal documentation though.
> 
> > But you're right, if set_len() guarantees a default of what happens to the
> > value(s) when the caller doesn't take ownership, we could avoid the safety
> > requirement.
> 
> Yeah the default is to forget the value.

It's not really a defined default though, it's just what ultimately happens as
the consequence of how Vec in general and set_len() are implemented.

But let's define it then; what about:

"[`Vec::set_len`] takes (or kepps) ownership of all elements within the range
[0; `new_len`] and abandons ownership of all values outside of this range, if
any."

The caller may take ownership of the abandoned elements."

I'd argue that giving up ownership, while offering someone else to take it means
that it implies that otherwise we'll just end up forgetting about the value.

Re: [PATCH 1/2] rust: alloc: extend safety requirements of Vec::set_len()

[Danilo Krummrich]

On Sun, Mar 16, 2025 at 07:59:34PM +0100, Danilo Krummrich wrote:
> But let's define it then; what about:
> 
> "[`Vec::set_len`] takes (or kepps) ownership of all elements within the range
> [0; `new_len`] and abandons ownership of all values outside of this range, if
> any."
> 
> The caller may take ownership of the abandoned elements."
> 
> I'd argue that giving up ownership, while offering someone else to take it means
> that it implies that otherwise we'll just end up forgetting about the value.

Btw. I'd still prefer if we could enforce that the caller has to document what
should happen to the abandoned value. But I acknowledge that the safety comment
isn't the scope for it.

It'd be great if e.g. clippy would give us a tool to do something analogous to
safety comments.

It think it would be useful to enfoce some additional safety documentation. For
instance, I think the kernel would much benefit if we could enforce that
mem::forget() must be justified with a comment, since as mentioned ina previous
mail, it can cause fatal bugs, for instance when used on lock guards.

Re: [PATCH 1/2] rust: alloc: extend safety requirements of Vec::set_len()

[Tamir Duberstein]

On Sun, Mar 16, 2025 at 3:09 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Sun, Mar 16, 2025 at 07:59:34PM +0100, Danilo Krummrich wrote:
> > But let's define it then; what about:
> >
> > "[`Vec::set_len`] takes (or kepps) ownership of all elements within the range
> > [0; `new_len`] and abandons ownership of all values outside of this range, if
> > any."
> >
> > The caller may take ownership of the abandoned elements."
> >
> > I'd argue that giving up ownership, while offering someone else to take it means
> > that it implies that otherwise we'll just end up forgetting about the value.
>
> Btw. I'd still prefer if we could enforce that the caller has to document what
> should happen to the abandoned value. But I acknowledge that the safety comment
> isn't the scope for it.
>
> It'd be great if e.g. clippy would give us a tool to do something analogous to
> safety comments.
>
> It think it would be useful to enfoce some additional safety documentation. For
> instance, I think the kernel would much benefit if we could enforce that
> mem::forget() must be justified with a comment, since as mentioned ina previous
> mail, it can cause fatal bugs, for instance when used on lock guards.

There are other examples; ManuallyDrop and Box::leak are two that
immediately come to mind.

But focusing on Vec::set_len again, could we return a mut slice to the
tail when new_len < old_len? Something like:

diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
index c12844764671..e5f857d723ec 100644
--- a/rust/kernel/alloc/kvec.rs
+++ b/rust/kernel/alloc/kvec.rs
@@ -191,9 +191,16 @@ pub fn len(&self) -> usize {
     /// - If `new_len` is greater than `self.len`, all elements
within the interval
     ///   [`self.len`,`new_len`) must be initialized.
     #[inline]
-    pub unsafe fn set_len(&mut self, new_len: usize) {
+    pub unsafe fn set_len(&mut self, new_len: usize) -> &mut [T] {
         debug_assert!(new_len <= self.capacity());
-        self.len = new_len;
+        let old_len = core::mem::replace(&mut self.len, new_len);
+        match old_len.checked_sub(new_len) {
+            None => &mut [],
+            Some(len) => {
+                // SAFETY: ...
+                unsafe {
slice::from_raw_parts_mut(self.as_mut_ptr().add(new_len), len) }
+            }
+        }
     }

Would that sufficiently communicate to the caller that they should
deal with this memory?

Re: [PATCH 1/2] rust: alloc: extend safety requirements of Vec::set_len()

[Danilo Krummrich]

On Sun, Mar 16, 2025 at 03:30:27PM -0400, Tamir Duberstein wrote:
> On Sun, Mar 16, 2025 at 3:09 PM Danilo Krummrich <dakr@kernel.org> wrote:
> >
> > On Sun, Mar 16, 2025 at 07:59:34PM +0100, Danilo Krummrich wrote:
> > > But let's define it then; what about:
> > >
> > > "[`Vec::set_len`] takes (or kepps) ownership of all elements within the range
> > > [0; `new_len`] and abandons ownership of all values outside of this range, if
> > > any."
> > >
> > > The caller may take ownership of the abandoned elements."
> > >
> > > I'd argue that giving up ownership, while offering someone else to take it means
> > > that it implies that otherwise we'll just end up forgetting about the value.
> >
> > Btw. I'd still prefer if we could enforce that the caller has to document what
> > should happen to the abandoned value. But I acknowledge that the safety comment
> > isn't the scope for it.
> >
> > It'd be great if e.g. clippy would give us a tool to do something analogous to
> > safety comments.
> >
> > It think it would be useful to enfoce some additional safety documentation. For
> > instance, I think the kernel would much benefit if we could enforce that
> > mem::forget() must be justified with a comment, since as mentioned ina previous
> > mail, it can cause fatal bugs, for instance when used on lock guards.
> 
> There are other examples; ManuallyDrop and Box::leak are two that
> immediately come to mind.
> 
> But focusing on Vec::set_len again, could we return a mut slice to the
> tail when new_len < old_len? Something like:
> 
> diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
> index c12844764671..e5f857d723ec 100644
> --- a/rust/kernel/alloc/kvec.rs
> +++ b/rust/kernel/alloc/kvec.rs
> @@ -191,9 +191,16 @@ pub fn len(&self) -> usize {
>      /// - If `new_len` is greater than `self.len`, all elements
> within the interval
>      ///   [`self.len`,`new_len`) must be initialized.
>      #[inline]
> -    pub unsafe fn set_len(&mut self, new_len: usize) {
> +    pub unsafe fn set_len(&mut self, new_len: usize) -> &mut [T] {
>          debug_assert!(new_len <= self.capacity());
> -        self.len = new_len;
> +        let old_len = core::mem::replace(&mut self.len, new_len);
> +        match old_len.checked_sub(new_len) {
> +            None => &mut [],
> +            Some(len) => {
> +                // SAFETY: ...
> +                unsafe {
> slice::from_raw_parts_mut(self.as_mut_ptr().add(new_len), len) }
> +            }
> +        }
>      }
> 
> Would that sufficiently communicate to the caller that they should
> deal with this memory?

I think that is a good idea. I'm not sure I like that this is useless when
new_len > self.len, but it also doesn't hurt too much, I guess.

Feel free to send the corresponding patch. Also, feel free to add the
corresponding comment about ownership while at it.

I'd just drop this patch then.

Re: [PATCH 1/2] rust: alloc: extend safety requirements of Vec::set_len()

[Tamir Duberstein]

On Sun, Mar 16, 2025 at 4:54 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Sun, Mar 16, 2025 at 03:30:27PM -0400, Tamir Duberstein wrote:
> > On Sun, Mar 16, 2025 at 3:09 PM Danilo Krummrich <dakr@kernel.org> wrote:
> > >
> > > On Sun, Mar 16, 2025 at 07:59:34PM +0100, Danilo Krummrich wrote:
> > > > But let's define it then; what about:
> > > >
> > > > "[`Vec::set_len`] takes (or kepps) ownership of all elements within the range
> > > > [0; `new_len`] and abandons ownership of all values outside of this range, if
> > > > any."
> > > >
> > > > The caller may take ownership of the abandoned elements."
> > > >
> > > > I'd argue that giving up ownership, while offering someone else to take it means
> > > > that it implies that otherwise we'll just end up forgetting about the value.
> > >
> > > Btw. I'd still prefer if we could enforce that the caller has to document what
> > > should happen to the abandoned value. But I acknowledge that the safety comment
> > > isn't the scope for it.
> > >
> > > It'd be great if e.g. clippy would give us a tool to do something analogous to
> > > safety comments.
> > >
> > > It think it would be useful to enfoce some additional safety documentation. For
> > > instance, I think the kernel would much benefit if we could enforce that
> > > mem::forget() must be justified with a comment, since as mentioned ina previous
> > > mail, it can cause fatal bugs, for instance when used on lock guards.
> >
> > There are other examples; ManuallyDrop and Box::leak are two that
> > immediately come to mind.
> >
> > But focusing on Vec::set_len again, could we return a mut slice to the
> > tail when new_len < old_len? Something like:
> >
> > diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
> > index c12844764671..e5f857d723ec 100644
> > --- a/rust/kernel/alloc/kvec.rs
> > +++ b/rust/kernel/alloc/kvec.rs
> > @@ -191,9 +191,16 @@ pub fn len(&self) -> usize {
> >      /// - If `new_len` is greater than `self.len`, all elements
> > within the interval
> >      ///   [`self.len`,`new_len`) must be initialized.
> >      #[inline]
> > -    pub unsafe fn set_len(&mut self, new_len: usize) {
> > +    pub unsafe fn set_len(&mut self, new_len: usize) -> &mut [T] {
> >          debug_assert!(new_len <= self.capacity());
> > -        self.len = new_len;
> > +        let old_len = core::mem::replace(&mut self.len, new_len);
> > +        match old_len.checked_sub(new_len) {
> > +            None => &mut [],
> > +            Some(len) => {
> > +                // SAFETY: ...
> > +                unsafe {
> > slice::from_raw_parts_mut(self.as_mut_ptr().add(new_len), len) }
> > +            }
> > +        }
> >      }
> >
> > Would that sufficiently communicate to the caller that they should
> > deal with this memory?
>
> I think that is a good idea. I'm not sure I like that this is useless when
> new_len > self.len, but it also doesn't hurt too much, I guess.
>
> Feel free to send the corresponding patch. Also, feel free to add the
> corresponding comment about ownership while at it.
>
> I'd just drop this patch then.

Looking at the usages of `set_len` that we have today (excluding the
pending nova patches for truncate and resize), they all amount to
`v.set_len(v.len() + n)`. What do you think about replacing this
method with `inc_len`? In the nova patches the only need for
decrementing the length would be in `truncate`, so `dec_len` can be
introduced there or the length can be decremented there directly.

Re: [PATCH 1/2] rust: alloc: extend safety requirements of Vec::set_len()

[Danilo Krummrich]

On Sun, Mar 16, 2025 at 05:10:30PM -0400, Tamir Duberstein wrote:
> On Sun, Mar 16, 2025 at 4:54 PM Danilo Krummrich <dakr@kernel.org> wrote:
> >
> > On Sun, Mar 16, 2025 at 03:30:27PM -0400, Tamir Duberstein wrote:
> > > On Sun, Mar 16, 2025 at 3:09 PM Danilo Krummrich <dakr@kernel.org> wrote:
> > > >
> > > > On Sun, Mar 16, 2025 at 07:59:34PM +0100, Danilo Krummrich wrote:
> > > > > But let's define it then; what about:
> > > > >
> > > > > "[`Vec::set_len`] takes (or kepps) ownership of all elements within the range
> > > > > [0; `new_len`] and abandons ownership of all values outside of this range, if
> > > > > any."
> > > > >
> > > > > The caller may take ownership of the abandoned elements."
> > > > >
> > > > > I'd argue that giving up ownership, while offering someone else to take it means
> > > > > that it implies that otherwise we'll just end up forgetting about the value.
> > > >
> > > > Btw. I'd still prefer if we could enforce that the caller has to document what
> > > > should happen to the abandoned value. But I acknowledge that the safety comment
> > > > isn't the scope for it.
> > > >
> > > > It'd be great if e.g. clippy would give us a tool to do something analogous to
> > > > safety comments.
> > > >
> > > > It think it would be useful to enfoce some additional safety documentation. For
> > > > instance, I think the kernel would much benefit if we could enforce that
> > > > mem::forget() must be justified with a comment, since as mentioned ina previous
> > > > mail, it can cause fatal bugs, for instance when used on lock guards.
> > >
> > > There are other examples; ManuallyDrop and Box::leak are two that
> > > immediately come to mind.
> > >
> > > But focusing on Vec::set_len again, could we return a mut slice to the
> > > tail when new_len < old_len? Something like:
> > >
> > > diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
> > > index c12844764671..e5f857d723ec 100644
> > > --- a/rust/kernel/alloc/kvec.rs
> > > +++ b/rust/kernel/alloc/kvec.rs
> > > @@ -191,9 +191,16 @@ pub fn len(&self) -> usize {
> > >      /// - If `new_len` is greater than `self.len`, all elements
> > > within the interval
> > >      ///   [`self.len`,`new_len`) must be initialized.
> > >      #[inline]
> > > -    pub unsafe fn set_len(&mut self, new_len: usize) {
> > > +    pub unsafe fn set_len(&mut self, new_len: usize) -> &mut [T] {
> > >          debug_assert!(new_len <= self.capacity());
> > > -        self.len = new_len;
> > > +        let old_len = core::mem::replace(&mut self.len, new_len);
> > > +        match old_len.checked_sub(new_len) {
> > > +            None => &mut [],
> > > +            Some(len) => {
> > > +                // SAFETY: ...
> > > +                unsafe {
> > > slice::from_raw_parts_mut(self.as_mut_ptr().add(new_len), len) }
> > > +            }
> > > +        }
> > >      }
> > >
> > > Would that sufficiently communicate to the caller that they should
> > > deal with this memory?
> >
> > I think that is a good idea. I'm not sure I like that this is useless when
> > new_len > self.len, but it also doesn't hurt too much, I guess.
> >
> > Feel free to send the corresponding patch. Also, feel free to add the
> > corresponding comment about ownership while at it.
> >
> > I'd just drop this patch then.
> 
> Looking at the usages of `set_len` that we have today (excluding the
> pending nova patches for truncate and resize), they all amount to
> `v.set_len(v.len() + n)`. What do you think about replacing this
> method with `inc_len`? In the nova patches the only need for
> decrementing the length would be in `truncate`, so `dec_len` can be
> introduced there or the length can be decremented there directly.

I think splitting it in inc_len() and dec_len() is good. Note that other methods
need dec_len() too, e.g. [1].

[1] https://lore.kernel.org/rust-for-linux/20250311-iov-iter-v1-4-f6c9134ea824@google.com/

Re: [PATCH 1/2] rust: alloc: extend safety requirements of Vec::set_len()

[Tamir Duberstein]

On Sun, Mar 16, 2025 at 5:17 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Sun, Mar 16, 2025 at 05:10:30PM -0400, Tamir Duberstein wrote:
> > On Sun, Mar 16, 2025 at 4:54 PM Danilo Krummrich <dakr@kernel.org> wrote:
> > >
> > > On Sun, Mar 16, 2025 at 03:30:27PM -0400, Tamir Duberstein wrote:
> > > > On Sun, Mar 16, 2025 at 3:09 PM Danilo Krummrich <dakr@kernel.org> wrote:
> > > > >
> > > > > On Sun, Mar 16, 2025 at 07:59:34PM +0100, Danilo Krummrich wrote:
> > > > > > But let's define it then; what about:
> > > > > >
> > > > > > "[`Vec::set_len`] takes (or kepps) ownership of all elements within the range
> > > > > > [0; `new_len`] and abandons ownership of all values outside of this range, if
> > > > > > any."
> > > > > >
> > > > > > The caller may take ownership of the abandoned elements."
> > > > > >
> > > > > > I'd argue that giving up ownership, while offering someone else to take it means
> > > > > > that it implies that otherwise we'll just end up forgetting about the value.
> > > > >
> > > > > Btw. I'd still prefer if we could enforce that the caller has to document what
> > > > > should happen to the abandoned value. But I acknowledge that the safety comment
> > > > > isn't the scope for it.
> > > > >
> > > > > It'd be great if e.g. clippy would give us a tool to do something analogous to
> > > > > safety comments.
> > > > >
> > > > > It think it would be useful to enfoce some additional safety documentation. For
> > > > > instance, I think the kernel would much benefit if we could enforce that
> > > > > mem::forget() must be justified with a comment, since as mentioned ina previous
> > > > > mail, it can cause fatal bugs, for instance when used on lock guards.
> > > >
> > > > There are other examples; ManuallyDrop and Box::leak are two that
> > > > immediately come to mind.
> > > >
> > > > But focusing on Vec::set_len again, could we return a mut slice to the
> > > > tail when new_len < old_len? Something like:
> > > >
> > > > diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
> > > > index c12844764671..e5f857d723ec 100644
> > > > --- a/rust/kernel/alloc/kvec.rs
> > > > +++ b/rust/kernel/alloc/kvec.rs
> > > > @@ -191,9 +191,16 @@ pub fn len(&self) -> usize {
> > > >      /// - If `new_len` is greater than `self.len`, all elements
> > > > within the interval
> > > >      ///   [`self.len`,`new_len`) must be initialized.
> > > >      #[inline]
> > > > -    pub unsafe fn set_len(&mut self, new_len: usize) {
> > > > +    pub unsafe fn set_len(&mut self, new_len: usize) -> &mut [T] {
> > > >          debug_assert!(new_len <= self.capacity());
> > > > -        self.len = new_len;
> > > > +        let old_len = core::mem::replace(&mut self.len, new_len);
> > > > +        match old_len.checked_sub(new_len) {
> > > > +            None => &mut [],
> > > > +            Some(len) => {
> > > > +                // SAFETY: ...
> > > > +                unsafe {
> > > > slice::from_raw_parts_mut(self.as_mut_ptr().add(new_len), len) }
> > > > +            }
> > > > +        }
> > > >      }
> > > >
> > > > Would that sufficiently communicate to the caller that they should
> > > > deal with this memory?
> > >
> > > I think that is a good idea. I'm not sure I like that this is useless when
> > > new_len > self.len, but it also doesn't hurt too much, I guess.
> > >
> > > Feel free to send the corresponding patch. Also, feel free to add the
> > > corresponding comment about ownership while at it.
> > >
> > > I'd just drop this patch then.
> >
> > Looking at the usages of `set_len` that we have today (excluding the
> > pending nova patches for truncate and resize), they all amount to
> > `v.set_len(v.len() + n)`. What do you think about replacing this
> > method with `inc_len`? In the nova patches the only need for
> > decrementing the length would be in `truncate`, so `dec_len` can be
> > introduced there or the length can be decremented there directly.
>
> I think splitting it in inc_len() and dec_len() is good. Note that other methods
> need dec_len() too, e.g. [1].
>
> [1] https://lore.kernel.org/rust-for-linux/20250311-iov-iter-v1-4-f6c9134ea824@google.com/

Ok, I'll send a 2-patch series that adds `dec_len` as a private
helper. I see no need for it to be pub at this time.

Re: [PATCH 1/2] rust: alloc: extend safety requirements of Vec::set_len()

[Tamir Duberstein]

On Sun, Mar 16, 2025 at 5:20 PM Tamir Duberstein <tamird@gmail.com> wrote:
>
> On Sun, Mar 16, 2025 at 5:17 PM Danilo Krummrich <dakr@kernel.org> wrote:
> >
> > On Sun, Mar 16, 2025 at 05:10:30PM -0400, Tamir Duberstein wrote:
> > > On Sun, Mar 16, 2025 at 4:54 PM Danilo Krummrich <dakr@kernel.org> wrote:
> > > >
> > > > On Sun, Mar 16, 2025 at 03:30:27PM -0400, Tamir Duberstein wrote:
> > > > > On Sun, Mar 16, 2025 at 3:09 PM Danilo Krummrich <dakr@kernel.org> wrote:
> > > > > >
> > > > > > On Sun, Mar 16, 2025 at 07:59:34PM +0100, Danilo Krummrich wrote:
> > > > > > > But let's define it then; what about:
> > > > > > >
> > > > > > > "[`Vec::set_len`] takes (or kepps) ownership of all elements within the range
> > > > > > > [0; `new_len`] and abandons ownership of all values outside of this range, if
> > > > > > > any."
> > > > > > >
> > > > > > > The caller may take ownership of the abandoned elements."
> > > > > > >
> > > > > > > I'd argue that giving up ownership, while offering someone else to take it means
> > > > > > > that it implies that otherwise we'll just end up forgetting about the value.
> > > > > >
> > > > > > Btw. I'd still prefer if we could enforce that the caller has to document what
> > > > > > should happen to the abandoned value. But I acknowledge that the safety comment
> > > > > > isn't the scope for it.
> > > > > >
> > > > > > It'd be great if e.g. clippy would give us a tool to do something analogous to
> > > > > > safety comments.
> > > > > >
> > > > > > It think it would be useful to enfoce some additional safety documentation. For
> > > > > > instance, I think the kernel would much benefit if we could enforce that
> > > > > > mem::forget() must be justified with a comment, since as mentioned ina previous
> > > > > > mail, it can cause fatal bugs, for instance when used on lock guards.
> > > > >
> > > > > There are other examples; ManuallyDrop and Box::leak are two that
> > > > > immediately come to mind.
> > > > >
> > > > > But focusing on Vec::set_len again, could we return a mut slice to the
> > > > > tail when new_len < old_len? Something like:
> > > > >
> > > > > diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
> > > > > index c12844764671..e5f857d723ec 100644
> > > > > --- a/rust/kernel/alloc/kvec.rs
> > > > > +++ b/rust/kernel/alloc/kvec.rs
> > > > > @@ -191,9 +191,16 @@ pub fn len(&self) -> usize {
> > > > >      /// - If `new_len` is greater than `self.len`, all elements
> > > > > within the interval
> > > > >      ///   [`self.len`,`new_len`) must be initialized.
> > > > >      #[inline]
> > > > > -    pub unsafe fn set_len(&mut self, new_len: usize) {
> > > > > +    pub unsafe fn set_len(&mut self, new_len: usize) -> &mut [T] {
> > > > >          debug_assert!(new_len <= self.capacity());
> > > > > -        self.len = new_len;
> > > > > +        let old_len = core::mem::replace(&mut self.len, new_len);
> > > > > +        match old_len.checked_sub(new_len) {
> > > > > +            None => &mut [],
> > > > > +            Some(len) => {
> > > > > +                // SAFETY: ...
> > > > > +                unsafe {
> > > > > slice::from_raw_parts_mut(self.as_mut_ptr().add(new_len), len) }
> > > > > +            }
> > > > > +        }
> > > > >      }
> > > > >
> > > > > Would that sufficiently communicate to the caller that they should
> > > > > deal with this memory?
> > > >
> > > > I think that is a good idea. I'm not sure I like that this is useless when
> > > > new_len > self.len, but it also doesn't hurt too much, I guess.
> > > >
> > > > Feel free to send the corresponding patch. Also, feel free to add the
> > > > corresponding comment about ownership while at it.
> > > >
> > > > I'd just drop this patch then.
> > >
> > > Looking at the usages of `set_len` that we have today (excluding the
> > > pending nova patches for truncate and resize), they all amount to
> > > `v.set_len(v.len() + n)`. What do you think about replacing this
> > > method with `inc_len`? In the nova patches the only need for
> > > decrementing the length would be in `truncate`, so `dec_len` can be
> > > introduced there or the length can be decremented there directly.
> >
> > I think splitting it in inc_len() and dec_len() is good. Note that other methods
> > need dec_len() too, e.g. [1].
> >
> > [1] https://lore.kernel.org/rust-for-linux/20250311-iov-iter-v1-4-f6c9134ea824@google.com/
>
> Ok, I'll send a 2-patch series that adds `dec_len` as a private
> helper. I see no need for it to be pub at this time.

Actually, I don't think we need `dec_len` after all. `clear` can be
implemented as `self.truncate(0)`. Do you agree?

Re: [PATCH 1/2] rust: alloc: extend safety requirements of Vec::set_len()

[Danilo Krummrich]

On Sun, Mar 16, 2025 at 05:52:33PM -0400, Tamir Duberstein wrote:
> Actually, I don't think we need `dec_len` after all. `clear` can be
> implemented as `self.truncate(0)`. Do you agree?

Indeed, but I still think it would be nice to have dec_len(). pop() would be
another candidate for instance.

Re: [PATCH 1/2] rust: alloc: extend safety requirements of Vec::set_len()

[Benno Lossin]

On Sun Mar 16, 2025 at 8:09 PM CET, Danilo Krummrich wrote:
> On Sun, Mar 16, 2025 at 07:59:34PM +0100, Danilo Krummrich wrote:
>> But let's define it then; what about:
>> 
>> "[`Vec::set_len`] takes (or kepps) ownership of all elements within the range
>> [0; `new_len`] and abandons ownership of all values outside of this range, if
>> any."
>> 
>> The caller may take ownership of the abandoned elements."
>> 
>> I'd argue that giving up ownership, while offering someone else to take it means
>> that it implies that otherwise we'll just end up forgetting about the value.
>
> Btw. I'd still prefer if we could enforce that the caller has to document what
> should happen to the abandoned value. But I acknowledge that the safety comment
> isn't the scope for it.
>
> It'd be great if e.g. clippy would give us a tool to do something analogous to
> safety comments.
>
> It think it would be useful to enfoce some additional safety documentation. For
> instance, I think the kernel would much benefit if we could enforce that
> mem::forget() must be justified with a comment, since as mentioned ina previous
> mail, it can cause fatal bugs, for instance when used on lock guards.

I get where you're coming from, but this probably will very quickly get
out of hand.

For example, I can define `forget` safely:

    fn forget<T>(value: T) {
        struct Cycle<T> {
            this: RefCell<Option<Arc<Self>>>,
            value: T,
        }
        let cycle = Arc::new(Cycle { this: RefCell::new(None), value });
        *cycle.this.borrow_mut() = Some(cycle.clone());
    }

How would you ensure that this kind of pattern doesn't get written
accidentally (or with many indirections)?

---
Cheers,
Benno

Re: [PATCH 1/2] rust: alloc: extend safety requirements of Vec::set_len()

[Danilo Krummrich]

On Mon, Mar 17, 2025 at 09:52:07AM +0000, Benno Lossin wrote:
> On Sun Mar 16, 2025 at 8:09 PM CET, Danilo Krummrich wrote:
> > On Sun, Mar 16, 2025 at 07:59:34PM +0100, Danilo Krummrich wrote:
> >> But let's define it then; what about:
> >> 
> >> "[`Vec::set_len`] takes (or kepps) ownership of all elements within the range
> >> [0; `new_len`] and abandons ownership of all values outside of this range, if
> >> any."
> >> 
> >> The caller may take ownership of the abandoned elements."
> >> 
> >> I'd argue that giving up ownership, while offering someone else to take it means
> >> that it implies that otherwise we'll just end up forgetting about the value.
> >
> > Btw. I'd still prefer if we could enforce that the caller has to document what
> > should happen to the abandoned value. But I acknowledge that the safety comment
> > isn't the scope for it.
> >
> > It'd be great if e.g. clippy would give us a tool to do something analogous to
> > safety comments.
> >
> > It think it would be useful to enfoce some additional safety documentation. For
> > instance, I think the kernel would much benefit if we could enforce that
> > mem::forget() must be justified with a comment, since as mentioned ina previous
> > mail, it can cause fatal bugs, for instance when used on lock guards.
> 
> I get where you're coming from, but this probably will very quickly get
> out of hand.
> 
> For example, I can define `forget` safely:
> 
>     fn forget<T>(value: T) {
>         struct Cycle<T> {
>             this: RefCell<Option<Arc<Self>>>,
>             value: T,
>         }
>         let cycle = Arc::new(Cycle { this: RefCell::new(None), value });
>         *cycle.this.borrow_mut() = Some(cycle.clone());
>     }
> 
> How would you ensure that this kind of pattern doesn't get written
> accidentally (or with many indirections)?

I don't think that the possibility of writing safe (but yet buggy) code is an
argument against having the possibility of enforcing that a caller must write a
comment for justification on certain things, such as mem::forget().

But there's another reason I think having something like this could be
problematic: It might set the wrong incentive, as in "hey, I can just use a
"sanity requirement" in my function rather figuring out how to ensure it through
the type system, etc.".

Re: [PATCH 1/2] rust: alloc: extend safety requirements of Vec::set_len()

[Benno Lossin]

On Mon Mar 17, 2025 at 12:12 PM CET, Danilo Krummrich wrote:
> On Mon, Mar 17, 2025 at 09:52:07AM +0000, Benno Lossin wrote:
>> On Sun Mar 16, 2025 at 8:09 PM CET, Danilo Krummrich wrote:
>> > On Sun, Mar 16, 2025 at 07:59:34PM +0100, Danilo Krummrich wrote:
>> >> But let's define it then; what about:
>> >> 
>> >> "[`Vec::set_len`] takes (or kepps) ownership of all elements within the range
>> >> [0; `new_len`] and abandons ownership of all values outside of this range, if
>> >> any."
>> >> 
>> >> The caller may take ownership of the abandoned elements."
>> >> 
>> >> I'd argue that giving up ownership, while offering someone else to take it means
>> >> that it implies that otherwise we'll just end up forgetting about the value.
>> >
>> > Btw. I'd still prefer if we could enforce that the caller has to document what
>> > should happen to the abandoned value. But I acknowledge that the safety comment
>> > isn't the scope for it.
>> >
>> > It'd be great if e.g. clippy would give us a tool to do something analogous to
>> > safety comments.
>> >
>> > It think it would be useful to enfoce some additional safety documentation. For
>> > instance, I think the kernel would much benefit if we could enforce that
>> > mem::forget() must be justified with a comment, since as mentioned ina previous
>> > mail, it can cause fatal bugs, for instance when used on lock guards.
>> 
>> I get where you're coming from, but this probably will very quickly get
>> out of hand.
>> 
>> For example, I can define `forget` safely:
>> 
>>     fn forget<T>(value: T) {
>>         struct Cycle<T> {
>>             this: RefCell<Option<Arc<Self>>>,
>>             value: T,
>>         }
>>         let cycle = Arc::new(Cycle { this: RefCell::new(None), value });
>>         *cycle.this.borrow_mut() = Some(cycle.clone());
>>     }
>> 
>> How would you ensure that this kind of pattern doesn't get written
>> accidentally (or with many indirections)?
>
> I don't think that the possibility of writing safe (but yet buggy) code is an
> argument against having the possibility of enforcing that a caller must write a
> comment for justification on certain things, such as mem::forget().

My argument is that the problem of forgetting a value is not
self-contained like `unsafe` code is. Even if we were to document all
`forget` or `ManuallyDrop::new` invocations (which we definitely should)
we wouldn't get the security that one can't accidentally forget a lock
guard. I'm totally in favor of mandating an explaining comment above
`forget` calls (but not as a `SAFETY` comment).

> But there's another reason I think having something like this could be
> problematic: It might set the wrong incentive, as in "hey, I can just use a
> "sanity requirement" in my function rather figuring out how to ensure it through
> the type system, etc.".

I don't understand your point here, can you explain it more?

---
Cheers,
Benno

Re: [PATCH 1/2] rust: alloc: extend safety requirements of Vec::set_len()

[Danilo Krummrich]

On Mon, Mar 17, 2025 at 02:57:51PM +0000, Benno Lossin wrote:
> On Mon Mar 17, 2025 at 12:12 PM CET, Danilo Krummrich wrote:
> > On Mon, Mar 17, 2025 at 09:52:07AM +0000, Benno Lossin wrote:
> >> On Sun Mar 16, 2025 at 8:09 PM CET, Danilo Krummrich wrote:
> >> > On Sun, Mar 16, 2025 at 07:59:34PM +0100, Danilo Krummrich wrote:
> >> >> But let's define it then; what about:
> >> >> 
> >> >> "[`Vec::set_len`] takes (or kepps) ownership of all elements within the range
> >> >> [0; `new_len`] and abandons ownership of all values outside of this range, if
> >> >> any."
> >> >> 
> >> >> The caller may take ownership of the abandoned elements."
> >> >> 
> >> >> I'd argue that giving up ownership, while offering someone else to take it means
> >> >> that it implies that otherwise we'll just end up forgetting about the value.
> >> >
> >> > Btw. I'd still prefer if we could enforce that the caller has to document what
> >> > should happen to the abandoned value. But I acknowledge that the safety comment
> >> > isn't the scope for it.
> >> >
> >> > It'd be great if e.g. clippy would give us a tool to do something analogous to
> >> > safety comments.
> >> >
> >> > It think it would be useful to enfoce some additional safety documentation. For
> >> > instance, I think the kernel would much benefit if we could enforce that
> >> > mem::forget() must be justified with a comment, since as mentioned ina previous
> >> > mail, it can cause fatal bugs, for instance when used on lock guards.
> >> 
> >> I get where you're coming from, but this probably will very quickly get
> >> out of hand.
> >> 
> >> For example, I can define `forget` safely:
> >> 
> >>     fn forget<T>(value: T) {
> >>         struct Cycle<T> {
> >>             this: RefCell<Option<Arc<Self>>>,
> >>             value: T,
> >>         }
> >>         let cycle = Arc::new(Cycle { this: RefCell::new(None), value });
> >>         *cycle.this.borrow_mut() = Some(cycle.clone());
> >>     }
> >> 
> >> How would you ensure that this kind of pattern doesn't get written
> >> accidentally (or with many indirections)?
> >
> > I don't think that the possibility of writing safe (but yet buggy) code is an
> > argument against having the possibility of enforcing that a caller must write a
> > comment for justification on certain things, such as mem::forget().
> 
> My argument is that the problem of forgetting a value is not
> self-contained like `unsafe` code is. Even if we were to document all
> `forget` or `ManuallyDrop::new` invocations (which we definitely should)
> we wouldn't get the security that one can't accidentally forget a lock
> guard. I'm totally in favor of mandating an explaining comment above
> `forget` calls (but not as a `SAFETY` comment).

Oh, I see where the misunderstanding might lie.

Let's take a look at FileDescriptorReservation::fd_install(). My proposal is to
have something like:

	// SANITY: `fd_install` consumed file descriptor
	core::mem::forget(self);
	// SANITY: `fd_install` consumed file reference
	core::mem::forget(file);

Where we have e.g. clippy to complain if there is no "SANITY" (or whatever we
call it) comment for mem::forget().

I'm not proposing a SAFETY comment.

> 
> > But there's another reason I think having something like this could be
> > problematic: It might set the wrong incentive, as in "hey, I can just use a
> > "sanity requirement" in my function rather figuring out how to ensure it through
> > the type system, etc.".
> 
> I don't understand your point here, can you explain it more?

Does the explanation above make this concern clear for you?

Re: [PATCH 1/2] rust: alloc: extend safety requirements of Vec::set_len()

[Miguel Ojeda]

On Mon, Mar 17, 2025 at 4:57 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> Oh, I see where the misunderstanding might lie.
>
> Let's take a look at FileDescriptorReservation::fd_install(). My proposal is to
> have something like:
>
>         // SANITY: `fd_install` consumed file descriptor
>         core::mem::forget(self);
>         // SANITY: `fd_install` consumed file reference
>         core::mem::forget(file);
>
> Where we have e.g. clippy to complain if there is no "SANITY" (or whatever we
> call it) comment for mem::forget().
>
> I'm not proposing a SAFETY comment.

Clippy could "easily" do that, but if we are to do something like
that, then I would want to the ability to mark which ones need extra
comments in the signature/item (e.g. as an attribute) , i.e. instead
of in a global list somewhere like `.clippy.toml`.

It is also related to the "colored `unsafe`" idea we discussed very
early, but without extra blocks on the call, just a comment.

Cheers,
Miguel

Re: [PATCH 1/2] rust: alloc: extend safety requirements of Vec::set_len()

[Benno Lossin]

On Mon Mar 17, 2025 at 4:57 PM CET, Danilo Krummrich wrote:
> On Mon, Mar 17, 2025 at 02:57:51PM +0000, Benno Lossin wrote:
>> On Mon Mar 17, 2025 at 12:12 PM CET, Danilo Krummrich wrote:
>> > On Mon, Mar 17, 2025 at 09:52:07AM +0000, Benno Lossin wrote:
>> >> On Sun Mar 16, 2025 at 8:09 PM CET, Danilo Krummrich wrote:
>> >> > On Sun, Mar 16, 2025 at 07:59:34PM +0100, Danilo Krummrich wrote:
>> >> >> But let's define it then; what about:
>> >> >> 
>> >> >> "[`Vec::set_len`] takes (or kepps) ownership of all elements within the range
>> >> >> [0; `new_len`] and abandons ownership of all values outside of this range, if
>> >> >> any."
>> >> >> 
>> >> >> The caller may take ownership of the abandoned elements."
>> >> >> 
>> >> >> I'd argue that giving up ownership, while offering someone else to take it means
>> >> >> that it implies that otherwise we'll just end up forgetting about the value.
>> >> >
>> >> > Btw. I'd still prefer if we could enforce that the caller has to document what
>> >> > should happen to the abandoned value. But I acknowledge that the safety comment
>> >> > isn't the scope for it.
>> >> >
>> >> > It'd be great if e.g. clippy would give us a tool to do something analogous to
>> >> > safety comments.
>> >> >
>> >> > It think it would be useful to enfoce some additional safety documentation. For
>> >> > instance, I think the kernel would much benefit if we could enforce that
>> >> > mem::forget() must be justified with a comment, since as mentioned ina previous
>> >> > mail, it can cause fatal bugs, for instance when used on lock guards.
>> >> 
>> >> I get where you're coming from, but this probably will very quickly get
>> >> out of hand.
>> >> 
>> >> For example, I can define `forget` safely:
>> >> 
>> >>     fn forget<T>(value: T) {
>> >>         struct Cycle<T> {
>> >>             this: RefCell<Option<Arc<Self>>>,
>> >>             value: T,
>> >>         }
>> >>         let cycle = Arc::new(Cycle { this: RefCell::new(None), value });
>> >>         *cycle.this.borrow_mut() = Some(cycle.clone());
>> >>     }
>> >> 
>> >> How would you ensure that this kind of pattern doesn't get written
>> >> accidentally (or with many indirections)?
>> >
>> > I don't think that the possibility of writing safe (but yet buggy) code is an
>> > argument against having the possibility of enforcing that a caller must write a
>> > comment for justification on certain things, such as mem::forget().
>> 
>> My argument is that the problem of forgetting a value is not
>> self-contained like `unsafe` code is. Even if we were to document all
>> `forget` or `ManuallyDrop::new` invocations (which we definitely should)
>> we wouldn't get the security that one can't accidentally forget a lock
>> guard. I'm totally in favor of mandating an explaining comment above
>> `forget` calls (but not as a `SAFETY` comment).
>
> Oh, I see where the misunderstanding might lie.
>
> Let's take a look at FileDescriptorReservation::fd_install(). My proposal is to
> have something like:
>
> 	// SANITY: `fd_install` consumed file descriptor
> 	core::mem::forget(self);
> 	// SANITY: `fd_install` consumed file reference
> 	core::mem::forget(file);
>
> Where we have e.g. clippy to complain if there is no "SANITY" (or whatever we
> call it) comment for mem::forget().
>
> I'm not proposing a SAFETY comment.

Ok, that's good :)

As for having a dedicated `SANITY` comment, I'm not sold yet. I'll think
a bit more.

>> > But there's another reason I think having something like this could be
>> > problematic: It might set the wrong incentive, as in "hey, I can just use a
>> > "sanity requirement" in my function rather figuring out how to ensure it through
>> > the type system, etc.".
>> 
>> I don't understand your point here, can you explain it more?
>
> Does the explanation above make this concern clear for you?

Do you mean having sanity requirements would lead to people putting less
effort into designing a type system solution?

---
Cheers,
Benno

Re: [PATCH 1/2] rust: alloc: extend safety requirements of Vec::set_len()

[Danilo Krummrich]

On Mon, Mar 17, 2025 at 05:33:51PM +0000, Benno Lossin wrote:
> On Mon Mar 17, 2025 at 4:57 PM CET, Danilo Krummrich wrote:
> > On Mon, Mar 17, 2025 at 02:57:51PM +0000, Benno Lossin wrote:
> >> On Mon Mar 17, 2025 at 12:12 PM CET, Danilo Krummrich wrote:
> >> > On Mon, Mar 17, 2025 at 09:52:07AM +0000, Benno Lossin wrote:
> >> >> On Sun Mar 16, 2025 at 8:09 PM CET, Danilo Krummrich wrote:
> >> >> > On Sun, Mar 16, 2025 at 07:59:34PM +0100, Danilo Krummrich wrote:
> >> >> >> But let's define it then; what about:
> >> >> >> 
> >> >> >> "[`Vec::set_len`] takes (or kepps) ownership of all elements within the range
> >> >> >> [0; `new_len`] and abandons ownership of all values outside of this range, if
> >> >> >> any."
> >> >> >> 
> >> >> >> The caller may take ownership of the abandoned elements."
> >> >> >> 
> >> >> >> I'd argue that giving up ownership, while offering someone else to take it means
> >> >> >> that it implies that otherwise we'll just end up forgetting about the value.
> >> >> >
> >> >> > Btw. I'd still prefer if we could enforce that the caller has to document what
> >> >> > should happen to the abandoned value. But I acknowledge that the safety comment
> >> >> > isn't the scope for it.
> >> >> >
> >> >> > It'd be great if e.g. clippy would give us a tool to do something analogous to
> >> >> > safety comments.
> >> >> >
> >> >> > It think it would be useful to enfoce some additional safety documentation. For
> >> >> > instance, I think the kernel would much benefit if we could enforce that
> >> >> > mem::forget() must be justified with a comment, since as mentioned ina previous
> >> >> > mail, it can cause fatal bugs, for instance when used on lock guards.
> >> >> 
> >> >> I get where you're coming from, but this probably will very quickly get
> >> >> out of hand.
> >> >> 
> >> >> For example, I can define `forget` safely:
> >> >> 
> >> >>     fn forget<T>(value: T) {
> >> >>         struct Cycle<T> {
> >> >>             this: RefCell<Option<Arc<Self>>>,
> >> >>             value: T,
> >> >>         }
> >> >>         let cycle = Arc::new(Cycle { this: RefCell::new(None), value });
> >> >>         *cycle.this.borrow_mut() = Some(cycle.clone());
> >> >>     }
> >> >> 
> >> >> How would you ensure that this kind of pattern doesn't get written
> >> >> accidentally (or with many indirections)?
> >> >
> >> > I don't think that the possibility of writing safe (but yet buggy) code is an
> >> > argument against having the possibility of enforcing that a caller must write a
> >> > comment for justification on certain things, such as mem::forget().
> >> 
> >> My argument is that the problem of forgetting a value is not
> >> self-contained like `unsafe` code is. Even if we were to document all
> >> `forget` or `ManuallyDrop::new` invocations (which we definitely should)
> >> we wouldn't get the security that one can't accidentally forget a lock
> >> guard. I'm totally in favor of mandating an explaining comment above
> >> `forget` calls (but not as a `SAFETY` comment).
> >
> > Oh, I see where the misunderstanding might lie.
> >
> > Let's take a look at FileDescriptorReservation::fd_install(). My proposal is to
> > have something like:
> >
> > 	// SANITY: `fd_install` consumed file descriptor
> > 	core::mem::forget(self);
> > 	// SANITY: `fd_install` consumed file reference
> > 	core::mem::forget(file);
> >
> > Where we have e.g. clippy to complain if there is no "SANITY" (or whatever we
> > call it) comment for mem::forget().
> >
> > I'm not proposing a SAFETY comment.
> 
> Ok, that's good :)
> 
> As for having a dedicated `SANITY` comment, I'm not sold yet. I'll think
> a bit more.
> 
> >> > But there's another reason I think having something like this could be
> >> > problematic: It might set the wrong incentive, as in "hey, I can just use a
> >> > "sanity requirement" in my function rather figuring out how to ensure it through
> >> > the type system, etc.".
> >> 
> >> I don't understand your point here, can you explain it more?
> >
> > Does the explanation above make this concern clear for you?
> 
> Do you mean having sanity requirements would lead to people putting less
> effort into designing a type system solution?

That'd be one concern, yes.

Re: [PATCH 1/2] rust: alloc: extend safety requirements of Vec::set_len()

[Danilo Krummrich]

> On Sat, Mar 15, 2025 at 12:06 PM Tamir Duberstein <tamird@gmail.com> wrote:
> >
> > On Sat, Mar 15, 2025 at 11:45 AM Danilo Krummrich <dakr@kernel.org> wrote:
> > >
> > > diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
> > > index ae9d072741ce..8540d9e2b717 100644
> > > --- a/rust/kernel/alloc/kvec.rs
> > > +++ b/rust/kernel/alloc/kvec.rs
> > > @@ -189,7 +189,9 @@ pub fn len(&self) -> usize {
> > >      ///
> > >      /// - `new_len` must be less than or equal to [`Self::capacity`].
> > >      /// - If `new_len` is greater than `self.len`, all elements within the interval
> > > -    ///   [`self.len`,`new_len`) must be initialized.
> > > +    ///   [`self.len`,`new_len`) must be initialized,
> >
> > Why a comma here? There's a period on the previous list item, and a
> > period seems more appropriate before the next item.
> 
> Did you see this one? Was it intentional?

Yes, it's the continuation of the sentence of the previous one.

Re: [PATCH 1/2] rust: alloc: extend safety requirements of Vec::set_len()

[Alice Ryhl]

On Sat, Mar 15, 2025 at 04:43:01PM +0100, Danilo Krummrich wrote:
> Extend the safety requirements of set_len() to consider the case when
> the new length is smaller than the old length.
> 
> Fixes: 2aac4cd7dae3 ("rust: alloc: implement kernel `Vec` type")
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---
>  rust/kernel/alloc/kvec.rs | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
> index ae9d072741ce..8540d9e2b717 100644
> --- a/rust/kernel/alloc/kvec.rs
> +++ b/rust/kernel/alloc/kvec.rs
> @@ -189,7 +189,9 @@ pub fn len(&self) -> usize {
>      ///
>      /// - `new_len` must be less than or equal to [`Self::capacity`].
>      /// - If `new_len` is greater than `self.len`, all elements within the interval
> -    ///   [`self.len`,`new_len`) must be initialized.
> +    ///   [`self.len`,`new_len`) must be initialized,
> +    /// - if `new_len` is smaller than `self.len`, all elements within the interval
> +    ///   [`new_len`, `self.len`) must either be dropped or copied and taken ownership of.

This should not be a safety requirement. How about instead adding a
guarantee to the caller that the caller may safely take ownership of the
values?

Alice

Re: [PATCH 1/2] rust: alloc: extend safety requirements of Vec::set_len()

[Benno Lossin]

On Sun Mar 16, 2025 at 7:59 PM CET, Danilo Krummrich wrote:
> On Sun, Mar 16, 2025 at 05:40:29PM +0000, Benno Lossin wrote:
>> On Sun Mar 16, 2025 at 2:46 PM CET, Danilo Krummrich wrote:
>> > On Sun, Mar 16, 2025 at 09:13:50AM -0400, Tamir Duberstein wrote:
>> >> On Sun, Mar 16, 2025 at 9:01 AM Danilo Krummrich <dakr@kernel.org> wrote:
>> >> > On Sun, Mar 16, 2025 at 08:42:43AM -0400, Tamir Duberstein wrote:
>> >> > > On Sun, Mar 16, 2025 at 8:31 AM Danilo Krummrich <dakr@kernel.org> wrote:
>> >> > >
>> >> > > > Let's go with just "must be taken ownership of" then. Unless there's subsequent
>> >> > > > feedback, I won't send a new version for this, since you both already gave your
>> >> > > > (conditional) RB for this.
>> 
>> I didn't realize the `forget` argument immediately, so I sadly have to
>> take my RB back.
>> 
>> >> > > What does it mean to take ownership, if not to run the destructor?
>> >> >
>> >> > Taking responsibility over the decision of what should happen to the value, i.e.
>> >> > forget about the value, drop it right away, keep it alive at a different memory
>> >> > location.
>> >> >
>> >> > > Given Benno's insight, I think the safety text is correct as it
>> >> > > existed before this patch.
>> >> >
>> >> > Without the addition there's no requirement for the caller to take ownership,
>> >> > but that's what we want here. Without the requirement it would be on set_len()
>> >> > to take a decision on what should happen with the value.
>> >> 
>> >> It isn't a safety requirement, right? I think it's fine to document,
>> >> but given that neglecting to run a destructor is safe, it doesn't seem
>> >> to affect safety.
>> >
>> > I don't think there's a rule that safety requirements of unsafe functions must
>> > only be used to guarantee memory safety, etc.
>> 
>> Well that's their only point. Safety documentation should only be
>> concerned with memory safety.
>
> Oops, I wanted to say "must not only be used to *directly* guarantee memory
> safety, etc.".
>
> For instance, it is also used to justify type invariants (that exist to prevent
> UB).
>
> So, what I wanted to say is that setting self.len by iteself is never directly a
> safety critical thing.

In my mental model there is no difference between "possibly immediately
UB" things and "possibly later UB" things. Both of the following
examples produce UB:

    let mut vec: KVec<u32> = KVec::new();
    unsafe { vec.set_len(1) };
    let elem = vec[0]; // UB

and

   let mut vec: KVec<u32> = KVec::new();
   let ptr = vec.as_ptr();
   let elem = unsafe { ptr.read() }; // UB

While in the first example, the UB only occurs when calling a safe
function, it is clearly the fault of the `unsafe` block before that
caused it.

>> You can still put it in the normal documentation though.
>> 
>> > But you're right, if set_len() guarantees a default of what happens to the
>> > value(s) when the caller doesn't take ownership, we could avoid the safety
>> > requirement.
>> 
>> Yeah the default is to forget the value.
>
> It's not really a defined default though, it's just what ultimately happens as
> the consequence of how Vec in general and set_len() are implemented.

What would be an alternative? I'd argue that forgetting is the only
sensible option.

> But let's define it then; what about:
>
> "[`Vec::set_len`] takes (or kepps) ownership of all elements within the range
> [0; `new_len`] and abandons ownership of all values outside of this range, if
> any."
>
> The caller may take ownership of the abandoned elements."

For some reason I don't like the wording of ownership in this case...
I'll try to come up with something better, since you're splitting the
function into `inc` and `dec` anyways (which I think is a good idea), so
I'll reply there.

---
Cheers,
Benno