On 05.08.24 17:19, Danilo Krummrich wrote:
> So far the kernel's `Box` and `Vec` types can't be used by userspace
> test cases, since all users of those types (e.g. `CString`) use kernel
> allocators for instantiation.
>
> In order to allow userspace test cases to make use of such types as
> well, implement the `Cmalloc` allocator within the allocator_test module
> and type alias all kernel allocators to `Cmalloc`. The `Cmalloc`
> allocator uses libc's realloc() function as allocator backend.
>
> Signed-off-by: Danilo Krummrich <dakr@xxxxxxxxxx>
> ---
> I know, having an `old_size` parameter would indeed help implementing `Cmalloc`.
>
> However, I really don't want test infrastructure to influence the design of
> kernel internal APIs.
>
> Besides that, adding the `old_size` parameter would have the consequence that
> we'd either need to honor it for kernel allocators too (which adds another
> source of potential failure) or ignore it for all kernel allocators (which
> potentially tricks people into taking wrong assumptions on how the API works).
> ---
> rust/kernel/alloc/allocator_test.rs | 91 ++++++++++++++++++++++++++---
> 1 file changed, 84 insertions(+), 7 deletions(-)
>
> diff --git a/rust/kernel/alloc/allocator_test.rs b/rust/kernel/alloc/allocator_test.rs
> index 1b2642c547ec..05fd75b89238 100644
> --- a/rust/kernel/alloc/allocator_test.rs
> +++ b/rust/kernel/alloc/allocator_test.rs
> @@ -2,20 +2,97 @@
>
> #![allow(missing_docs)]
>
> -use super::{AllocError, Allocator, Flags};
> +use super::{flags::*, AllocError, Allocator, Flags};
> use core::alloc::Layout;
> +use core::ptr;
> use core::ptr::NonNull;
>
> -pub struct Kmalloc;
> +pub struct Cmalloc;
> +pub type Kmalloc = Cmalloc;
> pub type Vmalloc = Kmalloc;
> pub type KVmalloc = Kmalloc;
>
> -unsafe impl Allocator for Kmalloc {
> +extern "C" {
> + #[link_name = "aligned_alloc"]
> + fn libc_aligned_alloc(align: usize, size: usize) -> *mut core::ffi::c_void;
> +
> + #[link_name = "free"]
> + fn libc_free(ptr: *mut core::ffi::c_void);
> +
> + // Do not use this function for production code! For test cases only it's
> + // probably fine if used with care.
> + #[link_name = "malloc_usable_size"]
> + fn libc_malloc_usable_size(ptr: *mut core::ffi::c_void) -> usize;
> +}
> +
> +unsafe impl Allocator for Cmalloc {
> + fn alloc(layout: Layout, flags: Flags) -> Result<NonNull<[u8]>, AllocError> {
> + let layout = layout.pad_to_align();
> +
> + // 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;
> +
> + if flags.contains(__GFP_ZERO) && !raw_ptr.is_null() {
> + // SAFETY: `raw_ptr` points to memory successfully allocated with `libc_aligned_alloc`.
> + let size = unsafe { libc_malloc_usable_size(raw_ptr.cast()) };
> +
> + // SAFETY: `raw_ptr` points to memory successfully allocated with `libc_aligned_alloc`
> + // of at least `size` bytes.
> + unsafe { core::ptr::write_bytes(raw_ptr, 0, size) };
> + }
> +
> + let ptr = if layout.size() == 0 {
> + NonNull::dangling()
Why do you call `libc_aligned_alloc` when you return `dangling()`
anyways when size is zero? I would move this check upwards.
> + } else {
> + NonNull::new(raw_ptr).ok_or(AllocError)?
Would also make sense to do this above the null check.
> + };
> +
> + Ok(NonNull::slice_from_raw_parts(ptr, layout.size()))
> + }
> +
> unsafe fn realloc(
> - _ptr: Option<NonNull<u8>>,
> - _layout: Layout,
> - _flags: Flags,
> + ptr: Option<NonNull<u8>>,
> + layout: Layout,
> + flags: Flags,
> ) -> Result<NonNull<[u8]>, AllocError> {
> - panic!();
> + let layout = layout.pad_to_align();
> + let src: *mut u8 = if let Some(src) = ptr {
> + src.as_ptr().cast()
> + } else {
> + ptr::null_mut()
> + };
> +
> + if layout.size() == 0 {
> + // SAFETY: `src` is either NULL or has previously been allocatored with this
> + // `Allocator`.
> + unsafe { libc_free(src.cast()) };
> +
> + return Ok(NonNull::slice_from_raw_parts(NonNull::dangling(), 0));
> + }
> +
> + let dst = Self::alloc(layout, flags)?;
> +
> + if src.is_null() {
> + return Ok(dst);
> + }
> +
> + // SAFETY: `src` is either NULL or has previously been allocatored with this `Allocator`.
> + let old_size = unsafe { libc_malloc_usable_size(src.cast()) };
Citing man malloc_usable_size(3):
CAVEATS
The value returned by malloc_usable_size() may be greater than
the requested size of the allocation because of various internal
implementation details, none of which the programmer should rely
on. This function is intended to only be used for diagnostics
and statistics; writing to the excess memory without first
calling realloc(3) to resize the allocation is not supported.
The returned value is only valid at the time of the call.
While you don't write, you might read below, which might not be OK?
---
Cheers,
Benno
> +
> + // SAFETY: `src` has previously been allocated with this `Allocator`; `dst` has just been
> + // newly allocated. Taking the minimum of their sizes guarantees that we do not exceed
> + // either bounds.
> + unsafe {
> + // Always copy -- do not rely on potential spare memory reported by
> + // malloc_usable_size() which technically may still be sufficient.
> + ptr::copy_nonoverlapping(
> + src,
> + dst.as_ptr().cast(),
> + core::cmp::min(layout.size(), old_size),
> + )
> + };
> +
> + Ok(dst)
> }
> }
> --
> 2.45.2
>
