Re: [RFC PATCH] drm: panthor: add dev_coredumpv support

[Alice Ryhl <aliceryhl@google.com>]

On Tue, Jul 23, 2024 at 3:41 PM Daniel Almeida
<daniel.almeida@collabora.com> wrote:
>
> Hi Alice, thanks for the review!
>
>
> >> +        fn alloc_mem(&mut self, size: usize) -> Option<*mut u8> {
> >> +            assert!(size % 8 == 0, "Allocation size must be 8-byte aligned");
> >> +            if isize::try_from(size).unwrap() == isize::MAX {
> >> +                return None;
> >> +            } else if self.pos + size > self.capacity {
> >> +                kernel::pr_debug!("DumpAllocator out of memory");
> >> +                None
> >> +            } else {
> >> +                let offset = self.pos;
> >> +                self.pos += size;
> >> +
> >> +                // Safety: we know that this is a valid allocation, so
> >> +                // dereferencing is safe. We don't ever return two pointers to
> >> +                // the same address, so we adhere to the aliasing rules. We make
> >> +                // sure that the memory is zero-initialized before being handed
> >> +                // out (this happens when the allocator is first created) and we
> >> +                // enforce a 8 byte alignment rule.
> >> +                Some(unsafe { self.mem.as_ptr().offset(offset as isize) as *mut u8 })
> >> +            }
> >> +        }
> >> +
> >> +        pub(crate) fn alloc<T>(&mut self) -> Option<&mut T> {
> >> +            let mem = self.alloc_mem(core::mem::size_of::<T>())? as *mut T;
> >> +            // Safety: we uphold safety guarantees in alloc_mem(), so this is
> >> +            // safe to dereference.
> >
> > This code doesn't properly handle when T requires a large alignment.
> >
>
> Can you expand a bit on this? IIRC the alignment of a structure/enum will be dictated
> by the field with the largest alignment requirement, right? Given that the largest primitive
> allowed in the kernel is u64/i64, shouldn’t this suffice, e.g.:

It's possible for Rust types to have a larger alignment using e.g.
#[repr(align(64))].

>  +            assert!(size % 8 == 0, "Allocation size must be 8-byte aligned");
>
>
> >> +            Some(unsafe { &mut *mem })
> >> +        }
> >> +
> >> +        pub(crate) fn alloc_bytes(&mut self, num_bytes: usize) -> Option<&mut [u8]> {
> >> +            let mem = self.alloc_mem(num_bytes)?;
> >> +
> >> +            // Safety: we uphold safety guarantees in alloc_mem(), so this is
> >> +            // safe to build a slice
> >> +            Some(unsafe { core::slice::from_raw_parts_mut(mem, num_bytes) })
> >> +        }
> >
> > Using references for functions that allocate is generally wrong.
> > References imply that you don't have ownership of the memory, but
> > allocator functions would normally return ownership of the allocation.
> > As-is, the code seems to leak these allocations.
>
> All the memory must be given to dev_coredumpv(), which will then take
> ownership.  dev_coredumpv() will free all the memory, so there should be no
> leaks here.
>
> I’ve switched to KVec in v2, so that will also cover the error paths,
> which do leak in this version, sadly.
>
> As-is, all the memory is pre-allocated as a single chunk. When space is carved
> for a given T, a &mut is returned so that the data can be written in-place at
> the right spot in said chunk.
>
> Not only there shouldn’t be any leaks, but I can actually decode this from
> userspace.
>
> I agree that this pattern isn’t usual, but I don’t see anything
> incorrect. Maybe I missed something?

Interesting. So the memory is deallocated when self is destroyed? A
bit unusual, but I agree it is correct if so. Sorry for the confusion
:)

Alice