From 96c7c2f4d5bd94b15fe63448c087f01607b56f4a Mon Sep 17 00:00:00 2001 From: Danilo Krummrich Date: Tue, 18 Apr 2023 12:04:53 +0200 Subject: drm/scheduler: set entity to NULL in drm_sched_entity_pop_job() It already happend a few times that patches slipped through which implemented access to an entity through a job that was already removed from the entities queue. Since jobs and entities might have different lifecycles, this can potentially cause UAF bugs. In order to make it obvious that a jobs entity pointer shouldn't be accessed after drm_sched_entity_pop_job() was called successfully, set the jobs entity pointer to NULL once the job is removed from the entity queue. Moreover, debugging a potential NULL pointer dereference is way easier than potentially corrupted memory through a UAF. Signed-off-by: Danilo Krummrich Link: https://lore.kernel.org/r/20230418100453.4433-1-dakr@redhat.com Reviewed-by: Luben Tuikov Signed-off-by: Luben Tuikov --- drivers/gpu/drm/scheduler/sched_entity.c | 6 ++++++ 1 file changed, 6 insertions(+) (limited to 'drivers/gpu/drm/scheduler/sched_entity.c') diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c index e0a8890a62e2..3e2a31d8190e 100644 --- a/drivers/gpu/drm/scheduler/sched_entity.c +++ b/drivers/gpu/drm/scheduler/sched_entity.c @@ -448,6 +448,12 @@ struct drm_sched_job *drm_sched_entity_pop_job(struct drm_sched_entity *entity) drm_sched_rq_update_fifo(entity, next->submit_ts); } + /* Jobs and entities might have different lifecycles. Since we're + * removing the job from the entities queue, set the jobs entity pointer + * to NULL to prevent any future access of the entity through this job. + */ + sched_job->entity = NULL; + return sched_job; } -- cgit v1.2.3-70-g09d2 From 539f9ee4b52a8bec95ff064e22dd2fb1e258e818 Mon Sep 17 00:00:00 2001 From: Christian König Date: Mon, 17 Apr 2023 13:36:02 +0200 Subject: drm/scheduler: properly forward fence errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a hw fence is signaled with an error properly forward that to the finished fence. Signed-off-by: Christian König Reviewed-by: Luben Tuikov Link: https://patchwork.freedesktop.org/patch/msgid/20230420115752.31470-1-christian.koenig@amd.com --- drivers/gpu/drm/scheduler/sched_entity.c | 4 +--- drivers/gpu/drm/scheduler/sched_fence.c | 4 +++- drivers/gpu/drm/scheduler/sched_main.c | 18 ++++++++---------- include/drm/gpu_scheduler.h | 2 +- 4 files changed, 13 insertions(+), 15 deletions(-) (limited to 'drivers/gpu/drm/scheduler/sched_entity.c') diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c index 3e2a31d8190e..1795cd7e42ed 100644 --- a/drivers/gpu/drm/scheduler/sched_entity.c +++ b/drivers/gpu/drm/scheduler/sched_entity.c @@ -144,7 +144,7 @@ static void drm_sched_entity_kill_jobs_work(struct work_struct *wrk) { struct drm_sched_job *job = container_of(wrk, typeof(*job), work); - drm_sched_fence_finished(job->s_fence); + drm_sched_fence_finished(job->s_fence, -ESRCH); WARN_ON(job->s_fence->parent); job->sched->ops->free_job(job); } @@ -195,8 +195,6 @@ static void drm_sched_entity_kill(struct drm_sched_entity *entity) while ((job = to_drm_sched_job(spsc_queue_pop(&entity->job_queue)))) { struct drm_sched_fence *s_fence = job->s_fence; - dma_fence_set_error(&s_fence->finished, -ESRCH); - dma_fence_get(&s_fence->finished); if (!prev || dma_fence_add_callback(prev, &job->finish_cb, drm_sched_entity_kill_jobs_cb)) diff --git a/drivers/gpu/drm/scheduler/sched_fence.c b/drivers/gpu/drm/scheduler/sched_fence.c index fe9c6468e440..ef120475e7c6 100644 --- a/drivers/gpu/drm/scheduler/sched_fence.c +++ b/drivers/gpu/drm/scheduler/sched_fence.c @@ -53,8 +53,10 @@ void drm_sched_fence_scheduled(struct drm_sched_fence *fence) dma_fence_signal(&fence->scheduled); } -void drm_sched_fence_finished(struct drm_sched_fence *fence) +void drm_sched_fence_finished(struct drm_sched_fence *fence, int result) { + if (result) + dma_fence_set_error(&fence->finished, result); dma_fence_signal(&fence->finished); } diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c index c3582c4fc299..8d248ce233c8 100644 --- a/drivers/gpu/drm/scheduler/sched_main.c +++ b/drivers/gpu/drm/scheduler/sched_main.c @@ -262,7 +262,7 @@ drm_sched_rq_select_entity_fifo(struct drm_sched_rq *rq) * * Finish the job's fence and wake up the worker thread. */ -static void drm_sched_job_done(struct drm_sched_job *s_job) +static void drm_sched_job_done(struct drm_sched_job *s_job, int result) { struct drm_sched_fence *s_fence = s_job->s_fence; struct drm_gpu_scheduler *sched = s_fence->sched; @@ -273,7 +273,7 @@ static void drm_sched_job_done(struct drm_sched_job *s_job) trace_drm_sched_process_job(s_fence); dma_fence_get(&s_fence->finished); - drm_sched_fence_finished(s_fence); + drm_sched_fence_finished(s_fence, result); dma_fence_put(&s_fence->finished); wake_up_interruptible(&sched->wake_up_worker); } @@ -287,7 +287,7 @@ static void drm_sched_job_done_cb(struct dma_fence *f, struct dma_fence_cb *cb) { struct drm_sched_job *s_job = container_of(cb, struct drm_sched_job, cb); - drm_sched_job_done(s_job); + drm_sched_job_done(s_job, f->error); } /** @@ -537,12 +537,12 @@ void drm_sched_start(struct drm_gpu_scheduler *sched, bool full_recovery) r = dma_fence_add_callback(fence, &s_job->cb, drm_sched_job_done_cb); if (r == -ENOENT) - drm_sched_job_done(s_job); + drm_sched_job_done(s_job, fence->error); else if (r) DRM_DEV_ERROR(sched->dev, "fence add callback failed (%d)\n", r); } else - drm_sched_job_done(s_job); + drm_sched_job_done(s_job, 0); } if (full_recovery) { @@ -1059,15 +1059,13 @@ static int drm_sched_main(void *param) r = dma_fence_add_callback(fence, &sched_job->cb, drm_sched_job_done_cb); if (r == -ENOENT) - drm_sched_job_done(sched_job); + drm_sched_job_done(sched_job, fence->error); else if (r) DRM_DEV_ERROR(sched->dev, "fence add callback failed (%d)\n", r); } else { - if (IS_ERR(fence)) - dma_fence_set_error(&s_fence->finished, PTR_ERR(fence)); - - drm_sched_job_done(sched_job); + drm_sched_job_done(sched_job, IS_ERR(fence) ? + PTR_ERR(fence) : 0); } wake_up(&sched->job_scheduled); diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h index c0ec6719282a..3b4800e0b24b 100644 --- a/include/drm/gpu_scheduler.h +++ b/include/drm/gpu_scheduler.h @@ -598,7 +598,7 @@ void drm_sched_fence_init(struct drm_sched_fence *fence, void drm_sched_fence_free(struct drm_sched_fence *fence); void drm_sched_fence_scheduled(struct drm_sched_fence *fence); -void drm_sched_fence_finished(struct drm_sched_fence *fence); +void drm_sched_fence_finished(struct drm_sched_fence *fence, int result); unsigned long drm_sched_suspend_timeout(struct drm_gpu_scheduler *sched); void drm_sched_resume_timeout(struct drm_gpu_scheduler *sched, -- cgit v1.2.3-70-g09d2 From 70102d77ff22dd88a0111b1c3bac5099ac5d0425 Mon Sep 17 00:00:00 2001 From: Christian König Date: Mon, 17 Apr 2023 17:32:11 +0200 Subject: drm/scheduler: add drm_sched_entity_error and use rcu for last_scheduled MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Switch to using RCU handling for the last scheduled job and add a function to return the error code of it. Signed-off-by: Christian König Reviewed-by: Luben Tuikov Link: https://patchwork.freedesktop.org/patch/msgid/20230420115752.31470-2-christian.koenig@amd.com --- drivers/gpu/drm/scheduler/sched_entity.c | 39 +++++++++++++++++++++++++------- include/drm/gpu_scheduler.h | 3 ++- 2 files changed, 33 insertions(+), 9 deletions(-) (limited to 'drivers/gpu/drm/scheduler/sched_entity.c') diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c index 1795cd7e42ed..cfb433e92005 100644 --- a/drivers/gpu/drm/scheduler/sched_entity.c +++ b/drivers/gpu/drm/scheduler/sched_entity.c @@ -72,7 +72,7 @@ int drm_sched_entity_init(struct drm_sched_entity *entity, entity->num_sched_list = num_sched_list; entity->priority = priority; entity->sched_list = num_sched_list > 1 ? sched_list : NULL; - entity->last_scheduled = NULL; + RCU_INIT_POINTER(entity->last_scheduled, NULL); RB_CLEAR_NODE(&entity->rb_tree_node); if(num_sched_list) @@ -140,6 +140,27 @@ bool drm_sched_entity_is_ready(struct drm_sched_entity *entity) return true; } +/** + * drm_sched_entity_error - return error of last scheduled job + * @entity: scheduler entity to check + * + * Opportunistically return the error of the last scheduled job. Result can + * change any time when new jobs are pushed to the hw. + */ +int drm_sched_entity_error(struct drm_sched_entity *entity) +{ + struct dma_fence *fence; + int r; + + rcu_read_lock(); + fence = rcu_dereference(entity->last_scheduled); + r = fence ? fence->error : 0; + rcu_read_unlock(); + + return r; +} +EXPORT_SYMBOL(drm_sched_entity_error); + static void drm_sched_entity_kill_jobs_work(struct work_struct *wrk) { struct drm_sched_job *job = container_of(wrk, typeof(*job), work); @@ -191,7 +212,9 @@ static void drm_sched_entity_kill(struct drm_sched_entity *entity) /* Make sure this entity is not used by the scheduler at the moment */ wait_for_completion(&entity->entity_idle); - prev = dma_fence_get(entity->last_scheduled); + /* The entity is guaranteed to not be used by the scheduler */ + prev = rcu_dereference_check(entity->last_scheduled, true); + dma_fence_get(prev); while ((job = to_drm_sched_job(spsc_queue_pop(&entity->job_queue)))) { struct drm_sched_fence *s_fence = job->s_fence; @@ -278,8 +301,8 @@ void drm_sched_entity_fini(struct drm_sched_entity *entity) entity->dependency = NULL; } - dma_fence_put(entity->last_scheduled); - entity->last_scheduled = NULL; + dma_fence_put(rcu_dereference_check(entity->last_scheduled, true)); + RCU_INIT_POINTER(entity->last_scheduled, NULL); } EXPORT_SYMBOL(drm_sched_entity_fini); @@ -421,9 +444,9 @@ struct drm_sched_job *drm_sched_entity_pop_job(struct drm_sched_entity *entity) if (entity->guilty && atomic_read(entity->guilty)) dma_fence_set_error(&sched_job->s_fence->finished, -ECANCELED); - dma_fence_put(entity->last_scheduled); - - entity->last_scheduled = dma_fence_get(&sched_job->s_fence->finished); + dma_fence_put(rcu_dereference_check(entity->last_scheduled, true)); + rcu_assign_pointer(entity->last_scheduled, + dma_fence_get(&sched_job->s_fence->finished)); /* * If the queue is empty we allow drm_sched_entity_select_rq() to @@ -477,7 +500,7 @@ void drm_sched_entity_select_rq(struct drm_sched_entity *entity) */ smp_rmb(); - fence = entity->last_scheduled; + fence = rcu_dereference_check(entity->last_scheduled, true); /* stay on the same engine if the previous job hasn't finished */ if (fence && !dma_fence_is_signaled(fence)) diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h index 3b4800e0b24b..fd15f8ae0c3f 100644 --- a/include/drm/gpu_scheduler.h +++ b/include/drm/gpu_scheduler.h @@ -201,7 +201,7 @@ struct drm_sched_entity { * by the scheduler thread, can be accessed locklessly from * drm_sched_job_arm() iff the queue is empty. */ - struct dma_fence *last_scheduled; + struct dma_fence __rcu *last_scheduled; /** * @last_user: last group leader pushing a job into the entity. @@ -588,6 +588,7 @@ void drm_sched_entity_push_job(struct drm_sched_job *sched_job); void drm_sched_entity_set_priority(struct drm_sched_entity *entity, enum drm_sched_priority priority); bool drm_sched_entity_is_ready(struct drm_sched_entity *entity); +int drm_sched_entity_error(struct drm_sched_entity *entity); void drm_sched_fence_set_parent(struct drm_sched_fence *s_fence, struct dma_fence *fence); -- cgit v1.2.3-70-g09d2 From 3655c5900f4d49881ad09e3893e5f5516b06a9f1 Mon Sep 17 00:00:00 2001 From: Luben Tuikov Date: Wed, 17 May 2023 19:35:50 -0400 Subject: drm/sched: Rename to drm_sched_wakeup_if_can_queue() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rename drm_sched_wakeup() to drm_sched_wakeup_if_canqueue() since the former is misleading, as it wakes up the GPU scheduler _only if_ more jobs can be queued to the underlying hardware. This distinction is important to make, since the wake conditional in the GPU scheduler thread wakes up when other conditions are also true, e.g. when there are jobs to be cleaned. For instance, a user might want to wake up the scheduler only because there are more jobs to clean, but whether we can queue more jobs is irrelevant. v2: Separate "canqueue" to "can_queue". (Alex D.) Cc: Christian König Cc: Alex Deucher Signed-off-by: Luben Tuikov Link: https://lore.kernel.org/r/20230517233550.377847-2-luben.tuikov@amd.com Reviewed-by: Alex Deucher --- drivers/gpu/drm/scheduler/sched_entity.c | 4 ++-- drivers/gpu/drm/scheduler/sched_main.c | 6 +++--- include/drm/gpu_scheduler.h | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) (limited to 'drivers/gpu/drm/scheduler/sched_entity.c') diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c index cfb433e92005..68e807ae136a 100644 --- a/drivers/gpu/drm/scheduler/sched_entity.c +++ b/drivers/gpu/drm/scheduler/sched_entity.c @@ -342,7 +342,7 @@ static void drm_sched_entity_wakeup(struct dma_fence *f, container_of(cb, struct drm_sched_entity, cb); drm_sched_entity_clear_dep(f, cb); - drm_sched_wakeup(entity->rq->sched); + drm_sched_wakeup_if_can_queue(entity->rq->sched); } /** @@ -565,7 +565,7 @@ void drm_sched_entity_push_job(struct drm_sched_job *sched_job) if (drm_sched_policy == DRM_SCHED_POLICY_FIFO) drm_sched_rq_update_fifo(entity, submit_ts); - drm_sched_wakeup(entity->rq->sched); + drm_sched_wakeup_if_can_queue(entity->rq->sched); } } EXPORT_SYMBOL(drm_sched_entity_push_job); diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c index 8739322c3032..b352227a6055 100644 --- a/drivers/gpu/drm/scheduler/sched_main.c +++ b/drivers/gpu/drm/scheduler/sched_main.c @@ -860,12 +860,12 @@ static bool drm_sched_can_queue(struct drm_gpu_scheduler *sched) } /** - * drm_sched_wakeup - Wake up the scheduler when it is ready - * + * drm_sched_wakeup_if_can_queue - Wake up the scheduler * @sched: scheduler instance * + * Wake up the scheduler if we can queue jobs. */ -void drm_sched_wakeup(struct drm_gpu_scheduler *sched) +void drm_sched_wakeup_if_can_queue(struct drm_gpu_scheduler *sched) { if (drm_sched_can_queue(sched)) wake_up_interruptible(&sched->wake_up_worker); diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h index 31d1f5166c79..e95b4837e5a3 100644 --- a/include/drm/gpu_scheduler.h +++ b/include/drm/gpu_scheduler.h @@ -549,7 +549,7 @@ void drm_sched_entity_modify_sched(struct drm_sched_entity *entity, unsigned int num_sched_list); void drm_sched_job_cleanup(struct drm_sched_job *job); -void drm_sched_wakeup(struct drm_gpu_scheduler *sched); +void drm_sched_wakeup_if_can_queue(struct drm_gpu_scheduler *sched); void drm_sched_stop(struct drm_gpu_scheduler *sched, struct drm_sched_job *bad); void drm_sched_start(struct drm_gpu_scheduler *sched, bool full_recovery); void drm_sched_resubmit_jobs(struct drm_gpu_scheduler *sched); -- cgit v1.2.3-70-g09d2 From 4f9b94d848696166011bead3109541ec2a523bb8 Mon Sep 17 00:00:00 2001 From: ZhenGuo Yin Date: Tue, 9 May 2023 17:46:17 +0800 Subject: drm/scheduler: avoid infinite loop if entity's dependency is a scheduled error fence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit [Why] drm_sched_entity_add_dependency_cb ignores the scheduled fence and return false. If entity's dependency is a scheduler error fence and drm_sched_stop is called due to TDR, drm_sched_entity_pop_job will wait for the dependency infinitely. [How] Do not wait or ignore the scheduled error fence, add drm_sched_entity_wakeup callback for the dependency with scheduled error fence. Signed-off-by: ZhenGuo Yin Acked-by: Alex Deucher Reviewed-by: Christian König Signed-off-by: Alex Deucher --- drivers/gpu/drm/scheduler/sched_entity.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'drivers/gpu/drm/scheduler/sched_entity.c') diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c index 68e807ae136a..b2bbc8a68b30 100644 --- a/drivers/gpu/drm/scheduler/sched_entity.c +++ b/drivers/gpu/drm/scheduler/sched_entity.c @@ -384,7 +384,7 @@ static bool drm_sched_entity_add_dependency_cb(struct drm_sched_entity *entity) } s_fence = to_drm_sched_fence(fence); - if (s_fence && s_fence->sched == sched && + if (!fence->error && s_fence && s_fence->sched == sched && !test_bit(DRM_SCHED_FENCE_DONT_PIPELINE, &fence->flags)) { /* -- cgit v1.2.3-70-g09d2