linux-kernel - Re: [PATCH 3/4] drm/vc4: tests: Stop allocating the state in test init

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <5e353e6d-6e81-411b-8eef-dd4a241ca1ac@igalia.com>
Date: Sun, 30 Mar 2025 15:10:03 -0300
From: Maíra Canal <mcanal@...lia.com>
To: Maxime Ripard <mripard@...nel.org>
Cc: Dave Stevenson <dave.stevenson@...pberrypi.com>,
 Raspberry Pi Kernel Maintenance <kernel-list@...pberrypi.com>,
 Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
 Thomas Zimmermann <tzimmermann@...e.de>, David Airlie <airlied@...il.com>,
 Simona Vetter <simona@...ll.ch>, dri-devel@...ts.freedesktop.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/4] drm/vc4: tests: Stop allocating the state in test
 init

Hi Maxime,

On 28/03/25 12:32, Maxime Ripard wrote:
> On Sun, Mar 23, 2025 at 03:48:17PM -0300, Maíra Canal wrote:
>> Hi Maxime,
>>
>> On 18/03/25 11:17, Maxime Ripard wrote:
>>> The vc4-pv-muxing-combinations and vc5-pv-muxing-combinations test
>>> suites use a common test init function which, in part, allocates the
>>> drm atomic state the test will use.
>>>
>>> That allocation relies on  drm_kunit_helper_atomic_state_alloc(), and
>>> thus requires a struct drm_modeset_acquire_ctx. This context will then
>>> be stored in the allocated state->acquire_ctx field.
>>>
>>> However, the context is local to the test init function, and is cleared
>>> as soon as drm_kunit_helper_atomic_state_alloc() is done. We thus end up
>>> with an dangling pointer to a cleared context in state->acquire_ctx for
>>> our test to consumes.
>>>
>>> We should really allocate the context and the state in the test
>>> functions, so we can also control when we're done with it.
>>>
>>> Fixes: 30188df0c387 ("drm/tests: Drop drm_kunit_helper_acquire_ctx_alloc()")
>>> Signed-off-by: Maxime Ripard <mripard@...nel.org>
>>> ---
>>>    drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c | 41 +++++++++++++++++---------
>>>    1 file changed, 27 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..52c04ef33206bf4f9e21e3c8b7cea932824a67fa 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;
>>
>> Can't we add `struct drm_modeset_acquire_ctx` here? Then, we can be sure
>> that the context exists during the entire test case.
>>
>> Also, we can add `drm_modeset_drop_locks()` and
>> `drm_modeset_acquire_fini()` to a exit function in the kunit suite.
> 
> That's what we were doing before, but the kunit functions and exit
> functions are run in a separate thread by design, which then cause
> lockdep to complain.
> 
> It's not great, but we can't really change either :/
> 

Thanks for the explanation, Maxime. In that case,

Reviewed-by: Maíra Canal <mcanal@...lia.com>

Best Regards,
- Maíra

> Maxime