Skip to contentSkip to conversation

drm/sched: Fix race in drm_sched_entity_select_rq()

PATCH Applied Philipp Stanner 3 participants 3 messages Oct 22, 2025 – Oct 27, 2025

Subsystems (5)
Stable branch DRM drivers Media (V4L/DVB) DRM misc +1 more subsystems
lore
Share & tools
lore

Philipp Stanner

author maintainer
#0 lore raw

PATCH 1/1 drm/sched: Fix race in drm_sched_entity_select_rq()

In a past bug fix it was forgotten that entity access must be protected by the entity lock. That's a data race and potentially UB.

Move the spin_unlock() to the appropriate position.

Cc [email protected] # v5.13+
Fixes ac4eb83ab255 ("drm/sched: select new rq even if there is only one v3")
Signed-off-by Philipp Stanner <[email protected]>
---
 drivers/gpu/drm/scheduler/sched_entity.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/gpu/drm/scheduler/sched_entity.c +2 -1
index 5a4697f636f2..aa222166de58 100644
@@ -552,10 +552,11 @@ void drm_sched_entity_select_rq(struct drm_sched_entity *entity)
 		drm_sched_rq_remove_entity(entity->rq, entity);
 		entity->rq = rq;
 	}
-	spin_unlock(&entity->lock);
 
 	if (entity->num_sched_list == 1)
 		entity->sched_list = NULL;
+
+	spin_unlock(&entity->lock);
 }
 
 /**
Signature · 1 line
2.49.0
Reviewed-by on v1 1/1
#1 lore raw
Quoted 19 lines · Philipp Stanner

In a past bug fix it was forgotten that entity access must be protected by the entity lock. That's a data race and potentially UB.

Move the spin_unlock() to the appropriate position.

Cc [email protected] # v5.13+
Fixes ac4eb83ab255 ("drm/sched: select new rq even if there is only one v3")
Signed-off-by Philipp Stanner <[email protected]>
---
  drivers/gpu/drm/scheduler/sched_entity.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)
drivers/gpu/drm/scheduler/sched_entity.c +0 -0
index 5a4697f636f2..aa222166de58 100644
@@ -552,10 +552,11 @@ void drm_sched_entity_select_rq(struct drm_sched_entity *entity)
  		drm_sched_rq_remove_entity(entity->rq, entity);
  		entity->rq = rq;

drivers/gpu/drm/scheduler/sched_entity.c void drm_sched_entity_select_rq(struct drm_sched_entity *entity) · line 562

Quoted 10 lines
drivers/gpu/drm/scheduler/sched_entity.c +2 -1
  	}
-	spin_unlock(&entity->lock);
 
  	if (entity->num_sched_list == 1)
  		entity->sched_list = NULL;
+
+	spin_unlock(&entity->lock);
  }
 
  /**

Agreed, unlock at the end is correct.

Reviewed-by Tvrtko Ursulin <[email protected]>

Luckily only amdgpu calls drm_sched_entity_modify_sched(), and I am not sure if with hypothetical multi-threaded submit it could be possible to hit it or not.

One thread would have to clear the sched_list, while another would have to have passed the !sched_list at the beginning of the function, and the job_queue count and last_scheduled completed guards. Which would mean job has to get completed between the unlock and clearing the pointer.

Also, take a look at this when you can please:

https://lore.kernel.org/dri-devel/[email protected]/

This would also have fixed this issue by removing clearing of entity->sched_list. Latter being just a hack to work around the issue of confused container ownership rules.

Regards,

Tvrtko

Applied
#2 lore raw
Quoted 34 lines · Tvrtko Ursulin
Quoted 26 lines · Philipp Stanner

In a past bug fix it was forgotten that entity access must be protected by the entity lock. That's a data race and potentially UB.

Move the spin_unlock() to the appropriate position.

Cc [email protected] # v5.13+
Fixes ac4eb83ab255 ("drm/sched: select new rq even if there is only one v3")
Signed-off-by Philipp Stanner <[email protected]>

---
  drivers/gpu/drm/scheduler/sched_entity.c | 3 ++-

  1 file changed, 2 insertions(+), 1 deletion(-)
drivers/gpu/drm/scheduler/sched_entity.c +0 -1
index 5a4697f636f2..aa222166de58 100644
@@ -552,10 +552,11 @@ void drm_sched_entity_select_rq(struct drm_sched_entity *entity)
   		drm_sched_rq_remove_entity(entity->rq, entity);
   		entity->rq = rq;
   	}
-	spin_unlock(&entity->lock);


   if (entity->num_sched_list == 1)
   entity->sched_list = NULL;

drivers/gpu/drm/scheduler/sched_entity.c +2 -0
+
+	spin_unlock(&entity->lock);
Quoted 3 lines

  }

  /**

Agreed, unlock at the end is correct.

Quoted 1 line
Reviewed-by Tvrtko Ursulin <[email protected]>

Applied to drm-misc-fixes.

Thx
P.

Quoted 22 lines

Luckily only amdgpu calls drm_sched_entity_modify_sched(), and I am not sure if with hypothetical multi-threaded submit it could be possible to hit it or not.

One thread would have to clear the sched_list, while another would have to have passed the !sched_list at the beginning of the function, and the job_queue count and last_scheduled completed guards. Which would mean job has to get completed between the unlock and clearing the pointer.

Also, take a look at this when you can please:

https://lore.kernel.org/dri-devel/[email protected]/

This would also have fixed this issue by removing clearing of entity->sched_list. Latter being just a hack to work around the issue of confused container ownership rules.

Regards,

Tvrtko