Re: [PATCH v4 04/28] rust: alloc: implement `Allocator` for `Kmalloc`

[Danilo Krummrich <dakr@kernel.org>]

On Thu, Aug 08, 2024 at 08:55:29AM +0000, Benno Lossin wrote:
> On 08.08.24 01:05, Danilo Krummrich wrote:
> > On Wed, Aug 07, 2024 at 08:15:41PM +0000, Benno Lossin wrote:
> >>> So, that's not permitted. `free` can't be called with a dangling pointer. The
> >>> kernel free functions (*1) can't handle it, and I can't detect it, since a
> >>> dangling pointer does not have a descrete value.
> >>
> >> That is true, but only if we do not have access to the old layout of the
> >> allocation. If we add `old_layout` as a parameter, then we can handle
> >> dangling pointers.
> > 
> > Then we'd need `free` to be `fn free(ptr: NonNull<u8>, layout: Layout)` just to
> > compare whether `ptr.as_ptr() == layout.align() as _`. Same for `realloc`, but
> > that's less weird.
> 
> I don't think that's a problem (though we would check against the size
> and not compare the address!). `Allocator` from stdlib also has the
> extra argument.

Because `Allocator` from stdlib actually needs it, e.g. they have to open code
`realloc`, since userspace allocators don't give proper alignment guarantees.

Again, the kernel allocators neither need, nor take and honor this extra
information.

> 
> > It's also not that we safe code with that. `Box`, `Vec`, any other user, still
> > would have to create the `Layout` before they call `A::free`.
> 
> But for `Box` it would just be `Layout::<T>::new()` and `Vec` needs
> `Layout::<T>::new().repeat(self.cap)`.

Let's take `Vec` for instance, there it's either

current code
------------
```
	if cap != 0 {
		unsafe { A::free(self.ptr.as_non_null().cast()) };
	}
```

your proposal
-------------
```
	let layout = Layout::<T>::array(self.cap).unwrap();
	unsafe { A::free(self.ptr.as_non_null().cast(), layout) };
```

I really don't see how that's an improvement.

> 
> Though for `repeat` we need the `alloc_layout_extra` feature, which is
> an argument against doing this.
> 
> >>> Surely, we could also let the caller pass the old alignment, but this all sounds
> >>> complicated for something that is very trivial for the caller to take care of,
> >>> i.e. just don't try to free something that was never actually allocated.
> >>>
> >>> It can also lead to subtle bugs, e.g. what if someone calls `Box::from_raw` for
> >>> a ZST with some other random pointer? Currently, that doesn't hurt us, which for
> >>> robustness, seems to be a good thing.
> >>
> >> I think we should forbid that. To me it's just plain wrong to take a
> >> random integer literal and cast it to a ZST. IIRC it even is UB if that
> >> points to a previously allocated object that has been freed (but I don't
> >> remember where I read it...).
> > 
> > I think my argument about robustness still holds even if we forbid it.
> > 
> > The documentation says "For operations of size zero, every pointer is valid,
> > including the null pointer." [1]
> 
> How does this increase robustness? I am not allowed to call `free` with
> a random pointer, only pointers returned by that allocator. The same
> should also be true for `Box::from_raw`. That way the ZST dangling
> pointer stuff leaks into that API.

This point was about your first proposal to use the alignment. With cosidering
only the alignment we can't handle the case where `Box::from_raw` is called with
a random pointer for a ZST, which *technically* isn't illegal. We can define it
as illegal, but I'd consider it to be more robust, if we don't oops in case.

But as you say checking the size instead of the alignment does not have this
problem.

> 
> > [1] https://doc.rust-lang.org/std/ptr/index.html
> > 
> >>
> >> Also if we use the size of the old layout instead of comparing alignment
> >> with the address of the pointer, then we avoid this issue.
> > 
> > That is just another problem when passing the old `Layout` (or maybe just the
> > old size as usize). Neither do we need the old size, nor do we honor it with any
> > kernel allocator. This has the following implications:
> > 
> > (1) We never see any error if the old size that is passed is garbage (unless
> >     it's non-zero, when it should be zero and vice versa), which is bad.
> 
> We could add debug support for that though?

And add even more complexity just to avoid a simple `if !zero` check before
calling `free`? I don't like that.

Just to clarify, I'm not against passing the old layout in general. But I really
don't want to pass something in that is not required *and* not honored by a
single kernel allocator.

IMO, the only other valid reason to accept unneeded arguments would be if we
could use the `Allocator` interface from stdlib.

> 
> > (2) We'd need `free` to be `fn free(ptr: NonNull<u8>, size: usize)`, which is
> >     confusing, because it implies that an actual free relies on this size for
> >     freeing the memory.
> 
> I don't think that it is confusing to ask for the old layout.

It is always confusing if a function asks for information that it doesn't need
and doesn't consider, because asking for it in the first place creates the
impression that it is indeed needed and considered.

> 
> > If we want to avoid (1) and (2), we'd need to make it
> > `fn free(ptr: NonNull<u8>, zero: bool)` instead, but then also the caller can
> > just check this boolean and conditionally call `free`.
> 
> Yeah having `free` with a `zero: bool` sounds like a bad idea.
> 
> > I don't really see why it's better to let `free` do the `if !zero` check.
> 
> You did not reply to my last argument:
> 
> >> I think it's better to just let `Box` and `Vec` figure out if calling `free` is
> >> the right thing to do. The code for that is simple and obvious, i.e. check if
> >> `T` is a ZST.
> >
> > I don't think that it is as simple as you make it out to be. To me this
> > is functionality that we can move into one place and never have to think
> > about again.
> > Also if we at some point decide to add some sort of debugging allocator
> > (am I right in assuming that such a thing already exists for C?) then we

The C side allocators have quite a lot of debugging capabilities, but there is
no separate one.

> > might want to tag on extra data in the allocation, in that case it would
> > be very useful if the allocator also saw zero-sized allocations, since
> > we should check all allocations.

What would such a debugging allocator do on the Rust side?

The allocators we have are just simple wrappers around the real kernel
allocators from the C side.

I don't really see the need for some sort of debugging allocator.