From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f44.google.com (mail-wm1-f44.google.com [209.85.128.44]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 9C8724071C6 for ; Fri, 10 Jul 2026 13:20:33 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.128.44 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1783689635; cv=none; b=Bapzp0uP3YjcjTc2WRtzy9FE680cdjiF6Dmc3wieu2Vjh/itl+mDb+Id9o8BdvfXnqPqxB9dA/SkpI7OEiyalyhHMQZYQsDQqv5vy8hwv1v/ZD8YuGjJXuKI99Fi2jSRnoaf026DzNh+5e9psb5pFCjySWTCyjMJ2ERtfAur1cM= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1783689635; c=relaxed/simple; bh=UejQ2tV1vpSbuRqeCtDuhTUkrBOfLmX/6tJllynpEYQ=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=Z+D9KVoUIglxN5a8d5zJkQgsa+EDb+23In0bQAlcAP384ppzcYkkGTq7PkZ7fzSw5DqBg+6HQBkJvmNHOdQICunLlOTcNnKvysOUzWhI4WnwJwx9QEtJ0eGVDbo3DZLG47NZSIZmYgwWh2IaMQ0rEg9qVEv4IsU3oMwngwU3xh0= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=ursulin.net; spf=pass smtp.mailfrom=ursulin.net; dkim=pass (2048-bit key) header.d=ursulin.net header.i=@ursulin.net header.b=gRJgaYBY; arc=none smtp.client-ip=209.85.128.44 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=ursulin.net Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=ursulin.net Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=ursulin.net header.i=@ursulin.net header.b="gRJgaYBY" Received: by mail-wm1-f44.google.com with SMTP id 5b1f17b1804b1-493bc8fda98so5881775e9.0 for ; Fri, 10 Jul 2026 06:20:33 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ursulin.net; s=google; t=1783689632; x=1784294432; darn=vger.kernel.org; h=content-transfer-encoding:content-type:in-reply-to:from :content-language:references:cc:to:subject:user-agent:mime-version :date:message-id:from:to:cc:subject:date:message-id:reply-to :content-type; bh=OrnQdgHYX1jQdSLNgqv4eif0HeOC+ShJkpWPBMwNWUo=; b=gRJgaYBY37uSGHwMs9Vw3g/iaPbDp/gatPG1kwcfVVYQEvJn6F9nVoiGyQwoYZzpWk jFR22HOK9hB8BOKeJYN82MJHJaQouCgZJhcBS6pflInvRtU4vw0jfdeBVc+qhfmaNCD/ SijumdbNoLqPD33KTDhD9Ec7D5lo3j4xTNTnImtyDlgPVGGGK3okTT3XzVh2FVpIx2p/ x6vBjnqNe8EzKwFHWKLAJVn5ZskDrBLYpx35P1Q5lgJCtGzy3nDy3pII+g75bA0WIdGk GAWNe/lNM0VhIGlR+GcUu3aJYfabcCQ8GTQjv7I8kRxt3ThQHa4L7wdSRQfeg0eZT2Ga 8dYA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1783689632; x=1784294432; h=content-transfer-encoding:content-type:in-reply-to:from :content-language:references:cc:to:subject:user-agent:mime-version :date:message-id:x-gm-gg:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to:content-type; bh=OrnQdgHYX1jQdSLNgqv4eif0HeOC+ShJkpWPBMwNWUo=; b=GppkPTX4JQP7Nbaxfg9K7g3/SGlcEBC53B0zxPJomFVjEvYH2LNhdkL58PkxUUfVcF 3H5XAmV4CYb7A+bXK78lSbYjbDccyJfGdS5QBD/vr/0zyo92ZuYDkIucvFX1gQBz8gq9 GRC1Ba/vcJnEuFn/g0mfOKHJilwte5Ctx8wzvq5q2od/Z231L1Dx4VN/3YBT44RyRS0/ CSsaEic6fevfES80gDCRDWYN0UmE3javiAEmCp1Z5y0l7C01tOq8C3sD5uYIi6C0yEYb iN8qAtg99uSXFbbabRsLu9mkB0L78ak0iJSq1Ht7z30uxyauhnFk/eKDtT0g+wdfYv9S LHYQ== X-Forwarded-Encrypted: i=1; AHgh+Rr52ADCf0K9pPvHHWCpdjC0b62OpePva9pYWAOTTj5BOol9eQWs/XRZrryt67gd3l6QeyEbuhDP+pJ1TcA=@vger.kernel.org X-Gm-Message-State: AOJu0YyLjfpv7uzJDLnBISw9C/gw2aS4mnp+JS1/6e8ef42gLqo9zaic Tpxm3I8SKRlVU9IoNEsJ0+lrxazM9tJWNVXKv9nRfP/jEcY6ptZOr6n/dTcmbBsvbKM= X-Gm-Gg: AfdE7cn/fT0SNsYg5MtK0VgctyZUDw/v0iIwLUOBW76UrLvrhBRpe1Ahd42k6oUOkPh lWSCHuaM7e3INbjIB91old7SgwpyEe3zwaEY9x/hka58utd2AdaMM7rGg2+qSGK3uIbECMqgGn4 1X2DtNZxY0PNl/bS8tndpi+YpSyVSfllZ6inQXGs7+wBYJ99vq313ugMeUHVjbdRUV24yH1M207 bK7bXaFqFzMm2Ms03ZE3mDqIgELI7sPXYLIcKt33RXsABE9ccW3RxmFseC+B+us96dxX334b4gE it274bobR8IXspdVbo8P4eTSx0dKAOehBahYL4Qx9q91ZXUy+6ypGZUh58TSc8/nI4fK88+3eFl IxA1oTZrNJqRm3+s9B3hhGHz9Drfs3YfBnWGeGzdV4XXoIs3wXW10RfmC8XlKF3KiAh1/DDeFEp XZ+7gomDmYGMMUffG9z7iJN+1KI106OaHrVg== X-Received: by 2002:a05:600c:628b:b0:493:e947:b24a with SMTP id 5b1f17b1804b1-493e947b27fmr92954715e9.14.1783689631346; Fri, 10 Jul 2026 06:20:31 -0700 (PDT) Received: from [192.168.0.116] ([90.240.106.137]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-493eb6df417sm145120185e9.8.2026.07.10.06.20.30 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 10 Jul 2026 06:20:30 -0700 (PDT) Message-ID: Date: Fri, 10 Jul 2026 14:20:29 +0100 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [RFC PATCH] drm/sched/tests: Let the DRM scheduler manage job lifetimes To: Marco Pagani Cc: Matthew Brost , Danilo Krummrich , Philipp Stanner , =?UTF-8?Q?Christian_K=C3=B6nig?= , Maarten Lankhorst , Maxime Ripard , Thomas Zimmermann , David Airlie , Simona Vetter , dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org References: <20260707114807.154572-1-marco.pagani@linux.dev> <46ffa75c-83e2-4f4b-9d5b-e43fdfa12176@ursulin.net> Content-Language: en-GB From: Tvrtko Ursulin In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit On 09/07/2026 22:53, Marco Pagani wrote: > On 08/07/2026 11:20, Tvrtko Ursulin wrote: >> >> On 07/07/2026 12:48, Marco Pagani wrote: >>> Currently, the mock scheduler uses KUnit-managed memory for jobs. This ties >>> the job's memory lifetime to the test suite rather than the DRM scheduler's >>> callbacks. This does not represent real driver behavior and can lead to >>> potential Use-After-Free bugs in the tests. >> >> Since you say use-after-free, that means something belonging to a test >> wasn't properly cleaned up, right? Could you help me out review by >> describing one example? > > Sure, I experienced a UAF myself while writing the sched_parallel test > suite (v3->v4). The UAF happened in v3 because of a reference that the > scheduler backend keeps to the fence of the last scheduled job (in > drm_sched_run_job_work()->drm_sched_entity_pop_job()). If the job is > released before its entity, the pointer becomes dangling. > > On a NR_CPUS > 20 system, this caused a UAF during the teardown phase > because jobs were freed before their entities by the KUnit actions > implicitly registered by drm_mock_sched_job_new() when allocating the > memory. To solve this in v4, I had to move KUnit release action for the > entities to ensure that entities are freed before jobs. Why wouldn't it work to idle the entities and the scheduler before the test exits? There is even kunit_suite.exit which some other tests use. > I also have another UAF reported by our internal CI that I haven't yet > fully investigated in drm_sched_basic: > > KUNIT: drm_sched: BUG: KASAN: null-ptr-deref in drm_sched_basic_test+0x3b4/0x7c0 [drm_sched_tests] This one is interesting. Is it maybe the dep_chain case? Perhaps prev fence reference needs to be taken before submit and carrier over to next iteration. >> Idea was that tests are self contained, ie. when a tests exits it is >> guaranteed all resources belonging to it are gone. Everything else >> should be a test bug which IMO means it is fine to crash if a new tests >> gets that wrong. Just idle at the end of the test and everything should >> work. > > I fully agree that test should be self-contained. However, my point is > that KUnit infrastructure should serve as a sandbox to avoid leaking > resources and not force or perturb the lifetime of test items (jobs in > this case) within the boundaries of the test. > > I think the problem with the current design of the mock scheduler is > that drm_mock_sched_job_new() implicitly registers a cleanup action to > free the job's memory independently of the job's state. Even more subtly, > this approach implicitly defines a LIFO teardown order depending on when > drm_mock_sched_job_new() is called in the test. > > This may confuse test developers who might be unaware of this behavior, > as they are likely to assume that the lifetime of the job will be always > guaranteed during the entire test, as I've experienced myself. Are you saying the unexpected thing is that job can be safely dereferenced for the duration of the test? Or that it cannot? Latter would be a bug somewhere AFAICT. >>> Update the mock scheduler to let the lifetime of jobs be managed by the DRM >>> scheduler's asynchronous callbacks instead of KUnit managed memory. Add a >>> kref reference counter to track the job's lifetime between test suites and >>> the scheduler. >>> >>> Finally, to avoid memory leaks in the event of an early test abortion, >>> register a cleanup KUnit action that automatically puts the reference to >>> the job. >> >> So kunit managed memory, freed on test exit, is replaced by kunit >> managed actions, which also free on test exit? > > The idea is to use KUnit actions to guarantee tests against resource > leaks by dropping a reference, and let the same callback used by the > scheduler to free the memory, instead of abruptly free the memory from > right under the scheduler's nose. 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. Regards, Tvrtko >> Some random thoughts below while I wait to understand the motivation and >> problem statement better: >> >>> Signed-off-by: Marco Pagani >>> --- >>> .../gpu/drm/scheduler/tests/mock_scheduler.c | 68 ++++++++++++++----- >>> drivers/gpu/drm/scheduler/tests/sched_tests.h | 6 ++ >>> 2 files changed, 57 insertions(+), 17 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/scheduler/tests/mock_scheduler.c b/drivers/gpu/drm/scheduler/tests/mock_scheduler.c >>> index 14403a762335..51f81082f37a 100644 >>> --- a/drivers/gpu/drm/scheduler/tests/mock_scheduler.c >>> +++ b/drivers/gpu/drm/scheduler/tests/mock_scheduler.c >>> @@ -96,6 +96,14 @@ drm_mock_sched_job_signal_timer(struct hrtimer *hrtimer) >>> return HRTIMER_NORESTART; >>> } >>> >>> +static void drm_mock_sched_job_cleanup_action(void *ptr) >>> +{ >>> + struct drm_mock_sched_job *job = ptr; >>> + >>> + job->test = NULL; >>> + drm_mock_sched_job_put(job); >>> +} >>> + >>> /** >>> * drm_mock_sched_job_new - Create a new mock scheduler job >>> * >>> @@ -111,23 +119,34 @@ drm_mock_sched_job_new(struct kunit *test, >>> struct drm_mock_sched_job *job; >>> int ret; >>> >>> - job = kunit_kzalloc(test, sizeof(*job), GFP_KERNEL); >>> + /* Let the DRM Scheduler manage the lifetime of the job */ >> >> However that does not match real driver behaviour either (circling back >> to the justification from the commit message). It is the drivers which >> own this memory, not the DRM scheduler. So in this case mock scheduler, >> which is a kunit test client, etc. > > I'm not entirely following on this. Sure, it is the driver that > allocates and owns the job's memory, but it is the the scheduler backend > that manage the job's lifecycle and decides when a it is finished and > the underlying memory can be freed. To be more precise, I think it is > fair to say that when a job is submitted, the ownership is transferred > from the driver the to scheduler backend until the job is finished. > > >>> + job = kzalloc_obj(*job, GFP_KERNEL); >>> KUNIT_ASSERT_NOT_NULL(test, job); >>> >>> + kref_init(&job->refcount); >>> + job->test = test; >>> + >>> ret = drm_sched_job_init(&job->base, >>> &entity->base, >>> 1, >>> NULL, >>> 1); >>> - KUNIT_ASSERT_EQ(test, ret, 0); >>> - >>> - job->test = test; >>> + if (ret) { >>> + kfree(job); >>> + KUNIT_ASSERT_EQ_MSG(test, ret, 0, "drm_sched_job_init failed"); >>> + } >>> >>> init_completion(&job->done); >>> INIT_LIST_HEAD(&job->link); >>> hrtimer_setup(&job->timer, drm_mock_sched_job_signal_timer, >>> CLOCK_MONOTONIC, HRTIMER_MODE_ABS); >>> >>> + ret = kunit_add_action(test, drm_mock_sched_job_cleanup_action, job); >>> + if (ret) { >>> + drm_mock_sched_job_put(job); >>> + KUNIT_ASSERT_EQ_MSG(test, ret, 0, "kunit_add_action failed"); >>> + } >>> + >>> return job; >>> } >>> >>> @@ -152,7 +171,7 @@ static void drm_mock_sched_hw_fence_release(struct dma_fence *fence) >>> >>> hrtimer_cancel(&job->timer); >>> >>> - /* Containing job is freed by the kunit framework */ >>> + drm_mock_sched_job_put(job); >>> } >>> >>> static const struct dma_fence_ops drm_mock_sched_hw_fence_ops = { >>> @@ -173,6 +192,8 @@ static struct dma_fence *mock_sched_run_job(struct drm_sched_job *sched_job) >>> sched->hw_timeline.context, >>> atomic_inc_return(&sched->hw_timeline.next_seqno)); >>> >>> + kref_get(&job->refcount); >>> + >>> dma_fence_get(&job->hw_fence); /* Reference for the job_list */ >>> >>> spin_lock_irq(&sched->lock); >>> @@ -200,6 +221,17 @@ static struct dma_fence *mock_sched_run_job(struct drm_sched_job *sched_job) >>> return &job->hw_fence; >>> } >>> >>> +static void mock_sched_free_job(struct drm_sched_job *sched_job) >>> +{ >> >> Free which is actually a reference put is non-obvious API. > > Uhm, this is not an API but the implementation of the .free_job method > of drm_mock_scheduler_ops. I've kept the function name since it matches > the name of backend op. > > >>> + struct drm_mock_sched_job *job = drm_sched_job_to_mock_job(sched_job); >>> + >>> + /* Only if the fence has been successfully initialized */ >>> + if (job->hw_fence.ops) >>> + dma_fence_put(&job->hw_fence); >>> + >>> + drm_mock_sched_job_put(job); >>> +} >>> + >>> /* >>> * Normally, drivers would take appropriate measures in this callback, such as >>> * killing the entity the faulty job is associated with, resetting the hardware >>> @@ -232,21 +264,26 @@ mock_sched_timedout_job(struct drm_sched_job *sched_job) >>> } >>> spin_unlock_irqrestore(&sched->lock, flags); >>> >>> - dma_fence_put(&job->hw_fence); >>> - drm_sched_job_cleanup(sched_job); >>> - /* Mock job itself is freed by the kunit framework. */ >>> + mock_sched_free_job(sched_job); >>> >>> return DRM_GPU_SCHED_STAT_RESET; >>> } >>> >>> -static void mock_sched_free_job(struct drm_sched_job *sched_job) >>> +static void drm_mock_sched_job_release(struct kref *ref) >>> { >>> - struct drm_mock_sched_job *job = drm_sched_job_to_mock_job(sched_job); >>> + struct drm_mock_sched_job *job; >>> + >>> + job = container_of(ref, struct drm_mock_sched_job, refcount); >>> >>> - dma_fence_put(&job->hw_fence); >>> - drm_sched_job_cleanup(sched_job); >>> + drm_sched_job_cleanup(&job->base); >>> >>> - /* Mock job itself is freed by the kunit framework. */ >>> + kfree(job); >>> +} >>> + >>> +void drm_mock_sched_job_put(struct drm_mock_sched_job *job) >>> +{ >>> + if (job) >>> + kref_put(&job->refcount, drm_mock_sched_job_release); >>> } >>> >>> static void mock_sched_cancel_job(struct drm_sched_job *sched_job) >>> @@ -265,10 +302,7 @@ static void mock_sched_cancel_job(struct drm_sched_job *sched_job) >>> } >>> spin_unlock_irqrestore(&sched->lock, flags); >>> >>> - /* >>> - * The GPU Scheduler will call drm_sched_backend_ops.free_job(), still. >>> - * Mock job itself is freed by the kunit framework. >>> - */ >>> + /* The GPU Scheduler will call drm_sched_backend_ops.free_job() */ >>> } >>> >>> static const struct drm_sched_backend_ops drm_mock_scheduler_ops = { >>> diff --git a/drivers/gpu/drm/scheduler/tests/sched_tests.h b/drivers/gpu/drm/scheduler/tests/sched_tests.h >>> index 553d45abd057..e4d33f0bf935 100644 >>> --- a/drivers/gpu/drm/scheduler/tests/sched_tests.h >>> +++ b/drivers/gpu/drm/scheduler/tests/sched_tests.h >>> @@ -13,6 +13,7 @@ >>> #include >>> #include >>> #include >>> +#include >>> >>> #include >>> >>> @@ -90,6 +91,7 @@ struct drm_mock_sched_entity { >>> */ >>> struct drm_mock_sched_job { >>> struct drm_sched_job base; >>> + struct kref refcount; >>> >>> struct completion done; >>> >>> @@ -144,6 +146,8 @@ struct drm_mock_sched_job * >>> drm_mock_sched_job_new(struct kunit *test, >>> struct drm_mock_sched_entity *entity); >>> >>> +void drm_mock_sched_job_put(struct drm_mock_sched_job *job); >>> + >>> /** >>> * drm_mock_sched_job_submit - Arm and submit a job in one go >>> * >>> @@ -151,6 +155,8 @@ drm_mock_sched_job_new(struct kunit *test, >>> */ >>> static inline void drm_mock_sched_job_submit(struct drm_mock_sched_job *job) >>> { >>> + kref_get(&job->refcount); >>> + >> >> This feels fragile because if someone writes a test which uses arm+push >> directly bad things will happen. What is this extra reference for versus >> the one obtained by creating a job? > > Fair point. I've consider taking the reference also before run in > mock_sched_run_job() or directly in drm_mock_sched_job_new(), but both > approaches felt suboptimal. I have to think about this. I'll come up > with a better approach for v2. > > >> Regards, >> >> Tvrtko >> >>> drm_sched_job_arm(&job->base); >>> drm_sched_entity_push_job(&job->base); >>> } >> > > Thanks, > Marco