On 16/07/2026 14:14, Tvrtko Ursulin wrote:
>
>
> On 15/07/2026 17:02, Marco Pagani wrote:
>
> 8<>
>
>>> I first want to understand exactly what the issue is. The current design
>>> exactly cannot free the memory under the scheduler nose unless there is
>>> a test bug where test exited and has left the scheduler with unprocessed
>>> jobs. I am not saying we cannot have reference counted jobs but, again,
>>> at the moment I don't understand the problem.
>>
>> I think that the current design of the mock scheduler can indeed free
>> test objects' memory under the scheduler backend's nose. Consider the
>> example discussed above: the mock scheduler is instantiated and released
>> in the .init and .exit functions respectively, while jobs are allocated
>> in the test body using drm_mock_sched_job_new(). In this rather basic
>> use case, jobs' memory is freed before the entities and the scheduler
>> are released in the .exit function.
>
> So at first I thought that as long as entity is freed at each test exit,
> which includes idling, and is done by all current test cases (apart from
> the parallel one where you did it via actions) it is all fine. The
> entity->last_scheduled you mention is not a concern then since it is
> cleared etc.
>
> However, now I think that even if entity is idled and freed before test
> exit that does nothing for the jobs not yet processed by the
> drm_sched_free_job_work()-er.
>
> Even though I was testing with KASAN I possibly did not manage to hit
> that race. Strange but I guess possible.
It was also a surprise for me when Philipp reported a suspected UAF.
Definitely a subtle bug to catch. I managed to reproduce it by building
the kernel with KASAN and kunit.autorun enabled, and then simply booting
it with no rootfs using qemu with this slightly odd configuration:
qemu-system-x86_64 -enable-kvm -m 48G -smp cpus=18 -kernel [...]
> Unless I am missing something with the above analysis, one options is
> along the lines of what you propose - forgo using kunit managed
> allocations for anything mock scheduler owned. Second would be to stop
> using test suite init/exit for the scheduler management and just make
> each test create own scheduler and free it as it exits.
I think the second approach is still fragile. If we move
drm_mock_sched_fini() at the end of each test body, any KUNIT_ASSERT_*()
failure will cause the test to abort immediately, skipping the teardown
and leaving the scheduler in an inconsistent state that could cause any
sort of problem.
Moreover, initializing/tearing down the scheduler in the test's
init/exit functions is the most intuitive and possibly the best approach
(DRY) from a test code perspective. Asking test writers to not use the
standard init/exit functions and enforcing this limitation is a burden
that I think no one deserves to take.
> Regardless of the option, as long as it is safe against UAFs we are I
> think good. Memory leaks on test failures are not that interesting that
> it would warrant coming up with anything more complicated.
> Does that make sense? If so, question is which option is simpler?
> Probably just making tests create and destroy schedulers since then
> there is no need to deal with any memory freeing. It would be just two
> lines added to a subset of tests, the ones which rely on .init/.exit.
I thought so too when developing the drm_gem_shmem_test some time ago,
but then I changed my mind after dealing with downstream CI. A single
test leaking some memory is not optimal, but not even a big deal per se.
However, consider hundreds of KUnit tests running periodically on a CI.
If all failing tests did not care about leaking memory, they could
seriously bog down the system. I think it's responsible to have tests
that do not leak memory when they fail.
I'll see if I can come up with a cleaner version of the kref approach,
taking into account your recommendations and Sashiko's bug report.
Thank you for your suggestions,
Marco
> Regards,
>
> Tvrtko
>
