| Message ID | 1459703647-25795-2-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, Apr 03, 2016 at 06:14:06PM +0100, Chris Wilson wrote: > Holding a reference to the containing task_struct is not sufficient to > prevent the mm_struct from being reaped under memory pressure. If this > happens whilst we are calling get_user_pages(), explosions errupt - > sometimes an immediate GPF, sometimes page flag corruption. To prevent > the target mm from being reaped as we are reading from it, acquire a > reference before we begin. > > Testcase: igt/gem_shrink/*userptr > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: Micha? Winiarski <michal.winiarski@intel.com> > Cc: stable@vger.kernel.org Reviewed-by: Micha? Winiarski <michal.winiarski@intel.com> -Micha? > --- > drivers/gpu/drm/i915/i915_gem_userptr.c | 29 +++++++++++++++++------------ > 1 file changed, 17 insertions(+), 12 deletions(-)
On 03/04/16 18:14, Chris Wilson wrote: > Holding a reference to the containing task_struct is not sufficient to > prevent the mm_struct from being reaped under memory pressure. If this > happens whilst we are calling get_user_pages(), explosions errupt - > sometimes an immediate GPF, sometimes page flag corruption. To prevent > the target mm from being reaped as we are reading from it, acquire a > reference before we begin. > > Testcase: igt/gem_shrink/*userptr > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: Micha? Winiarski <michal.winiarski@intel.com> > Cc: stable@vger.kernel.org > --- > drivers/gpu/drm/i915/i915_gem_userptr.c | 29 +++++++++++++++++------------ > 1 file changed, 17 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c > index 92b39186b05a..960bb37f458f 100644 > --- a/drivers/gpu/drm/i915/i915_gem_userptr.c > +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c > @@ -547,19 +547,24 @@ __i915_gem_userptr_get_pages_worker(struct work_struct *_work) > if (pvec != NULL) { > struct mm_struct *mm = obj->userptr.mm->mm; > > - down_read(&mm->mmap_sem); > - while (pinned < npages) { > - ret = get_user_pages_remote(work->task, mm, > - obj->userptr.ptr + pinned * PAGE_SIZE, > - npages - pinned, > - !obj->userptr.read_only, 0, > - pvec + pinned, NULL); > - if (ret < 0) > - break; > - > - pinned += ret; > + ret = -EFAULT; > + if (atomic_inc_not_zero(&mm->mm_users)) { > + down_read(&mm->mmap_sem); > + while (pinned < npages) { > + ret = get_user_pages_remote > + (work->task, mm, > + obj->userptr.ptr + pinned * PAGE_SIZE, > + npages - pinned, > + !obj->userptr.read_only, 0, > + pvec + pinned, NULL); > + if (ret < 0) > + break; > + > + pinned += ret; > + } > + up_read(&mm->mmap_sem); > + mmput(mm); > } > - up_read(&mm->mmap_sem); > } > > mutex_lock(&dev->struct_mutex); > Strange, doesn't this mean that the atomic_inc(¤t->mm->mm_count) is not doing what we thought it would? Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c index 92b39186b05a..960bb37f458f 100644 --- a/drivers/gpu/drm/i915/i915_gem_userptr.c +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c @@ -547,19 +547,24 @@ __i915_gem_userptr_get_pages_worker(struct work_struct *_work) if (pvec != NULL) { struct mm_struct *mm = obj->userptr.mm->mm; - down_read(&mm->mmap_sem); - while (pinned < npages) { - ret = get_user_pages_remote(work->task, mm, - obj->userptr.ptr + pinned * PAGE_SIZE, - npages - pinned, - !obj->userptr.read_only, 0, - pvec + pinned, NULL); - if (ret < 0) - break; - - pinned += ret; + ret = -EFAULT; + if (atomic_inc_not_zero(&mm->mm_users)) { + down_read(&mm->mmap_sem); + while (pinned < npages) { + ret = get_user_pages_remote + (work->task, mm, + obj->userptr.ptr + pinned * PAGE_SIZE, + npages - pinned, + !obj->userptr.read_only, 0, + pvec + pinned, NULL); + if (ret < 0) + break; + + pinned += ret; + } + up_read(&mm->mmap_sem); + mmput(mm); } - up_read(&mm->mmap_sem); } mutex_lock(&dev->struct_mutex);
Holding a reference to the containing task_struct is not sufficient to prevent the mm_struct from being reaped under memory pressure. If this happens whilst we are calling get_user_pages(), explosions errupt - sometimes an immediate GPF, sometimes page flag corruption. To prevent the target mm from being reaped as we are reading from it, acquire a reference before we begin. Testcase: igt/gem_shrink/*userptr Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Cc: Micha? Winiarski <michal.winiarski@intel.com> Cc: stable@vger.kernel.org --- drivers/gpu/drm/i915/i915_gem_userptr.c | 29 +++++++++++++++++------------ 1 file changed, 17 insertions(+), 12 deletions(-)