| Message ID | 20230928182019.10256-3-minipli@grsecurity.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: fix rb-tree/llist/list confusion | expand |
On 28/09/2023 19:20, Mathias Krause wrote: > Chaining user engines happens in multiple passes during driver > initialization, mutating its type along the way. It starts off with a > simple lock-less linked list (struct llist_node/head) populated by > intel_engine_add_user() which later gets sorted and converted to an > intermediate regular list (struct list_head) just to be converted once > more to its final rb-tree structure (struct rb_node/root) in > intel_engines_driver_register(). > > All of these types overlay the uabi_node/uabi_engines members which is > unfortunate but safe if one takes care about using the rb-tree based > structure only after the conversion has completed. However, mistakes > happen and commit 1ec23ed7126e ("drm/i915: Use uabi engines for the > default engine map") violated that assumption, as the multiple type > evolution was all to easy hidden behind casts papering over it. > > Make the type evolution of uabi_node/uabi_engines more visible by > putting all members into an anonymous union and use the correctly typed > member in its various users. This allows us to drop quite some ugly > casts and, hopefully, make the evolution of the members better > recognisable to avoid future mistakes. > > Signed-off-by: Mathias Krause <minipli@grsecurity.net> > --- > drivers/gpu/drm/i915/gt/intel_engine_types.h | 10 +++++++++- > drivers/gpu/drm/i915/gt/intel_engine_user.c | 17 +++++++---------- > drivers/gpu/drm/i915/i915_drv.h | 17 ++++++++++++++++- > 3 files changed, 32 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h > index a7e677598004..7585fffac60b 100644 > --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h > +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h > @@ -402,7 +402,15 @@ struct intel_engine_cs { > > unsigned long context_tag; > > - struct rb_node uabi_node; > + /* > + * The type evolves during initialization, see related comment for > + * struct drm_i915_private's uabi_engines member. > + */ > + union { > + struct llist_node uabi_llist; > + struct list_head uabi_list; > + struct rb_node uabi_node; > + }; > > struct intel_sseu sseu; > > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_user.c b/drivers/gpu/drm/i915/gt/intel_engine_user.c > index dcedff41a825..118164ddbb2e 100644 > --- a/drivers/gpu/drm/i915/gt/intel_engine_user.c > +++ b/drivers/gpu/drm/i915/gt/intel_engine_user.c > @@ -38,8 +38,7 @@ intel_engine_lookup_user(struct drm_i915_private *i915, u8 class, u8 instance) > > void intel_engine_add_user(struct intel_engine_cs *engine) > { > - llist_add((struct llist_node *)&engine->uabi_node, > - (struct llist_head *)&engine->i915->uabi_engines); > + llist_add(&engine->uabi_llist, &engine->i915->uabi_engines_llist); > } > > static const u8 uabi_classes[] = { > @@ -54,9 +53,9 @@ static int engine_cmp(void *priv, const struct list_head *A, > const struct list_head *B) > { > const struct intel_engine_cs *a = > - container_of((struct rb_node *)A, typeof(*a), uabi_node); > + container_of(A, typeof(*a), uabi_list); > const struct intel_engine_cs *b = > - container_of((struct rb_node *)B, typeof(*b), uabi_node); > + container_of(B, typeof(*b), uabi_list); > > if (uabi_classes[a->class] < uabi_classes[b->class]) > return -1; > @@ -73,7 +72,7 @@ static int engine_cmp(void *priv, const struct list_head *A, > > static struct llist_node *get_engines(struct drm_i915_private *i915) > { > - return llist_del_all((struct llist_head *)&i915->uabi_engines); > + return llist_del_all(&i915->uabi_engines_llist); > } > > static void sort_engines(struct drm_i915_private *i915, > @@ -83,9 +82,8 @@ static void sort_engines(struct drm_i915_private *i915, > > llist_for_each_safe(pos, next, get_engines(i915)) { > struct intel_engine_cs *engine = > - container_of((struct rb_node *)pos, typeof(*engine), > - uabi_node); > - list_add((struct list_head *)&engine->uabi_node, engines); > + container_of(pos, typeof(*engine), uabi_llist); > + list_add(&engine->uabi_list, engines); > } > list_sort(NULL, engines, engine_cmp); > } > @@ -213,8 +211,7 @@ void intel_engines_driver_register(struct drm_i915_private *i915) > p = &i915->uabi_engines.rb_node; > list_for_each_safe(it, next, &engines) { > struct intel_engine_cs *engine = > - container_of((struct rb_node *)it, typeof(*engine), > - uabi_node); > + container_of(it, typeof(*engine), uabi_list); > > if (intel_gt_has_unrecoverable_error(engine->gt)) > continue; /* ignore incomplete engines */ > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 7a8ce7239bc9..c8690d1d5e51 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -222,7 +222,22 @@ struct drm_i915_private { > bool mchbar_need_disable; > } gmch; > > - struct rb_root uabi_engines; > + /* > + * Chaining user engines happens in multiple stages, starting with a > + * simple lock-less linked list created by intel_engine_add_user(), > + * which later gets sorted and converted to an intermediate regular > + * list, just to be converted once again to its final rb tree structure > + * in intel_engines_driver_register(). > + * > + * Make sure to use the right iterator helper, depending on if the code > + * in question runs before or after intel_engines_driver_register() -- > + * for_each_uabi_engine() can only be used afterwards! > + */ > + union { > + struct llist_head uabi_engines_llist; > + struct list_head uabi_engines_list; > + struct rb_root uabi_engines; > + }; > unsigned int engine_uabi_class_count[I915_LAST_UABI_ENGINE_CLASS + 1]; > > /* protects the irq masks */ Thanks again! Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Regards, Tvrtko
On 29/09/2023 12:00, Tvrtko Ursulin wrote: > > On 28/09/2023 19:20, Mathias Krause wrote: >> Chaining user engines happens in multiple passes during driver >> initialization, mutating its type along the way. It starts off with a >> simple lock-less linked list (struct llist_node/head) populated by >> intel_engine_add_user() which later gets sorted and converted to an >> intermediate regular list (struct list_head) just to be converted once >> more to its final rb-tree structure (struct rb_node/root) in >> intel_engines_driver_register(). >> >> All of these types overlay the uabi_node/uabi_engines members which is >> unfortunate but safe if one takes care about using the rb-tree based >> structure only after the conversion has completed. However, mistakes >> happen and commit 1ec23ed7126e ("drm/i915: Use uabi engines for the >> default engine map") violated that assumption, as the multiple type >> evolution was all to easy hidden behind casts papering over it. >> >> Make the type evolution of uabi_node/uabi_engines more visible by >> putting all members into an anonymous union and use the correctly typed >> member in its various users. This allows us to drop quite some ugly >> casts and, hopefully, make the evolution of the members better >> recognisable to avoid future mistakes. >> >> Signed-off-by: Mathias Krause <minipli@grsecurity.net> >> --- >> drivers/gpu/drm/i915/gt/intel_engine_types.h | 10 +++++++++- >> drivers/gpu/drm/i915/gt/intel_engine_user.c | 17 +++++++---------- >> drivers/gpu/drm/i915/i915_drv.h | 17 ++++++++++++++++- >> 3 files changed, 32 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h >> b/drivers/gpu/drm/i915/gt/intel_engine_types.h >> index a7e677598004..7585fffac60b 100644 >> --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h >> +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h >> @@ -402,7 +402,15 @@ struct intel_engine_cs { >> unsigned long context_tag; >> - struct rb_node uabi_node; >> + /* >> + * The type evolves during initialization, see related comment for >> + * struct drm_i915_private's uabi_engines member. >> + */ >> + union { >> + struct llist_node uabi_llist; >> + struct list_head uabi_list; >> + struct rb_node uabi_node; >> + }; >> struct intel_sseu sseu; >> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_user.c >> b/drivers/gpu/drm/i915/gt/intel_engine_user.c >> index dcedff41a825..118164ddbb2e 100644 >> --- a/drivers/gpu/drm/i915/gt/intel_engine_user.c >> +++ b/drivers/gpu/drm/i915/gt/intel_engine_user.c >> @@ -38,8 +38,7 @@ intel_engine_lookup_user(struct drm_i915_private >> *i915, u8 class, u8 instance) >> void intel_engine_add_user(struct intel_engine_cs *engine) >> { >> - llist_add((struct llist_node *)&engine->uabi_node, >> - (struct llist_head *)&engine->i915->uabi_engines); >> + llist_add(&engine->uabi_llist, &engine->i915->uabi_engines_llist); >> } >> static const u8 uabi_classes[] = { >> @@ -54,9 +53,9 @@ static int engine_cmp(void *priv, const struct >> list_head *A, >> const struct list_head *B) >> { >> const struct intel_engine_cs *a = >> - container_of((struct rb_node *)A, typeof(*a), uabi_node); >> + container_of(A, typeof(*a), uabi_list); >> const struct intel_engine_cs *b = >> - container_of((struct rb_node *)B, typeof(*b), uabi_node); >> + container_of(B, typeof(*b), uabi_list); >> if (uabi_classes[a->class] < uabi_classes[b->class]) >> return -1; >> @@ -73,7 +72,7 @@ static int engine_cmp(void *priv, const struct >> list_head *A, >> static struct llist_node *get_engines(struct drm_i915_private *i915) >> { >> - return llist_del_all((struct llist_head *)&i915->uabi_engines); >> + return llist_del_all(&i915->uabi_engines_llist); >> } >> static void sort_engines(struct drm_i915_private *i915, >> @@ -83,9 +82,8 @@ static void sort_engines(struct drm_i915_private *i915, >> llist_for_each_safe(pos, next, get_engines(i915)) { >> struct intel_engine_cs *engine = >> - container_of((struct rb_node *)pos, typeof(*engine), >> - uabi_node); >> - list_add((struct list_head *)&engine->uabi_node, engines); >> + container_of(pos, typeof(*engine), uabi_llist); >> + list_add(&engine->uabi_list, engines); >> } >> list_sort(NULL, engines, engine_cmp); >> } >> @@ -213,8 +211,7 @@ void intel_engines_driver_register(struct >> drm_i915_private *i915) >> p = &i915->uabi_engines.rb_node; >> list_for_each_safe(it, next, &engines) { >> struct intel_engine_cs *engine = >> - container_of((struct rb_node *)it, typeof(*engine), >> - uabi_node); >> + container_of(it, typeof(*engine), uabi_list); >> if (intel_gt_has_unrecoverable_error(engine->gt)) >> continue; /* ignore incomplete engines */ >> diff --git a/drivers/gpu/drm/i915/i915_drv.h >> b/drivers/gpu/drm/i915/i915_drv.h >> index 7a8ce7239bc9..c8690d1d5e51 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -222,7 +222,22 @@ struct drm_i915_private { >> bool mchbar_need_disable; >> } gmch; >> - struct rb_root uabi_engines; >> + /* >> + * Chaining user engines happens in multiple stages, starting with a >> + * simple lock-less linked list created by intel_engine_add_user(), >> + * which later gets sorted and converted to an intermediate regular >> + * list, just to be converted once again to its final rb tree >> structure >> + * in intel_engines_driver_register(). >> + * >> + * Make sure to use the right iterator helper, depending on if >> the code >> + * in question runs before or after >> intel_engines_driver_register() -- >> + * for_each_uabi_engine() can only be used afterwards! >> + */ >> + union { >> + struct llist_head uabi_engines_llist; >> + struct list_head uabi_engines_list; >> + struct rb_root uabi_engines; >> + }; >> unsigned int engine_uabi_class_count[I915_LAST_UABI_ENGINE_CLASS >> + 1]; >> /* protects the irq masks */ > > Thanks again! > > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Patches pushed to drm-intel-gt-next. Regards, Tvrtko
On 03.10.23 09:32, Tvrtko Ursulin wrote: > On 29/09/2023 12:00, Tvrtko Ursulin wrote: >> [...] >> Thanks again! >> >> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Patches pushed to drm-intel-gt-next. > Thanks, Tvrtko! I guess this implies no backport of the first patch to older kernels, as it affects v6.3+? Cheers, Mathias
On 04/10/2023 10:52, Mathias Krause wrote: > On 03.10.23 09:32, Tvrtko Ursulin wrote: >> On 29/09/2023 12:00, Tvrtko Ursulin wrote: >>> [...] >>> Thanks again! >>> >>> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> >> Patches pushed to drm-intel-gt-next. >> > > Thanks, Tvrtko! > > I guess this implies no backport of the first patch to older kernels, as > it affects v6.3+? It will land in 6.6+ from purely drm and drm-intel flows but indeed, we forgot about the stable tag. I am however pretty sure it will get picked up nevertheless, since the Fixes: tag is correct, and I think stable maintainers use it from their scripts to identify interesting patches. Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h index a7e677598004..7585fffac60b 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h @@ -402,7 +402,15 @@ struct intel_engine_cs { unsigned long context_tag; - struct rb_node uabi_node; + /* + * The type evolves during initialization, see related comment for + * struct drm_i915_private's uabi_engines member. + */ + union { + struct llist_node uabi_llist; + struct list_head uabi_list; + struct rb_node uabi_node; + }; struct intel_sseu sseu; diff --git a/drivers/gpu/drm/i915/gt/intel_engine_user.c b/drivers/gpu/drm/i915/gt/intel_engine_user.c index dcedff41a825..118164ddbb2e 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_user.c +++ b/drivers/gpu/drm/i915/gt/intel_engine_user.c @@ -38,8 +38,7 @@ intel_engine_lookup_user(struct drm_i915_private *i915, u8 class, u8 instance) void intel_engine_add_user(struct intel_engine_cs *engine) { - llist_add((struct llist_node *)&engine->uabi_node, - (struct llist_head *)&engine->i915->uabi_engines); + llist_add(&engine->uabi_llist, &engine->i915->uabi_engines_llist); } static const u8 uabi_classes[] = { @@ -54,9 +53,9 @@ static int engine_cmp(void *priv, const struct list_head *A, const struct list_head *B) { const struct intel_engine_cs *a = - container_of((struct rb_node *)A, typeof(*a), uabi_node); + container_of(A, typeof(*a), uabi_list); const struct intel_engine_cs *b = - container_of((struct rb_node *)B, typeof(*b), uabi_node); + container_of(B, typeof(*b), uabi_list); if (uabi_classes[a->class] < uabi_classes[b->class]) return -1; @@ -73,7 +72,7 @@ static int engine_cmp(void *priv, const struct list_head *A, static struct llist_node *get_engines(struct drm_i915_private *i915) { - return llist_del_all((struct llist_head *)&i915->uabi_engines); + return llist_del_all(&i915->uabi_engines_llist); } static void sort_engines(struct drm_i915_private *i915, @@ -83,9 +82,8 @@ static void sort_engines(struct drm_i915_private *i915, llist_for_each_safe(pos, next, get_engines(i915)) { struct intel_engine_cs *engine = - container_of((struct rb_node *)pos, typeof(*engine), - uabi_node); - list_add((struct list_head *)&engine->uabi_node, engines); + container_of(pos, typeof(*engine), uabi_llist); + list_add(&engine->uabi_list, engines); } list_sort(NULL, engines, engine_cmp); } @@ -213,8 +211,7 @@ void intel_engines_driver_register(struct drm_i915_private *i915) p = &i915->uabi_engines.rb_node; list_for_each_safe(it, next, &engines) { struct intel_engine_cs *engine = - container_of((struct rb_node *)it, typeof(*engine), - uabi_node); + container_of(it, typeof(*engine), uabi_list); if (intel_gt_has_unrecoverable_error(engine->gt)) continue; /* ignore incomplete engines */ diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 7a8ce7239bc9..c8690d1d5e51 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -222,7 +222,22 @@ struct drm_i915_private { bool mchbar_need_disable; } gmch; - struct rb_root uabi_engines; + /* + * Chaining user engines happens in multiple stages, starting with a + * simple lock-less linked list created by intel_engine_add_user(), + * which later gets sorted and converted to an intermediate regular + * list, just to be converted once again to its final rb tree structure + * in intel_engines_driver_register(). + * + * Make sure to use the right iterator helper, depending on if the code + * in question runs before or after intel_engines_driver_register() -- + * for_each_uabi_engine() can only be used afterwards! + */ + union { + struct llist_head uabi_engines_llist; + struct list_head uabi_engines_list; + struct rb_root uabi_engines; + }; unsigned int engine_uabi_class_count[I915_LAST_UABI_ENGINE_CLASS + 1]; /* protects the irq masks */
Chaining user engines happens in multiple passes during driver initialization, mutating its type along the way. It starts off with a simple lock-less linked list (struct llist_node/head) populated by intel_engine_add_user() which later gets sorted and converted to an intermediate regular list (struct list_head) just to be converted once more to its final rb-tree structure (struct rb_node/root) in intel_engines_driver_register(). All of these types overlay the uabi_node/uabi_engines members which is unfortunate but safe if one takes care about using the rb-tree based structure only after the conversion has completed. However, mistakes happen and commit 1ec23ed7126e ("drm/i915: Use uabi engines for the default engine map") violated that assumption, as the multiple type evolution was all to easy hidden behind casts papering over it. Make the type evolution of uabi_node/uabi_engines more visible by putting all members into an anonymous union and use the correctly typed member in its various users. This allows us to drop quite some ugly casts and, hopefully, make the evolution of the members better recognisable to avoid future mistakes. Signed-off-by: Mathias Krause <minipli@grsecurity.net> --- drivers/gpu/drm/i915/gt/intel_engine_types.h | 10 +++++++++- drivers/gpu/drm/i915/gt/intel_engine_user.c | 17 +++++++---------- drivers/gpu/drm/i915/i915_drv.h | 17 ++++++++++++++++- 3 files changed, 32 insertions(+), 12 deletions(-)