| Message ID | 20230905085832.2103-3-thomas.hellstrom@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/drm_exec, drm/drm_kunit: Fix / WA for uaf and lock alloc tracking. | expand |
Hi, On Tue, Sep 05, 2023 at 10:58:31AM +0200, Thomas Hellström wrote: > Check that object freeing from within drm_exec_fini() works as expected > and doesn't generate any warnings. > > Cc: Christian König <christian.koenig@amd.com> > Cc: dri-devel@lists.freedesktop.org > Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> > --- > drivers/gpu/drm/tests/drm_exec_test.c | 47 +++++++++++++++++++++++++++ > 1 file changed, 47 insertions(+) > > diff --git a/drivers/gpu/drm/tests/drm_exec_test.c b/drivers/gpu/drm/tests/drm_exec_test.c > index 563949d777dd..294c25f49cc7 100644 > --- a/drivers/gpu/drm/tests/drm_exec_test.c > +++ b/drivers/gpu/drm/tests/drm_exec_test.c > @@ -170,6 +170,52 @@ static void test_prepare_array(struct kunit *test) > drm_gem_private_object_fini(&gobj2); > } > > +static const struct drm_gem_object_funcs put_funcs = { > + .free = (void *)kfree, > +}; > + > +/* > + * Check that freeing objects from within drm_exec_fini() > + * behaves as expected. > + */ > +static void test_early_put(struct kunit *test) > +{ > + struct drm_exec_priv *priv = test->priv; > + struct drm_gem_object *gobj1; > + struct drm_gem_object *gobj2; > + struct drm_gem_object *array[2]; > + struct drm_exec exec; > + int ret; > + > + gobj1 = kzalloc(sizeof(*gobj1), GFP_KERNEL); > + KUNIT_EXPECT_NOT_ERR_OR_NULL(test, gobj1); > + if (!gobj1) > + return; > + > + gobj2 = kzalloc(sizeof(*gobj2), GFP_KERNEL); > + KUNIT_EXPECT_NOT_ERR_OR_NULL(test, gobj2); > + if (!gobj2) { > + kfree(gobj1); > + return; > + } > + > + gobj1->funcs = &put_funcs; > + gobj2->funcs = &put_funcs; > + array[0] = gobj1; > + array[1] = gobj2; > + drm_gem_private_object_init(priv->drm, gobj1, PAGE_SIZE); > + drm_gem_private_object_init(priv->drm, gobj2, PAGE_SIZE); > + > + drm_exec_init(&exec, DRM_EXEC_INTERRUPTIBLE_WAIT); > + drm_exec_until_all_locked(&exec) > + ret = drm_exec_prepare_array(&exec, array, ARRAY_SIZE(array), > + 1); > + KUNIT_EXPECT_EQ(test, ret, 0); > + drm_gem_object_put(gobj1); > + drm_gem_object_put(gobj2); > + drm_exec_fini(&exec); It doesn't look like you actually check that "freeing objects from within drm_exec_fini() behaves as expected." What is the expectation here, and how is it checked? Maxime
Hi, On 9/5/23 14:05, Maxime Ripard wrote: > Hi, > > On Tue, Sep 05, 2023 at 10:58:31AM +0200, Thomas Hellström wrote: >> Check that object freeing from within drm_exec_fini() works as expected >> and doesn't generate any warnings. >> >> Cc: Christian König <christian.koenig@amd.com> >> Cc: dri-devel@lists.freedesktop.org >> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> >> --- >> drivers/gpu/drm/tests/drm_exec_test.c | 47 +++++++++++++++++++++++++++ >> 1 file changed, 47 insertions(+) >> >> diff --git a/drivers/gpu/drm/tests/drm_exec_test.c b/drivers/gpu/drm/tests/drm_exec_test.c >> index 563949d777dd..294c25f49cc7 100644 >> --- a/drivers/gpu/drm/tests/drm_exec_test.c >> +++ b/drivers/gpu/drm/tests/drm_exec_test.c >> @@ -170,6 +170,52 @@ static void test_prepare_array(struct kunit *test) >> drm_gem_private_object_fini(&gobj2); >> } >> >> +static const struct drm_gem_object_funcs put_funcs = { >> + .free = (void *)kfree, >> +}; >> + >> +/* >> + * Check that freeing objects from within drm_exec_fini() >> + * behaves as expected. >> + */ >> +static void test_early_put(struct kunit *test) >> +{ >> + struct drm_exec_priv *priv = test->priv; >> + struct drm_gem_object *gobj1; >> + struct drm_gem_object *gobj2; >> + struct drm_gem_object *array[2]; >> + struct drm_exec exec; >> + int ret; >> + >> + gobj1 = kzalloc(sizeof(*gobj1), GFP_KERNEL); >> + KUNIT_EXPECT_NOT_ERR_OR_NULL(test, gobj1); >> + if (!gobj1) >> + return; >> + >> + gobj2 = kzalloc(sizeof(*gobj2), GFP_KERNEL); >> + KUNIT_EXPECT_NOT_ERR_OR_NULL(test, gobj2); >> + if (!gobj2) { >> + kfree(gobj1); >> + return; >> + } >> + >> + gobj1->funcs = &put_funcs; >> + gobj2->funcs = &put_funcs; >> + array[0] = gobj1; >> + array[1] = gobj2; >> + drm_gem_private_object_init(priv->drm, gobj1, PAGE_SIZE); >> + drm_gem_private_object_init(priv->drm, gobj2, PAGE_SIZE); >> + >> + drm_exec_init(&exec, DRM_EXEC_INTERRUPTIBLE_WAIT); >> + drm_exec_until_all_locked(&exec) >> + ret = drm_exec_prepare_array(&exec, array, ARRAY_SIZE(array), >> + 1); >> + KUNIT_EXPECT_EQ(test, ret, 0); >> + drm_gem_object_put(gobj1); >> + drm_gem_object_put(gobj2); >> + drm_exec_fini(&exec); > It doesn't look like you actually check that "freeing objects from > within drm_exec_fini() behaves as expected." What is the expectation > here, and how is it checked? Hm. Good question, I've been manually checking dmesg for lockdep splats. Is there a way to automate that? /Thomas > Maxime
On Tue, Sep 05, 2023 at 02:32:38PM +0200, Thomas Hellström wrote: > Hi, > > On 9/5/23 14:05, Maxime Ripard wrote: > > Hi, > > > > On Tue, Sep 05, 2023 at 10:58:31AM +0200, Thomas Hellström wrote: > > > Check that object freeing from within drm_exec_fini() works as expected > > > and doesn't generate any warnings. > > > > > > Cc: Christian König <christian.koenig@amd.com> > > > Cc: dri-devel@lists.freedesktop.org > > > Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> > > > --- > > > drivers/gpu/drm/tests/drm_exec_test.c | 47 +++++++++++++++++++++++++++ > > > 1 file changed, 47 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/tests/drm_exec_test.c b/drivers/gpu/drm/tests/drm_exec_test.c > > > index 563949d777dd..294c25f49cc7 100644 > > > --- a/drivers/gpu/drm/tests/drm_exec_test.c > > > +++ b/drivers/gpu/drm/tests/drm_exec_test.c > > > @@ -170,6 +170,52 @@ static void test_prepare_array(struct kunit *test) > > > drm_gem_private_object_fini(&gobj2); > > > } > > > +static const struct drm_gem_object_funcs put_funcs = { > > > + .free = (void *)kfree, > > > +}; > > > + > > > +/* > > > + * Check that freeing objects from within drm_exec_fini() > > > + * behaves as expected. > > > + */ > > > +static void test_early_put(struct kunit *test) > > > +{ > > > + struct drm_exec_priv *priv = test->priv; > > > + struct drm_gem_object *gobj1; > > > + struct drm_gem_object *gobj2; > > > + struct drm_gem_object *array[2]; > > > + struct drm_exec exec; > > > + int ret; > > > + > > > + gobj1 = kzalloc(sizeof(*gobj1), GFP_KERNEL); > > > + KUNIT_EXPECT_NOT_ERR_OR_NULL(test, gobj1); > > > + if (!gobj1) > > > + return; > > > + > > > + gobj2 = kzalloc(sizeof(*gobj2), GFP_KERNEL); > > > + KUNIT_EXPECT_NOT_ERR_OR_NULL(test, gobj2); > > > + if (!gobj2) { > > > + kfree(gobj1); > > > + return; > > > + } > > > + > > > + gobj1->funcs = &put_funcs; > > > + gobj2->funcs = &put_funcs; > > > + array[0] = gobj1; > > > + array[1] = gobj2; > > > + drm_gem_private_object_init(priv->drm, gobj1, PAGE_SIZE); > > > + drm_gem_private_object_init(priv->drm, gobj2, PAGE_SIZE); > > > + > > > + drm_exec_init(&exec, DRM_EXEC_INTERRUPTIBLE_WAIT); > > > + drm_exec_until_all_locked(&exec) > > > + ret = drm_exec_prepare_array(&exec, array, ARRAY_SIZE(array), > > > + 1); > > > + KUNIT_EXPECT_EQ(test, ret, 0); > > > + drm_gem_object_put(gobj1); > > > + drm_gem_object_put(gobj2); > > > + drm_exec_fini(&exec); > > It doesn't look like you actually check that "freeing objects from > > within drm_exec_fini() behaves as expected." What is the expectation > > here, and how is it checked? > > Hm. Good question, I've been manually checking dmesg for lockdep splats. Is > there a way to automate that? I'm not familiar with the drm_exec API, but judging by the code I'd assume you want to check that gobj1 and gobj2 are actually freed using kfree? If so, I've used tested for that by creating a waitqueue and completing it from the free function. You won't be certain that you have gone through kfree, but you'll know that drm_gem_object_funcs.free will have been called which is what you actually care about I think? So something along those lines would work I think: struct test_gem_object { struct drm_gem_object base; wait_queue_head_t freed_wq; bool freed_done; }; void free_test_gem_object(struct drm_gem_object *obj) { struct test_gem_object *test_obj = container_of(obj, struct test_gem_object, base) test_obj->freed_done = true; wake_up(&test_obj->freed_wq); }; static const struct drm_gem_object_funcs put_funcs = { .free = free_test_gem_object, } static void test_early_put(struct kunit *test) { struct test_gem_object *gobj1; ... gobj1 = kunit_kzalloc(test, sizeof(*gobj1), GFP_KERNEL); ... gobj1->base.funcs = &put_funcs; ... array[0] = &gobj1->base; drm_exec_fini(&exec); ret = wait_event_interruptible_timeout(gobj1->freed_wq, gobj1->freed_done, msecs_to_jiffies(TIMEOUT_MS)); KUNIT_EXPECT_GT(test, ret, 0); } I guess? Maxime
Hi, Maxime On 9/5/23 15:16, Maxime Ripard wrote: > On Tue, Sep 05, 2023 at 02:32:38PM +0200, Thomas Hellström wrote: >> Hi, >> >> On 9/5/23 14:05, Maxime Ripard wrote: >>> Hi, >>> >>> On Tue, Sep 05, 2023 at 10:58:31AM +0200, Thomas Hellström wrote: >>>> Check that object freeing from within drm_exec_fini() works as expected >>>> and doesn't generate any warnings. >>>> >>>> Cc: Christian König <christian.koenig@amd.com> >>>> Cc: dri-devel@lists.freedesktop.org >>>> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> >>>> --- >>>> drivers/gpu/drm/tests/drm_exec_test.c | 47 +++++++++++++++++++++++++++ >>>> 1 file changed, 47 insertions(+) >>>> >>>> diff --git a/drivers/gpu/drm/tests/drm_exec_test.c b/drivers/gpu/drm/tests/drm_exec_test.c >>>> index 563949d777dd..294c25f49cc7 100644 >>>> --- a/drivers/gpu/drm/tests/drm_exec_test.c >>>> +++ b/drivers/gpu/drm/tests/drm_exec_test.c >>>> @@ -170,6 +170,52 @@ static void test_prepare_array(struct kunit *test) >>>> drm_gem_private_object_fini(&gobj2); >>>> } >>>> +static const struct drm_gem_object_funcs put_funcs = { >>>> + .free = (void *)kfree, >>>> +}; >>>> + >>>> +/* >>>> + * Check that freeing objects from within drm_exec_fini() >>>> + * behaves as expected. >>>> + */ >>>> +static void test_early_put(struct kunit *test) >>>> +{ >>>> + struct drm_exec_priv *priv = test->priv; >>>> + struct drm_gem_object *gobj1; >>>> + struct drm_gem_object *gobj2; >>>> + struct drm_gem_object *array[2]; >>>> + struct drm_exec exec; >>>> + int ret; >>>> + >>>> + gobj1 = kzalloc(sizeof(*gobj1), GFP_KERNEL); >>>> + KUNIT_EXPECT_NOT_ERR_OR_NULL(test, gobj1); >>>> + if (!gobj1) >>>> + return; >>>> + >>>> + gobj2 = kzalloc(sizeof(*gobj2), GFP_KERNEL); >>>> + KUNIT_EXPECT_NOT_ERR_OR_NULL(test, gobj2); >>>> + if (!gobj2) { >>>> + kfree(gobj1); >>>> + return; >>>> + } >>>> + >>>> + gobj1->funcs = &put_funcs; >>>> + gobj2->funcs = &put_funcs; >>>> + array[0] = gobj1; >>>> + array[1] = gobj2; >>>> + drm_gem_private_object_init(priv->drm, gobj1, PAGE_SIZE); >>>> + drm_gem_private_object_init(priv->drm, gobj2, PAGE_SIZE); >>>> + >>>> + drm_exec_init(&exec, DRM_EXEC_INTERRUPTIBLE_WAIT); >>>> + drm_exec_until_all_locked(&exec) >>>> + ret = drm_exec_prepare_array(&exec, array, ARRAY_SIZE(array), >>>> + 1); >>>> + KUNIT_EXPECT_EQ(test, ret, 0); >>>> + drm_gem_object_put(gobj1); >>>> + drm_gem_object_put(gobj2); >>>> + drm_exec_fini(&exec); >>> It doesn't look like you actually check that "freeing objects from >>> within drm_exec_fini() behaves as expected." What is the expectation >>> here, and how is it checked? >> Hm. Good question, I've been manually checking dmesg for lockdep splats. Is >> there a way to automate that? > I'm not familiar with the drm_exec API, but judging by the code I'd > assume you want to check that gobj1 and gobj2 are actually freed using > kfree? Actually not. What's important here is that the call to drm_exec_fini(), which puts the last references to gobj1 and gobj2 doesn't trigger any lockdep splats, like the one in the commit message of patch 3/3. So to make more sense, the test could perhaps be conditioned on CONFIG_DEBUG_LOCK_ALLOC. Still it would require manual checking of dmesg() after being run. /Thomas
On Tue, Sep 05, 2023 at 03:42:58PM +0200, Thomas Hellström wrote: > Hi, Maxime > > On 9/5/23 15:16, Maxime Ripard wrote: > > On Tue, Sep 05, 2023 at 02:32:38PM +0200, Thomas Hellström wrote: > > > Hi, > > > > > > On 9/5/23 14:05, Maxime Ripard wrote: > > > > Hi, > > > > > > > > On Tue, Sep 05, 2023 at 10:58:31AM +0200, Thomas Hellström wrote: > > > > > Check that object freeing from within drm_exec_fini() works as expected > > > > > and doesn't generate any warnings. > > > > > > > > > > Cc: Christian König <christian.koenig@amd.com> > > > > > Cc: dri-devel@lists.freedesktop.org > > > > > Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> > > > > > --- > > > > > drivers/gpu/drm/tests/drm_exec_test.c | 47 +++++++++++++++++++++++++++ > > > > > 1 file changed, 47 insertions(+) > > > > > > > > > > diff --git a/drivers/gpu/drm/tests/drm_exec_test.c b/drivers/gpu/drm/tests/drm_exec_test.c > > > > > index 563949d777dd..294c25f49cc7 100644 > > > > > --- a/drivers/gpu/drm/tests/drm_exec_test.c > > > > > +++ b/drivers/gpu/drm/tests/drm_exec_test.c > > > > > @@ -170,6 +170,52 @@ static void test_prepare_array(struct kunit *test) > > > > > drm_gem_private_object_fini(&gobj2); > > > > > } > > > > > +static const struct drm_gem_object_funcs put_funcs = { > > > > > + .free = (void *)kfree, > > > > > +}; > > > > > + > > > > > +/* > > > > > + * Check that freeing objects from within drm_exec_fini() > > > > > + * behaves as expected. > > > > > + */ > > > > > +static void test_early_put(struct kunit *test) > > > > > +{ > > > > > + struct drm_exec_priv *priv = test->priv; > > > > > + struct drm_gem_object *gobj1; > > > > > + struct drm_gem_object *gobj2; > > > > > + struct drm_gem_object *array[2]; > > > > > + struct drm_exec exec; > > > > > + int ret; > > > > > + > > > > > + gobj1 = kzalloc(sizeof(*gobj1), GFP_KERNEL); > > > > > + KUNIT_EXPECT_NOT_ERR_OR_NULL(test, gobj1); > > > > > + if (!gobj1) > > > > > + return; > > > > > + > > > > > + gobj2 = kzalloc(sizeof(*gobj2), GFP_KERNEL); > > > > > + KUNIT_EXPECT_NOT_ERR_OR_NULL(test, gobj2); > > > > > + if (!gobj2) { > > > > > + kfree(gobj1); > > > > > + return; > > > > > + } > > > > > + > > > > > + gobj1->funcs = &put_funcs; > > > > > + gobj2->funcs = &put_funcs; > > > > > + array[0] = gobj1; > > > > > + array[1] = gobj2; > > > > > + drm_gem_private_object_init(priv->drm, gobj1, PAGE_SIZE); > > > > > + drm_gem_private_object_init(priv->drm, gobj2, PAGE_SIZE); > > > > > + > > > > > + drm_exec_init(&exec, DRM_EXEC_INTERRUPTIBLE_WAIT); > > > > > + drm_exec_until_all_locked(&exec) > > > > > + ret = drm_exec_prepare_array(&exec, array, ARRAY_SIZE(array), > > > > > + 1); > > > > > + KUNIT_EXPECT_EQ(test, ret, 0); > > > > > + drm_gem_object_put(gobj1); > > > > > + drm_gem_object_put(gobj2); > > > > > + drm_exec_fini(&exec); > > > > It doesn't look like you actually check that "freeing objects from > > > > within drm_exec_fini() behaves as expected." What is the expectation > > > > here, and how is it checked? > > > Hm. Good question, I've been manually checking dmesg for lockdep splats. Is > > > there a way to automate that? > > I'm not familiar with the drm_exec API, but judging by the code I'd > > assume you want to check that gobj1 and gobj2 are actually freed using > > kfree? > > Actually not. What's important here is that the call to drm_exec_fini(), > which puts the last references to gobj1 and gobj2 doesn't trigger any > lockdep splats, like the one in the commit message of patch 3/3. So to make > more sense, the test could perhaps be conditioned on > CONFIG_DEBUG_LOCK_ALLOC. Still it would require manual checking of dmesg() > after being run. I'm not aware of something to check on lockdep's status when running a kunit test, but I'm not sure anyone is expected to look at the dmesg trace when running kunit to find out whether the test succeeded or not. It looks like there was an attempt at some point to fail the test if there was a lockdep error: https://lore.kernel.org/all/20200814205527.1833459-1-urielguajardojr@gmail.com/ It doesn't look like it's been merged though. David, Brendan, do you know why it wasn't merged or if there is a good option for us there? At the very least, I think a comment after the call to drm_exec_fini to make it clear that the error would be in the kernel logs, and a better one on the test definition to explicitly say what you want to make sure of, and how one can check it's been done would be great. Maxime
diff --git a/drivers/gpu/drm/tests/drm_exec_test.c b/drivers/gpu/drm/tests/drm_exec_test.c index 563949d777dd..294c25f49cc7 100644 --- a/drivers/gpu/drm/tests/drm_exec_test.c +++ b/drivers/gpu/drm/tests/drm_exec_test.c @@ -170,6 +170,52 @@ static void test_prepare_array(struct kunit *test) drm_gem_private_object_fini(&gobj2); } +static const struct drm_gem_object_funcs put_funcs = { + .free = (void *)kfree, +}; + +/* + * Check that freeing objects from within drm_exec_fini() + * behaves as expected. + */ +static void test_early_put(struct kunit *test) +{ + struct drm_exec_priv *priv = test->priv; + struct drm_gem_object *gobj1; + struct drm_gem_object *gobj2; + struct drm_gem_object *array[2]; + struct drm_exec exec; + int ret; + + gobj1 = kzalloc(sizeof(*gobj1), GFP_KERNEL); + KUNIT_EXPECT_NOT_ERR_OR_NULL(test, gobj1); + if (!gobj1) + return; + + gobj2 = kzalloc(sizeof(*gobj2), GFP_KERNEL); + KUNIT_EXPECT_NOT_ERR_OR_NULL(test, gobj2); + if (!gobj2) { + kfree(gobj1); + return; + } + + gobj1->funcs = &put_funcs; + gobj2->funcs = &put_funcs; + array[0] = gobj1; + array[1] = gobj2; + drm_gem_private_object_init(priv->drm, gobj1, PAGE_SIZE); + drm_gem_private_object_init(priv->drm, gobj2, PAGE_SIZE); + + drm_exec_init(&exec, DRM_EXEC_INTERRUPTIBLE_WAIT); + drm_exec_until_all_locked(&exec) + ret = drm_exec_prepare_array(&exec, array, ARRAY_SIZE(array), + 1); + KUNIT_EXPECT_EQ(test, ret, 0); + drm_gem_object_put(gobj1); + drm_gem_object_put(gobj2); + drm_exec_fini(&exec); +} + static void test_multiple_loops(struct kunit *test) { struct drm_exec exec; @@ -198,6 +244,7 @@ static struct kunit_case drm_exec_tests[] = { KUNIT_CASE(test_prepare), KUNIT_CASE(test_prepare_array), KUNIT_CASE(test_multiple_loops), + KUNIT_CASE(test_early_put), {} };
Check that object freeing from within drm_exec_fini() works as expected and doesn't generate any warnings. Cc: Christian König <christian.koenig@amd.com> Cc: dri-devel@lists.freedesktop.org Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> --- drivers/gpu/drm/tests/drm_exec_test.c | 47 +++++++++++++++++++++++++++ 1 file changed, 47 insertions(+)