On Sun, 15 Sep 2024 20:22:42 +0100 Gary Guo <gary@xxxxxxxxxxx> wrote: > On Sun, 15 Sep 2024 19:02:40 +0200 > Danilo Krummrich <dakr@xxxxxxxxxx> wrote: > > > Hi Gary, > > > > thanks for taking a look. > > > > On Sun, Sep 15, 2024 at 04:28:13PM +0100, Gary Guo wrote: > > > On Thu, 12 Sep 2024 00:52:37 +0200 > > > Danilo Krummrich <dakr@xxxxxxxxxx> wrote: > > > > > > > Add a kernel specific `Allocator` trait, that in contrast to the one in > > > > Rust's core library doesn't require unstable features and supports GFP > > > > flags. > > > > > > > > Subsequent patches add the following trait implementors: `Kmalloc`, > > > > `Vmalloc` and `KVmalloc`. > > > > > > Hi Danilo, > > > > > > I think the current design is unsound regarding ZST. > > > > > > Let's say that `Allocator::alloc` gets called with a ZST type with > > > alignment of 4096. Your implementation will call into `krelloc` with > > > new_size of 0, which gets turned into of `kfree` of null pointer, which > > > is no-op. Everything is fine so far. Krealloc returns `ZERO_SIZE_PTR`, > > > and then implementation of `<Kmalloc as Allocator>::realloc` throws it > > > away and returns `NonNull::dangling`. > > > > > > Since `NonNull::dangling` is called with T=u8, this means the pointer > > > returns is 1, and it's invalid for ZSTs with larger alignments. > > > > Right, this interface is not meant to handle "allocations" for ZSTs. > > > > But you're right, since `alloc` is a safe function, we should return a properly > > aligned pointer. > > > > > > > > And this is unfixable even if the realloc implementation is changed. > > > Let's say the realloc now returns a dangling pointer that is suitable > > > aligned. Now let's see what happens when the `Allocator::free` is > > > called. `kfree` would be trying to free a Rust-side ZST pointer, but it > > > has no way to know that it's ZST! > > > > Right, that's why it's not valid to call `free` with dangling pointers. > > > > From the safety comment of `free`: > > > > "`ptr` must point to an existing and valid memory allocation created by this > > `Allocator` and must not be a dangling pointer." > > > > We still need the same in `realloc` though. > > I don't agree with this reading. If you allocate something with `alloc` > and it doesn't return an error then you should be able to feed it to > `free`. Whether the allocator does actual allocation when size is zero > or return a dangling pointer shouldn't matter to the caller. > > The fact you `Kmalloc` returns a dangling pointer for ZST is an > implementation detail and the caller shouldn't care (and it also > couldn't check whether it's a dangling pointer). Nothing in your > `alloc` doc mention about dangling pointer return for zero-sized alloc > at all. > > > > > > > > > I can see 3 ways of fixing this: > > > 1. Reject ZSTs that have larger alignment than 16 and fix the realloc > > > implementation to return suitable aligned ZST pointer. I don't > > > particularly like the idea of allocating ZST can fail though. > > > 2. Say ZST must be handled by the caller, and make alloc function > > > unsafe. This means that we essentially revert to the `GlobalAlloc` > > > design of Rust, and all callers have to check for ZST. > > > 3. Accept the `old_layout` and use it to check whether the allocation > > > is ZST allocation. > > > > > > My personal preference is 3. > > > > There is also 4. > > > > Let `alloc` and `realloc` return a properly aligned dangling pointer for > > `size == 0` and don't accept dangling pointers in `realloc` and `free`. > > I'll consider the API design to be bad if I can't pass allocated pointer to > free. If caller needs to handle ZST specially then we might as well > just ban it completely. > > > And 5. > > > > Reject the combination of `None` and `size == 0` entirely, as earlier proposed > > by Benno. > > > > I'm fine with both, 4. and 5. with a slight preference for 4. > > > > I'd also go along with 1., as a mix of 4. and 5. > > > > I really don't like making `alloc` unsafe, and I really don't want to have > > `old_layout` in `free`. Please let's not discuss this again. :-) > > I don't buy it. > > Your argument for having `old_layout` is so that the caller doesn't > need to care about the size. But as demonstrated the caller *does* need > to care about whether the size is zero. > > Our previous discussion doesn't cover the particular case of ZST and > you said that it reason arise that we need this extra parameter, then > it could be added. It feels to me that sane behaviour when it comes > to ZST allocation is a very good reason. Just to add that if you *really* want to avoid the old layout param, another approach accceptable to me is to have a `NonZeroLayout` and have `alloc` only accept that. Either `Allocator` trait handles ZST well or it refuses to handle them at all. No "it works for alloc but not for free" please. Best, Gary > > > > > > > > > > > Reviewed-by: Alice Ryhl <aliceryhl@xxxxxxxxxx> > > > > Signed-off-by: Danilo Krummrich <dakr@xxxxxxxxxx> > > > > --- > > > > rust/kernel/alloc.rs | 112 +++++++++++++++++++++++++++++++++++++++++++ > > > > 1 file changed, 112 insertions(+) >
