Re: [PATCH 01/13] arm64: lib: __arch_clear_user(): fold fixups into body

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

On Wed, Oct 13, 2021 at 08:55:31PM +0100, Robin Murphy wrote:
> On 2021-10-13 12:00, Mark Rutland wrote:
> > 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...

;)

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