rust: alloc: Fix `ArrayLayout` allocations

[PATCH] rust: alloc: Fix `ArrayLayout` allocations

Posted by Asahi Lina 1 year, 2 months ago

We were accidentally allocating a layout for the *square* of the object
size due to a variable shadowing mishap.

Fixes memory bloat and page allocation failures in drm/asahi.

Reported-by: Janne Grunau <j@jannau.net>
Fixes: 9e7bbfa18276 ("rust: alloc: introduce `ArrayLayout`")
Signed-off-by: Asahi Lina <lina@asahilina.net>
---
 rust/kernel/alloc/layout.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/rust/kernel/alloc/layout.rs b/rust/kernel/alloc/layout.rs
index 7e0c2f46157b772248450a77ff445091e17fdfd7..4b3cd7fdc816c158e63ac74014cbfc0794547e81 100644
--- a/rust/kernel/alloc/layout.rs
+++ b/rust/kernel/alloc/layout.rs
@@ -45,7 +45,7 @@ pub const fn empty() -> Self {
     /// When `len * size_of::<T>()` overflows or when `len * size_of::<T>() > isize::MAX`.
     pub const fn new(len: usize) -> Result<Self, LayoutError> {
         match len.checked_mul(core::mem::size_of::<T>()) {
-            Some(len) if len <= ISIZE_MAX => {
+            Some(size) if size <= ISIZE_MAX => {
                 // INVARIANT: We checked above that `len * size_of::<T>() <= isize::MAX`.
                 Ok(Self {
                     len,

---
base-commit: b2603f8ac8217bc59f5c7f248ac248423b9b99cb
change-id: 20241123-rust-fix-arraylayout-0b1009d89fb7

Cheers,
~~ Lina

Re: [PATCH] rust: alloc: Fix `ArrayLayout` allocations

Posted by Miguel Ojeda 1 year, 2 months ago

On Sat, Nov 23, 2024 at 11:30 AM Asahi Lina <lina@asahilina.net> wrote:
>
> We were accidentally allocating a layout for the *square* of the object
> size due to a variable shadowing mishap.
>
> Fixes memory bloat and page allocation failures in drm/asahi.
>
> Reported-by: Janne Grunau <j@jannau.net>
> Fixes: 9e7bbfa18276 ("rust: alloc: introduce `ArrayLayout`")
> Signed-off-by: Asahi Lina <lina@asahilina.net>

Applied to `rust-next` -- thanks everyone!

Cheers,
Miguel

Re: [PATCH] rust: alloc: Fix `ArrayLayout` allocations

Posted by Danilo Krummrich 1 year, 2 months ago

On Sat, Nov 23, 2024 at 07:29:38PM +0900, Asahi Lina wrote:
> We were accidentally allocating a layout for the *square* of the object
> size due to a variable shadowing mishap.
> 
> Fixes memory bloat and page allocation failures in drm/asahi.
> 
> Reported-by: Janne Grunau <j@jannau.net>
> Fixes: 9e7bbfa18276 ("rust: alloc: introduce `ArrayLayout`")
> Signed-off-by: Asahi Lina <lina@asahilina.net>

Good catch!

Acked-by: Danilo Krummrich <dakr@kernel.org>

(I'm just back from moving and just starting to catch up on what was going on
in the last few weeks.)

Is this related to the performance regression that has been observed by Andreas?
Or did it turn out to be a false positive?

- Danilo

> ---
>  rust/kernel/alloc/layout.rs | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/rust/kernel/alloc/layout.rs b/rust/kernel/alloc/layout.rs
> index 7e0c2f46157b772248450a77ff445091e17fdfd7..4b3cd7fdc816c158e63ac74014cbfc0794547e81 100644
> --- a/rust/kernel/alloc/layout.rs
> +++ b/rust/kernel/alloc/layout.rs
> @@ -45,7 +45,7 @@ pub const fn empty() -> Self {
>      /// When `len * size_of::<T>()` overflows or when `len * size_of::<T>() > isize::MAX`.
>      pub const fn new(len: usize) -> Result<Self, LayoutError> {
>          match len.checked_mul(core::mem::size_of::<T>()) {
> -            Some(len) if len <= ISIZE_MAX => {
> +            Some(size) if size <= ISIZE_MAX => {
>                  // INVARIANT: We checked above that `len * size_of::<T>() <= isize::MAX`.
>                  Ok(Self {
>                      len,
> 
> ---
> base-commit: b2603f8ac8217bc59f5c7f248ac248423b9b99cb
> change-id: 20241123-rust-fix-arraylayout-0b1009d89fb7
> 
> Cheers,
> ~~ Lina
>

Re: [PATCH] rust: alloc: Fix `ArrayLayout` allocations

Posted by Janne Grunau 1 year, 2 months ago

On Sat, Nov 23, 2024 at 09:08:35PM +0100, Danilo Krummrich wrote:
> On Sat, Nov 23, 2024 at 07:29:38PM +0900, Asahi Lina wrote:
> > We were accidentally allocating a layout for the *square* of the object
> > size due to a variable shadowing mishap.
> > 
> > Fixes memory bloat and page allocation failures in drm/asahi.
> > 
> > Reported-by: Janne Grunau <j@jannau.net>
> > Fixes: 9e7bbfa18276 ("rust: alloc: introduce `ArrayLayout`")
> > Signed-off-by: Asahi Lina <lina@asahilina.net>
> 
> Good catch!
> 
> Acked-by: Danilo Krummrich <dakr@kernel.org>
> 
> (I'm just back from moving and just starting to catch up on what was going on
> in the last few weeks.)
> 
> Is this related to the performance regression that has been observed by Andreas?
> Or did it turn out to be a false positive?

No idea. We noticed KVec allocation errors with page order 4 to 8 under
memory pressure which weren't observed with the previous allocator (or at
least not that easily).

I haven't noticed performance regressions with asahi's usage. glmark2
score was roughly at the expected value.

Janne

Re: [PATCH] rust: alloc: Fix `ArrayLayout` allocations

Posted by Miguel Ojeda 1 year, 2 months ago

On Sat, Nov 23, 2024 at 11:30 AM Asahi Lina <lina@asahilina.net> wrote:
>
> We were accidentally allocating a layout for the *square* of the object
> size due to a variable shadowing mishap.

Good catch, thanks! (Square?)

`clippy::shadow_reuse` would catch this, but it does catch a lot more
things, sadly.

I sent:

    https://github.com/rust-lang/rust-clippy/issues/3433#issuecomment-2495547322

and added it to the Clippy list. If one of the ideas in the issue are
implemented, then I think we should easily enable it.

Similarly, we could do `shadow_unrelated` -- that one seems easier,
and perhaps we should do it anyway, at least a couple cases I saw
would make the code clearer.

Cheers,
Miguel

Re: [PATCH] rust: alloc: Fix `ArrayLayout` allocations

Posted by Boqun Feng 1 year, 2 months ago

On Sat, Nov 23, 2024 at 06:39:23PM +0100, Miguel Ojeda wrote:
> On Sat, Nov 23, 2024 at 11:30 AM Asahi Lina <lina@asahilina.net> wrote:
> >
> > We were accidentally allocating a layout for the *square* of the object
> > size due to a variable shadowing mishap.
> 
> Good catch, thanks! (Square?)
> 

While we are at it, I think it'll be good to add some example/tests for
those functions of ArrayLayout, for example, the below will catch this:

I will open a good-first-issue.

Regards,
Boqun

------------------------>8
diff --git a/rust/kernel/alloc/layout.rs b/rust/kernel/alloc/layout.rs
index 7e0c2f46157b..bb3ce3b2218b 100644
--- a/rust/kernel/alloc/layout.rs
+++ b/rust/kernel/alloc/layout.rs
@@ -7,6 +7,7 @@
 use core::{alloc::Layout, marker::PhantomData};
 
 /// Error when constructing an [`ArrayLayout`].
+#[derive(Debug)]
 pub struct LayoutError;
 
 /// A layout for an array `[T; n]`.
@@ -43,6 +44,20 @@ pub const fn empty() -> Self {
     /// # Errors
     ///
     /// When `len * size_of::<T>()` overflows or when `len * size_of::<T>() > isize::MAX`.
+    ///
+    /// # Examples
+    ///
+    /// ```rust
+    /// use kernel::alloc::layout::ArrayLayout;
+    ///
+    /// // No overflow.
+    /// let layout = ArrayLayout::<i32>::new(12);
+    /// assert_eq!(layout.expect("sizeof(i32) * 12 is 48, not overflow").len(), 12);
+    ///
+    /// // Overflow, should return `Err`.
+    /// let layout = ArrayLayout::<i32>::new(isize::MAX as usize);
+    /// assert!(layout.is_err());
+    /// ```
     pub const fn new(len: usize) -> Result<Self, LayoutError> {
         match len.checked_mul(core::mem::size_of::<T>()) {
             Some(len) if len <= ISIZE_MAX => {

Re: [PATCH] rust: alloc: Fix `ArrayLayout` allocations

Posted by Miguel Ojeda 1 year, 2 months ago

On Mon, Nov 25, 2024 at 5:20 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> While we are at it, I think it'll be good to add some example/tests for
> those functions of ArrayLayout, for example, the below will catch this:
>
> I will open a good-first-issue.

Indeed, thanks!

Cheers,
Miguel

Re: [PATCH] rust: alloc: Fix `ArrayLayout` allocations

Posted by Miguel Ojeda 1 year, 2 months ago

On Sat, Nov 23, 2024 at 6:39 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> (Square?)

Ah, you mean eventually on the actual allocation, never mind!

Cheers,
Miguel

Re: [PATCH] rust: alloc: Fix `ArrayLayout` allocations

Posted by Neal Gompa 1 year, 2 months ago

On Sat, Nov 23, 2024 at 5:30 AM Asahi Lina <lina@asahilina.net> wrote:
>
> We were accidentally allocating a layout for the *square* of the object
> size due to a variable shadowing mishap.
>
> Fixes memory bloat and page allocation failures in drm/asahi.
>
> Reported-by: Janne Grunau <j@jannau.net>
> Fixes: 9e7bbfa18276 ("rust: alloc: introduce `ArrayLayout`")
> Signed-off-by: Asahi Lina <lina@asahilina.net>
> ---
>  rust/kernel/alloc/layout.rs | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/rust/kernel/alloc/layout.rs b/rust/kernel/alloc/layout.rs
> index 7e0c2f46157b772248450a77ff445091e17fdfd7..4b3cd7fdc816c158e63ac74014cbfc0794547e81 100644
> --- a/rust/kernel/alloc/layout.rs
> +++ b/rust/kernel/alloc/layout.rs
> @@ -45,7 +45,7 @@ pub const fn empty() -> Self {
>      /// When `len * size_of::<T>()` overflows or when `len * size_of::<T>() > isize::MAX`.
>      pub const fn new(len: usize) -> Result<Self, LayoutError> {
>          match len.checked_mul(core::mem::size_of::<T>()) {
> -            Some(len) if len <= ISIZE_MAX => {
> +            Some(size) if size <= ISIZE_MAX => {
>                  // INVARIANT: We checked above that `len * size_of::<T>() <= isize::MAX`.
>                  Ok(Self {
>                      len,
>
> ---
> base-commit: b2603f8ac8217bc59f5c7f248ac248423b9b99cb
> change-id: 20241123-rust-fix-arraylayout-0b1009d89fb7
>
> Cheers,
> ~~ Lina
>
>

The joy of logic bugs. :(

Reviewed-by: Neal Gompa <neal@gompa.dev>


-- 
真実はいつも一つ！/ Always, there's only one truth!