Quoting Matthew Auld (2021-01-22 17:54:16)
> On Fri, 22 Jan 2021 at 17:43, Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> wrote:
> >
> > 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@xxxxxxxxxxxxxxxxxx>
> > > Signed-off-by: Matthew Auld <matthew.auld@xxxxxxxxx>
> > > ---
> > > 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.
>
> But this code is user triggable, like with igt("create-massive")? What
> am I missing?
GEM_WARN_ON() only emits a warning in debug builds, and we try very hard
not to let that be enabled in production.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
