On 28/09/2023 19:20, Mathias Krause wrote:
Commit 1ec23ed7126e ("drm/i915: Use uabi engines for the default engine
map") switched from using for_each_engine() to for_each_uabi_engine() to
iterate over the user engines. While this seems to be a sensible change,
it's only safe to do when the engines are actually chained using the
rb-tree structure which is not the case during early driver
initialization where it can be either a lock-less list or regular
double-linked list.
In fact, the modesetting initialization code may end up calling
default_engines() through the fb helper code while the engines list
is still llist_node-based:
i915_driver_probe() ->
intel_display_driver_probe() ->
intel_fbdev_init() ->
drm_fb_helper_init() ->
drm_client_init() ->
drm_client_open() ->
drm_file_alloc() ->
i915_driver_open() ->
i915_gem_open() ->
i915_gem_context_open() ->
i915_gem_create_context() ->
default_engines()
Using for_each_uabi_engine() in default_engines() is therefore wrong, as
it would try to interpret the llist as rb-tree, making it find no engine
at all, as the rb_left and rb_right members will still be NULL, as they
haven't been initialized yet.
To fix this type confusion register the engines earlier and at the same
time reduce the amount of code that has to deal with the intermediate
llist state.
Reported-by: sanitiy checks in grsecurity
Typo in sanity - can fix up while merging.
Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
Fixes: 1ec23ed7126e ("drm/i915: Use uabi engines for the default engine map")
Signed-off-by: Mathias Krause <minipli@xxxxxxxxxxxxxx>
Cc: Jonathan Cavitt <jonathan.cavitt@xxxxxxxxx>
Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
---
Tested on my ADL systems just fine, no rb-tree related type confusion
any more, as expected.
drivers/gpu/drm/i915/i915_gem.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 1f65bb33dd21..a8551ce322de 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1199,6 +1199,13 @@ int i915_gem_init(struct drm_i915_private *dev_priv)
goto err_unlock;
}
+ /*
+ * Register engines early to ensure the engine list is in its final
+ * rb-tree form, lowering the amount of code that has to deal with
+ * the intermediate llist state.
+ */
+ intel_engines_driver_register(dev_priv);
Comment feels a bit too low level (for this high level component
interface) and would probably be enough to say this completes the GEM
initialization by populating the uabi engines, which are needed for GEM
contexts.
Anyway, it is not that important. Many thanks for working on this and I
will merge it once it passes our CI.
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
Regards,
Tvrtko
+
return 0;
/*
@@ -1246,8 +1253,6 @@ int i915_gem_init(struct drm_i915_private *dev_priv)
void i915_gem_driver_register(struct drm_i915_private *i915)
{
i915_gem_driver_register__shrinker(i915);
-
- intel_engines_driver_register(i915);
}
void i915_gem_driver_unregister(struct drm_i915_private *i915)
