Re: [PATCH 8/8] arm64: Rewrite __arch_clear_user()

[Mark Rutland <mark.rutland@arm.com>]

On Tue, May 11, 2021 at 05:12:38PM +0100, Robin Murphy wrote:
> 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!

> 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".

> 
> 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

> -	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 ...

>  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 ...

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

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

> +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.

Thanks,
Mark.

> +	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