Re: [PATCH v6 22/26] rust: alloc: implement `Cmalloc` in module allocator_test

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

On 30.08.24 00:25, Danilo Krummrich wrote:
> On Thu, Aug 29, 2024 at 07:14:18PM +0000, Benno Lossin wrote:
>> On 16.08.24 02:11, Danilo Krummrich wrote:
>>> +impl Cmalloc {
>>> +    /// Adjust the size and alignment such that we can additionally store `CmallocData` right
>>> +    /// before the actual data described by `layout`.
>>> +    ///
>>> +    /// Example:
>>> +    ///
>>> +    /// For `CmallocData` assume an alignment of 8 and a size of 16.
>>> +    /// For `layout` assume and alignment of 16 and a size of 64.
>>
>> This looks like you want it rendered as bulletpoints (but it won't).
> 
> Actually, that wasn't my intention, but I'm fine changing that.

I see, in that case not putting a newline there is also fine with me.
But I think bulletpoints are probably easier to read.

>>> +    fn alloc_store_data(layout: Layout) -> Result<NonNull<u8>, AllocError> {
>>> +        let requested_size = layout.size();
>>> +
>>> +        let layout = Self::layout_adjust(layout)?;
>>> +        let min_align = layout.align() / 2;
>>> +
>>> +        // SAFETY: Returns either NULL or a pointer to a memory allocation that satisfies or
>>> +        // exceeds the given size and alignment requirements.
>>> +        let raw_ptr = unsafe { libc_aligned_alloc(layout.align(), layout.size()) } as *mut u8;
>>> +
>>> +        let priv_ptr = NonNull::new(raw_ptr).ok_or(AllocError)?;
>>> +
>>> +        // SAFETY: Advance the pointer by `min_align`. The adjustments from `Self::layout_adjust`
>>> +        // ensure that after this operation the original size and alignment requirements are still
>>> +        // satisfied or exceeded.
>>
>> This SAFETY comment should address why it's OK to call `add`. You
>> justify something different, namely why the allocation still satisfies
>> the requirements of `layout`. That is something that this function
>> should probably guarantee.
> 
> So, I guess you're arguing that instead I should say that, we're still within
> the bounds of the same allocated object and don't exceed `isize`?

Yes.

>>> +    unsafe fn free_read_data(ptr: NonNull<u8>) {
>>> +        // SAFETY: `ptr` has been created by `Self::alloc_store_data`.
>>> +        let data = unsafe { Self::data(ptr) };
>>> +
>>> +        // SAFETY: `ptr` has been created by `Self::alloc_store_data`.
>>> +        let priv_ptr = unsafe { ptr.as_ptr().sub(data.offset) };
>>> +
>>> +        // SAFETY: `priv_ptr` has previously been allocatored with this `Allocator`.
>>> +        unsafe { libc_free(priv_ptr.cast()) };
>>> +    }
>>> +}
>>> +
>>> +unsafe impl Allocator for Cmalloc {
>>> +    fn alloc(layout: Layout, flags: Flags) -> Result<NonNull<[u8]>, AllocError> {
>>> +        if layout.size() == 0 {
>>> +            return Ok(NonNull::slice_from_raw_parts(NonNull::dangling(), 0));
>>> +        }
>>> +
>>> +        let ptr = Self::alloc_store_data(layout)?;
>>> +
>>> +        if flags.contains(__GFP_ZERO) {
>>> +            // SAFETY: `Self::alloc_store_data` guarantees that `ptr` points to memory of at least
>>> +            // `layout.size()` bytes.
>>> +            unsafe { ptr.as_ptr().write_bytes(0, layout.size()) };
>>> +        }
>>
>> This makes me wonder, what other flags should we handle for this
>> allocator?
> 
> I don't think there are any other flags that we can handle. The only other one
> that'd make sense is __GFP_NOFAIL, but we can't guarantee that.
> 
> If any specific gfp flags are needed, I think it's simply not a candidate for a
> userspace test.
> 
> If we really want to do something here, we could whitelist the flags we ignore,
> since they do not matter (such as __GFP_NOWARN) and panic() for everything else.
> 
> But I don't think that's really needed.

Makes sense, just wanted to check that this has been accounted for.

---
Cheers,
Benno