[Intel-gfx] [PATCH 2/2] drm/i915: consolidate 2big error checking for object sizes

[Chris Wilson]

Quoting Matthew Auld (2021-01-22 17:35:46)
> Throw it into a simple helper, and throw a warning if we encounter an
> object which has been initialised with an object size that exceeds our
> limit of INT_MAX pages.
> 
> Suggested-by: Chris Wilson <chris at chris-wilson.co.uk>
> Signed-off-by: Matthew Auld <matthew.auld at intel.com>
> ---
>  drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c  |  9 +------
>  drivers/gpu/drm/i915/gem/i915_gem_object.c  |  2 ++
>  drivers/gpu/drm/i915/gem/i915_gem_object.h  | 26 +++++++++++++++++++++
>  drivers/gpu/drm/i915/gem/i915_gem_region.c  | 12 +---------
>  drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 16 +------------
>  5 files changed, 31 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
> index dc11497f830b..5cc8a0b2387f 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
> @@ -244,14 +244,7 @@ struct drm_gem_object *i915_gem_prime_import(struct drm_device *dev,
>                 }
>         }
>  
> -       /*
> -        * XXX: There is a prevalence of the assumption that we fit the
> -        * object's page count inside a 32bit _signed_ variable. Let's document
> -        * this and catch if we ever need to fix it. In the meantime, if you do
> -        * spot such a local variable, please consider fixing!
> -        */
> -
> -       if (dma_buf->size >> PAGE_SHIFT > INT_MAX)
> +       if (i915_gem_object_size_2big(dma_buf->size))
>                 return ERR_PTR(-E2BIG);
>  
>         /* need to attach */
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> index 70f798405f7f..d3702ea8c6aa 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> @@ -62,6 +62,8 @@ void i915_gem_object_init(struct drm_i915_gem_object *obj,
>                           const struct drm_i915_gem_object_ops *ops,
>                           struct lock_class_key *key)
>  {
> +       GEM_CHECK_SIZE_OVERFLOW(obj->base.size);
> +
>         __mutex_init(&obj->mm.lock, ops->name ?: "obj->mm.lock", key);
>  
>         spin_lock_init(&obj->vma.lock);
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h
> index d0ae834d787a..d9cef56533ca 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
> @@ -16,6 +16,32 @@
>  #include "i915_gem_gtt.h"
>  #include "i915_vma_types.h"
>  
> +/*
> + * XXX: There is a prevalence of the assumption that we fit the
> + * object's page count inside a 32bit _signed_ variable. Let's document
> + * this and catch if we ever need to fix it. In the meantime, if you do
> + * spot such a local variable, please consider fixing!
> + *
> + * Aside from our own locals (for which we have no excuse!):
> + * - sg_table embeds unsigned int for num_pages
> + * - get_user_pages*() mixed ints with longs
> + */
> +#define GEM_CHECK_SIZE_OVERFLOW(sz) \
> +       GEM_WARN_ON((sz) >> PAGE_SHIFT > INT_MAX)
> +
> +static inline bool i915_gem_object_size_2big(u64 size)
> +{
> +       struct drm_i915_gem_object *obj;
> +
> +       if (size >> PAGE_SHIFT > INT_MAX)
> +               return true;

Macro here^

Make it easy to grep, everybody gets a warning as a reminder to fix it.
And a prize to whoever does manage to trigger the warning.

> +
> +       if (overflows_type(size, obj->base.size))
> +               return true;
> +
> +       return false;
> +}