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

[Danilo Krummrich <dakr@kernel.org>]

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.