Message-ID: <9e22d4ab-eff2-4c69-8a2f-f55a8eaeb892@proton.me>
Date: Wed, 07 Aug 2024 20:15:41 +0000
From: Benno Lossin <benno.lossin@...ton.me>
To: Danilo Krummrich <dakr@...nel.org>
Cc: ojeda@...nel.org, alex.gaynor@...il.com, wedsonaf@...il.com, boqun.feng@...il.com, gary@...yguo.net, bjorn3_gh@...tonmail.com, a.hindborg@...sung.com, aliceryhl@...gle.com, akpm@...ux-foundation.org, daniel.almeida@...labora.com, faith.ekstrand@...labora.com, boris.brezillon@...labora.com, lina@...hilina.net, mcanal@...lia.com, zhiw@...dia.com, acurrid@...dia.com, cjia@...dia.com, jhubbard@...dia.com, airlied@...hat.com, ajanulgu@...hat.com, lyude@...hat.com, linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org, linux-mm@...ck.org
Subject: Re: [PATCH v4 04/28] rust: alloc: implement `Allocator` for `Kmalloc`

On 07.08.24 12:11, Danilo Krummrich wrote:
> 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.

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.

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

I don't like that.

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

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.

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

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

I agree with you that we shouldn't rely on that.

---
Cheers,
Benno

Powered by blists - more mailing lists