diff mbox series

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

Message ID 20190708140327.26825-1-chris@chris-wilson.co.uk (mailing list archive)
State New, archived
Headers show
Series drm/i915/userptr: Acquire the page lock around set_page_dirty() | expand

Commit Message

Chris Wilson July 8, 2019, 2:03 p.m. UTC 
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(-)

Comments

Sasha Levin July 9, 2019, 12:45 a.m. UTC | #1 
Hi,

[This is an automated email]

This commit has been processed because it contains a "Fixes:" tag,
fixing commit: 5cc9ed4b9a7a drm/i915: Introduce mapping of user pages into video memory (userptr) ioctl.

The bot has tested the following trees: v5.1.16, v4.19.57, v4.14.132, v4.9.184, v4.4.184.

v5.1.16: Build OK!
v4.19.57: Build OK!
v4.14.132: Build OK!
v4.9.184: Failed to apply! Possible dependencies:
    0e70447605f4 ("drm/i915: Move common code out of i915_gpu_error.c")
    1b36595ffb35 ("drm/i915: Show RING registers through debugfs")
    275f039db56f ("drm/i915: Move user fault tracking to a separate list")
    3594a3e21f1f ("drm/i915: Remove superfluous locking around userfault_list")
    3b3f1650b1ca ("drm/i915: Allocate intel_engine_cs structure only for the enabled engines")
    7c108fd8feac ("drm/i915: Move fence cancellation to runtime suspend")
    8baa1f04b9ed ("drm/i915: Update debugfs describe_obj() to show fault-mappable")
    96d776345277 ("drm/i915: Use a radixtree for random access to the object's backing storage")
    9c870d03674f ("drm/i915: Use RPM as the barrier for controlling user mmap access")
    a4f5ea64f0a8 ("drm/i915: Refactor object page API")
    d636951ec01b ("drm/i915: Cleanup instdone collection")
    f8a7fde45610 ("drm/i915: Defer active reference until required")

v4.4.184: Failed to apply! Possible dependencies:
    09cbfeaf1a5a ("mm, fs: get rid of PAGE_CACHE_* and page_cache_{get,release} macros")
    0a798eb92e6d ("drm/i915: Refactor duplicate object vmap functions")
    0b5372727be3 ("drm/i915/cmdparser: Use cached vmappings")
    0e749e54244e ("dax: increase granularity of dax_clear_blocks() operations")
    0eb973d31d0a ("drm/i915: Cache ringbuffer GTT VMA")
    43394c7d0d36 ("drm/i915: Extract i915_gem_obj_prepare_shmem_write()")
    4420cfd3f51c ("staging: lustre: format properly all comment blocks for LNet core")
    52db400fcd50 ("pmem, dax: clean up clear_pmem()")
    5fd88337d209 ("staging: lustre: fix all conditional comparison to zero in LNet layer")
    85d1225ec066 ("drm/i915: Introduce & use new lightweight SGL iterators")
    a188222b6ed2 ("net: Rename NETIF_F_ALL_CSUM to NETIF_F_CSUM_MASK")
    a4f5ea64f0a8 ("drm/i915: Refactor object page API")
    b2e0d1625e19 ("dax: fix lifetime of in-kernel dax mappings with dax_map_atomic()")
    b9bcd14a2b91 ("drm/i915: Extract checking for backing struct pages to a helper")
    d1a5f2b4d8a1 ("block: use DAX for partition table reads")
    def0c5f6b0cd ("drm/i915: Map the ringbuffer using WB on LLC machines")
    e10624f8c097 ("pmem: fail io-requests to known bad blocks")


NOTE: The patch will not be queued to stable trees until it is upstream.

How should we proceed with this patch?

--
Thanks,
Sasha
Tvrtko Ursulin July 9, 2019, 6:24 a.m. UTC | #2 
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
diff mbox series

Patch

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