Re: [PATCH] arm64: vdso: Fix CFI info in sigreturn.

From: Will Deacon <will@kernel.org>
To: Dave Martin <Dave.Martin@arm.com>
Cc: Mark Rutland <Mark.Rutland@arm.com>,
	Tamas Zsoldos <Tamas.Zsoldos@arm.com>,
	Vincenzo Frascino <Vincenzo.Frascino@arm.com>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>,
	Daniel Kiss <Daniel.Kiss@arm.com>
Subject: Re: [PATCH] arm64: vdso: Fix CFI info in sigreturn.
Date: Tue, 19 May 2020 12:34:13 +0100	[thread overview]
Message-ID: <20200519113412.GA13430@willie-the-truck> (raw)
In-Reply-To: <20200519092934.GC5031@arm.com>

On Tue, May 19, 2020 at 10:29:37AM +0100, Dave Martin wrote:
> On Mon, May 18, 2020 at 05:00:32PM +0000, Daniel Kiss wrote:
> > >> diff --git a/arch/arm64/kernel/vdso/sigreturn.S b/arch/arm64/kernel/vdso/sigreturn.S
> > >> index 12324863d5c2..5d50ee92faa4 100644
> > >> --- a/arch/arm64/kernel/vdso/sigreturn.S
> > >> +++ b/arch/arm64/kernel/vdso/sigreturn.S
> > >> @@ -13,13 +13,13 @@
> > >> 
> > >> 	.text
> > >> 
> > >> -	nop
> > >> -SYM_FUNC_START(__kernel_rt_sigreturn)
> > >> 	.cfi_startproc
> > >> 	.cfi_signal_frame
> > >> 	.cfi_def_cfa	x29, 0
> > >> 	.cfi_offset	x29, 0 * 8
> > >> 	.cfi_offset	x30, 1 * 8
> > > 
> > > Hmm, recovering x29,x30 like this will be wrong if the signal handler
> > > munges sigcontext in the meantime (say, doing some kind of userspace
> > > context switch).
> > > 
> > > They should be pulled out of sigcontext instead really.  AFAIK, that's
> > > what ".cfi_signal_frame" is supposed to tell the unwinder.  I'm not sure
> > > why we have these additional, conflicting annotations here.

I think we should just remove the .cfi_def_cfa and .cfi_offset directives
from this sequence. Daniel, Tamas, is that ok with you?

> > The unwinder won’t find the “cfi_signal_frame” until it figures out the unwind entry.
> > 
> > > Any ideas, Will?
> > > 
> > > This probably isn't related to the bug here, but it would be good to
> > > understand.
> > > 
> > >> +	nop  /* placeholder for bl signalhandler */
> > > 
> > > Will can correct me on this, but I seem to remember something about nop
> > > being there for padding, so that there is a guaranteed gap between
> > > unwind entries.

I think it is to do with unwinding, where something was subtracting 1
from the return address to identify the caller. There's an old relic here
that sheds a bit of light on it all (we inherited this from ppc, it seems):

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=26208#c15

However, looking at gdb just now, it looks like it tries to match the
instructions in the trampoline in order to identify a signal frame. We just
changed that with the BTI patches, so I think it's now broken. Given that we
don't need the BTI C in there (we only call the thing via RET), I'll send a
patch to remove it.

Will

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