Re: [PATCH v7 01/26] rust: alloc: add `Allocator` trait

[Gary Guo]

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(+)  
>