| Message ID | 20170301173928.118028-1-michal.wajdeczko@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 01/03/2017 17:39, Michal Wajdeczko wrote: > Engine related definitions are located in different files > and it is easy to break their cross dependency. > > Additionally use GEM_WARN_ON to catch invalid engine index. > > v2: compare against array size > > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Jani Nikula <jani.nikula@intel.com> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > --- > drivers/gpu/drm/i915/intel_engine_cs.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c > index a238304..c1f58b5 100644 > --- a/drivers/gpu/drm/i915/intel_engine_cs.c > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c > @@ -84,11 +84,16 @@ static const struct engine_info { > > static int > intel_engine_setup(struct drm_i915_private *dev_priv, > - enum intel_engine_id id) > + unsigned int id) > { > const struct engine_info *info = &intel_engines[id]; > struct intel_engine_cs *engine; > > + BUILD_BUG_ON(ARRAY_SIZE(intel_engines) != > + ARRAY_SIZE(dev_priv->engine)); > + if (GEM_WARN_ON(id >= ARRAY_SIZE(intel_engines))) > + return -EINVAL; > + > GEM_BUG_ON(dev_priv->engine[id]); > engine = kzalloc(sizeof(*engine), GFP_KERNEL); > if (!engine) > I would add asserts for id >= ARRAY_SIZE(intel_engines) and id >= ARRAY_SIZE(dev_priv->engine). That provides guarantees for no out of bounds access within this function and should be enough for this function. The rest sounds just like trouble for now. Also I am not sure about negative enum, do we elsewhere check for that class of errors? Is it worth it? Maybe then just cast to unsigned in the above mentioned asserts? Regards, Tvrtko
On Wed, Mar 01, 2017 at 07:29:15PM +0000, Tvrtko Ursulin wrote: > > On 01/03/2017 17:39, Michal Wajdeczko wrote: > > Engine related definitions are located in different files > > and it is easy to break their cross dependency. > > > > Additionally use GEM_WARN_ON to catch invalid engine index. > > > > v2: compare against array size > > > > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > > Cc: Jani Nikula <jani.nikula@intel.com> > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > --- > > drivers/gpu/drm/i915/intel_engine_cs.c | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c > > index a238304..c1f58b5 100644 > > --- a/drivers/gpu/drm/i915/intel_engine_cs.c > > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c > > @@ -84,11 +84,16 @@ static const struct engine_info { > > > > static int > > intel_engine_setup(struct drm_i915_private *dev_priv, > > - enum intel_engine_id id) > > + unsigned int id) > > { > > const struct engine_info *info = &intel_engines[id]; > > struct intel_engine_cs *engine; > > > > + BUILD_BUG_ON(ARRAY_SIZE(intel_engines) != > > + ARRAY_SIZE(dev_priv->engine)); > > + if (GEM_WARN_ON(id >= ARRAY_SIZE(intel_engines))) > > + return -EINVAL; > > + > > GEM_BUG_ON(dev_priv->engine[id]); > > engine = kzalloc(sizeof(*engine), GFP_KERNEL); > > if (!engine) > > > > I would add asserts for id >= ARRAY_SIZE(intel_engines) and id >= > ARRAY_SIZE(dev_priv->engine). That provides guarantees for no out of bounds > access within this function and should be enough for this function. True, but then we will loose early (ie. at build time) detection that our engine array definitions are not in sync (which was primary reason for this patch). > > The rest sounds just like trouble for now. I would not call extra check as a trouble. But if you prefer freedom over robustness, then I will step back. > > Also I am not sure about negative enum, do we elsewhere check for that class > of errors? Is it worth it? Maybe then just cast to unsigned in the above > mentioned asserts? Note that the caller function is not using enum at all, this id/index is defined there as plain "unsigned". Then it is promoted in this function only, where we start to use it as index again (except id assignment). IMHO enums are not the best choice for indexing arrays, and if you really want to do so, you should add some guards to prevent unexpected use. Using cast in these two asserts will do the trick. Let's try this as compromise ;) -Michal
On 01/03/2017 20:07, Michal Wajdeczko wrote: > On Wed, Mar 01, 2017 at 07:29:15PM +0000, Tvrtko Ursulin wrote: >> >> On 01/03/2017 17:39, Michal Wajdeczko wrote: >>> Engine related definitions are located in different files >>> and it is easy to break their cross dependency. >>> >>> Additionally use GEM_WARN_ON to catch invalid engine index. >>> >>> v2: compare against array size >>> >>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> >>> Cc: Jani Nikula <jani.nikula@intel.com> >>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >>> Cc: Chris Wilson <chris@chris-wilson.co.uk> >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>> --- >>> drivers/gpu/drm/i915/intel_engine_cs.c | 7 ++++++- >>> 1 file changed, 6 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c >>> index a238304..c1f58b5 100644 >>> --- a/drivers/gpu/drm/i915/intel_engine_cs.c >>> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c >>> @@ -84,11 +84,16 @@ static const struct engine_info { >>> >>> static int >>> intel_engine_setup(struct drm_i915_private *dev_priv, >>> - enum intel_engine_id id) >>> + unsigned int id) >>> { >>> const struct engine_info *info = &intel_engines[id]; >>> struct intel_engine_cs *engine; >>> >>> + BUILD_BUG_ON(ARRAY_SIZE(intel_engines) != >>> + ARRAY_SIZE(dev_priv->engine)); >>> + if (GEM_WARN_ON(id >= ARRAY_SIZE(intel_engines))) >>> + return -EINVAL; >>> + >>> GEM_BUG_ON(dev_priv->engine[id]); >>> engine = kzalloc(sizeof(*engine), GFP_KERNEL); >>> if (!engine) >>> >> >> I would add asserts for id >= ARRAY_SIZE(intel_engines) and id >= >> ARRAY_SIZE(dev_priv->engine). That provides guarantees for no out of bounds >> access within this function and should be enough for this function. > > True, but then we will loose early (ie. at build time) detection that our > engine array definitions are not in sync (which was primary reason for this > patch). > >> >> The rest sounds just like trouble for now. > > I would not call extra check as a trouble. > But if you prefer freedom over robustness, then I will step back. Lets call it flexibility and pragmatism. :) >> Also I am not sure about negative enum, do we elsewhere check for that class >> of errors? Is it worth it? Maybe then just cast to unsigned in the above >> mentioned asserts? > > Note that the caller function is not using enum at all, this id/index is defined > there as plain "unsigned". Then it is promoted in this function only, where we > start to use it as index again (except id assignment). Maybe we should make the loop in intel_engines_init_early use the id instead of i then... > IMHO enums are not the best choice for indexing arrays, and if you really want > to do so, you should add some guards to prevent unexpected use. ... although it is still a local function with a single call site so doesn't get me too excited. But since I'm usually all for data type tidy so feel free to do it. > Using cast in these two asserts will do the trick. > > Let's try this as compromise ;) Yes I think this compromise is robust enough! ;) Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c index a238304..c1f58b5 100644 --- a/drivers/gpu/drm/i915/intel_engine_cs.c +++ b/drivers/gpu/drm/i915/intel_engine_cs.c @@ -84,11 +84,16 @@ static const struct engine_info { static int intel_engine_setup(struct drm_i915_private *dev_priv, - enum intel_engine_id id) + unsigned int id) { const struct engine_info *info = &intel_engines[id]; struct intel_engine_cs *engine; + BUILD_BUG_ON(ARRAY_SIZE(intel_engines) != + ARRAY_SIZE(dev_priv->engine)); + if (GEM_WARN_ON(id >= ARRAY_SIZE(intel_engines))) + return -EINVAL; + GEM_BUG_ON(dev_priv->engine[id]); engine = kzalloc(sizeof(*engine), GFP_KERNEL); if (!engine)
Engine related definitions are located in different files and it is easy to break their cross dependency. Additionally use GEM_WARN_ON to catch invalid engine index. v2: compare against array size Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> Cc: Jani Nikula <jani.nikula@intel.com> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> --- drivers/gpu/drm/i915/intel_engine_cs.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)