On Thu, Feb 28, 2019 at 10:01:45PM +0000, Chris Wilson wrote:
> Quoting Guenter Roeck (2019-02-28 21:57:03)
> > 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 <ada...@microsoft.com>
> > > > > Suggested-by: Linus Torvalds <torva...@linux-foundation.org>
> > > > > Signed-off-by: Joonas Lahtinen <joonas.lahti...@linux.intel.com>
> > > > > Cc: <sta...@vger.kernel.org> # v4.0+
> > > > > Cc: Akash Goel <akash.g...@intel.com>
> > > > > Cc: Chris Wilson <ch...@chris-wilson.co.uk>
> > > > > Cc: Tvrtko Ursulin <tvrtko.ursu...@linux.intel.com>
> > > > > Cc: Adam Zabrocki <ada...@microsoft.com>
> > > > > Reviewed-by: Chris Wilson <ch...@chris-wilson.co.uk>
> > > > > Reviewed-by: Tvrtko Ursulin <tvrtko.ursu...@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
>
> Not really, the ABI has never handled less than page-sized requests.
> It's a mistake from the very beginning that it was not rejected as being
> the invalid size it was.
>
> > 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 ?
>
> It's a violation of mmap(2).
>
> Is probably what we will have to do if you ring the regression bell loud
> enough, and do not see the folly of your ways. :-p
I won't ring any bells; I don't play such games. I'll make a local change
in our kernel to fix the problem, quoting your statement that less than
page-sized requests were never supposed to be supported, and add a note
that we'll have to handle this with a local patch going forward.
Guenter
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
