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

[Danilo Krummrich <dakr@kernel.org>]

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.