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

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

[Joonas Lahtinen]

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(-)

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
-- 
2.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[CI v3 2/2] drm/i915: Handle vm_mmap error during I915_GEM_MMAP ioctl with WC set

[Joonas Lahtinen]

Add err goto label and use it when VMA can't be established or changes
underneath.

v2:
- Dropping Fixes: as it's indeed impossible to race an object to the
  error address. (Chris)
v3:
- Use IS_ERR_VALUE (Chris)

Reported-by: Adam Zabrocki <adamza@microsoft.com>
Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.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: Tvrtko Ursulin <tvrtko.ursulin@intel.com> #v2
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 52639f749908..6728ea5c71d4 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1740,6 +1740,9 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data,
 	addr = vm_mmap(obj->base.filp, 0, args->size,
 		       PROT_READ | PROT_WRITE, MAP_SHARED,
 		       args->offset);
+	if (IS_ERR_VALUE(addr))
+		goto err;
+
 	if (args->flags & I915_MMAP_WC) {
 		struct mm_struct *mm = current->mm;
 		struct vm_area_struct *vma;
@@ -1755,17 +1758,22 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data,
 		else
 			addr = -ENOMEM;
 		up_write(&mm->mmap_sem);
+		if (IS_ERR_VALUE(addr))
+			goto err;
 
 		/* This may race, but that's ok, it only gets set */
 		WRITE_ONCE(obj->frontbuffer_ggtt_origin, ORIGIN_CPU);
 	}
 	i915_gem_object_put(obj);
-	if (IS_ERR((void *)addr))
-		return addr;
 
 	args->addr_ptr = (u64)addr;
 
 	return 0;
+
+err:
+	i915_gem_object_put(obj);
+
+	return addr;
 }
 
 static unsigned int tile_row_pages(const struct drm_i915_gem_object *obj)
-- 
2.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for series starting with [CI,v3,1/2] drm/i915: Prevent a race during I915_GEM_MMAP ioctl with WC set

[Patchwork]

== Series Details ==

Series: series starting with [CI,v3,1/2] drm/i915: Prevent a race during I915_GEM_MMAP ioctl with WC set
URL   : https://patchwork.freedesktop.org/series/56334/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5557 -> Patchwork_12167
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/56334/revisions/1/mbox/

Known issues
------------

  Here are the changes found in Patchwork_12167 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_suspend@basic-s3:
    - fi-kbl-7567u:       PASS -> DMESG-WARN [fdo#108566]

  * igt@i915_selftest@live_hangcheck:
    - fi-kbl-7560u:       PASS -> INCOMPLETE [fdo#108044] / [fdo#108744]

  * igt@kms_frontbuffer_tracking@basic:
    - fi-hsw-peppy:       NOTRUN -> DMESG-FAIL [fdo#102614]

  
#### Possible fixes ####

  * igt@kms_busy@basic-flip-b:
    - fi-gdg-551:         FAIL [fdo#103182] -> PASS

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
    - fi-blb-e6850:       INCOMPLETE [fdo#107718] -> PASS

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#102614]: https://bugs.freedesktop.org/show_bug.cgi?id=102614
  [fdo#103182]: https://bugs.freedesktop.org/show_bug.cgi?id=103182
  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
  [fdo#108044]: https://bugs.freedesktop.org/show_bug.cgi?id=108044
  [fdo#108566]: https://bugs.freedesktop.org/show_bug.cgi?id=108566
  [fdo#108744]: https://bugs.freedesktop.org/show_bug.cgi?id=108744
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109527]: https://bugs.freedesktop.org/show_bug.cgi?id=109527


Participating hosts (47 -> 44)
------------------------------

  Additional (3): fi-skl-guc fi-skl-6770hq fi-hsw-peppy 
  Missing    (6): fi-kbl-soraka fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-icl-y fi-bdw-samus 


Build changes
-------------

    * Linux: CI_DRM_5557 -> Patchwork_12167

  CI_DRM_5557: 72302b1c5245655423f75a857aec82f037991b6f @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4812: 592b854fead32c2b0dac7198edfb9a6bffd66932 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_12167: 5c7622375d17ae7b60b426b326e6456290c54e5e @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

5c7622375d17 drm/i915: Handle vm_mmap error during I915_GEM_MMAP ioctl with WC set
cef98c4a8c36 drm/i915: Prevent a race during I915_GEM_MMAP ioctl with WC set

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12167/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for series starting with [CI,v3,1/2] drm/i915: Prevent a race during I915_GEM_MMAP ioctl with WC set

[Patchwork]

== Series Details ==

Series: series starting with [CI,v3,1/2] drm/i915: Prevent a race during I915_GEM_MMAP ioctl with WC set
URL   : https://patchwork.freedesktop.org/series/56334/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5557_full -> Patchwork_12167_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

Known issues
------------

  Here are the changes found in Patchwork_12167_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_ctx_isolation@rcs0-dirty-switch:
    - shard-apl:          PASS -> INCOMPLETE [fdo#103927]

  * igt@gem_exec_schedule@pi-ringfull-bsd2:
    - shard-kbl:          NOTRUN -> FAIL [fdo#103158] +2

  * igt@gem_mmap_gtt@hang:
    - shard-kbl:          NOTRUN -> FAIL [fdo#109469]

  * igt@kms_busy@extended-modeset-hang-newfb-render-b:
    - shard-kbl:          NOTRUN -> DMESG-WARN [fdo#107956] +4

  * igt@kms_busy@extended-modeset-hang-newfb-with-reset-render-b:
    - shard-snb:          NOTRUN -> DMESG-WARN [fdo#107956]

  * igt@kms_busy@extended-pageflip-hang-newfb-render-a:
    - shard-apl:          NOTRUN -> DMESG-WARN [fdo#107956]

  * igt@kms_color@pipe-a-degamma:
    - shard-kbl:          NOTRUN -> FAIL [fdo#104782] / [fdo#108145]

  * igt@kms_content_protection@atomic:
    - shard-kbl:          NOTRUN -> FAIL [fdo#108597] +1

  * igt@kms_cursor_crc@cursor-128x128-dpms:
    - shard-kbl:          NOTRUN -> FAIL [fdo#103232]

  * igt@kms_cursor_crc@cursor-128x42-random:
    - shard-glk:          PASS -> FAIL [fdo#103232]

  * igt@kms_cursor_crc@cursor-64x21-random:
    - shard-apl:          PASS -> FAIL [fdo#103232] +1

  * igt@kms_cursor_crc@cursor-64x64-suspend:
    - shard-kbl:          NOTRUN -> FAIL [fdo#103191] / [fdo#103232]

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-pwrite:
    - shard-apl:          PASS -> FAIL [fdo#103167]

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-fullscreen:
    - shard-kbl:          NOTRUN -> FAIL [fdo#103167]

  * igt@kms_frontbuffer_tracking@fbc-2p-primscrn-spr-indfb-draw-mmap-cpu:
    - shard-glk:          PASS -> FAIL [fdo#103167] +1

  * igt@kms_frontbuffer_tracking@fbc-suspend:
    - shard-glk:          PASS -> INCOMPLETE [fdo#103359] / [k.org#198133]

  * igt@kms_plane@plane-position-covered-pipe-c-planes:
    - shard-glk:          PASS -> FAIL [fdo#103166] +2

  * igt@kms_plane_alpha_blend@pipe-a-constant-alpha-max:
    - shard-apl:          NOTRUN -> FAIL [fdo#108145]

  * igt@kms_plane_alpha_blend@pipe-c-alpha-7efc:
    - shard-kbl:          NOTRUN -> FAIL [fdo#108145] / [fdo#108590] +3

  * igt@kms_plane_alpha_blend@pipe-c-alpha-opaque-fb:
    - shard-kbl:          NOTRUN -> FAIL [fdo#108145] +4

  * igt@kms_universal_plane@universal-plane-pipe-a-functional:
    - shard-apl:          PASS -> FAIL [fdo#103166]

  * igt@pm_rpm@system-suspend:
    - shard-kbl:          NOTRUN -> INCOMPLETE [fdo#103665] / [fdo#107807]

  
#### Possible fixes ####

  * igt@gem_exec_blt@cold-min:
    - shard-glk:          DMESG-WARN [fdo#105763] / [fdo#106538] -> PASS

  * igt@i915_selftest@live_hangcheck:
    - shard-snb:          INCOMPLETE [fdo#105411] -> PASS

  * igt@kms_cursor_crc@cursor-256x85-random:
    - shard-apl:          FAIL [fdo#103232] -> PASS +1

  * igt@kms_cursor_crc@cursor-size-change:
    - shard-glk:          FAIL [fdo#103232] -> PASS +3

  * igt@kms_flip@basic-plain-flip:
    - shard-hsw:          DMESG-WARN [fdo#102614] -> PASS

  * igt@kms_frontbuffer_tracking@fbc-1p-offscren-pri-indfb-draw-blt:
    - shard-snb:          {SKIP} [fdo#109271] -> PASS +5

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-move:
    - shard-glk:          FAIL [fdo#103167] -> PASS +2

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-onoff:
    - shard-apl:          FAIL [fdo#103167] -> PASS

  * igt@kms_plane@pixel-format-pipe-b-planes-source-clamping:
    - shard-apl:          FAIL [fdo#108948] -> PASS

  * igt@kms_plane@plane-position-covered-pipe-c-planes:
    - shard-apl:          FAIL [fdo#103166] -> PASS +1

  * igt@kms_plane_alpha_blend@pipe-a-alpha-opaque-fb:
    - shard-apl:          FAIL [fdo#108145] -> PASS

  * igt@kms_plane_multiple@atomic-pipe-a-tiling-y:
    - shard-glk:          FAIL [fdo#103166] -> PASS

  * igt@kms_setmode@basic:
    - shard-apl:          FAIL [fdo#99912] -> PASS

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#102614]: https://bugs.freedesktop.org/show_bug.cgi?id=102614
  [fdo#103158]: https://bugs.freedesktop.org/show_bug.cgi?id=103158
  [fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
  [fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232
  [fdo#103359]: https://bugs.freedesktop.org/show_bug.cgi?id=103359
  [fdo#103665]: https://bugs.freedesktop.org/show_bug.cgi?id=103665
  [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
  [fdo#104782]: https://bugs.freedesktop.org/show_bug.cgi?id=104782
  [fdo#105411]: https://bugs.freedesktop.org/show_bug.cgi?id=105411
  [fdo#105763]: https://bugs.freedesktop.org/show_bug.cgi?id=105763
  [fdo#106538]: https://bugs.freedesktop.org/show_bug.cgi?id=106538
  [fdo#107807]: https://bugs.freedesktop.org/show_bug.cgi?id=107807
  [fdo#107956]: https://bugs.freedesktop.org/show_bug.cgi?id=107956
  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#108590]: https://bugs.freedesktop.org/show_bug.cgi?id=108590
  [fdo#108597]: https://bugs.freedesktop.org/show_bug.cgi?id=108597
  [fdo#108948]: https://bugs.freedesktop.org/show_bug.cgi?id=108948
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#109469]: https://bugs.freedesktop.org/show_bug.cgi?id=109469
  [fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912
  [k.org#198133]: https://bugzilla.kernel.org/show_bug.cgi?id=198133


Participating hosts (7 -> 5)
------------------------------

  Missing    (2): shard-skl shard-iclb 


Build changes
-------------

    * Linux: CI_DRM_5557 -> Patchwork_12167

  CI_DRM_5557: 72302b1c5245655423f75a857aec82f037991b6f @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4812: 592b854fead32c2b0dac7198edfb9a6bffd66932 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_12167: 5c7622375d17ae7b60b426b326e6456290c54e5e @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12167/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Guenter Roeck]

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

Re: [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 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

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

[Chris Wilson]

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

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

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

[Chris Wilson]

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

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

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

[Chris Wilson]

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

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

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

[Chris Wilson]

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

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