| Message ID | 20210527162650.1182544-21-jason@jlekstrand.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/gem: ioctl clean-ups (v6) | expand |
On Thu, May 27, 2021 at 11:26:41AM -0500, Jason Ekstrand wrote: > What we really want to check is that size of the engines array, i.e. > args->size - sizeof(*user) is divisible by the element size, i.e. > sizeof(*user->engines) because that's what's required for computing the > array length right below the check. However, we're currently not doing > this and instead doing a compile-time check that sizeof(*user) is > divisible by sizeof(*user->engines) and avoiding the subtraction. As > far as I can tell, the only reason for the more confusing pair of checks > is to avoid a single subtraction of a constant. > > The other thing the BUILD_BUG_ON might be trying to implicitly check is > that offsetof(user->engines) == sizeof(*user) and we don't have any > weird padding throwing us off. However, that's not the check it's doing > and it's not even a reliable way to do that check. > > Signed-off-by: Jason Ekstrand <jason@jlekstrand.net> Yeah a non-dense compiler should be able to figure this out, plus set_engines isn't a hotpath. Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/i915/gem/i915_gem_context.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c > index 12a148ba421b6..cf7c281977a3e 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c > @@ -1758,9 +1758,8 @@ set_engines(struct i915_gem_context *ctx, > goto replace; > } > > - BUILD_BUG_ON(!IS_ALIGNED(sizeof(*user), sizeof(*user->engines))); > if (args->size < sizeof(*user) || > - !IS_ALIGNED(args->size, sizeof(*user->engines))) { > + !IS_ALIGNED(args->size - sizeof(*user), sizeof(*user->engines))) { > drm_dbg(&i915->drm, "Invalid size for engine array: %d\n", > args->size); > return -EINVAL; > -- > 2.31.1 >
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c index 12a148ba421b6..cf7c281977a3e 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c @@ -1758,9 +1758,8 @@ set_engines(struct i915_gem_context *ctx, goto replace; } - BUILD_BUG_ON(!IS_ALIGNED(sizeof(*user), sizeof(*user->engines))); if (args->size < sizeof(*user) || - !IS_ALIGNED(args->size, sizeof(*user->engines))) { + !IS_ALIGNED(args->size - sizeof(*user), sizeof(*user->engines))) { drm_dbg(&i915->drm, "Invalid size for engine array: %d\n", args->size); return -EINVAL;
What we really want to check is that size of the engines array, i.e. args->size - sizeof(*user) is divisible by the element size, i.e. sizeof(*user->engines) because that's what's required for computing the array length right below the check. However, we're currently not doing this and instead doing a compile-time check that sizeof(*user) is divisible by sizeof(*user->engines) and avoiding the subtraction. As far as I can tell, the only reason for the more confusing pair of checks is to avoid a single subtraction of a constant. The other thing the BUILD_BUG_ON might be trying to implicitly check is that offsetof(user->engines) == sizeof(*user) and we don't have any weird padding throwing us off. However, that's not the check it's doing and it's not even a reliable way to do that check. Signed-off-by: Jason Ekstrand <jason@jlekstrand.net> --- drivers/gpu/drm/i915/gem/i915_gem_context.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)