Date: Wed, 7 Dec 2016 12:10:26 +0000
From: Abel Vesa <abelvesa@gmail.com>
To: Robin Murphy <robin.murphy@arm.com>
Cc: Abel Vesa <abelvesa@linux.com>, linux@armlinux.org.uk,
        jpoimboe@redhat.com, jeyu@redhat.com, jikos@kernel.org, mbenes@suse.cz,
        pmladek@suse.com, linux-arm-kernel@lists.infradead.org,
        geert+renesas@glider.be, ard.biesheuvel@linaro.org,
        jean-philippe.brucker@arm.com, gregkh@linuxfoundation.org,
        stefano.stabellini@eu.citrix.com, emil.l.velikov@gmail.com,
        linux-kernel@vger.kernel.org, rostedt@goodmis.org,
        jens.wiklander@linaro.org, chris.brandt@renesas.com, mingo@redhat.com,
        viro@zeniv.linux.org.uk, live-patching@vger.kernel.org,
        akpm@linux-foundation.org, mchehab@kernel.org, davem@davemloft.net,
        linux@roeck-us.net
Subject: Re: [PATCH 4/7] arm: Add ftrace with regs support
Message-ID: <20161207121026.GB22174@gce>
References: <1481043967-15602-1-git-send-email-abelvesa@linux.com>
 <1481043967-15602-5-git-send-email-abelvesa@linux.com>
 <cf63d647-b5ce-a41b-31ee-6d14da60f541@arm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <cf63d647-b5ce-a41b-31ee-6d14da60f541@arm.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3009
Lines: 105

On Wed, Dec 07, 2016 at 11:58:24AM +0000, Robin Murphy wrote:
> Hi Abel,
> 
> On 06/12/16 17:06, Abel Vesa wrote:
> > This adds __ftrace_regs_caller which, unlike __ftrace_caller,
> > adds register saving/restoring and livepatch handling if
> > the pc register gets modified by klp_ftrace_handler.
> > 
> > Signed-off-by: Abel Vesa <abelvesa@linux.com>
> > ---
> >  arch/arm/kernel/entry-ftrace.S | 49 ++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 49 insertions(+)
> > 
> > diff --git a/arch/arm/kernel/entry-ftrace.S b/arch/arm/kernel/entry-ftrace.S
> > index c73c403..b6ada5c 100644
> > --- a/arch/arm/kernel/entry-ftrace.S
> > +++ b/arch/arm/kernel/entry-ftrace.S
> > @@ -92,6 +92,46 @@
> >  2:	mcount_exit
> >  .endm
> >  
> > +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
> > +
> > +.macro __ftrace_regs_caller suffix
> > +
> > +	stmdb	sp!, {r0-r15}
> 
> As the kbuild robot reported, you can't do this. For ARM, it's unknown
> what the value stored for r13 will be, and for a Thumb-2 kernel the
> whole instruction is flat out unpredictable (i.e. it might just undef or
> anything).
> 
> > +	mov	r3, sp
> > +
> > +	ldr	r10, [sp, #60]
> > +
> > +	mcount_get_lr   r1                      @ lr of instrumented func
> > +	mcount_adjust_addr	r0, lr		@ instrumented function
> > +
> > +	.globl ftrace_regs_call\suffix
> > +ftrace_regs_call\suffix:
> > +	bl	ftrace_stub
> > +
> > +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> > +	.globl ftrace_regs_graph_call\suffix
> > +ftrace_regs_graph_call\suffix:
> > +	mov	r0, r0
> > +#endif
> > +#ifdef CONFIG_LIVEPATCH
> > +	ldr	r0, [sp, #60]
> > +	cmp	r0, r10
> > +	beq	ftrace_regs_caller_end
> > +	ldmia	sp!, {r0-r12}
> > +	add	sp, sp, #8
> > +	ldmia	sp!, {r11}
> > +	sub	sp, r12, #16
> > +	str	r11, [sp, #12]
> > +	ldmia	sp!, {r11, r12, lr, pc}
> > +#endif
> > +ftrace_regs_caller_end\suffix:
> > +	ldmia	sp!, {r0-r14}
> 
> Again, the value of SP at this point is now unknown (regardless of
> whether what the value on memory was correct or not). Not good.
> 
> Either use non-writeback addressing modes, or avoid saving/restoring SP
> at all (AFAICS it isn't necessary, since if the SP was changed in
> between, you then wouldn't know where to restore the saved registers
> from anyway).
> 
> Robin.
>
Yep, as I said in the cover letter, I'll try to fix that in the next
iteration of this patch. You're right, sp doesn't need to be pushed or
popped. I'll send a another patch series which will include a fix for 
that tomorrow, latest.

Thanks. 
> > +	add	sp, sp, #4
> > +	mov	pc, lr
> > +.endm
> > +
> > +#endif
> > +
> >  .macro __ftrace_caller suffix
> >  	mcount_enter
> >  
> > @@ -212,6 +252,15 @@ UNWIND(.fnstart)
> >  	__ftrace_caller
> >  UNWIND(.fnend)
> >  ENDPROC(ftrace_caller)
> > +
> > +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
> > +ENTRY(ftrace_regs_caller)
> > +UNWIND(.fnstart)
> > +	__ftrace_regs_caller
> > +UNWIND(.fnend)
> > +ENDPROC(ftrace_regs_caller)
> > +#endif
> > +
> >  #endif
> >  
> >  #ifdef CONFIG_FUNCTION_GRAPH_TRACER
> > 
>