[PATCH v5] rust: alloc: satisfy POSIX alignment requirement

[PATCH v5] rust: alloc: satisfy POSIX alignment requirement

[Tamir Duberstein]

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.

Acked-by: Danilo Krummrich <dakr@kernel.org>
Fixes: dd09538fb409 ("rust: alloc: implement `Cmalloc` in module allocator_test")
Signed-off-by: Tamir Duberstein <tamird@gmail.com>
---
Changes in v5:
- Remove errant newline in commit message. (Miguel Ojeda)
- Use more succinct expression. (Gary Guo)
- Drop and then add Danilo's Acked-by again.
- Link to v4: https://lore.kernel.org/r/20250210-aligned-alloc-v4-1-609c3a6fe139@gmail.com

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/20250206-aligned-alloc-v3-1-0cbc0ab0306d@gmail.com

Changes in v3:
- Replace `aligned_alloc` with `posix_memalign` for portability.
- Link to v2: https://lore.kernel.org/r/20250202-aligned-alloc-v2-1-5af0b5fdd46f@gmail.com

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/20250201-aligned-alloc-v1-1-c99a73f3cbd4@gmail.com
---
 rust/kernel/alloc/allocator_test.rs | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/rust/kernel/alloc/allocator_test.rs b/rust/kernel/alloc/allocator_test.rs
index e3240d16040b..17a475380253 100644
--- a/rust/kernel/alloc/allocator_test.rs
+++ b/rust/kernel/alloc/allocator_test.rs
@@ -62,6 +62,26 @@ 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 layout = layout.align_to(min_align).unwrap_or_else(|_err| {
+            crate::build_error!("invalid alignment")
+        });
+        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 dst = unsafe { libc_aligned_alloc(layout.align(), layout.size()) } as *mut u8;

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

Best regards,
-- 
Tamir Duberstein <tamird@gmail.com>

Re: [PATCH v5] rust: alloc: satisfy POSIX alignment requirement

[Danilo Krummrich]

On Wed, Feb 12, 2025 at 09:43:02AM -0500, Tamir Duberstein wrote:
> diff --git a/rust/kernel/alloc/allocator_test.rs b/rust/kernel/alloc/allocator_test.rs
> index e3240d16040b..17a475380253 100644
> --- a/rust/kernel/alloc/allocator_test.rs
> +++ b/rust/kernel/alloc/allocator_test.rs
> @@ -62,6 +62,26 @@ 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 layout = layout.align_to(min_align).unwrap_or_else(|_err| {
> +            crate::build_error!("invalid alignment")

That's not what I thought this patch will look like. I thought you'll directly
follow Gary's proposal, which is why I said you can keep the ACK.

build_error!() doesn't work here, there is no guarantee that this can be
evaluated at compile time.

I think this should just be:

let layout = layout.align_to(min_align).map_err(|_| AllocError)?.pad_to_align();

- Danilo

> +        });
> +        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 dst = unsafe { libc_aligned_alloc(layout.align(), layout.size()) } as *mut u8;

Re: [PATCH v5] rust: alloc: satisfy POSIX alignment requirement

[Tamir Duberstein]

On Wed, Feb 12, 2025 at 10:40 AM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Wed, Feb 12, 2025 at 09:43:02AM -0500, Tamir Duberstein wrote:
> > diff --git a/rust/kernel/alloc/allocator_test.rs b/rust/kernel/alloc/allocator_test.rs
> > index e3240d16040b..17a475380253 100644
> > --- a/rust/kernel/alloc/allocator_test.rs
> > +++ b/rust/kernel/alloc/allocator_test.rs
> > @@ -62,6 +62,26 @@ 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 layout = layout.align_to(min_align).unwrap_or_else(|_err| {
> > +            crate::build_error!("invalid alignment")
>
> That's not what I thought this patch will look like. I thought you'll directly
> follow Gary's proposal, which is why I said you can keep the ACK.
>
> build_error!() doesn't work here, there is no guarantee that this can be
> evaluated at compile time.

It's not guaranteed, but it does work. I could use some clarification
on the appropriate use of `build_error`. Here I'm using it to mean "I
want the compiler to let me know if the guarantees change". When is
that inappropriate?

> I think this should just be:
>
> let layout = layout.align_to(min_align).map_err(|_| AllocError)?.pad_to_align();
>
> - Danilo
>
> > +        });
> > +        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 dst = unsafe { libc_aligned_alloc(layout.align(), layout.size()) } as *mut u8;

Re: [PATCH v5] rust: alloc: satisfy POSIX alignment requirement

[Gary Guo]

On Wed, 12 Feb 2025 16:40:37 +0100
Danilo Krummrich <dakr@kernel.org> wrote:

> On Wed, Feb 12, 2025 at 09:43:02AM -0500, Tamir Duberstein wrote:
> > diff --git a/rust/kernel/alloc/allocator_test.rs b/rust/kernel/alloc/allocator_test.rs
> > index e3240d16040b..17a475380253 100644
> > --- a/rust/kernel/alloc/allocator_test.rs
> > +++ b/rust/kernel/alloc/allocator_test.rs
> > @@ -62,6 +62,26 @@ 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 layout = layout.align_to(min_align).unwrap_or_else(|_err| {
> > +            crate::build_error!("invalid alignment")  
> 
> That's not what I thought this patch will look like. I thought you'll directly
> follow Gary's proposal, which is why I said you can keep the ACK.
> 
> build_error!() doesn't work here, there is no guarantee that this can be
> evaluated at compile time.

`align_to` will only fail if `min_align` is not a valid alignment (i.e.
not power of two), which the compiler should be easy to notice that the
size of pointer is indeed power of 2.

I think both `build_error!` and `map_err` version below is fine to me. 

Best,
Gary

> 
> I think this should just be:
> 
> let layout = layout.align_to(min_align).map_err(|_| AllocError)?.pad_to_align();
> 
> - Danilo
> 
> > +        });
> > +        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 dst = unsafe { libc_aligned_alloc(layout.align(), layout.size()) } as *mut u8;  

Re: [PATCH v5] rust: alloc: satisfy POSIX alignment requirement

[Danilo Krummrich]

On Wed, Feb 12, 2025 at 04:38:48PM +0000, Gary Guo wrote:
> On Wed, 12 Feb 2025 16:40:37 +0100
> Danilo Krummrich <dakr@kernel.org> wrote:
> 
> > On Wed, Feb 12, 2025 at 09:43:02AM -0500, Tamir Duberstein wrote:
> > > diff --git a/rust/kernel/alloc/allocator_test.rs b/rust/kernel/alloc/allocator_test.rs
> > > index e3240d16040b..17a475380253 100644
> > > --- a/rust/kernel/alloc/allocator_test.rs
> > > +++ b/rust/kernel/alloc/allocator_test.rs
> > > @@ -62,6 +62,26 @@ 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 layout = layout.align_to(min_align).unwrap_or_else(|_err| {
> > > +            crate::build_error!("invalid alignment")  
> > 
> > That's not what I thought this patch will look like. I thought you'll directly
> > follow Gary's proposal, which is why I said you can keep the ACK.
> > 
> > build_error!() doesn't work here, there is no guarantee that this can be
> > evaluated at compile time.
> 
> `align_to` will only fail if `min_align` is not a valid alignment (i.e.
> not power of two), which the compiler should be easy to notice that the
> size of pointer is indeed power of 2.

From the documentation of align_to():

"Returns an error if the combination of self.size() and the given align violates
the conditions listed in Layout::from_size_align."

Formally self.size() may still be unknown at compile time.

Do I miss anything?

> 
> I think both `build_error!` and `map_err` version below is fine to me. 
> 
> Best,
> Gary
> 
> > 
> > I think this should just be:
> > 
> > let layout = layout.align_to(min_align).map_err(|_| AllocError)?.pad_to_align();
> > 
> > - Danilo
> > 
> > > +        });
> > > +        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 dst = unsafe { libc_aligned_alloc(layout.align(), layout.size()) } as *mut u8;  
> 

Re: [PATCH v5] rust: alloc: satisfy POSIX alignment requirement

[Tamir Duberstein]

On Wed, Feb 12, 2025 at 12:01 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Wed, Feb 12, 2025 at 04:38:48PM +0000, Gary Guo wrote:
> > On Wed, 12 Feb 2025 16:40:37 +0100
> > Danilo Krummrich <dakr@kernel.org> wrote:
> >
> > > On Wed, Feb 12, 2025 at 09:43:02AM -0500, Tamir Duberstein wrote:
> > > > diff --git a/rust/kernel/alloc/allocator_test.rs b/rust/kernel/alloc/allocator_test.rs
> > > > index e3240d16040b..17a475380253 100644
> > > > --- a/rust/kernel/alloc/allocator_test.rs
> > > > +++ b/rust/kernel/alloc/allocator_test.rs
> > > > @@ -62,6 +62,26 @@ 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 layout = layout.align_to(min_align).unwrap_or_else(|_err| {
> > > > +            crate::build_error!("invalid alignment")
> > >
> > > That's not what I thought this patch will look like. I thought you'll directly
> > > follow Gary's proposal, which is why I said you can keep the ACK.
> > >
> > > build_error!() doesn't work here, there is no guarantee that this can be
> > > evaluated at compile time.
> >
> > `align_to` will only fail if `min_align` is not a valid alignment (i.e.
> > not power of two), which the compiler should be easy to notice that the
> > size of pointer is indeed power of 2.
>
> From the documentation of align_to():
>
> "Returns an error if the combination of self.size() and the given align violates
> the conditions listed in Layout::from_size_align."
>
> Formally self.size() may still be unknown at compile time.
>
> Do I miss anything?

Formally, I agree. I tried testing (in allocator_test.rs):

#[cfg(test)]
mod tests {
    use super::*;

    #[test]
    fn test_allocate() {
        #[inline(never)]
        fn non_const_usize() -> usize {
            let x = 0;
            &x as *const _ as usize
        }

        let layout = Layout::array::<bool>(non_const_usize()).unwrap();
        let ptr = Cmalloc::alloc(layout, GFP_KERNEL).unwrap();
        let ptr = ptr.cast();
        // SAFETY:
        // - `ptr` was previously allocated with `Cmalloc`.
        // - `layout` is equal to the `Layout´ `ptr` was allocated with.
        unsafe { Cmalloc::free(ptr, layout) };
    }
}

and it compiled (and passed).

Re: [PATCH v5] rust: alloc: satisfy POSIX alignment requirement

[Danilo Krummrich]

On Wed, Feb 12, 2025 at 01:44:45PM -0500, Tamir Duberstein wrote:
> On Wed, Feb 12, 2025 at 12:01 PM Danilo Krummrich <dakr@kernel.org> wrote:
> >
> > On Wed, Feb 12, 2025 at 04:38:48PM +0000, Gary Guo wrote:
> > > On Wed, 12 Feb 2025 16:40:37 +0100
> > > Danilo Krummrich <dakr@kernel.org> wrote:
> > >
> > > > On Wed, Feb 12, 2025 at 09:43:02AM -0500, Tamir Duberstein wrote:
> > > > > diff --git a/rust/kernel/alloc/allocator_test.rs b/rust/kernel/alloc/allocator_test.rs
> > > > > index e3240d16040b..17a475380253 100644
> > > > > --- a/rust/kernel/alloc/allocator_test.rs
> > > > > +++ b/rust/kernel/alloc/allocator_test.rs
> > > > > @@ -62,6 +62,26 @@ 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 layout = layout.align_to(min_align).unwrap_or_else(|_err| {
> > > > > +            crate::build_error!("invalid alignment")
> > > >
> > > > That's not what I thought this patch will look like. I thought you'll directly
> > > > follow Gary's proposal, which is why I said you can keep the ACK.
> > > >
> > > > build_error!() doesn't work here, there is no guarantee that this can be
> > > > evaluated at compile time.
> > >
> > > `align_to` will only fail if `min_align` is not a valid alignment (i.e.
> > > not power of two), which the compiler should be easy to notice that the
> > > size of pointer is indeed power of 2.
> >
> > From the documentation of align_to():
> >
> > "Returns an error if the combination of self.size() and the given align violates
> > the conditions listed in Layout::from_size_align."
> >
> > Formally self.size() may still be unknown at compile time.
> >
> > Do I miss anything?
> 
> Formally, I agree. I tried testing (in allocator_test.rs):
> 
> #[cfg(test)]
> mod tests {
>     use super::*;
> 
>     #[test]
>     fn test_allocate() {
>         #[inline(never)]
>         fn non_const_usize() -> usize {
>             let x = 0;
>             &x as *const _ as usize
>         }
> 
>         let layout = Layout::array::<bool>(non_const_usize()).unwrap();
>         let ptr = Cmalloc::alloc(layout, GFP_KERNEL).unwrap();
>         let ptr = ptr.cast();
>         // SAFETY:
>         // - `ptr` was previously allocated with `Cmalloc`.
>         // - `layout` is equal to the `Layout´ `ptr` was allocated with.
>         unsafe { Cmalloc::free(ptr, layout) };
>     }
> }
> 
> and it compiled (and passed).

I suggest to try the following.

Move non_const_usize() into allocator_test.rs and within realloc(), try [1];
then try [2].

Besides that, I still think build_error!() can't be used here correctly, since
layout.size() might not be known at compile time. Please change things to what I
did suggest previously.

--

[1]
```
if non_const_usize() < 0x42 {
   crate::build_error!();
}
```

[2]
```
if non_const_usize() >= 0x42 {
   crate::build_error!();
}
```

Re: [PATCH v5] rust: alloc: satisfy POSIX alignment requirement

[Tamir Duberstein]

On Wed, Feb 12, 2025 at 3:01 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Wed, Feb 12, 2025 at 01:44:45PM -0500, Tamir Duberstein wrote:
> > On Wed, Feb 12, 2025 at 12:01 PM Danilo Krummrich <dakr@kernel.org> wrote:
> > >
> > > On Wed, Feb 12, 2025 at 04:38:48PM +0000, Gary Guo wrote:
> > > > On Wed, 12 Feb 2025 16:40:37 +0100
> > > > Danilo Krummrich <dakr@kernel.org> wrote:
> > > >
> > > > > On Wed, Feb 12, 2025 at 09:43:02AM -0500, Tamir Duberstein wrote:
> > > > > > diff --git a/rust/kernel/alloc/allocator_test.rs b/rust/kernel/alloc/allocator_test.rs
> > > > > > index e3240d16040b..17a475380253 100644
> > > > > > --- a/rust/kernel/alloc/allocator_test.rs
> > > > > > +++ b/rust/kernel/alloc/allocator_test.rs
> > > > > > @@ -62,6 +62,26 @@ 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 layout = layout.align_to(min_align).unwrap_or_else(|_err| {
> > > > > > +            crate::build_error!("invalid alignment")
> > > > >
> > > > > That's not what I thought this patch will look like. I thought you'll directly
> > > > > follow Gary's proposal, which is why I said you can keep the ACK.
> > > > >
> > > > > build_error!() doesn't work here, there is no guarantee that this can be
> > > > > evaluated at compile time.
> > > >
> > > > `align_to` will only fail if `min_align` is not a valid alignment (i.e.
> > > > not power of two), which the compiler should be easy to notice that the
> > > > size of pointer is indeed power of 2.
> > >
> > > From the documentation of align_to():
> > >
> > > "Returns an error if the combination of self.size() and the given align violates
> > > the conditions listed in Layout::from_size_align."
> > >
> > > Formally self.size() may still be unknown at compile time.
> > >
> > > Do I miss anything?
> >
> > Formally, I agree. I tried testing (in allocator_test.rs):
> >
> > #[cfg(test)]
> > mod tests {
> >     use super::*;
> >
> >     #[test]
> >     fn test_allocate() {
> >         #[inline(never)]
> >         fn non_const_usize() -> usize {
> >             let x = 0;
> >             &x as *const _ as usize
> >         }
> >
> >         let layout = Layout::array::<bool>(non_const_usize()).unwrap();
> >         let ptr = Cmalloc::alloc(layout, GFP_KERNEL).unwrap();
> >         let ptr = ptr.cast();
> >         // SAFETY:
> >         // - `ptr` was previously allocated with `Cmalloc`.
> >         // - `layout` is equal to the `Layout´ `ptr` was allocated with.
> >         unsafe { Cmalloc::free(ptr, layout) };
> >     }
> > }
> >
> > and it compiled (and passed).
>
> I suggest to try the following.
>
> Move non_const_usize() into allocator_test.rs and within realloc(), try [1];
> then try [2].
>
> Besides that, I still think build_error!() can't be used here correctly, since
> layout.size() might not be known at compile time. Please change things to what I
> did suggest previously.
>
> --
>
> [1]
> ```
> if non_const_usize() < 0x42 {
>    crate::build_error!();
> }
> ```
>
> [2]
> ```
> if non_const_usize() >= 0x42 {
>    crate::build_error!();
> }
> ```

Quite a good experiment, thanks for suggesting it. The result is that
one of these just panics at run-time. This means that it's trivially
easy to hold `build_{assert,error}!()` incorrectly! It only does the
right thing in a constant context (and the docs do say this) but it's
very easy to use in _any_ context. Looks like I wasn't the only one to
fall into the trap (rust/kernel/io.rs):

    #[inline]
    const fn io_addr_assert<U>(&self, offset: usize) -> usize {
        build_assert!(Self::offset_valid::<U>(offset, SIZE));

        self.addr() + offset
    }

since offset isn't known at compile time, this can easily be misused?

I'll change this to map_err. Thanks for your scrutiny.

Re: [PATCH v5] rust: alloc: satisfy POSIX alignment requirement

[Danilo Krummrich]

On Wed, Feb 12, 2025 at 03:47:11PM -0500, Tamir Duberstein wrote:
> Looks like I wasn't the only one to fall into the trap (rust/kernel/io.rs):
> 
>     #[inline]
>     const fn io_addr_assert<U>(&self, offset: usize) -> usize {
>         build_assert!(Self::offset_valid::<U>(offset, SIZE));
> 
>         self.addr() + offset
>     }
> 
> since offset isn't known at compile time, this can easily be misused?

Well, that's intentional.

iomem.readb(0x0)     // succeeds if SIZE >=1
iomem.readb(foo)     // fails if foo is not known at compile time
iomem.try_readb(foo) // succeeds if self.maxsize() >= 1

Re: [PATCH v5] rust: alloc: satisfy POSIX alignment requirement

[Tamir Duberstein]

On Wed, Feb 12, 2025 at 3:58 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Wed, Feb 12, 2025 at 03:47:11PM -0500, Tamir Duberstein wrote:
> > Looks like I wasn't the only one to fall into the trap (rust/kernel/io.rs):
> >
> >     #[inline]
> >     const fn io_addr_assert<U>(&self, offset: usize) -> usize {
> >         build_assert!(Self::offset_valid::<U>(offset, SIZE));
> >
> >         self.addr() + offset
> >     }
> >
> > since offset isn't known at compile time, this can easily be misused?
>
> Well, that's intentional.
>
> iomem.readb(0x0)     // succeeds if SIZE >=1
> iomem.readb(foo)     // fails if foo is not known at compile time

By "fails" here you mean fail to link, right?

> iomem.try_readb(foo) // succeeds if self.maxsize() >= 1

Apologies for being dense throughout this discussion. Could you check
my understanding?

The trick is that `build_error` is marked `#[export_name =
"rust_build_error"]` which isn't exported unless
CONFIG_RUST_BUILD_ASSERT_ALLOW is defined, causing linking to it to
fail. This even works for doctests, but not for #[test] in the kernel
crate because they are built as part of the crate. The only to way
make that work correctly is to put `build_error` in a crate all by
itself.

Re: [PATCH v5] rust: alloc: satisfy POSIX alignment requirement

[Tamir Duberstein]

On Wed, Feb 12, 2025 at 4:24 PM Tamir Duberstein <tamird@gmail.com> wrote:
>
> On Wed, Feb 12, 2025 at 3:58 PM Danilo Krummrich <dakr@kernel.org> wrote:
> >
> > On Wed, Feb 12, 2025 at 03:47:11PM -0500, Tamir Duberstein wrote:
> > > Looks like I wasn't the only one to fall into the trap (rust/kernel/io.rs):
> > >
> > >     #[inline]
> > >     const fn io_addr_assert<U>(&self, offset: usize) -> usize {
> > >         build_assert!(Self::offset_valid::<U>(offset, SIZE));
> > >
> > >         self.addr() + offset
> > >     }
> > >
> > > since offset isn't known at compile time, this can easily be misused?
> >
> > Well, that's intentional.
> >
> > iomem.readb(0x0)     // succeeds if SIZE >=1
> > iomem.readb(foo)     // fails if foo is not known at compile time
>
> By "fails" here you mean fail to link, right?
>
> > iomem.try_readb(foo) // succeeds if self.maxsize() >= 1
>
> Apologies for being dense throughout this discussion. Could you check
> my understanding?
>
> The trick is that `build_error` is marked `#[export_name =
> "rust_build_error"]` which isn't exported unless
> CONFIG_RUST_BUILD_ASSERT_ALLOW is defined, causing linking to it to
> fail. This even works for doctests, but not for #[test] in the kernel
> crate because they are built as part of the crate. The only to way
> make that work correctly is to put `build_error` in a crate all by
> itself.

Which of course, it is.

I thought maybe this was specific to building on macOS, but it
reproduces on Linux as well.

Gary, can you help me understand how the linker magic works? Is it
possible to make it work on the host as well?

Re: [PATCH v5] rust: alloc: satisfy POSIX alignment requirement

[Gary Guo]

On Wed, 12 Feb 2025 18:01:30 +0100
Danilo Krummrich <dakr@kernel.org> wrote:

> On Wed, Feb 12, 2025 at 04:38:48PM +0000, Gary Guo wrote:
> > On Wed, 12 Feb 2025 16:40:37 +0100
> > Danilo Krummrich <dakr@kernel.org> wrote:
> >   
> > > On Wed, Feb 12, 2025 at 09:43:02AM -0500, Tamir Duberstein wrote:  
> > > > diff --git a/rust/kernel/alloc/allocator_test.rs b/rust/kernel/alloc/allocator_test.rs
> > > > index e3240d16040b..17a475380253 100644
> > > > --- a/rust/kernel/alloc/allocator_test.rs
> > > > +++ b/rust/kernel/alloc/allocator_test.rs
> > > > @@ -62,6 +62,26 @@ 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 layout = layout.align_to(min_align).unwrap_or_else(|_err| {
> > > > +            crate::build_error!("invalid alignment")    
> > > 
> > > That's not what I thought this patch will look like. I thought you'll directly
> > > follow Gary's proposal, which is why I said you can keep the ACK.
> > > 
> > > build_error!() doesn't work here, there is no guarantee that this can be
> > > evaluated at compile time.  
> > 
> > `align_to` will only fail if `min_align` is not a valid alignment (i.e.
> > not power of two), which the compiler should be easy to notice that the
> > size of pointer is indeed power of 2.  
> 
> From the documentation of align_to():
> 
> "Returns an error if the combination of self.size() and the given align violates
> the conditions listed in Layout::from_size_align."
> 
> Formally self.size() may still be unknown at compile time.
> 
> Do I miss anything?

Ah, I think you're indeed correct. If I get a type that has the size of
`isize::MAX - 1` and alignment of 1, and then trying to align up to
pointer size will cause an error.

We should proceed with `map_err` then.

Best,
Gary

> 
> > 
> > I think both `build_error!` and `map_err` version below is fine to me. 
> > 
> > Best,
> > Gary
> >   
> > > 
> > > I think this should just be:
> > > 
> > > let layout = layout.align_to(min_align).map_err(|_| AllocError)?.pad_to_align();
> > > 
> > > - Danilo