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

[Danilo Krummrich <dakr@kernel.org>]

On Wed, Aug 07, 2024 at 07:14:13AM +0000, Benno Lossin wrote:
> On 06.08.24 20:55, Danilo Krummrich wrote:
> > On Tue, Aug 06, 2024 at 04:51:28PM +0000, Benno Lossin wrote:
> >> On 05.08.24 17:19, Danilo Krummrich wrote:
> >>> +        let raw_ptr = unsafe {
> >>> +            // If `size == 0` and `ptr != NULL` the memory behind the pointer is freed.
> >>> +            self.0(ptr.cast(), size, flags.0).cast()
> >>> +        };
> >>> +
> >>> +        let ptr = if size == 0 {
> >>> +            NonNull::dangling()
> >>
> >> If we call `realloc(Some(ptr), <layout with size = 0>, ...)`, then this
> >> leaks the pointer returned by the call to `self.0` above. I don't know
> >> what the return value of the different functions are that can appear in
> >> `self.0`, do they return NULL?
> > 
> > That is fine, we don't care about the return value. All `ReallocFunc` free the
> > memory behind `ptr` if called with a size of zero. But to answer the question,
> > they return either NULL or ZERO_SIZE_PTR.
> 
> I see, then it's fine. I think it would help if we know the exact
> behavior of `kmalloc` & friends (either add a link to C docs or write it
> down on `ReallocFunc`).
> 
> >> What about the following sequence:
> >>
> >>     let ptr = realloc(None, <layout with size = 0>, ...);
> >>     let ptr = realloc(Some(ptr), <layout with size = 0>, ...);
> >>
> >> Then the above call to `self.0` is done with a dangling pointer, can the
> >> functions that appear in `self.0` handle that?
> > 
> > This would be incorrect.
> > 
> > Calling `realloc(Some(ptr), <layout with size = 0>, ...)` frees the memory
> > behind `ptr`. This is guranteed behavior for all `ReallocFunc`s, i.e.
> > krealloc(), vrealloc(), kvrealloc().
> 
> Note that I don't use `ptr` afterwards, the code snippet above is
> equivalent to this:
> 
>     let ptr = Kmalloc::alloc(<layout with size = 0>, ...);
>     unsafe { Kmalloc::free(ptr) };
> 
> internally exactly the realloc calls that I put above should be called.

I think I misunderstood what you mean here.

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.

We can decide for a specific dangling pointer to be allowed, i.e. the dangling
pointer returned by `alloc` for a zero sized allocation is always
`dangling<u8>`, so we can assert that `free` is only allowed to be called with
what was previously returned by `alloc` or `free` and therefore disallow
dangling pointers with a different alignment.

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

*1: kfree() can handle dangling pointers up to 16 bytes aligned, see
ZERO_OR_NULL_PTR(x).

> 
> ---
> Cheers,
> Benno
>