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

[Guenter Roeck <linux@roeck-us.net>]

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@microsoft.com>
> > Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> > Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Cc: <stable@vger.kernel.org> # v4.0+
> > Cc: Akash Goel <akash.goel@intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> > Cc: Adam Zabrocki <adamza@microsoft.com>
> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@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.

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx