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@xxxxxxxxxxxxxx>
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@xxxxxxxx>
> ---
> 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
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
