* [PATCH 0/3] Fix alignment issue and prepare for rust 1.71
@ 2023-07-30 1:29 Boqun Feng
2023-07-30 1:29 ` [PATCH 1/3] rust: allocator: Prevent mis-aligned allocation Boqun Feng
` (3 more replies)
0 siblings, 4 replies; 13+ messages in thread
From: Boqun Feng @ 2023-07-30 1:29 UTC (permalink / raw)
To: rust-for-linux, linux-kernel, linux-mm
Cc: Miguel Ojeda, Alex Gaynor, Wedson Almeida Filho, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin,
Martin Rodriguez Reboredo, Alice Ryhl, Dariusz Sosnowski,
Geoffrey Thomas, Fox Chen, John Baublitz, Christoph Lameter,
Pekka Enberg, David Rientjes, Joonsoo Kim, Andrew Morton,
Vlastimil Babka, Roman Gushchin, Hyeonggon Yoo, Kees Cook,
stable
Hi Miguel,
I end up with this series:
(background: https://lore.kernel.org/rust-for-linux/CANiq72=pb18B6NOcXF03d0ctOP8kv2dqnUeNyEuSvuDb=vs-0g@mail.gmail.com/)
Patch #1: introduces the core helper function that help calculate the
correct size for krealloc(), and also use the helper function to fix
`__rust_*` ones, this should be backported to stable kernels hence the
Cc.
Patch #2: use the helper function in KernelAllocator
Patch #3: Bjorn's patch with correct use of the helper function.
I have to add a few more SAFETY comments in these `__rust_*` functions,
which may cause conflict with your 1.71 series.
Since all previous patches get refactored a bit, I dropped all the
Reviewed-bys, appreciate anyone to take a look, thanks!
Regards,
Boqun
Björn Roy Baron (1):
rust: alloc: Add realloc and alloc_zeroed to the GlobalAlloc impl
Boqun Feng (2):
rust: allocator: Prevent mis-aligned allocation
rust: allocator: Use krealloc_aligned() in KernelAllocator::alloc
rust/bindings/bindings_helper.h | 1 +
rust/kernel/allocator.rs | 107 ++++++++++++++++++++++++++------
2 files changed, 90 insertions(+), 18 deletions(-)
--
2.41.0
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/3] rust: allocator: Prevent mis-aligned allocation
2023-07-30 1:29 [PATCH 0/3] Fix alignment issue and prepare for rust 1.71 Boqun Feng
@ 2023-07-30 1:29 ` Boqun Feng
2023-07-30 20:43 ` Miguel Ojeda
2023-07-31 7:56 ` Vlastimil Babka
2023-07-30 1:29 ` [PATCH 2/3] rust: allocator: Use krealloc_aligned() in KernelAllocator::alloc Boqun Feng
` (2 subsequent siblings)
3 siblings, 2 replies; 13+ messages in thread
From: Boqun Feng @ 2023-07-30 1:29 UTC (permalink / raw)
To: rust-for-linux, linux-kernel, linux-mm
Cc: Miguel Ojeda, Alex Gaynor, Wedson Almeida Filho, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin,
Martin Rodriguez Reboredo, Alice Ryhl, Dariusz Sosnowski,
Geoffrey Thomas, Fox Chen, John Baublitz, Christoph Lameter,
Pekka Enberg, David Rientjes, Joonsoo Kim, Andrew Morton,
Vlastimil Babka, Roman Gushchin, Hyeonggon Yoo, Kees Cook,
stable, Andreas Hindborg
Currently the rust allocator 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.
Fix 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 the 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+
---
rust/bindings/bindings_helper.h | 1 +
rust/kernel/allocator.rs | 74 ++++++++++++++++++++++++++-------
2 files changed, 60 insertions(+), 15 deletions(-)
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 3e601ce2548d..058954961bfc 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -13,5 +13,6 @@
#include <linux/sched.h>
/* `bindgen` gets confused at certain things. */
+const size_t BINDINGS_ARCH_SLAB_MINALIGN = ARCH_SLAB_MINALIGN;
const gfp_t BINDINGS_GFP_KERNEL = GFP_KERNEL;
const gfp_t BINDINGS___GFP_ZERO = __GFP_ZERO;
diff --git a/rust/kernel/allocator.rs b/rust/kernel/allocator.rs
index 397a3dd57a9b..fae11d1fdba7 100644
--- a/rust/kernel/allocator.rs
+++ b/rust/kernel/allocator.rs
@@ -9,6 +9,36 @@
struct KernelAllocator;
+/// Calls `krealloc` with a proper size to alloc a new object aligned to `new_layout`'s alignment.
+///
+/// # Safety
+///
+/// - `ptr` can be either null or a pointer which has been allocated by this allocator.
+/// - `new_layout` must have a non-zero size.
+unsafe fn krealloc_aligned(ptr: *mut u8, new_layout: Layout, flags: bindings::gfp_t) -> *mut u8 {
+ // Customized layouts from `Layout::from_size_align()` can have size < align, so pad first.
+ let layout = new_layout.pad_to_align();
+
+ let mut size = layout.size();
+
+ if layout.align() > bindings::BINDINGS_ARCH_SLAB_MINALIGN {
+ // The alignment requirement exceeds the slab guarantee, thus try to enlarge 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 a multiple of
+ // `layout.align()`, so `next_power_of_two` gives enough alignment guarantee.
+ size = size.next_power_of_two();
+ }
+
+ // SAFETY:
+ // - `ptr` is either null or a pointer returned from a previous `k{re}alloc()` by the
+ // function safety requirement.
+ // - `size` is greater than 0 since it's either a `layout.size()` (which cannot be zero
+ // according to the function safety requirement) or a result from `next_power_of_two()`.
+ unsafe { bindings::krealloc(ptr as *const core::ffi::c_void, size, flags) as *mut u8 }
+}
+
unsafe impl GlobalAlloc for KernelAllocator {
unsafe fn alloc(&self, layout: Layout) -> *mut u8 {
// `krealloc()` is used instead of `kmalloc()` because the latter is
@@ -30,10 +60,20 @@ unsafe fn dealloc(&self, ptr: *mut u8, _layout: Layout) {
// to extract the object file that has them from the archive. For the moment,
// let's generate them ourselves instead.
//
+// Note: Although these are *safe* functions, but they are only generated at
+// `GlobalAlloc` callsites, hence we assume the parameters obey the same
+// `GlobalAlloc` function safety requirements: size and align should form a
+// valid layout, and size is greater than 0.
+//
// Note that `#[no_mangle]` implies exported too, nowadays.
#[no_mangle]
-fn __rust_alloc(size: usize, _align: usize) -> *mut u8 {
- unsafe { bindings::krealloc(core::ptr::null(), size, bindings::GFP_KERNEL) as *mut u8 }
+fn __rust_alloc(size: usize, align: usize) -> *mut u8 {
+ // SAFETY: See assumption above.
+ let layout = unsafe { Layout::from_size_align_unchecked(size, align) };
+
+ // SAFETY: `ptr::null_mut()` is null, per assumption above the size of `layout` is greater
+ // than 0.
+ unsafe { krealloc_aligned(ptr::null_mut(), layout, bindings::GFP_KERNEL) }
}
#[no_mangle]
@@ -42,23 +82,27 @@ fn __rust_dealloc(ptr: *mut u8, _size: usize, _align: usize) {
}
#[no_mangle]
-fn __rust_realloc(ptr: *mut u8, _old_size: usize, _align: usize, new_size: usize) -> *mut u8 {
- unsafe {
- bindings::krealloc(
- ptr as *const core::ffi::c_void,
- new_size,
- bindings::GFP_KERNEL,
- ) as *mut u8
- }
+fn __rust_realloc(ptr: *mut u8, _old_size: usize, align: usize, new_size: usize) -> *mut u8 {
+ // SAFETY: See assumption above.
+ let new_layout = unsafe { Layout::from_size_align_unchecked(new_size, align) };
+
+ // SAFETY: Per assumption above, `ptr` is allocated by `__rust_*` before, and the size of
+ // `new_layout` is greater than 0.
+ unsafe { krealloc_aligned(ptr, new_layout, bindings::GFP_KERNEL) }
}
#[no_mangle]
-fn __rust_alloc_zeroed(size: usize, _align: usize) -> *mut u8 {
+fn __rust_alloc_zeroed(size: usize, align: usize) -> *mut u8 {
+ // SAFETY: See assumption above.
+ let layout = unsafe { Layout::from_size_align_unchecked(size, align) };
+
+ // SAFETY: `ptr::null_mut()` is null, per assumption above the size of `layout` is greater
+ // than 0.
unsafe {
- bindings::krealloc(
- core::ptr::null(),
- size,
+ krealloc_aligned(
+ ptr::null_mut(),
+ layout,
bindings::GFP_KERNEL | bindings::__GFP_ZERO,
- ) as *mut u8
+ )
}
}
--
2.41.0
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/3] rust: allocator: Use krealloc_aligned() in KernelAllocator::alloc
2023-07-30 1:29 [PATCH 0/3] Fix alignment issue and prepare for rust 1.71 Boqun Feng
2023-07-30 1:29 ` [PATCH 1/3] rust: allocator: Prevent mis-aligned allocation Boqun Feng
@ 2023-07-30 1:29 ` Boqun Feng
2023-07-31 8:48 ` Andreas Hindborg (Samsung)
2023-07-30 1:29 ` [PATCH 3/3] rust: alloc: Add realloc and alloc_zeroed to the GlobalAlloc impl Boqun Feng
2023-07-30 20:43 ` [PATCH 0/3] Fix alignment issue and prepare for rust 1.71 Miguel Ojeda
3 siblings, 1 reply; 13+ messages in thread
From: Boqun Feng @ 2023-07-30 1:29 UTC (permalink / raw)
To: rust-for-linux, linux-kernel, linux-mm
Cc: Miguel Ojeda, Alex Gaynor, Wedson Almeida Filho, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin,
Martin Rodriguez Reboredo, Alice Ryhl, Dariusz Sosnowski,
Geoffrey Thomas, Fox Chen, John Baublitz, Christoph Lameter,
Pekka Enberg, David Rientjes, Joonsoo Kim, Andrew Morton,
Vlastimil Babka, Roman Gushchin, Hyeonggon Yoo, Kees Cook,
stable
This fixes the potential issue that when KernelAllocator is used, the
allocation may be mis-aligned due to SLAB's alignment guarantee.
Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
---
rust/kernel/allocator.rs | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/rust/kernel/allocator.rs b/rust/kernel/allocator.rs
index fae11d1fdba7..1aec688cf0e0 100644
--- a/rust/kernel/allocator.rs
+++ b/rust/kernel/allocator.rs
@@ -41,9 +41,9 @@ unsafe fn krealloc_aligned(ptr: *mut u8, new_layout: Layout, flags: bindings::gf
unsafe impl GlobalAlloc for KernelAllocator {
unsafe fn alloc(&self, layout: Layout) -> *mut u8 {
- // `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 }
+ // SAFETY: `ptr::null_mut()` is null and `layout` has a non-zero size by the function safety
+ // requirement.
+ unsafe { krealloc_aligned(ptr::null_mut(), layout, bindings::GFP_KERNEL) }
}
unsafe fn dealloc(&self, ptr: *mut u8, _layout: Layout) {
--
2.41.0
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 3/3] rust: alloc: Add realloc and alloc_zeroed to the GlobalAlloc impl
2023-07-30 1:29 [PATCH 0/3] Fix alignment issue and prepare for rust 1.71 Boqun Feng
2023-07-30 1:29 ` [PATCH 1/3] rust: allocator: Prevent mis-aligned allocation Boqun Feng
2023-07-30 1:29 ` [PATCH 2/3] rust: allocator: Use krealloc_aligned() in KernelAllocator::alloc Boqun Feng
@ 2023-07-30 1:29 ` Boqun Feng
2023-07-31 11:12 ` Andreas Hindborg (Samsung)
2023-07-30 20:43 ` [PATCH 0/3] Fix alignment issue and prepare for rust 1.71 Miguel Ojeda
3 siblings, 1 reply; 13+ messages in thread
From: Boqun Feng @ 2023-07-30 1:29 UTC (permalink / raw)
To: rust-for-linux, linux-kernel, linux-mm
Cc: Miguel Ojeda, Alex Gaynor, Wedson Almeida Filho, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin,
Martin Rodriguez Reboredo, Alice Ryhl, Dariusz Sosnowski,
Geoffrey Thomas, Fox Chen, John Baublitz, Christoph Lameter,
Pekka Enberg, David Rientjes, Joonsoo Kim, Andrew Morton,
Vlastimil Babka, Roman Gushchin, Hyeonggon Yoo, Kees Cook,
stable
From: Björn Roy Baron <bjorn3_gh@protonmail.com>
While there are default impls for these methods, using the respective C
api's is faster. Currently neither the existing nor these new
GlobalAlloc method implementations are actually called. Instead the
__rust_* function defined below the GlobalAlloc impl are used. With
rustc 1.71 these functions will be gone and all allocation calls will go
through the GlobalAlloc implementation.
Link: https://github.com/Rust-for-Linux/linux/issues/68
Signed-off-by: Björn Roy Baron <bjorn3_gh@protonmail.com>
[boqun: add size adjustment for alignment requirement]
Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
---
rust/kernel/allocator.rs | 27 +++++++++++++++++++++++++++
1 file changed, 27 insertions(+)
diff --git a/rust/kernel/allocator.rs b/rust/kernel/allocator.rs
index 1aec688cf0e0..6f1f50465ab3 100644
--- a/rust/kernel/allocator.rs
+++ b/rust/kernel/allocator.rs
@@ -51,6 +51,33 @@ unsafe fn dealloc(&self, ptr: *mut u8, _layout: Layout) {
bindings::kfree(ptr as *const core::ffi::c_void);
}
}
+
+ unsafe fn realloc(&self, ptr: *mut u8, layout: Layout, new_size: usize) -> *mut u8 {
+ // SAFETY:
+ // - `new_size`, when rounded up to the nearest multiple of `layout.align()`, will not
+ // overflow `isize` by the function safety requirement.
+ // - `layout.align()` is a proper alignment (i.e. not zero and must be a power of two).
+ let layout = unsafe { Layout::from_size_align_unchecked(new_size, layout.align()) };
+
+ // SAFETY:
+ // - `ptr` is either null or a pointer allocated by this allocator by the function safety
+ // requirement.
+ // - the size of `layout` is not zero because `new_size` is not zero by the function safety
+ // requirement.
+ unsafe { krealloc_aligned(ptr, layout, bindings::GFP_KERNEL) }
+ }
+
+ unsafe fn alloc_zeroed(&self, layout: Layout) -> *mut u8 {
+ // SAFETY: `ptr::null_mut()` is null and `layout` has a non-zero size by the function safety
+ // requirement.
+ unsafe {
+ krealloc_aligned(
+ ptr::null_mut(),
+ layout,
+ bindings::GFP_KERNEL | bindings::__GFP_ZERO,
+ )
+ }
+ }
}
#[global_allocator]
--
2.41.0
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/3] Fix alignment issue and prepare for rust 1.71
2023-07-30 1:29 [PATCH 0/3] Fix alignment issue and prepare for rust 1.71 Boqun Feng
` (2 preceding siblings ...)
2023-07-30 1:29 ` [PATCH 3/3] rust: alloc: Add realloc and alloc_zeroed to the GlobalAlloc impl Boqun Feng
@ 2023-07-30 20:43 ` Miguel Ojeda
3 siblings, 0 replies; 13+ messages in thread
From: Miguel Ojeda @ 2023-07-30 20:43 UTC (permalink / raw)
To: Boqun Feng
Cc: rust-for-linux, linux-kernel, linux-mm, Miguel Ojeda,
Alex Gaynor, Wedson Almeida Filho, Gary Guo,
Björn Roy Baron, Benno Lossin, Martin Rodriguez Reboredo,
Alice Ryhl, Dariusz Sosnowski, Geoffrey Thomas, Fox Chen,
John Baublitz, Christoph Lameter, Pekka Enberg, David Rientjes,
Joonsoo Kim, Andrew Morton, Vlastimil Babka, Roman Gushchin,
Hyeonggon Yoo, Kees Cook, stable
On Sun, Jul 30, 2023 at 3:29 AM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> Patch #1: introduces the core helper function that help calculate the
> correct size for krealloc(), and also use the helper function to fix
> `__rust_*` ones, this should be backported to stable kernels hence the
> Cc.
>
> Patch #2: use the helper function in KernelAllocator
>
> Patch #3: Bjorn's patch with correct use of the helper function.
Looks good to me, thanks a lot!
> I have to add a few more SAFETY comments in these `__rust_*` functions,
> which may cause conflict with your 1.71 series.
No worries.
> Since all previous patches get refactored a bit, I dropped all the
> Reviewed-bys, appreciate anyone to take a look, thanks!
Thanks Boqun!
Cheers,
Miguel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] rust: allocator: Prevent mis-aligned allocation
2023-07-30 1:29 ` [PATCH 1/3] rust: allocator: Prevent mis-aligned allocation Boqun Feng
@ 2023-07-30 20:43 ` Miguel Ojeda
2023-07-30 22:36 ` Björn Roy Baron
2023-07-30 22:41 ` Björn Roy Baron
2023-07-31 7:56 ` Vlastimil Babka
1 sibling, 2 replies; 13+ messages in thread
From: Miguel Ojeda @ 2023-07-30 20:43 UTC (permalink / raw)
To: Boqun Feng, Björn Roy Baron
Cc: rust-for-linux, linux-kernel, linux-mm, Miguel Ojeda,
Alex Gaynor, Wedson Almeida Filho, Gary Guo, Benno Lossin,
Martin Rodriguez Reboredo, Alice Ryhl, Dariusz Sosnowski,
Geoffrey Thomas, Fox Chen, John Baublitz, Christoph Lameter,
Pekka Enberg, David Rientjes, Joonsoo Kim, Andrew Morton,
Vlastimil Babka, Roman Gushchin, Hyeonggon Yoo, Kees Cook,
stable, Andreas Hindborg
On Sun, Jul 30, 2023 at 3:29 AM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> +// Note: Although these are *safe* functions, but they are only generated at
> +// `GlobalAlloc` callsites, hence we assume the parameters obey the same
> +// `GlobalAlloc` function safety requirements: size and align should form a
> +// valid layout, and size is greater than 0.
Thanks for adding all the `// SAFETY` comments here Boqun!
Björn, do they look good to you? (since you fixed the issue in the compiler)
On this comment in particular, "generated at `GlobalAlloc` callsites"
sounds a bit confusing to me. Would "... called by the compiler with
parameters that obey ..." make sense? Or does the sentence refer to
the normal case (i.e. when the functions are generated)? Anyway, it is
not a big deal.
Cheers,
Miguel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] rust: allocator: Prevent mis-aligned allocation
2023-07-30 20:43 ` Miguel Ojeda
@ 2023-07-30 22:36 ` Björn Roy Baron
2023-07-30 22:41 ` Björn Roy Baron
1 sibling, 0 replies; 13+ messages in thread
From: Björn Roy Baron @ 2023-07-30 22:36 UTC (permalink / raw)
To: miguel.ojeda.sandonis, boqun.feng
Cc: rust-for-linux, linux-kernel, linux-mm, ojeda, alex.gaynor,
wedsonaf, gary, benno.lossin, yakoyoku, aliceryhl, dsosnowski,
geofft, foxhlchen, john.m.baublitz, cl, penberg, rientjes,
iamjoonsoo.kim, akpm, vbabka, roman.gushchin, 42.hyeyoo,
keescook, stable, nmi
[-- Attachment #1: Type: text/plain, Size: 1130 bytes --]
On Jul 30, 2023, 10:43 PM, Miguel Ojeda < miguel.ojeda.sandonis@gmail.com> wrote:
> On Sun, Jul 30, 2023 at 3:29 AM Boqun > Feng <boqun.feng@gmail.com> wrote:
> >
> > +// Note: Although these are *safe* functions, but they are only generated at
> > +// `GlobalAlloc` callsites, hence we assume the parameters obey the same
> > +// `GlobalAlloc` function safety requirements: size and align should form a
> > +// valid layout, and size is greater than 0.
>
> Thanks for adding all the `// SAFETY` comments here Boqun!
>
> Björn, do they look good to you? (since you fixed the issue in the compiler)
Based on a quick look, yes. The __rust_* methods that are normally generated by the compiled directly jump to the respective global allocator method, so they have the same safety requirements.
>
> On this comment in particular, "generated at `GlobalAlloc` callsites"
sounds a bit confusing to me. Would "... called by the compiler with
parameters that obey ..." make sense? Or does the sentence refer to
the normal case (i.e. when the functions are generated)? Anyway, it is
not a big deal.
>
> Cheers,
> Miguel
Cheers,
Björn
[-- Attachment #2: Type: text/html, Size: 1280 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] rust: allocator: Prevent mis-aligned allocation
2023-07-30 20:43 ` Miguel Ojeda
2023-07-30 22:36 ` Björn Roy Baron
@ 2023-07-30 22:41 ` Björn Roy Baron
2023-07-30 22:53 ` Boqun Feng
1 sibling, 1 reply; 13+ messages in thread
From: Björn Roy Baron @ 2023-07-30 22:41 UTC (permalink / raw)
To: Miguel Ojeda
Cc: Boqun Feng, rust-for-linux, linux-kernel, linux-mm, Miguel Ojeda,
Alex Gaynor, Wedson Almeida Filho, Gary Guo, Benno Lossin,
Martin Rodriguez Reboredo, Alice Ryhl, Dariusz Sosnowski,
Geoffrey Thomas, Fox Chen, John Baublitz, Christoph Lameter,
Pekka Enberg, David Rientjes, Joonsoo Kim, Andrew Morton,
Vlastimil Babka, Roman Gushchin, Hyeonggon Yoo, Kees Cook,
stable, Andreas Hindborg
On Jul 30, 2023, 10:43 PM, Miguel Ojeda < miguel.ojeda.sandonis@gmail.com> wrote:
> On Sun, Jul 30, 2023 at 3:29 AM Boqun > Feng <boqun.feng@gmail.com> wrote:
> >
> > +// Note: Although these are *safe* functions, but they are only generated at
> > +// `GlobalAlloc` callsites, hence we assume the parameters obey the same
> > +// `GlobalAlloc` function safety requirements: size and align should form a
> > +// valid layout, and size is greater than 0.
>
> Thanks for adding all the `// SAFETY` comments here Boqun!
>
> Björn, do they look good to you? (since you fixed the issue in the compiler)
Based on a quick look, yes. The __rust_* methods that are normally generated by the compiled directly jump to the respective global allocator method, so they have the same safety requirements.
>
> On this comment in particular, "generated at `GlobalAlloc` callsites"
sounds a bit confusing to me. Would "... called by the compiler with
parameters that obey ..." make sense? Or does the sentence refer to
the normal case (i.e. when the functions are generated)? Anyway, it is
not a big deal.
>
> Cheers,
> Miguel
Cheers,
Björn
(resent as I accidentally sent html instead of plain text)
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] rust: allocator: Prevent mis-aligned allocation
2023-07-30 22:41 ` Björn Roy Baron
@ 2023-07-30 22:53 ` Boqun Feng
2023-07-31 0:13 ` Miguel Ojeda
0 siblings, 1 reply; 13+ messages in thread
From: Boqun Feng @ 2023-07-30 22:53 UTC (permalink / raw)
To: Björn Roy Baron
Cc: Miguel Ojeda, rust-for-linux, linux-kernel, linux-mm,
Miguel Ojeda, Alex Gaynor, Wedson Almeida Filho, Gary Guo,
Benno Lossin, Martin Rodriguez Reboredo, Alice Ryhl,
Dariusz Sosnowski, Geoffrey Thomas, Fox Chen, John Baublitz,
Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim,
Andrew Morton, Vlastimil Babka, Roman Gushchin, Hyeonggon Yoo,
Kees Cook, stable, Andreas Hindborg
On Sun, Jul 30, 2023 at 10:41:54PM +0000, Björn Roy Baron wrote:
> On Jul 30, 2023, 10:43 PM, Miguel Ojeda < miguel.ojeda.sandonis@gmail.com> wrote:
> > On Sun, Jul 30, 2023 at 3:29 AM Boqun > Feng <boqun.feng@gmail.com> wrote:
> > >
> > > +// Note: Although these are *safe* functions, but they are only generated at
> > > +// `GlobalAlloc` callsites, hence we assume the parameters obey the same
> > > +// `GlobalAlloc` function safety requirements: size and align should form a
> > > +// valid layout, and size is greater than 0.
> >
> > Thanks for adding all the `// SAFETY` comments here Boqun!
> >
> > Björn, do they look good to you? (since you fixed the issue in the compiler)
>
> Based on a quick look, yes. The __rust_* methods that are normally generated by the compiled directly jump to the respective global allocator method, so they have the same safety requirements.
>
Good to know, thanks!
> >
> > On this comment in particular, "generated at `GlobalAlloc` callsites"
> sounds a bit confusing to me. Would "... called by the compiler with
> parameters that obey ..." make sense? Or does the sentence refer to
Agreed. It's better. So reword as below:
// Note: Although these are *safe* functions, but they are called by the
// compiler with the parameters that obey the same `GlobalAlloc`
// function safety requirements: size and align should form a valid
// layout, and size is greater than 0.
Regards,
Boqun
> the normal case (i.e. when the functions are generated)? Anyway, it is
> not a big deal.
> >
> > Cheers,
> > Miguel
>
> Cheers,
> Björn
>
> (resent as I accidentally sent html instead of plain text)
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] rust: allocator: Prevent mis-aligned allocation
2023-07-30 22:53 ` Boqun Feng
@ 2023-07-31 0:13 ` Miguel Ojeda
0 siblings, 0 replies; 13+ messages in thread
From: Miguel Ojeda @ 2023-07-31 0:13 UTC (permalink / raw)
To: Boqun Feng
Cc: Björn Roy Baron, rust-for-linux, linux-kernel, linux-mm,
Miguel Ojeda, Alex Gaynor, Wedson Almeida Filho, Gary Guo,
Benno Lossin, Martin Rodriguez Reboredo, Alice Ryhl,
Dariusz Sosnowski, Geoffrey Thomas, Fox Chen, John Baublitz,
Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim,
Andrew Morton, Vlastimil Babka, Roman Gushchin, Hyeonggon Yoo,
Kees Cook, stable, Andreas Hindborg
On Mon, Jul 31, 2023 at 12:54 AM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> Good to know, thanks!
Yeah, thanks Björn!
> Agreed. It's better. So reword as below:
>
> // Note: Although these are *safe* functions, but they are called by the
> // compiler with the parameters that obey the same `GlobalAlloc`
> // function safety requirements: size and align should form a valid
> // layout, and size is greater than 0.
+1, thanks!
Applied to `rust-fixes`, but please feel free to send `Reviewed-by`s.
Cheers,
Miguel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] rust: allocator: Prevent mis-aligned allocation
2023-07-30 1:29 ` [PATCH 1/3] rust: allocator: Prevent mis-aligned allocation Boqun Feng
2023-07-30 20:43 ` Miguel Ojeda
@ 2023-07-31 7:56 ` Vlastimil Babka
1 sibling, 0 replies; 13+ messages in thread
From: Vlastimil Babka @ 2023-07-31 7:56 UTC (permalink / raw)
To: Boqun Feng, rust-for-linux, linux-kernel, linux-mm
Cc: Miguel Ojeda, Alex Gaynor, Wedson Almeida Filho, Gary Guo,
Björn Roy Baron, Benno Lossin, Martin Rodriguez Reboredo,
Alice Ryhl, Dariusz Sosnowski, Geoffrey Thomas, Fox Chen,
John Baublitz, Christoph Lameter, Pekka Enberg, David Rientjes,
Joonsoo Kim, Andrew Morton, Roman Gushchin, Hyeonggon Yoo,
Kees Cook, stable, Andreas Hindborg
On 7/30/23 03:29, Boqun Feng wrote:
> Currently the rust allocator 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.
>
> Fix 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 the 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+
Acked-by: Vlastimil Babka <vbabka@suse.cz>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] rust: allocator: Use krealloc_aligned() in KernelAllocator::alloc
2023-07-30 1:29 ` [PATCH 2/3] rust: allocator: Use krealloc_aligned() in KernelAllocator::alloc Boqun Feng
@ 2023-07-31 8:48 ` Andreas Hindborg (Samsung)
0 siblings, 0 replies; 13+ messages in thread
From: Andreas Hindborg (Samsung) @ 2023-07-31 8:48 UTC (permalink / raw)
To: Boqun Feng
Cc: rust-for-linux, linux-kernel, linux-mm, Miguel Ojeda,
Alex Gaynor, Wedson Almeida Filho, Gary Guo,
Björn Roy Baron, Benno Lossin, Martin Rodriguez Reboredo,
Alice Ryhl, Dariusz Sosnowski, Geoffrey Thomas, Fox Chen,
John Baublitz, Christoph Lameter, Pekka Enberg, David Rientjes,
Joonsoo Kim, Andrew Morton, Vlastimil Babka, Roman Gushchin,
Hyeonggon Yoo, Kees Cook, stable
Boqun Feng <boqun.feng@gmail.com> writes:
> This fixes the potential issue that when KernelAllocator is used, the
> allocation may be mis-aligned due to SLAB's alignment guarantee.
>
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> ---
> rust/kernel/allocator.rs | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/rust/kernel/allocator.rs b/rust/kernel/allocator.rs
> index fae11d1fdba7..1aec688cf0e0 100644
> --- a/rust/kernel/allocator.rs
> +++ b/rust/kernel/allocator.rs
> @@ -41,9 +41,9 @@ unsafe fn krealloc_aligned(ptr: *mut u8, new_layout: Layout, flags: bindings::gf
>
> unsafe impl GlobalAlloc for KernelAllocator {
> unsafe fn alloc(&self, layout: Layout) -> *mut u8 {
> - // `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 }
> + // SAFETY: `ptr::null_mut()` is null and `layout` has a non-zero size by the function safety
> + // requirement.
> + unsafe { krealloc_aligned(ptr::null_mut(), layout, bindings::GFP_KERNEL) }
> }
>
> unsafe fn dealloc(&self, ptr: *mut u8, _layout: Layout) {
Reviewed-by: Andreas Hindborg <a.hindborg@samsung.com>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] rust: alloc: Add realloc and alloc_zeroed to the GlobalAlloc impl
2023-07-30 1:29 ` [PATCH 3/3] rust: alloc: Add realloc and alloc_zeroed to the GlobalAlloc impl Boqun Feng
@ 2023-07-31 11:12 ` Andreas Hindborg (Samsung)
0 siblings, 0 replies; 13+ messages in thread
From: Andreas Hindborg (Samsung) @ 2023-07-31 11:12 UTC (permalink / raw)
To: Boqun Feng
Cc: rust-for-linux, linux-kernel, linux-mm, Miguel Ojeda,
Alex Gaynor, Wedson Almeida Filho, Gary Guo,
Björn Roy Baron, Benno Lossin, Martin Rodriguez Reboredo,
Alice Ryhl, Dariusz Sosnowski, Geoffrey Thomas, Fox Chen,
John Baublitz, Christoph Lameter, Pekka Enberg, David Rientjes,
Joonsoo Kim, Andrew Morton, Vlastimil Babka, Roman Gushchin,
Hyeonggon Yoo, Kees Cook, stable
Boqun Feng <boqun.feng@gmail.com> writes:
> From: Björn Roy Baron <bjorn3_gh@protonmail.com>
>
> While there are default impls for these methods, using the respective C
> api's is faster. Currently neither the existing nor these new
> GlobalAlloc method implementations are actually called. Instead the
> __rust_* function defined below the GlobalAlloc impl are used. With
> rustc 1.71 these functions will be gone and all allocation calls will go
> through the GlobalAlloc implementation.
>
> Link: https://github.com/Rust-for-Linux/linux/issues/68
> Signed-off-by: Björn Roy Baron <bjorn3_gh@protonmail.com>
> [boqun: add size adjustment for alignment requirement]
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> ---
Reviewed-by: Andreas Hindborg <a.hindborg@samsung.com>
> rust/kernel/allocator.rs | 27 +++++++++++++++++++++++++++
> 1 file changed, 27 insertions(+)
>
> diff --git a/rust/kernel/allocator.rs b/rust/kernel/allocator.rs
> index 1aec688cf0e0..6f1f50465ab3 100644
> --- a/rust/kernel/allocator.rs
> +++ b/rust/kernel/allocator.rs
> @@ -51,6 +51,33 @@ unsafe fn dealloc(&self, ptr: *mut u8, _layout: Layout) {
> bindings::kfree(ptr as *const core::ffi::c_void);
> }
> }
> +
> + unsafe fn realloc(&self, ptr: *mut u8, layout: Layout, new_size: usize) -> *mut u8 {
> + // SAFETY:
> + // - `new_size`, when rounded up to the nearest multiple of `layout.align()`, will not
> + // overflow `isize` by the function safety requirement.
> + // - `layout.align()` is a proper alignment (i.e. not zero and must be a power of two).
> + let layout = unsafe { Layout::from_size_align_unchecked(new_size, layout.align()) };
> +
> + // SAFETY:
> + // - `ptr` is either null or a pointer allocated by this allocator by the function safety
> + // requirement.
> + // - the size of `layout` is not zero because `new_size` is not zero by the function safety
> + // requirement.
> + unsafe { krealloc_aligned(ptr, layout, bindings::GFP_KERNEL) }
> + }
> +
> + unsafe fn alloc_zeroed(&self, layout: Layout) -> *mut u8 {
> + // SAFETY: `ptr::null_mut()` is null and `layout` has a non-zero size by the function safety
> + // requirement.
> + unsafe {
> + krealloc_aligned(
> + ptr::null_mut(),
> + layout,
> + bindings::GFP_KERNEL | bindings::__GFP_ZERO,
> + )
> + }
> + }
> }
>
> #[global_allocator]
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2023-07-31 11:14 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-07-30 1:29 [PATCH 0/3] Fix alignment issue and prepare for rust 1.71 Boqun Feng
2023-07-30 1:29 ` [PATCH 1/3] rust: allocator: Prevent mis-aligned allocation Boqun Feng
2023-07-30 20:43 ` Miguel Ojeda
2023-07-30 22:36 ` Björn Roy Baron
2023-07-30 22:41 ` Björn Roy Baron
2023-07-30 22:53 ` Boqun Feng
2023-07-31 0:13 ` Miguel Ojeda
2023-07-31 7:56 ` Vlastimil Babka
2023-07-30 1:29 ` [PATCH 2/3] rust: allocator: Use krealloc_aligned() in KernelAllocator::alloc Boqun Feng
2023-07-31 8:48 ` Andreas Hindborg (Samsung)
2023-07-30 1:29 ` [PATCH 3/3] rust: alloc: Add realloc and alloc_zeroed to the GlobalAlloc impl Boqun Feng
2023-07-31 11:12 ` Andreas Hindborg (Samsung)
2023-07-30 20:43 ` [PATCH 0/3] Fix alignment issue and prepare for rust 1.71 Miguel Ojeda
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox