diff mbox series

[2/2] drm/buddy: Add a testcase to verify the multiroot fini

Message ID 20241216130735.314298-2-Arunpravin.PaneerSelvam@amd.com (mailing list archive)
State New
Headers show
Series [1/2] drm/buddy: fix issue that force_merge cannot free all roots | expand

Commit Message

Arunpravin Paneer Selvam Dec. 16, 2024, 1:07 p.m. UTC 
- Added a testcase to verify the multiroot force merge fini.
- Added a new field in_use to track the mm freed status.

Signed-off-by: Arunpravin Paneer Selvam <Arunpravin.PaneerSelvam@amd.com>
Signed-off-by: Lin.Cao <lincao12@amd.com>
---
 drivers/gpu/drm/drm_buddy.c            | 20 ++++++++++++++++-
 drivers/gpu/drm/tests/drm_buddy_test.c | 30 ++++++++++++++++++--------
 include/drm/drm_buddy.h                |  2 ++
 3 files changed, 42 insertions(+), 10 deletions(-)

Comments

Matthew Auld Dec. 16, 2024, 6:22 p.m. UTC | #1 
On 16/12/2024 13:07, Arunpravin Paneer Selvam wrote:
> - Added a testcase to verify the multiroot force merge fini.
> - Added a new field in_use to track the mm freed status.
> 
> Signed-off-by: Arunpravin Paneer Selvam <Arunpravin.PaneerSelvam@amd.com>
> Signed-off-by: Lin.Cao <lincao12@amd.com>
> ---
>   drivers/gpu/drm/drm_buddy.c            | 20 ++++++++++++++++-
>   drivers/gpu/drm/tests/drm_buddy_test.c | 30 ++++++++++++++++++--------
>   include/drm/drm_buddy.h                |  2 ++
>   3 files changed, 42 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c
> index ca42e6081d27..39ce918b3a65 100644
> --- a/drivers/gpu/drm/drm_buddy.c
> +++ b/drivers/gpu/drm/drm_buddy.c
> @@ -102,6 +102,18 @@ static inline bool contains(u64 s1, u64 e1, u64 s2, u64 e2)
>   	return s1 <= s2 && e1 >= e2;
>   }
>   
> +static bool is_roots_freed(struct drm_buddy *mm)
> +{
> +	int i;
> +
> +	for (i = 0; i < mm->n_roots; ++i) {
> +		if (!drm_buddy_block_is_free(mm->roots[i]))
> +			return false;
> +	}
> +
> +	return true;
> +}
> +
>   static struct drm_buddy_block *
>   __get_buddy(struct drm_buddy_block *block)
>   {
> @@ -303,6 +315,8 @@ int drm_buddy_init(struct drm_buddy *mm, u64 size, u64 chunk_size)
>   		i++;
>   	} while (size);
>   
> +	mm->in_use = true;
> +
>   	return 0;
>   
>   out_free_roots:
> @@ -335,13 +349,17 @@ void drm_buddy_fini(struct drm_buddy *mm)
>   		start = drm_buddy_block_offset(mm->roots[i]);
>   		__force_merge(mm, start, start + size, order);
>   
> -		WARN_ON(!drm_buddy_block_is_free(mm->roots[i]));

So does this warn not pop? Or it does but kunit ignores it or something?

>   		drm_block_free(mm, mm->roots[i]);
>   
>   		root_size = mm->chunk_size << order;
>   		size -= root_size;
>   	}
>   
> +	mm->in_use = false;
> +
> +	if (WARN_ON(!is_roots_freed(mm)))

This looks like UAF under normal operation, since each root pointer 
within mm->roots is already gone.

How about something like this:

+ #include <kunit/test-bug.h>
+
  #include <linux/kmemleak.h>
  #include <linux/module.h>
  #include <linux/sizes.h>
@@ -335,7 +337,9 @@ void drm_buddy_fini(struct drm_buddy *mm)
                 start = drm_buddy_block_offset(mm->roots[i]);
                 __force_merge(mm, start, start + size, order);

-               WARN_ON(!drm_buddy_block_is_free(mm->roots[i]));
+               if (WARN_ON(!drm_buddy_block_is_free(mm->roots[i])))
+                       kunit_fail_current_test("buddy_fini() root");
+
                 drm_block_free(mm, mm->roots[i]);

                 root_size = mm->chunk_size << order;

And then also drop the in_use stuff. As a follow up could do that for 
all warnings in this file that don't result in error being returned to 
the caller...

> +		mm->in_use = true;
> +
>   	WARN_ON(mm->avail != mm->size);

...like this one.

 >   >   	kfree(mm->roots);
> diff --git a/drivers/gpu/drm/tests/drm_buddy_test.c b/drivers/gpu/drm/tests/drm_buddy_test.c
> index 9662c949d0e3..694b058ddd6d 100644
> --- a/drivers/gpu/drm/tests/drm_buddy_test.c
> +++ b/drivers/gpu/drm/tests/drm_buddy_test.c
> @@ -385,19 +385,31 @@ static void drm_test_buddy_alloc_clear(struct kunit *test)
>   	drm_buddy_fini(&mm);
>   
>   	/*
> -	 * Create a new mm with a non power-of-two size. Allocate a random size, free as
> -	 * cleared and then call fini. This will ensure the multi-root force merge during
> -	 * fini.
> +	 * Create a new mm with a non power-of-two size. Allocate a random size from each
> +	 * root, free as cleared and then call fini. This will ensure the multi-root
> +	 * force merge during fini.
>   	 */
> -	mm_size = 12 * SZ_4K;
> -	size = max(round_up(prandom_u32_state(&prng) % mm_size, ps), ps);
> +	mm_size = (SZ_4K << max_order) + (SZ_4K << (max_order - 2));
> +
>   	KUNIT_EXPECT_FALSE(test, drm_buddy_init(&mm, mm_size, ps));
> -	KUNIT_ASSERT_FALSE_MSG(test, drm_buddy_alloc_blocks(&mm, 0, mm_size,
> -							    size, ps, &allocated,
> -							    DRM_BUDDY_TOPDOWN_ALLOCATION),
> -				"buddy_alloc hit an error size=%u\n", size);
> +	KUNIT_EXPECT_EQ(test, mm.max_order, max_order);
> +	KUNIT_ASSERT_FALSE_MSG(test, drm_buddy_alloc_blocks(&mm, 0, SZ_4K << max_order,
> +							    4 * ps, ps, &allocated,
> +							    DRM_BUDDY_RANGE_ALLOCATION),
> +				"buddy_alloc hit an error size=%lu\n", 4 * ps);
> +	drm_buddy_free_list(&mm, &allocated, DRM_BUDDY_CLEARED);
> +	KUNIT_ASSERT_FALSE_MSG(test, drm_buddy_alloc_blocks(&mm, 0, SZ_4K << max_order,
> +							    2 * ps, ps, &allocated,
> +							    DRM_BUDDY_CLEAR_ALLOCATION),
> +				"buddy_alloc hit an error size=%lu\n", 2 * ps);
> +	drm_buddy_free_list(&mm, &allocated, DRM_BUDDY_CLEARED);
> +	KUNIT_ASSERT_FALSE_MSG(test, drm_buddy_alloc_blocks(&mm, SZ_4K << max_order, mm_size,
> +							    ps, ps, &allocated,
> +							    DRM_BUDDY_RANGE_ALLOCATION),
> +				"buddy_alloc hit an error size=%lu\n", ps);
>   	drm_buddy_free_list(&mm, &allocated, DRM_BUDDY_CLEARED);
>   	drm_buddy_fini(&mm);
> +	KUNIT_EXPECT_EQ(test, mm.in_use, false);
>   }
>   
>   static void drm_test_buddy_alloc_contiguous(struct kunit *test)
> diff --git a/include/drm/drm_buddy.h b/include/drm/drm_buddy.h
> index 9689a7c5dd36..d692d831ffdd 100644
> --- a/include/drm/drm_buddy.h
> +++ b/include/drm/drm_buddy.h
> @@ -86,6 +86,8 @@ struct drm_buddy {
>   	unsigned int n_roots;
>   	unsigned int max_order;
>   
> +	bool in_use;
> +
>   	/* Must be at least SZ_4K */
>   	u64 chunk_size;
>   	u64 size;
diff mbox series

Patch

diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c
index ca42e6081d27..39ce918b3a65 100644
--- a/drivers/gpu/drm/drm_buddy.c
+++ b/drivers/gpu/drm/drm_buddy.c
@@ -102,6 +102,18 @@  static inline bool contains(u64 s1, u64 e1, u64 s2, u64 e2)
 	return s1 <= s2 && e1 >= e2;
 }
 
+static bool is_roots_freed(struct drm_buddy *mm)
+{
+	int i;
+
+	for (i = 0; i < mm->n_roots; ++i) {
+		if (!drm_buddy_block_is_free(mm->roots[i]))
+			return false;
+	}
+
+	return true;
+}
+
 static struct drm_buddy_block *
 __get_buddy(struct drm_buddy_block *block)
 {
@@ -303,6 +315,8 @@  int drm_buddy_init(struct drm_buddy *mm, u64 size, u64 chunk_size)
 		i++;
 	} while (size);
 
+	mm->in_use = true;
+
 	return 0;
 
 out_free_roots:
@@ -335,13 +349,17 @@  void drm_buddy_fini(struct drm_buddy *mm)
 		start = drm_buddy_block_offset(mm->roots[i]);
 		__force_merge(mm, start, start + size, order);
 
-		WARN_ON(!drm_buddy_block_is_free(mm->roots[i]));
 		drm_block_free(mm, mm->roots[i]);
 
 		root_size = mm->chunk_size << order;
 		size -= root_size;
 	}
 
+	mm->in_use = false;
+
+	if (WARN_ON(!is_roots_freed(mm)))
+		mm->in_use = true;
+
 	WARN_ON(mm->avail != mm->size);
 
 	kfree(mm->roots);
diff --git a/drivers/gpu/drm/tests/drm_buddy_test.c b/drivers/gpu/drm/tests/drm_buddy_test.c
index 9662c949d0e3..694b058ddd6d 100644
--- a/drivers/gpu/drm/tests/drm_buddy_test.c
+++ b/drivers/gpu/drm/tests/drm_buddy_test.c
@@ -385,19 +385,31 @@  static void drm_test_buddy_alloc_clear(struct kunit *test)
 	drm_buddy_fini(&mm);
 
 	/*
-	 * Create a new mm with a non power-of-two size. Allocate a random size, free as
-	 * cleared and then call fini. This will ensure the multi-root force merge during
-	 * fini.
+	 * Create a new mm with a non power-of-two size. Allocate a random size from each
+	 * root, free as cleared and then call fini. This will ensure the multi-root
+	 * force merge during fini.
 	 */
-	mm_size = 12 * SZ_4K;
-	size = max(round_up(prandom_u32_state(&prng) % mm_size, ps), ps);
+	mm_size = (SZ_4K << max_order) + (SZ_4K << (max_order - 2));
+
 	KUNIT_EXPECT_FALSE(test, drm_buddy_init(&mm, mm_size, ps));
-	KUNIT_ASSERT_FALSE_MSG(test, drm_buddy_alloc_blocks(&mm, 0, mm_size,
-							    size, ps, &allocated,
-							    DRM_BUDDY_TOPDOWN_ALLOCATION),
-				"buddy_alloc hit an error size=%u\n", size);
+	KUNIT_EXPECT_EQ(test, mm.max_order, max_order);
+	KUNIT_ASSERT_FALSE_MSG(test, drm_buddy_alloc_blocks(&mm, 0, SZ_4K << max_order,
+							    4 * ps, ps, &allocated,
+							    DRM_BUDDY_RANGE_ALLOCATION),
+				"buddy_alloc hit an error size=%lu\n", 4 * ps);
+	drm_buddy_free_list(&mm, &allocated, DRM_BUDDY_CLEARED);
+	KUNIT_ASSERT_FALSE_MSG(test, drm_buddy_alloc_blocks(&mm, 0, SZ_4K << max_order,
+							    2 * ps, ps, &allocated,
+							    DRM_BUDDY_CLEAR_ALLOCATION),
+				"buddy_alloc hit an error size=%lu\n", 2 * ps);
+	drm_buddy_free_list(&mm, &allocated, DRM_BUDDY_CLEARED);
+	KUNIT_ASSERT_FALSE_MSG(test, drm_buddy_alloc_blocks(&mm, SZ_4K << max_order, mm_size,
+							    ps, ps, &allocated,
+							    DRM_BUDDY_RANGE_ALLOCATION),
+				"buddy_alloc hit an error size=%lu\n", ps);
 	drm_buddy_free_list(&mm, &allocated, DRM_BUDDY_CLEARED);
 	drm_buddy_fini(&mm);
+	KUNIT_EXPECT_EQ(test, mm.in_use, false);
 }
 
 static void drm_test_buddy_alloc_contiguous(struct kunit *test)
diff --git a/include/drm/drm_buddy.h b/include/drm/drm_buddy.h
index 9689a7c5dd36..d692d831ffdd 100644
--- a/include/drm/drm_buddy.h
+++ b/include/drm/drm_buddy.h
@@ -86,6 +86,8 @@  struct drm_buddy {
 	unsigned int n_roots;
 	unsigned int max_order;
 
+	bool in_use;
+
 	/* Must be at least SZ_4K */
 	u64 chunk_size;
 	u64 size;