[PATCH] rust: allocator: Prevents mis-aligned allocation

[PATCH] rust: allocator: Prevents mis-aligned allocation

[Boqun Feng]

Currently the KernelAllocator simply passes the size of the type Layout
to krealloc(), and in theory the alignment requirement from the type
Layout may be larger than the guarantee provided by SLAB, which means
the allocated object is mis-aligned.

Fixes this by adjusting the allocation size to the nearest power of two,
which SLAB always guarantees a size-aligned allocation. And because Rust
guarantees that original size must be a multiple of alignment and the
alignment must be a power of two, then the alignment requirement is
satisfied.

Suggested-by: Vlastimil Babka <vbabka@suse.cz>
Co-developed-by: Andreas Hindborg (Samsung) <nmi@metaspace.dk>
Signed-off-by: Andreas Hindborg (Samsung) <nmi@metaspace.dk>
Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
Cc: stable@vger.kernel.org # v6.1+
---
Some more explanation:

* Layout is a data structure describing a particular memory layout,
  conceptionally it has two fields: align and size.

  * align is guaranteed to be a power of two.
  * size can be smaller than align (only when the Layout is created via
    Layout::from_align_size())
  * After pad_to_align(), the size is guaranteed to be a multiple of
    align

For more information, please see: 

	https://doc.rust-lang.org/stable/std/alloc/struct.Layout.html

 rust/bindings/bindings_helper.h |  1 +
 rust/kernel/allocator.rs        | 17 ++++++++++++++++-
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 3e601ce2548d..6619ce95dd37 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -15,3 +15,4 @@
 /* `bindgen` gets confused at certain things. */
 const gfp_t BINDINGS_GFP_KERNEL = GFP_KERNEL;
 const gfp_t BINDINGS___GFP_ZERO = __GFP_ZERO;
+const size_t BINDINGS_ARCH_SLAB_MINALIGN = ARCH_SLAB_MINALIGN;
diff --git a/rust/kernel/allocator.rs b/rust/kernel/allocator.rs
index 397a3dd57a9b..66575cf87ce2 100644
--- a/rust/kernel/allocator.rs
+++ b/rust/kernel/allocator.rs
@@ -11,9 +11,24 @@
 
 unsafe impl GlobalAlloc for KernelAllocator {
     unsafe fn alloc(&self, layout: Layout) -> *mut u8 {
+        // Customized layouts from `Layout::from_size_align()` can have size < align, so pads first.
+        let layout = layout.pad_to_align();
+
+        let mut size = layout.size();
+
+        if layout.align() > bindings::BINDINGS_ARCH_SLAB_MINALIGN {
+            // The alignment requirement exceeds the slab guarantee, then tries to enlarges the size
+            // to use the "power-of-two" size/alignment guarantee (see comments in kmalloc() for
+            // more information).
+            //
+            // Note that `layout.size()` (after padding) is guaranteed to be muliples of
+            // `layout.align()`, so `next_power_of_two` gives enough alignment guarantee.
+            size = size.next_power_of_two();
+        }
+
         // `krealloc()` is used instead of `kmalloc()` because the latter is
         // an inline function and cannot be bound to as a result.
-        unsafe { bindings::krealloc(ptr::null(), layout.size(), bindings::GFP_KERNEL) as *mut u8 }
+        unsafe { bindings::krealloc(ptr::null(), size, bindings::GFP_KERNEL) as *mut u8 }
     }
 
     unsafe fn dealloc(&self, ptr: *mut u8, _layout: Layout) {
-- 
2.39.2

Re: [PATCH] rust: allocator: Prevents mis-aligned allocation

[Gary Guo]

On Tue, 13 Jun 2023 09:42:58 -0700
Boqun Feng <boqun.feng@gmail.com> wrote:

> Currently the KernelAllocator simply passes the size of the type Layout
> to krealloc(), and in theory the alignment requirement from the type
> Layout may be larger than the guarantee provided by SLAB, which means
> the allocated object is mis-aligned.
> 
> Fixes this by adjusting the allocation size to the nearest power of two,
> which SLAB always guarantees a size-aligned allocation. And because Rust
> guarantees that original size must be a multiple of alignment and the
> alignment must be a power of two, then the alignment requirement is
> satisfied.
> 
> Suggested-by: Vlastimil Babka <vbabka@suse.cz>
> Co-developed-by: Andreas Hindborg (Samsung) <nmi@metaspace.dk>
> Signed-off-by: Andreas Hindborg (Samsung) <nmi@metaspace.dk>
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> Cc: stable@vger.kernel.org # v6.1+

Reviewed-by: Gary Guo <gary@garyguo.net>

> ---
> Some more explanation:
> 
> * Layout is a data structure describing a particular memory layout,
>   conceptionally it has two fields: align and size.
> 
>   * align is guaranteed to be a power of two.
>   * size can be smaller than align (only when the Layout is created via
>     Layout::from_align_size())
>   * After pad_to_align(), the size is guaranteed to be a multiple of
>     align
> 
> For more information, please see: 
> 
> 	https://doc.rust-lang.org/stable/std/alloc/struct.Layout.html
> 
>  rust/bindings/bindings_helper.h |  1 +
>  rust/kernel/allocator.rs        | 17 ++++++++++++++++-
>  2 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index 3e601ce2548d..6619ce95dd37 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -15,3 +15,4 @@
>  /* `bindgen` gets confused at certain things. */
>  const gfp_t BINDINGS_GFP_KERNEL = GFP_KERNEL;
>  const gfp_t BINDINGS___GFP_ZERO = __GFP_ZERO;
> +const size_t BINDINGS_ARCH_SLAB_MINALIGN = ARCH_SLAB_MINALIGN;
> diff --git a/rust/kernel/allocator.rs b/rust/kernel/allocator.rs
> index 397a3dd57a9b..66575cf87ce2 100644
> --- a/rust/kernel/allocator.rs
> +++ b/rust/kernel/allocator.rs
> @@ -11,9 +11,24 @@
>  
>  unsafe impl GlobalAlloc for KernelAllocator {
>      unsafe fn alloc(&self, layout: Layout) -> *mut u8 {
> +        // Customized layouts from `Layout::from_size_align()` can have size < align, so pads first.
> +        let layout = layout.pad_to_align();
> +
> +        let mut size = layout.size();
> +
> +        if layout.align() > bindings::BINDINGS_ARCH_SLAB_MINALIGN {
> +            // The alignment requirement exceeds the slab guarantee, then tries to enlarges the size
> +            // to use the "power-of-two" size/alignment guarantee (see comments in kmalloc() for
> +            // more information).
> +            //
> +            // Note that `layout.size()` (after padding) is guaranteed to be muliples of
> +            // `layout.align()`, so `next_power_of_two` gives enough alignment guarantee.
> +            size = size.next_power_of_two();
> +        }
> +
>          // `krealloc()` is used instead of `kmalloc()` because the latter is
>          // an inline function and cannot be bound to as a result.
> -        unsafe { bindings::krealloc(ptr::null(), layout.size(), bindings::GFP_KERNEL) as *mut u8 }
> +        unsafe { bindings::krealloc(ptr::null(), size, bindings::GFP_KERNEL) as *mut u8 }
>      }
>  
>      unsafe fn dealloc(&self, ptr: *mut u8, _layout: Layout) {

Re: [PATCH] rust: allocator: Prevents mis-aligned allocation

[Alice Ryhl]

On 6/13/23 18:42, Boqun Feng wrote:
> Currently the KernelAllocator simply passes the size of the type Layout
> to krealloc(), and in theory the alignment requirement from the type
> Layout may be larger than the guarantee provided by SLAB, which means
> the allocated object is mis-aligned.
> 
> Fixes this by adjusting the allocation size to the nearest power of two,
> which SLAB always guarantees a size-aligned allocation. And because Rust
> guarantees that original size must be a multiple of alignment and the
> alignment must be a power of two, then the alignment requirement is
> satisfied.
> 
> Suggested-by: Vlastimil Babka <vbabka@suse.cz>
> Co-developed-by: Andreas Hindborg (Samsung) <nmi@metaspace.dk>
> Signed-off-by: Andreas Hindborg (Samsung) <nmi@metaspace.dk>
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> Cc: stable@vger.kernel.org # v6.1+

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

Re: [PATCH] rust: allocator: Prevents mis-aligned allocation

[Benno Lossin]

On 13.06.23 18:42, Boqun Feng wrote:
> Currently the KernelAllocator simply passes the size of the type Layout
> to krealloc(), and in theory the alignment requirement from the type
> Layout may be larger than the guarantee provided by SLAB, which means
> the allocated object is mis-aligned.
> 
> Fixes this by adjusting the allocation size to the nearest power of two,
> which SLAB always guarantees a size-aligned allocation. And because Rust
> guarantees that original size must be a multiple of alignment and the
> alignment must be a power of two, then the alignment requirement is
> satisfied.
> 
> Suggested-by: Vlastimil Babka <vbabka@suse.cz>
> Co-developed-by: Andreas Hindborg (Samsung) <nmi@metaspace.dk>
> Signed-off-by: Andreas Hindborg (Samsung) <nmi@metaspace.dk>
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> Cc: stable@vger.kernel.org # v6.1+

Reviewed-by: Benno Lossin <benno.lossin@proton.me>

-- 
Cheers,
Benno

> ---
> Some more explanation:
> 
> * Layout is a data structure describing a particular memory layout,
>    conceptionally it has two fields: align and size.
> 
>    * align is guaranteed to be a power of two.
>    * size can be smaller than align (only when the Layout is created via
>      Layout::from_align_size())
>    * After pad_to_align(), the size is guaranteed to be a multiple of
>      align
> 
> For more information, please see:
> 
> 	https://doc.rust-lang.org/stable/std/alloc/struct.Layout.html
> 
>   rust/bindings/bindings_helper.h |  1 +
>   rust/kernel/allocator.rs        | 17 ++++++++++++++++-
>   2 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index 3e601ce2548d..6619ce95dd37 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -15,3 +15,4 @@
>   /* `bindgen` gets confused at certain things. */
>   const gfp_t BINDINGS_GFP_KERNEL = GFP_KERNEL;
>   const gfp_t BINDINGS___GFP_ZERO = __GFP_ZERO;
> +const size_t BINDINGS_ARCH_SLAB_MINALIGN = ARCH_SLAB_MINALIGN;
> diff --git a/rust/kernel/allocator.rs b/rust/kernel/allocator.rs
> index 397a3dd57a9b..66575cf87ce2 100644
> --- a/rust/kernel/allocator.rs
> +++ b/rust/kernel/allocator.rs
> @@ -11,9 +11,24 @@
> 
>   unsafe impl GlobalAlloc for KernelAllocator {
>       unsafe fn alloc(&self, layout: Layout) -> *mut u8 {
> +        // Customized layouts from `Layout::from_size_align()` can have size < align, so pads first.
> +        let layout = layout.pad_to_align();
> +
> +        let mut size = layout.size();
> +
> +        if layout.align() > bindings::BINDINGS_ARCH_SLAB_MINALIGN {
> +            // The alignment requirement exceeds the slab guarantee, then tries to enlarges the size
> +            // to use the "power-of-two" size/alignment guarantee (see comments in kmalloc() for
> +            // more information).
> +            //
> +            // Note that `layout.size()` (after padding) is guaranteed to be muliples of
> +            // `layout.align()`, so `next_power_of_two` gives enough alignment guarantee.
> +            size = size.next_power_of_two();
> +        }
> +
>           // `krealloc()` is used instead of `kmalloc()` because the latter is
>           // an inline function and cannot be bound to as a result.
> -        unsafe { bindings::krealloc(ptr::null(), layout.size(), bindings::GFP_KERNEL) as *mut u8 }
> +        unsafe { bindings::krealloc(ptr::null(), size, bindings::GFP_KERNEL) as *mut u8 }
>       }
> 
>       unsafe fn dealloc(&self, ptr: *mut u8, _layout: Layout) {
> --
> 2.39.2
>

Re: [PATCH] rust: allocator: Prevents mis-aligned allocation

[Martin Rodriguez Reboredo]

On 6/13/23 13:42, Boqun Feng wrote:
> Currently the KernelAllocator simply passes the size of the type Layout
> to krealloc(), and in theory the alignment requirement from the type
> Layout may be larger than the guarantee provided by SLAB, which means
> the allocated object is mis-aligned.
> 
> Fixes this by adjusting the allocation size to the nearest power of two,
> which SLAB always guarantees a size-aligned allocation. And because Rust
> guarantees that original size must be a multiple of alignment and the
> alignment must be a power of two, then the alignment requirement is
> satisfied.
> 
> Suggested-by: Vlastimil Babka <vbabka@suse.cz>
> Co-developed-by: Andreas Hindborg (Samsung) <nmi@metaspace.dk>
> Signed-off-by: Andreas Hindborg (Samsung) <nmi@metaspace.dk>
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> Cc: stable@vger.kernel.org # v6.1+
> ---
> [...]

Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>

Re: [PATCH] rust: allocator: Prevents mis-aligned allocation

[Miguel Ojeda]

On Tue, Jun 13, 2023 at 6:44 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> Cc: stable@vger.kernel.org # v6.1+

Applied to `rust-next`, thanks!

However, should this go to stable? The actual functions being called
are the `__rust_*` ones (until they get removed in 1.71), no? Thus
this is not actually fixing the actual functions being called, right?

If that is correct, then the fix should change the functions below,
perhaps adding `krealloc_with_flags()` from the other patch (it does
not need to be a method, by the way), and calling it with a `Layout`
like the generated ones do. Then I can rebase `rust-next` on top of
the fix that adds the `krealloc_with_flags()`.

Cheers,
Miguel

Re: [PATCH] rust: allocator: Prevents mis-aligned allocation

[Boqun Feng]

On Sat, Jul 29, 2023 at 04:01:03PM +0200, Miguel Ojeda wrote:
> On Tue, Jun 13, 2023 at 6:44 PM Boqun Feng <boqun.feng@gmail.com> wrote:
> >
> > Cc: stable@vger.kernel.org # v6.1+
> 
> Applied to `rust-next`, thanks!
> 
> However, should this go to stable? The actual functions being called
> are the `__rust_*` ones (until they get removed in 1.71), no? Thus

Interesting, I wasn't aware of the `__rust_*` "hack" here, so you are
right, this doesn't fix the issue in stable kernels.

> this is not actually fixing the actual functions being called, right?
> 
> If that is correct, then the fix should change the functions below,
> perhaps adding `krealloc_with_flags()` from the other patch (it does
> not need to be a method, by the way), and calling it with a `Layout`
> like the generated ones do. Then I can rebase `rust-next` on top of

Sounds good, however I think it'll be better if I resend this one, and
the other one originally from Bjorn based on the introduction of
function `krealloc_with_flags` (I will name it as `krealloc_aligned`,
since it's a function that returns a aligned object with krealloc). 

Thoughts?

Regards,
Boqun

> the fix that adds the `krealloc_with_flags()`.
> 
> Cheers,
> Miguel