On 2021-05-12 11:48, Mark Rutland wrote:

On Tue, May 11, 2021 at 05:12:38PM +0100, Robin Murphy wrote:

quoted

Now that we're always using STTR variants rather than abstracting two
different addressing modes, the user_ldst macro here is frankly more
obfuscating than helpful.

FWIW, I completely agree; the user_ldst macros are a historical artifact
and I'm happy to see them go!

quoted

Rewrite __arch_clear_user() with regular
USER() annotations so that it's clearer what's going on, and take the
opportunity to minimise the branchiness in the most common paths, which
also allows the exception fixup to return a more accurate result.

IIUC this isn't always accurate for the {4,2,1}-byte cases; example
below. I'm not sure whether that's intentional since the commit message
says "more accurate" rather than "accurate".

Indeed, the "more" was definitely significant :)

quoted

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
  arch/arm64/lib/clear_user.S | 42 +++++++++++++++++++------------------
  1 file changed, 22 insertions(+), 20 deletions(-)

diff --git a/arch/arm64/lib/clear_user.S b/arch/arm64/lib/clear_user.S
index af9afcbec92c..1005345b4066 100644
--- a/arch/arm64/lib/clear_user.S
+++ b/arch/arm64/lib/clear_user.S

@@ -1,12 +1,9 @@
  /* SPDX-License-Identifier: GPL-2.0-only */
  /*
- * Based on arch/arm/lib/clear_user.S
- *
- * Copyright (C) 2012 ARM Ltd.
+ * Copyright (C) 2021 Arm Ltd.
   */
-#include <linux/linkage.h>
  
-#include <asm/asm-uaccess.h>
+#include <linux/linkage.h>
  #include <asm/assembler.h>
  
  	.text

@@ -19,25 +16,30 @@
   *
   * Alignment fixed up by hardware.
   */
+	.p2align 4
  SYM_FUNC_START(__arch_clear_user)

Say we're called with size in x1 == 0x7

quoted

-	mov	x2, x1			// save the size for fixup return
+	add	x2, x0, x1
  	subs	x1, x1, #8
  	b.mi	2f

... here we'll skip to the 4-byte case at 2f ...

quoted

  1:
-user_ldst 9f, sttr, xzr, x0, 8
+USER(9f, sttr	xzr, [x0])
+	add	x0, x0, #8
  	subs	x1, x1, #8
-	b.pl	1b
-2:	adds	x1, x1, #4
-	b.mi	3f
-user_ldst 9f, sttr, wzr, x0, 4
-	sub	x1, x1, #4
-3:	adds	x1, x1, #2
-	b.mi	4f
-user_ldst 9f, sttrh, wzr, x0, 2
-	sub	x1, x1, #2
-4:	adds	x1, x1, #1
-	b.mi	5f
-user_ldst 9f, sttrb, wzr, x0, 0
+	b.hi	1b
+USER(9f, sttr	xzr, [x2, #-8])
+	mov	x0, #0
+	ret
+
+2:	tbz	x1, #2, 3f

... bit 2 is non-zero, so we continue ...

quoted

+USER(9f, sttr	wzr, [x0])

... and if this faults, the fixup will report the correct address ...

quoted

+USER(9f, sttr	wzr, [x2, #-4])

... but if this faults, teh fixup handler will report that we didn't
copy all 7 bytes, rather than just the last 3, since we didn't update x0
after the first 4-byte STTR.

We could update x0 inline, or add separate fixup handlers to account for
that out-of-line.

If we think that under-estimating is fine, I reckon it'd be worth a
comment to make that clear.

Indeed for smaller amounts there's no change in fixup behaviour at all, 
but I have to assume that underestimating by up to 100% is probably OK 
since we've been underestimating by fully 100% for nearly 10 years now. 
I don't believe it's worth having any more complexity than necessary for 
the fault case - grepping for clear_user() usage suggests that nobody 
really cares about the return value beyond whether it's zero or not, so 
the minor "improvement" here is more of a nice-to-have TBH.

The existing comment doesn't actually explain anything either, which is 
why I didn't replace it, but I'm happy to add something if you like.

Cheers,
Robin.

Thanks,
Mark.

quoted

+	mov	x0, #0
+	ret
+
+3:	tbz	x1, #1, 4f
+USER(9f, sttrh	wzr, [x0])
+4:	tbz	x1, #0, 5f
+USER(9f, sttrb	wzr, [x2, #-1])
  5:	mov	x0, #0
  	ret
  SYM_FUNC_END(__arch_clear_user)

@@ -45,6 +47,6 @@ EXPORT_SYMBOL(__arch_clear_user)
  
  	.section .fixup,"ax"
  	.align	2
-9:	mov	x0, x2			// return the original size
+9:	sub	x0, x2, x0
  	ret
  	.previous

-- 
2.21.0.dirty


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel