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

[Benno Lossin <benno.lossin@proton.me>]

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.

> 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)`.

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.

> [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?

> (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.

> 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
> 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.

---
Cheers,
Benno