[PATCH 2/2] drm/buddy: Add a testcase to verify the multiroot fini
Paneer Selvam, Arunpravin
arunpravin.paneerselvam at amd.com
Fri Dec 20 12:54:52 UTC 2024
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 at amd.com>
>> Signed-off-by: Lin.Cao <lincao12 at 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;
>
More information about the amd-gfx
mailing list