| Message ID | 20190207085454.10598-1-joonas.lahtinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [CI,v3,1/2] drm/i915: Prevent a race during I915_GEM_MMAP ioctl with WC set | expand |
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 ? > +} > + > /** > * 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
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
Quoting Guenter Roeck (2019-02-28 21:32:41) > 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 ? Yes. The vma is page-aligned, your request isn't. What happens next is undefined behaviour, and almost certainly not what you expect -- you can't access the last bits of your framebuffer. -Chris
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@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. > 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
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 <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. > > > > 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 -Chris
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 <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. > > > > > > > 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
Quoting Guenter Roeck (2019-02-28 22:11:51) > 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 <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. > > > > > > > > > > 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. If you have userspace that is broken, we need to fix it. We can get away with quietly changing ABI only so long as nobody notices. It sounds like you have some userspace that will break if you updated the kernel; ergo we have a problem. We just need that as a clear statement so that we have the user impact recorded. -Chris
On Thu, Feb 28, 2019 at 10:18:32PM +0000, Chris Wilson wrote: > > If you have userspace that is broken, we need to fix it. We can get away > with quietly changing ABI only so long as nobody notices. It sounds like > you have some userspace that will break if you updated the kernel; ergo > we have a problem. > > We just need that as a clear statement so that we have the user impact > recorded. Yes, it does break our userspace code. I fixed the problem in our kernels with the following patch: - return vma->vm_start == addr && (vma->vm_end - vma->vm_start) == size; + return vma->vm_start == addr && (vma->vm_end - vma->vm_start) == roundup(size, PAGE_SIZE); Guenter
Quoting Guenter Roeck (2019-02-28 22:27:35) > On Thu, Feb 28, 2019 at 10:18:32PM +0000, Chris Wilson wrote: > > > > If you have userspace that is broken, we need to fix it. We can get away > > with quietly changing ABI only so long as nobody notices. It sounds like > > you have some userspace that will break if you updated the kernel; ergo > > we have a problem. > > > > We just need that as a clear statement so that we have the user impact > > recorded. > > Yes, it does break our userspace code. I fixed the problem in our kernels > with the following patch: > And what userspace is that? Details for the log, please. -Chris
On Thu, Feb 28, 2019 at 10:34:20PM +0000, Chris Wilson wrote: > Quoting Guenter Roeck (2019-02-28 22:27:35) > > On Thu, Feb 28, 2019 at 10:18:32PM +0000, Chris Wilson wrote: > > > > > > If you have userspace that is broken, we need to fix it. We can get away > > > with quietly changing ABI only so long as nobody notices. It sounds like > > > you have some userspace that will break if you updated the kernel; ergo > > > we have a problem. > > > > > > We just need that as a clear statement so that we have the user impact > > > recorded. > > > > Yes, it does break our userspace code. I fixed the problem in our kernels > > with the following patch: > > > And what userspace is that? Details for the log, please. Internal agreement is that we'll fix our userspace code. Guenter
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; +} + /** * 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
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. 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(-)