rust: alloc: satisfy POSIX alignment requirement

PATCH Superseded Tamir Duberstein 4 participants 9 messages Feb 10, 2025 – Feb 11, 2025

Subsystems (3)

Man-pages: manual pages for Linux -- Sections 2, 3, 4, 5, and 7 Rust Rust alloc

lore

Share & tools

lore

Tamir Duberstein

author

2025-02-10 14:55 UTC #0 lore raw

PATCH v4 1/1 rust: alloc: satisfy POSIX alignment requirement

ISO C's aligned_alloc is partially implementation-defined; on some systems it inherits stricter requirements from POSIX's posix_memalign.

This causes the call added in commit dd09538fb409 ("rust: alloc:
implement Cmalloc in module allocator_test") to fail on macOS because it doesn't meet the requirements of posix_memalign.

Adjust the call to meet the POSIX requirement and add a comment. This fixes failures in make rusttest on macOS.

Fixes dd09538fb409 ("rust: alloc: implement `Cmalloc` in module allocator_test")

Acked-by Danilo Krummrich <[email protected]>

Signed-off-by Tamir Duberstein <[email protected]>

---
Changes in v4:
- Revert to aligned_alloc and correct rationale. (Miguel Ojeda)
- Apply Danilo's Acked-by from v2.
- Rebase on v6.14-rc2.
- Link to v3: https://lore.kernel.org/r/[email protected]

Changes in v3:
- Replace aligned_alloc with posix_memalign for portability.
- Link to v2: https://lore.kernel.org/r/[email protected]

Changes in v2:
- Shorten some variable names. (Danilo Krummrich)
- Replace shadowing alignment variable with a second call to
Layout::align. (Danilo Krummrich)
- Link to v1: https://lore.kernel.org/r/[email protected]

---
 rust/kernel/alloc/allocator_test.rs | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

rust/kernel/alloc/allocator_test.rs +22 -1

index e3240d16040b..1c881ed73d79 100644
@@ -62,9 +62,30 @@ unsafe fn realloc(
             ));
         }
 
+        // ISO C (ISO/IEC 9899:2011) defines `aligned_alloc`:
+        //
+        // > The value of alignment shall be a valid alignment supported by the implementation
+        // [...].
+        //
+        // As an example of the "supported by the implementation" requirement, POSIX.1-2001 (IEEE
+        // 1003.1-2001) defines `posix_memalign`:
+        //
+        // > The value of alignment shall be a power of two multiple of sizeof (void *).
+        //
+        // and POSIX-based implementations of `aligned_alloc` inherit this requirement. At the time
+        // of writing, this is known to be the case on macOS (but not in glibc).
+        //
+        // Satisfy the stricter requirement to avoid spurious test failures on some platforms.
+        let min_align = core::mem::size_of::<*const crate::ffi::c_void>();
+        let (align, size) = if layout.align() < min_align {
+            (min_align, layout.size().div_ceil(min_align) * min_align)
+        } else {
+            (layout.align(), layout.size())
+        };
+
         // SAFETY: Returns either NULL or a pointer to a memory allocation that satisfies or
         // exceeds the given size and alignment requirements.
-        let dst = unsafe { libc_aligned_alloc(layout.align(), layout.size()) } as *mut u8;
+        let dst = unsafe { libc_aligned_alloc(align, size) } as *mut u8;
         let dst = NonNull::new(dst).ok_or(AllocError)?;
 
         if flags.contains(__GFP_ZERO) {

---
base-commit: a64dcfb451e254085a7daee5fe51bf22959d52d3
change-id: 20250201-aligned-alloc-b52cb2353c82

Best regards,

Signature · 1 line

Tamir Duberstein <[email protected]>

Miguel Ojeda

2025-02-11 15:18 UTC #1 lore raw

Quoted 3 lines · Tamir Duberstein

Fixes dd09538fb409 ("rust: alloc: implement `Cmalloc` in module allocator_test")

Hmm... The newline issue is still present in newer series you are sending. Maintainers need to fix this manually, so please fix it on your side.

Quoted 1 line

Acked-by Danilo Krummrich <[email protected]>

I understand that you re-picked the v2 ack, since it is (at least the code) similar again now, right?

Thanks!

Cheers,
Miguel

Tamir Duberstein

author

2025-02-11 15:21 UTC #2 lore raw

Quoted 9 lines · Miguel Ojeda

Quoted 3 lines · Tamir Duberstein
Fixes dd09538fb409 ("rust: alloc: implement `Cmalloc` in module allocator_test")
Hmm... The newline issue is still present in newer series you are sending. Maintainers need to fix this manually, so please fix it on your side.

Drat, will do.

Quoted 4 lines

Quoted 1 line
Acked-by Danilo Krummrich <[email protected]>
I understand that you re-picked the v2 ack, since it is (at least the code) similar again now, right?

Yep, I mentioned it under "Changes in v4".

Quoted 5 lines

Thanks!
Cheers,
Miguel

Miguel Ojeda

2025-02-11 21:46 UTC #3 lore raw

Quoted 2 lines · Tamir Duberstein

Drat, will do.

Thanks!

Quoted 1 line

Yep, I mentioned it under "Changes in v4".

I meant to confirm the reasoning -- it is all good, thanks! (personally I would probably have dropped it in a case like this, since the change in comments is substantial and Danilo was waiting for the clarification from Alejandro etc.).

Cheers,
Miguel

Tamir Duberstein

author

2025-02-11 21:51 UTC #4 lore raw

Quoted 9 lines · Miguel Ojeda

Quoted 2 lines · Tamir Duberstein
Drat, will do.
Thanks!
Quoted 1 line
Yep, I mentioned it under "Changes in v4".

Quoted 7 lines

I meant to confirm the reasoning -- it is all good, thanks! (personally I would probably have dropped it in a case like this, since the change in comments is substantial and Danilo was waiting for the clarification from Alejandro etc.).
Cheers,
Miguel

Got it! I've dropped it from v5 in which I've used Gary's pithier formulation. I'd like to get an acknowledgement that the commit message and code comment make sense before sending it.

It'd be great to get this into 6.14 so that local builds are clean for the next cycle.

Cheers.

Tamir

Danilo Krummrich

maintainer

2025-02-11 21:54 UTC #5 lore raw

In reply to Miguel Ojeda

Quoted 4 lines · Miguel Ojeda

Quoted 2 lines · Tamir Duberstein
Drat, will do.

Quoted 8 lines

Thanks!
Quoted 1 line
Yep, I mentioned it under "Changes in v4".
I meant to confirm the reasoning -- it is all good, thanks! (personally I would probably have dropped it in a case like this, since the change in comments is substantial and Danilo was waiting for the clarification from Alejandro etc.).

Agree with Miguel, better to drop it in such cases.

But no worries, Tamir. It was still valid in this case, which is why I did not complain. :)

Also feel free to keep it for v5, moving to Gary's simplification.

- Danilo

Tamir Duberstein

author

2025-02-11 21:57 UTC #6 lore raw

Quoted 9 lines · Danilo Krummrich

Agree with Miguel, better to drop it in such cases.
But no worries, Tamir. It was still valid in this case, which is why I did not complain. :)
Also feel free to keep it for v5, moving to Gary's simplification.
- Danilo

🫡

Thanks!

Gary Guo

reviewer

2025-02-11 16:52 UTC #7 lore raw

In reply to Tamir Duberstein

Quoted 52 lines · On Mon, 10 Feb 2025 09:55:19 -0500 Tamir Duberstein <[email protected]> wrote:

ISO C's aligned_alloc is partially implementation-defined; on some systems it inherits stricter requirements from POSIX's posix_memalign.
This causes the call added in commit dd09538fb409 ("rust: alloc:
implement Cmalloc in module allocator_test") to fail on macOS because it doesn't meet the requirements of posix_memalign.
Adjust the call to meet the POSIX requirement and add a comment. This fixes failures in make rusttest on macOS.
Fixes dd09538fb409 ("rust: alloc: implement `Cmalloc` in module allocator_test")
Acked-by Danilo Krummrich <[email protected]>
Signed-off-by Tamir Duberstein <[email protected]>
---
Changes in v4:
- Revert to aligned_alloc and correct rationale. (Miguel Ojeda)
- Apply Danilo's Acked-by from v2.
- Rebase on v6.14-rc2.
- Link to v3: https://lore.kernel.org/r/[email protected]
Changes in v3:
- Replace aligned_alloc with posix_memalign for portability.
- Link to v2: https://lore.kernel.org/r/[email protected]
Changes in v2:
- Shorten some variable names. (Danilo Krummrich)
- Replace shadowing alignment variable with a second call to
Layout::align. (Danilo Krummrich)
- Link to v1: https://lore.kernel.org/r/[email protected]
---
 rust/kernel/alloc/allocator_test.rs | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)
rust/kernel/alloc/allocator_test.rs +10 -0
index e3240d16040b..1c881ed73d79 100644
@@ -62,9 +62,30 @@ unsafe fn realloc(
             ));
         }
 
+        // ISO C (ISO/IEC 9899:2011) defines `aligned_alloc`:
+        //
+        // > The value of alignment shall be a valid alignment supported by the implementation
+        // [...].
+        //
+        // As an example of the "supported by the implementation" requirement, POSIX.1-2001 (IEEE
+        // 1003.1-2001) defines `posix_memalign`:
+        //
+        // > The value of alignment shall be a power of two multiple of sizeof (void *).
+        //

rust/kernel/alloc/allocator_test.rs unsafe fn realloc( · line 84

Quoted 10 lines

rust/kernel/alloc/allocator_test.rs +10 -0

+        // and POSIX-based implementations of `aligned_alloc` inherit this requirement. At the time
+        // of writing, this is known to be the case on macOS (but not in glibc).
+        //
+        // Satisfy the stricter requirement to avoid spurious test failures on some platforms.
+        let min_align = core::mem::size_of::<*const crate::ffi::c_void>();
+        let (align, size) = if layout.align() < min_align {
+            (min_align, layout.size().div_ceil(min_align) * min_align)
+        } else {
+            (layout.align(), layout.size())
+        };

I think this can be more concisely expressed as

	let layout = layout.align_to(min_align)?.pad_to_align();

Best,
Gary

Quoted 14 lines

rust/kernel/alloc/allocator_test.rs +2 -2

+
         // SAFETY: Returns either NULL or a pointer to a memory allocation that satisfies or
         // exceeds the given size and alignment requirements.
-        let dst = unsafe { libc_aligned_alloc(layout.align(), layout.size()) } as *mut u8;
+        let dst = unsafe { libc_aligned_alloc(align, size) } as *mut u8;
         let dst = NonNull::new(dst).ok_or(AllocError)?;
 
         if flags.contains(__GFP_ZERO) {
 
---

base-commit: a64dcfb451e254085a7daee5fe51bf22959d52d3
change-id: 20250201-aligned-alloc-b52cb2353c82

Best regards,

Tamir Duberstein

author

2025-02-11 17:12 UTC #8 lore raw

Quoted 69 lines · Gary Guo

Quoted 52 lines · On Mon, 10 Feb 2025 09:55:19 -0500 Tamir Duberstein <[email protected]> wrote:
ISO C's aligned_alloc is partially implementation-defined; on some systems it inherits stricter requirements from POSIX's posix_memalign.
This causes the call added in commit dd09538fb409 ("rust: alloc:
implement Cmalloc in module allocator_test") to fail on macOS because it doesn't meet the requirements of posix_memalign.
Adjust the call to meet the POSIX requirement and add a comment. This fixes failures in make rusttest on macOS.
Fixes dd09538fb409 ("rust: alloc: implement `Cmalloc` in module allocator_test")
Acked-by Danilo Krummrich <[email protected]>
Signed-off-by Tamir Duberstein <[email protected]>
---
Changes in v4:
- Revert to aligned_alloc and correct rationale. (Miguel Ojeda)
- Apply Danilo's Acked-by from v2.
- Rebase on v6.14-rc2.
- Link to v3: https://lore.kernel.org/r/[email protected]
Changes in v3:
- Replace aligned_alloc with posix_memalign for portability.
- Link to v2: https://lore.kernel.org/r/[email protected]
Changes in v2:
- Shorten some variable names. (Danilo Krummrich)
- Replace shadowing alignment variable with a second call to
Layout::align. (Danilo Krummrich)
- Link to v1: https://lore.kernel.org/r/[email protected]
---
 rust/kernel/alloc/allocator_test.rs | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)
rust/kernel/alloc/allocator_test.rs +10 -0
index e3240d16040b..1c881ed73d79 100644
@@ -62,9 +62,30 @@ unsafe fn realloc(
             ));
         }
 
+        // ISO C (ISO/IEC 9899:2011) defines `aligned_alloc`:
+        //
+        // > The value of alignment shall be a valid alignment supported by the implementation
+        // [...].
+        //
+        // As an example of the "supported by the implementation" requirement, POSIX.1-2001 (IEEE
+        // 1003.1-2001) defines `posix_memalign`:
+        //
+        // > The value of alignment shall be a power of two multiple of sizeof (void *).
+        //
rust/kernel/alloc/allocator_test.rs unsafe fn realloc( · line 84
Quoted 10 lines
rust/kernel/alloc/allocator_test.rs +10 -0
+        // and POSIX-based implementations of `aligned_alloc` inherit this requirement. At the time
+        // of writing, this is known to be the case on macOS (but not in glibc).
+        //
+        // Satisfy the stricter requirement to avoid spurious test failures on some platforms.
+        let min_align = core::mem::size_of::<*const crate::ffi::c_void>();
+        let (align, size) = if layout.align() < min_align {
+            (min_align, layout.size().div_ceil(min_align) * min_align)
+        } else {
+            (layout.align(), layout.size())
+        };
I think this can be more concisely expressed as

Quoted 1 line

        let layout = layout.align_to(min_align)?.pad_to_align();

Yep this works. Thanks Gary.