On Wed, Oct 13, 2021 at 08:55:31PM +0100, Robin Murphy wrote:

On 2021-10-13 12:00, Mark Rutland wrote:

quoted

Like other functions, __arch_clear_user() places its exception fixups in
the `.fixup` section without any clear association with
__arch_clear_user() itself. If we backtrace the fixup code, it will be
symbolized as an offset from the nearest prior symbol, which happens to
be `__entry_tramp_text_end`. Further, since the PC adjustment for the
fixup is akin to a direct branch rather than a function call,
__arch_clear_user() itself will be missing from the backtrace.

This is confusing and hinders debugging. In general this pattern will
also be problematic for CONFIG_LIVEPATCH, since fixups often return to
their associated function, but this isn't accurately captured in the
stacktrace.

To solve these issues for assembly functions, we must move fixups into
the body of the functions themselves, after the usual fast-path returns.
This patch does so for __arch_clear_user().

Inline assembly will be dealt with in subsequent patches.

Other than the improved backtracing, there should be no functional
change as a result of this patch.

Oh, I always assumed the .fixup section might have some special significance
of its own. If not, all the better - modulo one possible nit below, this is
fine by me.

Cheers, I'll go fix those up.

Also it's led me to see that copy_in_user() has finally left us,
hooray! Guess I've got no more excuses to put off that promised usercopy
rewrite other than finding the time now...

;)

quoted

diff --git a/arch/arm64/lib/clear_user.S b/arch/arm64/lib/clear_user.S
index a7efb2ad2a1c..dac13df4a1ed 100644
--- a/arch/arm64/lib/clear_user.S
+++ b/arch/arm64/lib/clear_user.S

@@ -45,13 +45,10 @@ USER(9f, sttrh	wzr, [x0])
  USER(7f, sttrb	wzr, [x2, #-1])
  5:	mov	x0, #0
  	ret
-SYM_FUNC_END(__arch_clear_user)
-EXPORT_SYMBOL(__arch_clear_user)
-	.section .fixup,"ax"

The one useful purpose this did serve is to provide a handy visual cue - if
you have any more concrete reason to respin the series, would you mind
sticking in a little comment like "// Exception fixup" here and in the other
two assembly routines, just so it's harder to overlook that the preceding
ret is the normal exit path?

I've added:

| // Exception fixups

... to the start of the fixups in patches 1-3.

Either way, for patches 1-3,

Acked-by: Robin Murphy <robin.murphy@arm.com>

Thanks!
 

I got totally lost trying to follow the _ASM_EXTABLE_UACCESS_* business, but
I don't think the rest of the series gets in the way of any outstanding
plans either (and FWIW I always thought "fixup->fixup" was pretty awful so
feel free to have an ack for patch #9 as well).

Thanks again!

Indeed, I guess the new type field should mean that we can implement
"proper" address-aware fault handlers without the Itanium trick if we
still need to.

Yes -- my thinking was that we can capture the address register(s) and
offset(s) in the data field, and *also* provide the ESR and/or FAR if
necessary in the exception handler.

What's the Itanium trick?

Thanks,
Mark.

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