linux-kernel - Re: [PATCH v3] rust: allocator_test: use `posix

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJ-ks9kvrBBLTnEmRPbrkoHYgg6wrXoTVXh8ecqYBSPTfpjx2g@mail.gmail.com>
Date: Fri, 7 Feb 2025 06:43:09 -0500
From: Tamir Duberstein <tamird@...il.com>
To: Danilo Krummrich <dakr@...nel.org>
Cc: Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, 
	Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>, 
	Björn Roy Baron <bjorn3_gh@...tonmail.com>, 
	Benno Lossin <benno.lossin@...ton.me>, Andreas Hindborg <a.hindborg@...nel.org>, 
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>, rust-for-linux@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] rust: allocator_test: use `posix_memalign`

On Thu, Feb 6, 2025 at 4:56 PM Danilo Krummrich <dakr@...nel.org> wrote:
>
> On Thu, Feb 06, 2025 at 03:49:00PM -0500, Tamir Duberstein wrote:
> > The implementation added in commit dd09538fb409 ("rust: alloc: implement
> > `Cmalloc` in module allocator_test") used `aligned_malloc` which has
> > implementation-defined requirements of its `alignment` parameter.
> >
> > The macOS implementation of `aligned_alloc` appears to be based on
> > `posix_memalign` and inherits the stricter requirements of that
> > function, causing test failures on macOS.
> >
> > Replace `aligned_alloc` with `posix_memalign` and comply with its
> > requirements. This ensures uniform behavior across systems.
> >
> > Fixes: dd09538fb409 ("rust: alloc: implement `Cmalloc` in module allocator_test")
> >
> > Signed-off-by: Tamir Duberstein <tamird@...il.com>
>
> Let's wait for clarification before moving on. :)
>
> > ---
> > I've intentionally not picked up Danilo's Acked-by from v2 because the
> > approach has changed quite a bit.
> > ---
> > Changes in v3:
> > - Replace `aligned_malloc` 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 | 27 +++++++++++++++++++++++----
> >  1 file changed, 23 insertions(+), 4 deletions(-)
> >
> > diff --git a/rust/kernel/alloc/allocator_test.rs b/rust/kernel/alloc/allocator_test.rs
> > index e3240d16040b..0aa68d955b39 100644
> > --- a/rust/kernel/alloc/allocator_test.rs
> > +++ b/rust/kernel/alloc/allocator_test.rs
> > @@ -23,8 +23,19 @@
> >  pub type KVmalloc = Kmalloc;
> >
> >  extern "C" {
> > -    #[link_name = "aligned_alloc"]
> > -    fn libc_aligned_alloc(align: usize, size: usize) -> *mut crate::ffi::c_void;
> > +    // NB: `posix_memalign` is intentionally used instead of `aligned_malloc`.
> > +    //
> > +    // ISO C (ISO/IEC 9899:2011) defines `aligned_malloc`:
> > +    //
> > +    // > The value of alignment shall be a valid alignment supported by the implementation [...].
> > +    //
> > +    // POSIX.1-2001 (IEEE 1003.1-2001) defines `posix_memalign`:
> > +    //
> > +    // > The value of alignment shall be a power of two multiple of sizeof (void *).
> > +    //
> > +    // `posix_memalign` is more portable than (but otherwise identical to) `aligned_malloc`.
> > +    #[link_name = "posix_memalign"]
> > +    fn libc_posix_memalign(align: usize, size: usize) -> *mut crate::ffi::c_void;
>
> I don't think this can work. posix_memalign() is defined as follows.
>
> int posix_memalign(void **memptr, size_t alignment, size_t size)

That's embarrassing! You're right of course, and yet somehow it worked
when I tested.

> Besides that, I don't think switching to posix_memalign() is desirable, it only
> seems to make the code more complicated.

It does make the code more complicated but prevents "works on my
machine" problems cropping up in the future.

I'm happy to wait for clarification, I'm just not sure whose court the
ball is in.