[PATCH] drm/i915/userptr: Acquire the page lock around set_page_dirty()

[PATCH] drm/i915/userptr: Acquire the page lock around set_page_dirty()

[Chris Wilson]

set_page_dirty says:

	For pages with a mapping this should be done under the page lock
	for the benefit of asynchronous memory errors who prefer a
	consistent dirty state. This rule can be broken in some special
	cases, but should be better not to.

	If the mapping doesn't provide a set_page_dirty a_op, then
	just fall through and assume that it wants buffer_heads.

Under those rules, it only safe for us to use the plain set_page_dirty()
calls for shmemfs/anonymous memory. Userptr may be used with real
mappings and so needs to use the locked version (set_page_dirty_lock).

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=203317
Fixes: 5cc9ed4b9a7a ("drm/i915: Introduce mapping of user pages into video memory (userptr) ioctl")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
index 16ccec7fb7da..32d208ede343 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
@@ -665,7 +665,15 @@ i915_gem_userptr_put_pages(struct drm_i915_gem_object *obj,
 
 	for_each_sgt_page(page, sgt_iter, pages) {
 		if (obj->mm.dirty)
-			set_page_dirty(page);
+			/*
+			 * As this may not be anonymous memory (e.g. shmem)
+			 * but exist on a real mapping, we have to lock
+			 * the page in order to dirty it -- holding
+			 * the page reference is not sufficient to
+			 * prevent the inode from being truncated.
+			 * Play safe and take the lock.
+			 */
+			set_page_dirty_lock(page);
 
 		mark_page_accessed(page);
 		put_page(page);
-- 
2.20.1

Re: [Intel-gfx] [PATCH] drm/i915/userptr: Acquire the page lock around set_page_dirty()

[Tvrtko Ursulin]

On 08/07/2019 15:03, Chris Wilson wrote:
> set_page_dirty says:
> 
> 	For pages with a mapping this should be done under the page lock
> 	for the benefit of asynchronous memory errors who prefer a
> 	consistent dirty state. This rule can be broken in some special
> 	cases, but should be better not to.
> 
> 	If the mapping doesn't provide a set_page_dirty a_op, then
> 	just fall through and assume that it wants buffer_heads.
> 
> Under those rules, it only safe for us to use the plain set_page_dirty()
> calls for shmemfs/anonymous memory. Userptr may be used with real
> mappings and so needs to use the locked version (set_page_dirty_lock).
> 
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=203317
> Fixes: 5cc9ed4b9a7a ("drm/i915: Introduce mapping of user pages into video memory (userptr) ioctl")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: stable@vger.kernel.org
> ---
>   drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 10 +++++++++-
>   1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> index 16ccec7fb7da..32d208ede343 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> @@ -665,7 +665,15 @@ i915_gem_userptr_put_pages(struct drm_i915_gem_object *obj,
>   
>   	for_each_sgt_page(page, sgt_iter, pages) {
>   		if (obj->mm.dirty)
> -			set_page_dirty(page);
> +			/*
> +			 * As this may not be anonymous memory (e.g. shmem)
> +			 * but exist on a real mapping, we have to lock
> +			 * the page in order to dirty it -- holding
> +			 * the page reference is not sufficient to
> +			 * prevent the inode from being truncated.
> +			 * Play safe and take the lock.
> +			 */
> +			set_page_dirty_lock(page);
>   
>   		mark_page_accessed(page);
>   		put_page(page);
> 

Not an expert but sounds plausible.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko