[Intel-gfx] [PATCH v2 1/2] drm/i915/userptr: add user_size limit check

[Chris Wilson]

Quoting Matthew Auld (2020-01-17 11:51:53)
> On Thu, 16 Jan 2020 at 21:19, Chris Wilson <chris at chris-wilson.co.uk> wrote:
> >
> > Quoting Matthew Auld (2020-01-16 20:31:49)
> > > Don't allow a mismatch between obj->base.size/vma->size and the actual
> > > number of pages for the backing store, which is limited to INT_MAX
> > > pages.
> > >
> > > Signed-off-by: Matthew Auld <matthew.auld at intel.com>
> > > Cc: Chris Wilson <chris at chris-wilson.co.uk>
> > > ---
> > >  drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 12 ++++++++++++
> > >  1 file changed, 12 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> > > index e5558af111e2..fef96a303d9d 100644
> > > --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> > > @@ -768,6 +768,18 @@ i915_gem_userptr_ioctl(struct drm_device *dev,
> > >         if (args->flags & ~(I915_USERPTR_READ_ONLY |
> > >                             I915_USERPTR_UNSYNCHRONIZED))
> > >                 return -EINVAL;
> > > +       /*
> > > +        * 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 (args->user_size >> PAGE_SHIFT > INT_MAX)
> > > +               return -E2BIG;
> >
> > I'm convinced that the following patch is the last bug (excusing
> > i915_gem_internal.c), and think we should commit to removing this limit.
> 
> You mean on our side? There is still all the sg_table stuff,
> __get_user_pages_fast etc.

Didn't notice the get_user_pages -- some use long, sone ints. oops.

sg_table I was thinking of just the sg_length snafu that we work around.
We can kill off sg_table itself as we never pass that outside of the
driver, and just assume our chunking is correct. (Basically lifting more
of lib/scatterlist.c into our control, one day we really should tell
them their code doesn't scale to our use.)

Ok. Let's collate this information into something like

       /*
        * 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
        */

We can send patches for get_user_pages...
-Chris