| 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 |
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;
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@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? WARN does pop but there is no interface to detect this warning by the KUNIT. > >> 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. Good idea, we need this from test case perspective, I will make changes and send the next version. Regards, Arun. > > > > 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 --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;