[Intel-gfx] [CI, v3, 1/2] drm/i915: Prevent a race during I915_GEM_MMAP ioctl with WC set

[Guenter Roeck]

On Thu, Feb 28, 2019 at 01:32:41PM -0800, Guenter Roeck wrote:
> On Thu, Feb 28, 2019 at 11:12:49AM -0800, Guenter Roeck wrote:
> > Hi,
> > 
> > On Thu, Feb 07, 2019 at 10:54:53AM +0200, Joonas Lahtinen wrote:
> > > Make sure the underlying VMA in the process address space is the
> > > same as it was during vm_mmap to avoid applying WC to wrong VMA.
> > > 
> > > A more long-term solution would be to have vm_mmap_locked variant
> > > in linux/mmap.h for when caller wants to hold mmap_sem for an
> > > extended duration.
> > > 
> > 
> > It seems like we may have a regression due to this patch. I am still
> > debugging, but I have a question; please see below.
> > 
> > Thanks,
> > Guenter
> > 
> > > v2:
> > > - Refactor the compare function
> > > 
> > > Fixes: 1816f9236303 ("drm/i915: Support creation of unbound wc user mappings for objects")
> > > Reported-by: Adam Zabrocki <adamza at microsoft.com>
> > > Suggested-by: Linus Torvalds <torvalds at linux-foundation.org>
> > > Signed-off-by: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> > > Cc: <stable at vger.kernel.org> # v4.0+
> > > Cc: Akash Goel <akash.goel at intel.com>
> > > Cc: Chris Wilson <chris at chris-wilson.co.uk>
> > > Cc: Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>
> > > Cc: Adam Zabrocki <adamza at microsoft.com>
> > > Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>
> > > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com> #v1
> > > ---
> > >  drivers/gpu/drm/i915/i915_gem.c | 12 +++++++++++-
> > >  1 file changed, 11 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > > index 05ce9176ac4e..52639f749908 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > > @@ -1681,6 +1681,16 @@ i915_gem_sw_finish_ioctl(struct drm_device *dev, void *data,
> > >  	return 0;
> > >  }
> > >  
> > > +static inline bool
> > > +__vma_matches(struct vm_area_struct *vma, struct file *filp,
> > > +	      unsigned long addr, unsigned long size)
> > > +{
> > > +	if (vma->vm_file != filp)
> > > +		return false;
> > > +
> > > +	return vma->vm_start == addr && (vma->vm_end - vma->vm_start) == size;
> > 
> > Shouldn't this be:
> > 	return vma->vm_start == addr && (vma->vm_end - vma->vm_start + 1) == size;
> > instead ?
> > 
> 
> Answer is no .. because vm_end points to the first byte after the
> end address.
> 
> The actual values are:
> 
> start=7d288f7f9000 end=7d288f84d000 end-start=54000 size=53400
> 
> meaning the size field passed in the ioctl is smaller than the total length
> of the area.
> 
> Question is now: Is the request/ioctl indeed invalid, ie does the requested
> size have to match the vma size ? This used to work until this patch was
> applied, and the change causes our test code to fail (and possibly minigbm,
> which is used by the test code). That doesn't mean that our code is correct
> (I see some related local changes in our version of minigbm), but it is
> annoying, and I am being asked to revert this patch as regression
> from our kernel releases.
> 

In i915_gem_create():

	size = roundup(size, PAGE_SIZE);
	if (size == 0)
		return -EINVAL;

This suggests to me that the requested size can be smaller than the
allocated size, which in turn suggests that the check
	(vma->vm_end - vma->vm_start) == size;
is wrong. Either it should be
	(vma->vm_end - vma->vm_start) >= size;
or possibly
	(vma->vm_end - vma->vm_start) == roundup(size, PAGE_SIZE);

Any comments/feedback/thoughts ?

Thanks,
Guenter

> Thanks,
> Guenter
> 
> > > +}
> > > +
> > >  /**
> > >   * i915_gem_mmap_ioctl - Maps the contents of an object, returning the address
> > >   *			 it is mapped to.
> > > @@ -1739,7 +1749,7 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data,
> > >  			return -EINTR;
> > >  		}
> > >  		vma = find_vma(mm, addr);
> > > -		if (vma)
> > > +		if (vma && __vma_matches(vma, obj->base.filp, addr, args->size))
> > >  			vma->vm_page_prot =
> > >  				pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
> > >  		else