On Mon, 28 Oct 2024 at 15:56, Josh Poimboeuf <jpoim...@kernel.org> wrote:
>
> The barrier_nospec() in 64-bit copy_from_user() is slow. Instead use
> pointer masking to force the user pointer to all 1's if the access_ok()
> mispredicted true for an invalid address.
>
> The kernel test robot reports a 2.6% improvement in the per_thread_ops
> benchmark (see link below).
Hmm. So it strikes me that this still does the "access_ok()", but
that's pointless for the actual pointer masking case. One of the whole
points of the pointer masking is that we can just do this without
actually checking the address (or length) at all.
That's why the strncpy_from_user() has the pattern of
if (can_do_masked_user_access()) {
... don't worry about the size of the address space ..
and I think this code should do that too.
IOW, I think we can do even better than your patch with something
(UNTESTED!) like the attached.
That will also mean that any other architecture that starts doing the
user address masking trick will pick up on this automatically.
Hmm?
Linus
include/linux/uaccess.h | 21 +++++++++++++++------
1 file changed, 15 insertions(+), 6 deletions(-)
diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
index 39c7cf82b0c2..43844510d5d0 100644
--- a/include/linux/uaccess.h
+++ b/include/linux/uaccess.h
@@ -38,6 +38,7 @@
#else
#define can_do_masked_user_access() 0
#define masked_user_access_begin(src) NULL
+ #define mask_user_address(src) (src)
#endif
/*
@@ -159,19 +160,27 @@ _inline_copy_from_user(void *to, const void __user *from, unsigned long n)
{
unsigned long res = n;
might_fault();
- if (!should_fail_usercopy() && likely(access_ok(from, n))) {
+ if (should_fail_usercopy())
+ goto fail;
+ if (can_do_masked_user_access())
+ from = mask_user_address(from);
+ else {
+ if (!access_ok(from, n))
+ goto fail;
/*
* Ensure that bad access_ok() speculation will not
* lead to nasty side effects *after* the copy is
* finished:
*/
barrier_nospec();
- instrument_copy_from_user_before(to, from, n);
- res = raw_copy_from_user(to, from, n);
- instrument_copy_from_user_after(to, from, n, res);
}
- if (unlikely(res))
- memset(to + (n - res), 0, res);
+ instrument_copy_from_user_before(to, from, n);
+ res = raw_copy_from_user(to, from, n);
+ instrument_copy_from_user_after(to, from, n, res);
+ if (likely(!res))
+ return 0;
+fail:
+ memset(to + (n - res), 0, res);
return res;
}
extern __must_check unsigned long
