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

[Danilo Krummrich <dakr@kernel.org>]

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/