[PATCH] rust: alloc: add missing trait item MIN_ALIGN to Cmalloc

[PATCH] rust: alloc: add missing trait item MIN_ALIGN to Cmalloc

[Danilo Krummrich]

Cmalloc is missing the trait item MIN_ALIGN introduced by commit
1b1a946dc2b5 ("rust: alloc: specify the minimum alignment of each
allocator"), causing the following error on the `rusttest` make target.

	error[E0046]: not all trait items implemented, missing: `MIN_ALIGN`
	   --> rust/kernel/alloc/allocator_test.rs:37:1
	    |
	37  | unsafe impl Allocator for Cmalloc {
	    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ missing `MIN_ALIGN` in implementation
	    |
	   ::: rust/kernel/alloc.rs:146:5
	    |
	146 |     const MIN_ALIGN: usize;
	    |     ---------------------- `MIN_ALIGN` from trait

Implement MIN_ALIGN for Cmalloc to fix this.

Reported-by: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
Closes: https://lore.kernel.org/all/CANiq72k0FSBTB2yOjiAy9PnAuyM=-PHxL3uQQ_Cv+zwswnr_bA@mail.gmail.com/
Fixes: 1b1a946dc2b5 ("rust: alloc: specify the minimum alignment of each allocator")
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/kernel/alloc/allocator_test.rs | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/rust/kernel/alloc/allocator_test.rs b/rust/kernel/alloc/allocator_test.rs
index a3074480bd8d..0d3c78ddcd69 100644
--- a/rust/kernel/alloc/allocator_test.rs
+++ b/rust/kernel/alloc/allocator_test.rs
@@ -35,6 +35,8 @@
 // - passing a pointer to a valid memory allocation created by this `Allocator` is always OK,
 // - `realloc` provides the guarantees as provided in the `# Guarantees` section.
 unsafe impl Allocator for Cmalloc {
+    const MIN_ALIGN: usize = bindings::ARCH_KMALLOC_MINALIGN;
+
     unsafe fn realloc(
         ptr: Option<NonNull<u8>>,
         layout: Layout,

base-commit: ac9eea3d08c25fb213deb113d246ff5dadb31fbc
-- 
2.50.1

Re: [PATCH] rust: alloc: add missing trait item MIN_ALIGN to Cmalloc

[Alice Ryhl]

On Sun, Aug 24, 2025 at 2:07 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> Cmalloc is missing the trait item MIN_ALIGN introduced by commit
> 1b1a946dc2b5 ("rust: alloc: specify the minimum alignment of each
> allocator"), causing the following error on the `rusttest` make target.
>
>         error[E0046]: not all trait items implemented, missing: `MIN_ALIGN`
>            --> rust/kernel/alloc/allocator_test.rs:37:1
>             |
>         37  | unsafe impl Allocator for Cmalloc {
>             | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ missing `MIN_ALIGN` in implementation
>             |
>            ::: rust/kernel/alloc.rs:146:5
>             |
>         146 |     const MIN_ALIGN: usize;
>             |     ---------------------- `MIN_ALIGN` from trait
>
> Implement MIN_ALIGN for Cmalloc to fix this.
>
> Reported-by: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
> Closes: https://lore.kernel.org/all/CANiq72k0FSBTB2yOjiAy9PnAuyM=-PHxL3uQQ_Cv+zwswnr_bA@mail.gmail.com/
> Fixes: 1b1a946dc2b5 ("rust: alloc: specify the minimum alignment of each allocator")
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>

Thanks for taking of it. Assuming a satisfactory answer to the quesiton below:

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

> ---
>  rust/kernel/alloc/allocator_test.rs | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/rust/kernel/alloc/allocator_test.rs b/rust/kernel/alloc/allocator_test.rs
> index a3074480bd8d..0d3c78ddcd69 100644
> --- a/rust/kernel/alloc/allocator_test.rs
> +++ b/rust/kernel/alloc/allocator_test.rs
> @@ -35,6 +35,8 @@
>  // - passing a pointer to a valid memory allocation created by this `Allocator` is always OK,
>  // - `realloc` provides the guarantees as provided in the `# Guarantees` section.
>  unsafe impl Allocator for Cmalloc {
> +    const MIN_ALIGN: usize = bindings::ARCH_KMALLOC_MINALIGN;

Is this the right value for normal malloc?

Alice

Re: [PATCH] rust: alloc: add missing trait item MIN_ALIGN to Cmalloc

[Danilo Krummrich]

On Sun Aug 24, 2025 at 2:36 PM CEST, Alice Ryhl wrote:
> On Sun, Aug 24, 2025 at 2:07 PM Danilo Krummrich <dakr@kernel.org> wrote:
>> +    const MIN_ALIGN: usize = bindings::ARCH_KMALLOC_MINALIGN;
>
> Is this the right value for normal malloc?

Heh! ARCH_KMALLOC_MINALIGN should be correct, because the Cmalloc implementation
should (ideally) enforce a minimum alignment of ARCH_KMALLOC_MINALIGN for
compatibility reasons.

However, double checking the existing code, it doesn't. Hence, the correct value
must either be

	const MIN_ALIGN: usize = align_of::<crate::ffi::c_ulonglong>();

instead. Or, we have to actually enforce ARCH_KMALLOC_MINALIGN.

Given that this fix is only for within the -rc cycles, i.e. we will remove
allocator_test.rs before the MIN_ALIGN stuff ever hits an actual release, just
using align_of::<crate::ffi::c_ulonglong>() should be fine.

Re: [PATCH] rust: alloc: add missing trait item MIN_ALIGN to Cmalloc

[Danilo Krummrich]

On Sun Aug 24, 2025 at 3:04 PM CEST, Danilo Krummrich wrote:
> On Sun Aug 24, 2025 at 2:36 PM CEST, Alice Ryhl wrote:
>> On Sun, Aug 24, 2025 at 2:07 PM Danilo Krummrich <dakr@kernel.org> wrote:
>>> +    const MIN_ALIGN: usize = bindings::ARCH_KMALLOC_MINALIGN;
>>
>> Is this the right value for normal malloc?
>
> Heh! ARCH_KMALLOC_MINALIGN should be correct, because the Cmalloc implementation
> should (ideally) enforce a minimum alignment of ARCH_KMALLOC_MINALIGN for
> compatibility reasons.
>
> However, double checking the existing code, it doesn't. Hence, the correct value
> must either be
>
> 	const MIN_ALIGN: usize = align_of::<crate::ffi::c_ulonglong>();
>
> instead. Or, we have to actually enforce ARCH_KMALLOC_MINALIGN.
>
> Given that this fix is only for within the -rc cycles, i.e. we will remove
> allocator_test.rs before the MIN_ALIGN stuff ever hits an actual release, just
> using align_of::<crate::ffi::c_ulonglong>() should be fine.

Just for completeness, allocator_test.rs is also broken for the assumption that
Vmalloc allocated memory is always PAGE_SIZE aligned.

Vmalloc::MIN_ALIGN, of course, does not report PAGE_SIZE in the case of
allocator_test.rs being in charge, but it's an assumption that people might
rightfully rely on in their (unsafe) code.

In the end, just another argument for getting rid of allocator_test.rs. :)

Re: [PATCH] rust: alloc: add missing trait item MIN_ALIGN to Cmalloc

[Danilo Krummrich]

On Sun Aug 24, 2025 at 3:18 PM CEST, Danilo Krummrich wrote:
> In the end, just another argument for getting rid of allocator_test.rs. :)

Actually, I think it's better to just do it:

Miguel, mind if I just apply [1] and [2]? (Just recognizing, probably this was
your intention anyways?)

[1] https://lore.kernel.org/rust-for-linux/20250726180750.2735836-1-ojeda@kernel.org/
[2] https://lore.kernel.org/rust-for-linux/20250816211900.2731720-1-ojeda@kernel.org/

Re: [PATCH] rust: alloc: add missing trait item MIN_ALIGN to Cmalloc

[Miguel Ojeda]

On Sun, Aug 24, 2025 at 3:29 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> Actually, I think it's better to just do it:
>
> Miguel, mind if I just apply [1] and [2]? (Just recognizing, probably this was
> your intention anyways?)

Yeah, since 1b1a946dc2b5 is in the same branch, and David already
reviewed the main patch too, I think it is easier for you than trying
to deal with all this otherwise.

(Whether applying or rebasing both are fine I would say, since
`rusttest` is not a big deal and the chances of people bisecting on
that are low.)

For the other fix I sent that you already applied through DRM, I think
we did the right thing, because it was not `alloc-next` and it was
fairly trivial, so it made sense.

Thanks!

Cheers,
Miguel

Re: [PATCH] rust: alloc: add missing trait item MIN_ALIGN to Cmalloc

[Danilo Krummrich]

On Sun Aug 24, 2025 at 4:07 PM CEST, Miguel Ojeda wrote:
> Yeah, since 1b1a946dc2b5 is in the same branch, and David already
> reviewed the main patch too, I think it is easier for you than trying
> to deal with all this otherwise.

Yeah, I also think there's nothing to backport.

I.e. the fact that Cmalloc does not provide the same minimum alignment
guarantees as Kmalloc and Vmalloc is not an issue for existing upstream code.

And fixing it for potential additions to the rusttest target in downstream code
doesn't seem worth the effort.

> For the other fix I sent that you already applied through DRM, I think
> we did the right thing, because it was not `alloc-next` and it was
> fairly trivial, so it made sense.

Yes, that one is correct.