drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c | 38 ++++++++++++++++---------- 1 file changed, 24 insertions(+), 14 deletions(-)
Commit 30188df0c387 ("drm/tests: Drop drm_kunit_helper_acquire_ctx_alloc()")
removed a kunit-managed function to get a drm_modeset_acquire_ctx.
It converted the vc4_pv_muxing_test_init() function, used by
drm_vc4_test_pv_muxing() and drm_vc4_test_pv_muxing_invalid(). However,
during that conversion, it went from being kzalloc'd to being allocated
on the stack.
vc4_pv_muxing_test_init() then uses that context to allocate a
drm_atomic_state using drm_kunit_helper_atomic_state_alloc(), which
stores a pointer to the locking context in the allocated state.
However, since vc4_pv_muxing_test_init() is a test init function, the
context is then cleared when we leave the function, and before executing
the test. We're then running the test with a dangling pointer, which
then leads to various crashes.
Rework the context initialization and state allocation to move them to
drm_vc4_test_pv_muxing() and drm_vc4_test_pv_muxing_invalid().
Fixes: 30188df0c387 ("drm/tests: Drop drm_kunit_helper_acquire_ctx_alloc()")
Reported-by: Catalin Marinas <catalin.marinas@arm.com>
Closes: https://lore.kernel.org/r/Z_95jWM2YMTGy3pi@arm.com/
Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c | 38 ++++++++++++++++----------
1 file changed, 24 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c b/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c
index 992e8f5c5c6ea8d92338a8fe739fa1115ff85338..a79b152f8b97add449cffc9674abec0df95239e3 100644
--- a/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c
+++ b/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c
@@ -18,11 +18,10 @@
#include "vc4_mock.h"
struct pv_muxing_priv {
struct vc4_dev *vc4;
- struct drm_atomic_state *state;
};
static bool check_fifo_conflict(struct kunit *test,
const struct drm_atomic_state *state)
{
@@ -675,14 +674,22 @@ KUNIT_ARRAY_PARAM(vc5_test_pv_muxing_invalid,
static void drm_vc4_test_pv_muxing(struct kunit *test)
{
const struct pv_muxing_param *params = test->param_value;
const struct pv_muxing_priv *priv = test->priv;
- struct drm_atomic_state *state = priv->state;
+ struct drm_modeset_acquire_ctx ctx;
+ struct drm_atomic_state *state;
+ struct drm_device *drm;
unsigned int i;
int ret;
+ drm_modeset_acquire_init(&ctx, 0);
+
+ drm = &priv->vc4->base;
+ state = drm_kunit_helper_atomic_state_alloc(test, drm, &ctx);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state);
+
for (i = 0; i < params->nencoders; i++) {
enum vc4_encoder_type enc_type = params->encoders[i];
ret = vc4_mock_atomic_add_output(test, state, enc_type);
KUNIT_ASSERT_EQ(test, ret, 0);
@@ -698,56 +705,59 @@ static void drm_vc4_test_pv_muxing(struct kunit *test)
enum vc4_encoder_type enc_type = params->encoders[i];
KUNIT_EXPECT_TRUE(test, check_channel_for_encoder(test, state, enc_type,
params->check_fn));
}
+
+ drm_modeset_drop_locks(&ctx);
+ drm_modeset_acquire_fini(&ctx);
}
static void drm_vc4_test_pv_muxing_invalid(struct kunit *test)
{
const struct pv_muxing_param *params = test->param_value;
const struct pv_muxing_priv *priv = test->priv;
- struct drm_atomic_state *state = priv->state;
+ struct drm_modeset_acquire_ctx ctx;
+ struct drm_atomic_state *state;
+ struct drm_device *drm;
unsigned int i;
int ret;
+ drm_modeset_acquire_init(&ctx, 0);
+
+ drm = &priv->vc4->base;
+ state = drm_kunit_helper_atomic_state_alloc(test, drm, &ctx);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state);
+
for (i = 0; i < params->nencoders; i++) {
enum vc4_encoder_type enc_type = params->encoders[i];
ret = vc4_mock_atomic_add_output(test, state, enc_type);
KUNIT_ASSERT_EQ(test, ret, 0);
}
ret = drm_atomic_check_only(state);
KUNIT_EXPECT_LT(test, ret, 0);
+
+ drm_modeset_drop_locks(&ctx);
+ drm_modeset_acquire_fini(&ctx);
}
static int vc4_pv_muxing_test_init(struct kunit *test)
{
const struct pv_muxing_param *params = test->param_value;
- struct drm_modeset_acquire_ctx ctx;
struct pv_muxing_priv *priv;
- struct drm_device *drm;
struct vc4_dev *vc4;
priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
KUNIT_ASSERT_NOT_NULL(test, priv);
test->priv = priv;
vc4 = params->mock_fn(test);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, vc4);
priv->vc4 = vc4;
- drm_modeset_acquire_init(&ctx, 0);
-
- drm = &vc4->base;
- priv->state = drm_kunit_helper_atomic_state_alloc(test, drm, &ctx);
- KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv->state);
-
- drm_modeset_drop_locks(&ctx);
- drm_modeset_acquire_fini(&ctx);
-
return 0;
}
static struct kunit_case vc4_pv_muxing_tests[] = {
KUNIT_CASE_PARAM(drm_vc4_test_pv_muxing,
---
base-commit: a5806cd506af5a7c19bcd596e4708b5c464bfd21
change-id: 20250520-drm-vc4-kunit-fixes-a681715c4409
Best regards,
--
Maxime Ripard <mripard@kernel.org>On 20/05/25 09:08, Maxime Ripard wrote:
> Commit 30188df0c387 ("drm/tests: Drop drm_kunit_helper_acquire_ctx_alloc()")
> removed a kunit-managed function to get a drm_modeset_acquire_ctx.
>
> It converted the vc4_pv_muxing_test_init() function, used by
> drm_vc4_test_pv_muxing() and drm_vc4_test_pv_muxing_invalid(). However,
> during that conversion, it went from being kzalloc'd to being allocated
> on the stack.
>
> vc4_pv_muxing_test_init() then uses that context to allocate a
> drm_atomic_state using drm_kunit_helper_atomic_state_alloc(), which
> stores a pointer to the locking context in the allocated state.
>
> However, since vc4_pv_muxing_test_init() is a test init function, the
> context is then cleared when we leave the function, and before executing
> the test. We're then running the test with a dangling pointer, which
> then leads to various crashes.
>
> Rework the context initialization and state allocation to move them to
> drm_vc4_test_pv_muxing() and drm_vc4_test_pv_muxing_invalid().
>
> Fixes: 30188df0c387 ("drm/tests: Drop drm_kunit_helper_acquire_ctx_alloc()")
> Reported-by: Catalin Marinas <catalin.marinas@arm.com>
> Closes: https://lore.kernel.org/r/Z_95jWM2YMTGy3pi@arm.com/
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> ---
> drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c | 38 ++++++++++++++++----------
> 1 file changed, 24 insertions(+), 14 deletions(-)
Hi Maxime,
Didn't you fix this issue in commit 7e0351ae91ed ("drm/vc4: tests: Stop
allocating the state in test init") [1]?
[1]
https://gitlab.freedesktop.org/drm/misc/kernel/-/commit/7e0351ae91ed2b6178abbfae96c3c6aaa1652567
Best Regards,
- Maíra
>
> diff --git a/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c b/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c
> index 992e8f5c5c6ea8d92338a8fe739fa1115ff85338..a79b152f8b97add449cffc9674abec0df95239e3 100644
> --- a/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c
> +++ b/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c
> @@ -18,11 +18,10 @@
>
> #include "vc4_mock.h"
>
> struct pv_muxing_priv {
> struct vc4_dev *vc4;
> - struct drm_atomic_state *state;
> };
>
> static bool check_fifo_conflict(struct kunit *test,
> const struct drm_atomic_state *state)
> {
> @@ -675,14 +674,22 @@ KUNIT_ARRAY_PARAM(vc5_test_pv_muxing_invalid,
>
> static void drm_vc4_test_pv_muxing(struct kunit *test)
> {
> const struct pv_muxing_param *params = test->param_value;
> const struct pv_muxing_priv *priv = test->priv;
> - struct drm_atomic_state *state = priv->state;
> + struct drm_modeset_acquire_ctx ctx;
> + struct drm_atomic_state *state;
> + struct drm_device *drm;
> unsigned int i;
> int ret;
>
> + drm_modeset_acquire_init(&ctx, 0);
> +
> + drm = &priv->vc4->base;
> + state = drm_kunit_helper_atomic_state_alloc(test, drm, &ctx);
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state);
> +
> for (i = 0; i < params->nencoders; i++) {
> enum vc4_encoder_type enc_type = params->encoders[i];
>
> ret = vc4_mock_atomic_add_output(test, state, enc_type);
> KUNIT_ASSERT_EQ(test, ret, 0);
> @@ -698,56 +705,59 @@ static void drm_vc4_test_pv_muxing(struct kunit *test)
> enum vc4_encoder_type enc_type = params->encoders[i];
>
> KUNIT_EXPECT_TRUE(test, check_channel_for_encoder(test, state, enc_type,
> params->check_fn));
> }
> +
> + drm_modeset_drop_locks(&ctx);
> + drm_modeset_acquire_fini(&ctx);
> }
>
> static void drm_vc4_test_pv_muxing_invalid(struct kunit *test)
> {
> const struct pv_muxing_param *params = test->param_value;
> const struct pv_muxing_priv *priv = test->priv;
> - struct drm_atomic_state *state = priv->state;
> + struct drm_modeset_acquire_ctx ctx;
> + struct drm_atomic_state *state;
> + struct drm_device *drm;
> unsigned int i;
> int ret;
>
> + drm_modeset_acquire_init(&ctx, 0);
> +
> + drm = &priv->vc4->base;
> + state = drm_kunit_helper_atomic_state_alloc(test, drm, &ctx);
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state);
> +
> for (i = 0; i < params->nencoders; i++) {
> enum vc4_encoder_type enc_type = params->encoders[i];
>
> ret = vc4_mock_atomic_add_output(test, state, enc_type);
> KUNIT_ASSERT_EQ(test, ret, 0);
> }
>
> ret = drm_atomic_check_only(state);
> KUNIT_EXPECT_LT(test, ret, 0);
> +
> + drm_modeset_drop_locks(&ctx);
> + drm_modeset_acquire_fini(&ctx);
> }
>
> static int vc4_pv_muxing_test_init(struct kunit *test)
> {
> const struct pv_muxing_param *params = test->param_value;
> - struct drm_modeset_acquire_ctx ctx;
> struct pv_muxing_priv *priv;
> - struct drm_device *drm;
> struct vc4_dev *vc4;
>
> priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
> KUNIT_ASSERT_NOT_NULL(test, priv);
> test->priv = priv;
>
> vc4 = params->mock_fn(test);
> KUNIT_ASSERT_NOT_ERR_OR_NULL(test, vc4);
> priv->vc4 = vc4;
>
> - drm_modeset_acquire_init(&ctx, 0);
> -
> - drm = &vc4->base;
> - priv->state = drm_kunit_helper_atomic_state_alloc(test, drm, &ctx);
> - KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv->state);
> -
> - drm_modeset_drop_locks(&ctx);
> - drm_modeset_acquire_fini(&ctx);
> -
> return 0;
> }
>
> static struct kunit_case vc4_pv_muxing_tests[] = {
> KUNIT_CASE_PARAM(drm_vc4_test_pv_muxing,
>
> ---
> base-commit: a5806cd506af5a7c19bcd596e4708b5c464bfd21
> change-id: 20250520-drm-vc4-kunit-fixes-a681715c4409
>
> Best regards,
On Sat, May 24, 2025 at 08:09:36AM -0300, Maíra Canal wrote:
> On 20/05/25 09:08, Maxime Ripard wrote:
> > Commit 30188df0c387 ("drm/tests: Drop drm_kunit_helper_acquire_ctx_alloc()")
> > removed a kunit-managed function to get a drm_modeset_acquire_ctx.
> >
> > It converted the vc4_pv_muxing_test_init() function, used by
> > drm_vc4_test_pv_muxing() and drm_vc4_test_pv_muxing_invalid(). However,
> > during that conversion, it went from being kzalloc'd to being allocated
> > on the stack.
> >
> > vc4_pv_muxing_test_init() then uses that context to allocate a
> > drm_atomic_state using drm_kunit_helper_atomic_state_alloc(), which
> > stores a pointer to the locking context in the allocated state.
> >
> > However, since vc4_pv_muxing_test_init() is a test init function, the
> > context is then cleared when we leave the function, and before executing
> > the test. We're then running the test with a dangling pointer, which
> > then leads to various crashes.
> >
> > Rework the context initialization and state allocation to move them to
> > drm_vc4_test_pv_muxing() and drm_vc4_test_pv_muxing_invalid().
> >
> > Fixes: 30188df0c387 ("drm/tests: Drop drm_kunit_helper_acquire_ctx_alloc()")
> > Reported-by: Catalin Marinas <catalin.marinas@arm.com>
> > Closes: https://lore.kernel.org/r/Z_95jWM2YMTGy3pi@arm.com/
> > Signed-off-by: Maxime Ripard <mripard@kernel.org>
> > ---
> > drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c | 38 ++++++++++++++++----------
> > 1 file changed, 24 insertions(+), 14 deletions(-)
>
> Hi Maxime,
>
> Didn't you fix this issue in commit 7e0351ae91ed ("drm/vc4: tests: Stop
> allocating the state in test init") [1]?
>
> [1] https://gitlab.freedesktop.org/drm/misc/kernel/-/commit/7e0351ae91ed2b6178abbfae96c3c6aaa1652567
I did, and forgot I guess.. :/
It should be on its way to 6.16-rc1 already, so hopefully it'll get
picked up by the stable kernel team at some point.
Maxime
On Tue, May 20, 2025 at 02:08:36PM +0200, Maxime Ripard wrote:
> Commit 30188df0c387 ("drm/tests: Drop drm_kunit_helper_acquire_ctx_alloc()")
> removed a kunit-managed function to get a drm_modeset_acquire_ctx.
>
> It converted the vc4_pv_muxing_test_init() function, used by
> drm_vc4_test_pv_muxing() and drm_vc4_test_pv_muxing_invalid(). However,
> during that conversion, it went from being kzalloc'd to being allocated
> on the stack.
>
> vc4_pv_muxing_test_init() then uses that context to allocate a
> drm_atomic_state using drm_kunit_helper_atomic_state_alloc(), which
> stores a pointer to the locking context in the allocated state.
>
> However, since vc4_pv_muxing_test_init() is a test init function, the
> context is then cleared when we leave the function, and before executing
> the test. We're then running the test with a dangling pointer, which
> then leads to various crashes.
>
> Rework the context initialization and state allocation to move them to
> drm_vc4_test_pv_muxing() and drm_vc4_test_pv_muxing_invalid().
>
> Fixes: 30188df0c387 ("drm/tests: Drop drm_kunit_helper_acquire_ctx_alloc()")
> Reported-by: Catalin Marinas <catalin.marinas@arm.com>
> Closes: https://lore.kernel.org/r/Z_95jWM2YMTGy3pi@arm.com/
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
FWIW, I no longer get the kernel panic:
Tested-by: Catalin Marinas <catalin.marinas@arm.com>
© 2016 - 2025 Red Hat, Inc.