| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <e3ef8af7-f3fb-a6aa-9838-a90b6d5516ac@gmail.com>
Date: Sat, 16 Jan 2021 05:11:16 +0000
From: Jinoh Kang <jinoh.kang.kr@...il.com>
To: Chris Wilson <chris@...is-wilson.co.uk>,
Marek Marczykowski-Górecki
<marmarek@...isiblethingslab.com>
Cc: Jani Nikula <jani.nikula@...ux.intel.com>,
Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>,
Rodrigo Vivi <rodrigo.vivi@...el.com>,
David Airlie <airlied@...ux.ie>,
Daniel Vetter <daniel@...ll.ch>,
Matthew Auld <matthew.auld@...el.com>,
intel-gfx@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drm/i915/userptr: detect un-GUP-able pages early
On 1/15/21 5:07 PM, Chris Wilson wrote:
> Quoting Chris Wilson (2021-01-15 16:56:42)
>> Quoting Jinoh Kang (2021-01-15 16:23:31)
>>> If GUP-ineligible pages are passed to a GEM userptr object, -EFAULT is
>>> returned only when the object is actually bound.
>>>
>>> The xf86-video-intel userspace driver cannot differentiate this
>>> condition, and marks the GPU as wedged.
>>
>> The idea was to call gem_set_domain on the object to validate the pages
>> after creation. I only did that for read-only... I did however make mesa
>> use set-domain for validation.
>
> Hmm, I remember a reason why we wanted to be lazy in populating the
> pages was that we would often be called to create a map that was never
> used. So the additional gup + bind was measurable, and we would rather
> just have a probe.
> -Chris
>
try_upload__blt uses the map immediately, so I guess that would be an
appropriate place to patch.
> Basically XShmAttachFd, which is not exposed on libX11.
To clarify: privcmd pages cannot actually be passed by fd, since it's
tightly bound with current->mm. There's some sysv shmop hooking hack
involved, which is injected in Xorg side.
---
Besides, is there an equivalent code path that lets you eagerly *unpin*
pages when closing an userptr object without waiting for the worker?
This is actually more of a problem in drivers/xen/grant-table.c side,
since it hangs the current task until all page refs are released, and
it didn't even use ZONE_DEVICE pages until recently (while still not
using dev_pagemap_ops::page_free, since the unpopulated-alloc pgmap
is not MEMORY_DEVICE_FS_DAX apparently).
(You could say that we should switch to DMA-BUF instead and that would
be a valid criticism. I'm merely figuring out what the best workaround
for the current status quo would be.)
I'm using something like the following:
---
drivers/gpu/drm/i915/gem/i915_gem_object.c | 8 ++++++++
drivers/gpu/drm/i915/gem/i915_gem_object_types.h | 1 +
drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 3 ++-
drivers/gpu/drm/i915/i915_params.c | 3 +++
drivers/gpu/drm/i915/i915_params.h | 1 +
5 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c
index 00d24000b5e8..4352a5788fd8 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
@@ -167,6 +167,14 @@ static void i915_gem_close_object(struct drm_gem_object *gem, struct drm_file *f
i915_lut_handle_free(lut);
i915_gem_object_put(obj);
}
+
+ if (i915_modparams.gem_userptr_close_immediate &&
+ i915_gem_object_type_has(obj, I915_GEM_OBJECT_IMM_RELEASE) &&
+ i915_gem_object_is_shrinkable(obj) &&
+ !atomic_read(&obj->mm.shrink_pin) &&
+ i915_gem_object_unbind(obj, I915_GEM_OBJECT_UNBIND_ACTIVE |
+ I915_GEM_OBJECT_UNBIND_TEST) == 0)
+ __i915_gem_object_put_pages(obj);
}
static void __i915_gem_free_object_rcu(struct rcu_head *head)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
index e2d9b7e1e152..0ac1dfed0b91 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
@@ -36,6 +36,7 @@ struct drm_i915_gem_object_ops {
#define I915_GEM_OBJECT_IS_PROXY BIT(3)
#define I915_GEM_OBJECT_NO_MMAP BIT(4)
#define I915_GEM_OBJECT_ASYNC_CANCEL BIT(5)
+#define I915_GEM_OBJECT_IMM_RELEASE BIT(7)
/* Interface between the GEM object and its backing storage.
* get_pages() is called once prior to the use of the associated set
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
index f2eaed6aca3d..baa91daf43a1 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
@@ -705,7 +705,8 @@ static const struct drm_i915_gem_object_ops i915_gem_userptr_ops = {
.flags = I915_GEM_OBJECT_HAS_STRUCT_PAGE |
I915_GEM_OBJECT_IS_SHRINKABLE |
I915_GEM_OBJECT_NO_MMAP |
- I915_GEM_OBJECT_ASYNC_CANCEL,
+ I915_GEM_OBJECT_ASYNC_CANCEL |
+ I915_GEM_OBJECT_IMM_RELEASE,
.get_pages = i915_gem_userptr_get_pages,
.put_pages = i915_gem_userptr_put_pages,
.dmabuf_export = i915_gem_userptr_dmabuf_export,
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index 7f139ea4a90b..4d056fd1b6e7 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -197,6 +197,9 @@ i915_param_named_unsafe(fake_lmem_start, ulong, 0400,
"Fake LMEM start offset (default: 0)");
#endif
+i915_param_named(gem_userptr_close_immediate, bool, 0600,
+ "Immediately release pages when userptr GEM object is closed (default:false)");
+
static __always_inline void _print_param(struct drm_printer *p,
const char *name,
const char *type,
diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
index 330c03e2b4f7..a94367a0345b 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -79,6 +79,7 @@ struct drm_printer;
param(bool, disable_display, false, 0400) \
param(bool, verbose_state_checks, true, 0) \
param(bool, nuclear_pageflip, false, 0400) \
+ param(bool, gem_userptr_close_immediate, false, 0600) \
param(bool, enable_dp_mst, true, 0600) \
param(bool, enable_gvt, false, 0400)
--
Sincerely,
Jinoh Kang
Powered by blists - more mailing lists