diff options
| author | Linus Torvalds <torvalds@linux-foundation.org> | 2026-10-21 07:19:30 -1000 |
|---|---|---|
| committer | Linus Torvalds <torvalds@linux-foundation.org> | 2026-10-21 07:19:30 -1000 |
| commit | 22a78a25438d246e4e951960523bee6a5155897d (patch) | |
| tree | a7f0fa921b89128fe082215e995ff8b091f8a07d /drivers/gpu | |
| parent | a804c5742bff224d4cb4d25dd383554d598be32b (diff) | |
| parent | 14b482c98dfe9683b12338561a6e9bd44f7337a9 (diff) | |
Merge __{get,put}_user() removal branch
This is long over-due: the __get_user() and __put_user() functions are
entirely historical, and are never the rigth thing to use any more. And
when mis-used (read: every case) they are not only slower than the right
thing, but also insecure because they don't check the user address
range.
Most of the usage sites have been converted long ago, but various
left-overs remained. So this started out as a single patch that I wrote
as a "show people how to convert __{get,put}_user() to modern code".
In particular, Thomas last year implemented the very nice new scoped
user access region helpers (see commit e497310b4ffb: "uaccess: Provide
scoped user access regions") that makes the modern interfaces
particularly easy to use and really removes any excuse for the old
horrors.
That small example patch then grew to this, partly using Claude as a
glorified coccinelle script to apply the pattern to other places.
There were a few different reasons for using __put_user() and
__get_user() rather than the alternatives, but the original historical
one was performance: when doing multiple accesses off the same base
pointer, doing a single range check followed by multiple non-checked
accesses used to create smaller and faster code.
The keyword here is "used to". It hasn't been true for years, because
the range check isn't actually the real cost these days. The real cost
of user accesses on modern hardware is turning the user accesses on and
off in the first place (on x86, using the STAC/CLAC instructions).
Those sequences get turned into 'scoped_user_read_access() { }' blocks,
see for example that first patch in the series: "signal: use scoped user
access for old-style sigaction".
Another, historical reason for using __get_user() and friends is that
you actually want to do a kernel pointer access, but the pointer might
not be entirely reliable (typically during fault handling), so you want
to have exception handling for the kernel access.
The modern way to do that kind of non-faulting kernel access is using
"get_kernel_nofault()", but some code predates that helper existing at
all, and used __get_user() as a way to just get a memory access with
exception handling. For that pattern, see for example "alpha: use
get_kernel_nofault() in dik_show_code()"
A third bad reason for using __get_user() and friends was that the
proper get_user() had a "might_fault()" in it, and code that ran in
atomic regions but hadn't disabled page faults would result in a
warning. That kind of code needs to explicitly disable page faults,
because "atomic region" is not well defined at run-time and can be
invisible (eg a preempt-off region in a non-preemptible kernel).
See "vdpa_sim: disable page faults around vringh_need_notify_iotlb()"
for an example of that pattern, where the local_bh_disable() now also
grows a pagefault_disable(), which makes the normal get_user() no longer
warn about the user access in non-process context, because it can see
that the potential page fault will not cause problems.
And finally, the really nasty reason to use __get_user() and friends:
pointers that can be either kernel pointers or user pointers. We've
gotten rid of almost all such cases, but a couple remained. See for
example "x86/sev: Use explicit user-or-kernel accessors for #VC memory
operands".
NOTE! This series removes all __get/put_user() calls from generic code
but only does the final removal of the whole interface on x86. Other
architectures have not been entirely converted, for a variety of reasons:
- arm64 makes __{get,put}_user() be exactly the same as
{get,put}_user() so it's harmless and didn't have the dangers of the
legacy use cases.
- any architecture that uses <asm-generic/uaccess.h> will still use
__{get,put}_user as the underlying helper function name
- several architectures had some legacy uses that weren't entirely
trivial and I didn't want to extend this patch series to things that
I don't know and that don't really matter.
but I did do a couple of the obvious "follows the exact same pattern"
just because it could be done mindlessly and automatically.
Let's see if I broke anything, but we should have done this years ago.
* uaccess-cleanups: (48 commits)
drm/i915/gem: ignore errors on presumed_offset write-back
powerpc/align: use get_user_instr() for a user-mode instruction
powerpc/signal64: use scoped user access in setup_trampoline()
powerpc/signal64: use get_user() for the single context reads
powerpc/signal32: use get_user() for the single context reads
KVM: PPC: Book3S HV: use get_user()/put_user() for the HPT fd
powerpc/powernv: use get_user()/put_user() in the LPC debugfs files
s390: use get_user()/put_user() instead of the unchecked variants
ARM: OABI: use scoped user access in epoll_put_uevent()
ARM: signal: stop using __put_user() for the signal frame
ARM: signal: check the FDPIC function descriptor address
alpha: split the prefetch check in do_page_fault() by mode
openrisc: use get_kernel_nofault() in show_registers()
alpha: use get_kernel_nofault() in dik_show_code()
nios2: use scoped user access for the signal frame
nios2: check the signal frame address before writing to it
hexagon: use scoped user access for the signal frame
alpha: use scoped user access when setting up signal frames
alpha: use scoped user access in the sigreturn path
alpha: use scoped user access in osf_sigaction()
...
Diffstat (limited to 'drivers/gpu')
| -rw-r--r-- | drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 63 | ||||
| -rw-r--r-- | drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c | 8 |
2 files changed, 43 insertions, 28 deletions
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c index d5aadced7da0..1b7f5bf13e50 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c @@ -1522,8 +1522,9 @@ static int eb_relocate_vma(struct i915_execbuffer *eb, struct eb_vma *ev) /* * We must check that the entire relocation array is safe - * to read. However, if the array is not writable the user loses - * the updated relocation values. + * to read. Whether it is also writable only shows when we + * write back the presumed offsets below, and if that fails we + * fall back to the slow path. */ if (unlikely(!access_ok(urelocs, remain * sizeof(*urelocs)))) return -EFAULT; @@ -1553,6 +1554,7 @@ static int eb_relocate_vma(struct i915_execbuffer *eb, struct eb_vma *ev) remain -= count; do { u64 offset = eb_relocate_entry(eb, ev, r); + int err; if (likely(offset == 0)) continue; @@ -1562,28 +1564,41 @@ static int eb_relocate_vma(struct i915_execbuffer *eb, struct eb_vma *ev) goto out; } /* - * Note that reporting an error now - * leaves everything in an inconsistent - * state as we have *already* changed - * the relocation value inside the - * object. As we have not changed the - * reloc.presumed_offset or will not - * change the execobject.offset, on the - * call we may not rewrite the value - * inside the object, leaving it - * dangling and causing a GPU hang. Unless - * userspace dynamically rebuilds the - * relocations on each execbuf rather than - * presume a static tree. + * Write back the presumed offset. We hold the + * objects' dma_resv locks here, so this must not + * take a page fault: that would take mmap_lock + * under dma_resv, the opposite of the order our + * own fault handler uses. * - * We did previously check if the relocations - * were writable (access_ok), an error now - * would be a strange race with mprotect, - * having already demonstrated that we - * can read from this userspace address. + * If the write fails (the page is read-only, + * copy-on-write, or gone since we read it), we + * must not just carry on. We have already changed + * the relocation value inside the object, and a + * stale presumed_offset could later match again + * and make us skip a relocation that is needed, + * hanging the GPU. So fall back to the slow path, + * as for a failed read: it retries after + * prefaulting and, failing that, marks every + * presumed_offset invalid with faults allowed and + * no locks held. */ offset = gen8_canonical_addr(offset & ~UPDATE); - __put_user(offset, &urelocs[r - stack].presumed_offset); + pagefault_disable(); + err = put_user(offset, &urelocs[r - stack].presumed_offset); + pagefault_enable(); + + /* + * Error handling is disabled on purpose. No, it makes + * no sense, and the code will loop over the relocations + * without writing them back and just waste CPU time, + * but that's apparently what the mprotect test-cases + * want. See commit 97a37c919f62 ("drm/i915/gem: Ignore + * readonly failures when updating relocs") + */ + if (unlikely(err) && 0) { + remain = -EFAULT; + goto out; + } } while (r++, --count); urelocs += ARRAY_SIZE(stack); } while (remain); @@ -1635,11 +1650,11 @@ static int check_relocations(const struct drm_i915_gem_exec_object2 *entry) end = addr + size; for (; addr < end; addr += PAGE_SIZE) { - int err = __get_user(c, addr); + int err = get_user(c, addr); if (err) return err; } - return __get_user(c, end - 1); + return get_user(c, end - 1); } static int eb_copy_relocations(const struct i915_execbuffer *eb) @@ -2837,7 +2852,7 @@ add_timeline_fence_array(struct i915_execbuffer *eb, if (user_fence.flags & __I915_EXEC_FENCE_UNKNOWN_FLAGS) return -EINVAL; - if (__get_user(point, user_values++)) + if (get_user(point, user_values++)) return -EFAULT; syncobj = drm_syncobj_find(eb->file, user_fence.handle); diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c index d01acfb7d93d..e6748856a0a8 100644 --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c @@ -1465,7 +1465,7 @@ static int __igt_mmap_access(struct drm_i915_private *i915, return addr; ptr = (unsigned long __user *)addr; - err = __put_user(A, ptr); + err = put_user(A, ptr); if (err) { pr_err("%s(%s): failed to write into user mmap\n", obj->mm.region->name, repr_mmap_type(type)); @@ -1490,7 +1490,7 @@ static int __igt_mmap_access(struct drm_i915_private *i915, intel_gt_flush_ggtt_writes(to_gt(i915)); - err = __get_user(y, ptr); + err = get_user(y, ptr); if (err) { pr_err("%s(%s): failed to read from user mmap\n", obj->mm.region->name, repr_mmap_type(type)); @@ -1738,12 +1738,12 @@ static int prefault_range(u64 start, u64 len) end = addr + len; for (; addr < end; addr += PAGE_SIZE) { - err = __get_user(c, addr); + err = get_user(c, addr); if (err) return err; } - return __get_user(c, end - 1); + return get_user(c, end - 1); } static int __igt_mmap_revoke(struct drm_i915_private *i915, |
