Re: [PATCH v2] arm64: Rewrite __arch_clear_user()

[Robin Murphy <robin.murphy@arm.com>]

On 2021-05-26 12:15, Mark Rutland wrote:
> On Fri, May 14, 2021 at 12:57:07PM +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. 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, while
>> also allowing the exception fixup to return an accurate result.
>>
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>>
>> v2: Tweak exception fixup to be exact
>>
>>   arch/arm64/lib/clear_user.S | 44 ++++++++++++++++++++-----------------
>>   1 file changed, 24 insertions(+), 20 deletions(-)
>>
>> diff --git a/arch/arm64/lib/clear_user.S b/arch/arm64/lib/clear_user.S
>> index af9afcbec92c..d30c03f6caed 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
> 
> Could we note the reason for the alignment in the commit message? IIUC
> that's for consistency with the other routines.

Yes, it's alignment for the loop as per the other routines - since the 
prologue (including BTI landing pad) is the same length as we're 
aligning to, then we may as well let any padding fall outside the 
function. I'll comment that in the code, since it might be a bit subtle.

> Regardless, this looks good to me. I hand-executed this for `sz` in the
> range 0 to 7 to make sure the logic and fixups were correct, and that
> all looks good to me. FWIW:
> 
> Reviewed-by: Mark Rutland <mark.rutland@arm.com>

Thanks! I'll clarify the relevant commit messages about the relicensing 
as well and repost the series shortly, unless there are any further 
comments.

Cheers,
Robin.

> 
> Mark.
> 
>>   SYM_FUNC_START(__arch_clear_user)
>> -	mov	x2, x1			// save the size for fixup return
>> +	add	x2, x0, x1
>>   	subs	x1, x1, #8
>>   	b.mi	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
>> +USER(9f, sttr	wzr, [x0])
>> +USER(8f, sttr	wzr, [x2, #-4])
>> +	mov	x0, #0
>> +	ret
>> +
>> +3:	tbz	x1, #1, 4f
>> +USER(9f, sttrh	wzr, [x0])
>> +4:	tbz	x1, #0, 5f
>> +USER(7f, sttrb	wzr, [x2, #-1])
>>   5:	mov	x0, #0
>>   	ret
>>   SYM_FUNC_END(__arch_clear_user)
>> @@ -45,6 +47,8 @@ EXPORT_SYMBOL(__arch_clear_user)
>>   
>>   	.section .fixup,"ax"
>>   	.align	2
>> -9:	mov	x0, x2			// return the original size
>> +7:	sub	x0, x2, #5	// Adjust for faulting on the final byte...
>> +8:	add	x0, x0, #4	// ...or the second word of the 4-7 byte case
>> +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