| Message ID | 20190107085656.22521-2-joonas.lahtinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm/i915: Prevent a race during I915_GEM_MMAP ioctl with WC set | expand |
Quoting Joonas Lahtinen (2019-01-07 08:56:56) > Add err goto label and use it when VMA can't be established or changes > underneath. > > Fixes: 1816f9236303 ("drm/i915: Support creation of unbound wc user mappings for objects") Dubious. All it changes is one branch where the error is forced to -ENOMEM. > Reported-by: Adam Zabrocki <adamza@microsoft.com> > 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> > --- > 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 f1d594a53978..97cbc0e27e3e 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -1738,6 +1738,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((void *)addr)) > + goto err; > + > if (args->flags & I915_MMAP_WC) { > struct mm_struct *mm = current->mm; > struct vm_area_struct *vma; > @@ -1753,17 +1756,22 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data, > else > addr = -ENOMEM; > up_write(&mm->mmap_sem); > + if (IS_ERR((void *)addr)) > + goto err; The issue still remains that we are returning having called vm_mmap and leaving the vma intact. And we've established above that calling vm_munmap() is a race. -Chris
On 07/01/2019 08:56, Joonas Lahtinen wrote: > Add err goto label and use it when VMA can't be established or changes > underneath. > > Fixes: 1816f9236303 ("drm/i915: Support creation of unbound wc user mappings for objects") > Reported-by: Adam Zabrocki <adamza@microsoft.com> > 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> > --- > 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 f1d594a53978..97cbc0e27e3e 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -1738,6 +1738,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((void *)addr)) > + goto err; > + > if (args->flags & I915_MMAP_WC) { > struct mm_struct *mm = current->mm; > struct vm_area_struct *vma; > @@ -1753,17 +1756,22 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data, > else > addr = -ENOMEM; > up_write(&mm->mmap_sem); > + if (IS_ERR((void *)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 = (uint64_t) addr; > > return 0; > + > +err: > + i915_gem_object_put(obj); > + > + return addr; > } > > static unsigned int tile_row_pages(const struct drm_i915_gem_object *obj) > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index f1d594a53978..97cbc0e27e3e 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -1738,6 +1738,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((void *)addr)) + goto err; + if (args->flags & I915_MMAP_WC) { struct mm_struct *mm = current->mm; struct vm_area_struct *vma; @@ -1753,17 +1756,22 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data, else addr = -ENOMEM; up_write(&mm->mmap_sem); + if (IS_ERR((void *)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 = (uint64_t) addr; return 0; + +err: + i915_gem_object_put(obj); + + return addr; } static unsigned int tile_row_pages(const struct drm_i915_gem_object *obj)
Add err goto label and use it when VMA can't be established or changes underneath. Fixes: 1816f9236303 ("drm/i915: Support creation of unbound wc user mappings for objects") Reported-by: Adam Zabrocki <adamza@microsoft.com> 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> --- drivers/gpu/drm/i915/i915_gem.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)