Hi Matthew, On 12/16/2024 11:52 PM, Matthew Auld wrote:
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@xxxxxxx>Signed-off-by: Lin.Cao <lincao12@xxxxxxx> --- 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?
WARN does pop but there is no interface to detect this warning by the KUNIT.
Good idea, we need this from test case perspective, I will make changes and send the next version.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.
Regards, Arun.
> > kfree(mm->roots);diff --git a/drivers/gpu/drm/tests/drm_buddy_test.c b/drivers/gpu/drm/tests/drm_buddy_test.cindex 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;
