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
