* [PATCH v2] arm64: Simplify __range_ok
@ 2020-03-27 3:09 Richard Henderson
2020-04-03 10:50 ` Mark Rutland
0 siblings, 1 reply; 2+ messages in thread
From: Richard Henderson @ 2020-03-27 3:09 UTC (permalink / raw)
To: linux-arm-kernel; +Cc: mark.rutland, robin.murphy
The general case is not quite as compact as the inline assembly,
but with a sufficiently advanced compiler it is only 6 insns vs 5.
The real improvement comes from assuming that limit is never tiny,
and using __builtin_constant_p to make sure the constant folding
does not go awry. This produces a 2 insn sequence even for older
compilers.
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
Thanks for the v1 review, Mark.
Text section size change:
$ scripts/bloat-o-meter -t vmlinux{-orig,} | tail -1
* When built with gcc-10 (master) + cmpti patch set:
Total: Before=12820824, After=12774708, chg -0.36%
* When built with gcc-7.5.0-3ubuntu1~18.04:
Total: Before=13564038, After=13513258, chg -0.37%
Changes in v2:
* Adjust initialization of iaddr.
* Use USER_DS as the constant limit and update commentary.
r~
---
arch/arm64/include/asm/uaccess.h | 32 +++++++++++++-------------------
1 file changed, 13 insertions(+), 19 deletions(-)
diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
index 32fc8061aa76..04ef201e6179 100644
--- a/arch/arm64/include/asm/uaccess.h
+++ b/arch/arm64/include/asm/uaccess.h
@@ -60,7 +60,8 @@ static inline void set_fs(mm_segment_t fs)
*/
static inline unsigned long __range_ok(const void __user *addr, unsigned long size)
{
- unsigned long ret, limit = current_thread_info()->addr_limit;
+ unsigned long limit = current_thread_info()->addr_limit;
+ unsigned long iaddr = (unsigned long)addr;
/*
* Asynchronous I/O running in a kernel thread does not have the
@@ -72,24 +73,17 @@ static inline unsigned long __range_ok(const void __user *addr, unsigned long si
addr = untagged_addr(addr);
__chk_user_ptr(addr);
- asm volatile(
- // A + B <= C + 1 for all A,B,C, in four easy steps:
- // 1: X = A + B; X' = X % 2^64
- " adds %0, %3, %2\n"
- // 2: Set C = 0 if X > 2^64, to guarantee X' > C in step 4
- " csel %1, xzr, %1, hi\n"
- // 3: Set X' = ~0 if X >= 2^64. For X == 2^64, this decrements X'
- // to compensate for the carry flag being set in step 4. For
- // X > 2^64, X' merely has to remain nonzero, which it does.
- " csinv %0, %0, xzr, cc\n"
- // 4: For X < 2^64, this gives us X' - C - 1 <= 0, where the -1
- // comes from the carry in being clear. Otherwise, we are
- // testing X' - C == 0, subject to the previous adjustments.
- " sbcs xzr, %0, %1\n"
- " cset %0, ls\n"
- : "=&r" (ret), "+r" (limit) : "Ir" (size), "0" (addr) : "cc");
-
- return ret;
+
+ /*
+ * The minimum value for limit is USER_DS, and quite a lot of
+ * range checks use sizeof(some_type). With both constants,
+ * we can rearrange the computation to avoid the need for
+ * 65-bit arithmetic.
+ */
+ if (__builtin_constant_p(size) && size > 0 && size < USER_DS)
+ return iaddr <= limit + 1 - size;
+
+ return (__uint128_t)iaddr + size <= (__uint128_t)limit + 1;
}
#define access_ok(addr, size) __range_ok(addr, size)
--
2.17.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH v2] arm64: Simplify __range_ok
2020-03-27 3:09 [PATCH v2] arm64: Simplify __range_ok Richard Henderson
@ 2020-04-03 10:50 ` Mark Rutland
0 siblings, 0 replies; 2+ messages in thread
From: Mark Rutland @ 2020-04-03 10:50 UTC (permalink / raw)
To: Richard Henderson; +Cc: robin.murphy, linux-arm-kernel
Hi Richard,
On Thu, Mar 26, 2020 at 08:09:18PM -0700, Richard Henderson wrote:
> The general case is not quite as compact as the inline assembly,
> but with a sufficiently advanced compiler it is only 6 insns vs 5.
>
> The real improvement comes from assuming that limit is never tiny,
> and using __builtin_constant_p to make sure the constant folding
> does not go awry. This produces a 2 insn sequence even for older
> compilers.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
Thanks for this; it looks good to me, minor comments below.
I believe the const cases capture all realistic contsnt sizes, and
handles those correctly. We miss a couple of constant cases that we
could catch (and might want to do so for completeness) but either way I
think this looks good.
> ---
>
> Thanks for the v1 review, Mark.
>
> Text section size change:
>
> $ scripts/bloat-o-meter -t vmlinux{-orig,} | tail -1
>
> * When built with gcc-10 (master) + cmpti patch set:
> Total: Before=12820824, After=12774708, chg -0.36%
>
> * When built with gcc-7.5.0-3ubuntu1~18.04:
> Total: Before=13564038, After=13513258, chg -0.37%
>
> Changes in v2:
> * Adjust initialization of iaddr.
> * Use USER_DS as the constant limit and update commentary.
>
>
> r~
>
> ---
> arch/arm64/include/asm/uaccess.h | 32 +++++++++++++-------------------
> 1 file changed, 13 insertions(+), 19 deletions(-)
>
> diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
> index 32fc8061aa76..04ef201e6179 100644
> --- a/arch/arm64/include/asm/uaccess.h
> +++ b/arch/arm64/include/asm/uaccess.h
> @@ -60,7 +60,8 @@ static inline void set_fs(mm_segment_t fs)
> */
> static inline unsigned long __range_ok(const void __user *addr, unsigned long size)
> {
> - unsigned long ret, limit = current_thread_info()->addr_limit;
> + unsigned long limit = current_thread_info()->addr_limit;
> + unsigned long iaddr = (unsigned long)addr;
>
> /*
> * Asynchronous I/O running in a kernel thread does not have the
> @@ -72,24 +73,17 @@ static inline unsigned long __range_ok(const void __user *addr, unsigned long si
> addr = untagged_addr(addr);
>
> __chk_user_ptr(addr);
> - asm volatile(
> - // A + B <= C + 1 for all A,B,C, in four easy steps:
> - // 1: X = A + B; X' = X % 2^64
> - " adds %0, %3, %2\n"
> - // 2: Set C = 0 if X > 2^64, to guarantee X' > C in step 4
> - " csel %1, xzr, %1, hi\n"
> - // 3: Set X' = ~0 if X >= 2^64. For X == 2^64, this decrements X'
> - // to compensate for the carry flag being set in step 4. For
> - // X > 2^64, X' merely has to remain nonzero, which it does.
> - " csinv %0, %0, xzr, cc\n"
> - // 4: For X < 2^64, this gives us X' - C - 1 <= 0, where the -1
> - // comes from the carry in being clear. Otherwise, we are
> - // testing X' - C == 0, subject to the previous adjustments.
> - " sbcs xzr, %0, %1\n"
> - " cset %0, ls\n"
> - : "=&r" (ret), "+r" (limit) : "Ir" (size), "0" (addr) : "cc");
> -
> - return ret;
> +
> + /*
> + * The minimum value for limit is USER_DS, and quite a lot of
> + * range checks use sizeof(some_type). With both constants,
> + * we can rearrange the computation to avoid the need for
> + * 65-bit arithmetic.
> + */
> + if (__builtin_constant_p(size) && size > 0 && size < USER_DS)
> + return iaddr <= limit + 1 - size;
As above, I believe this can be:
if (__builtin_constant_p(size) && size > 0 && size <= USER_DS + 1)
return iaddr <= limit + 1 - size;
... but either way I think this is sound, and that's just for symmetry
of the test cases.
> +
> + return (__uint128_t)iaddr + size <= (__uint128_t)limit + 1;
I believe this is obviously correct.
So with or without the change suggested above:
Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Mark.
> }
>
> #define access_ok(addr, size) __range_ok(addr, size)
> --
> 2.17.1
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2020-04-03 10:50 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-27 3:09 [PATCH v2] arm64: Simplify __range_ok Richard Henderson
2020-04-03 10:50 ` Mark Rutland
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).