Messages in this thread | | | Date | Tue, 28 Feb 2017 11:22:27 +0000 | From | Abel Vesa <> | Subject | Re: [PATCHv4] arm: ftrace: Adds support for CONFIG_DYNAMIC_FTRACE_WITH_REGS |
| |
On Tue, Feb 28, 2017 at 11:58:49AM +0100, Nicolai Stange wrote: > Hi Abel, > > On Tue, Feb 28 2017, Abel Vesa wrote: > > > On Mon, Feb 27, 2017 at 04:52:06PM +0100, Nicolai Stange wrote: > >> On Fri, Feb 24 2017, Abel Vesa wrote: > >> > >> <snip> > >> > >> > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig > >> > index fda6a46..877df5b 100644 > >> > --- a/arch/arm/Kconfig > >> > +++ b/arch/arm/Kconfig > >> > @@ -55,6 +55,7 @@ config ARM > >> > select HAVE_DMA_API_DEBUG > >> > select HAVE_DMA_CONTIGUOUS if MMU > >> > select HAVE_DYNAMIC_FTRACE if (!XIP_KERNEL) && !CPU_ENDIAN_BE32 && MMU > >> > + select HAVE_DYNAMIC_FTRACE_WITH_REGS if HAVE_DYNAMIC_FTRACE && OLD_MCOUNT > >> > >> > >> AFAICS, your code depends on the __gnu_mcount_nc calling conventions, > >> i.e. on gcc pushing the original lr on the stack. In particular, there's > >> no implementation of a ftrace_regs_caller_old or so. > >> > >> So shouldn't this read as !OLD_MCOUNT instead? > > The implementation of __ftrace_modify_code which sets the kernel text to rw > > needs OLD_MCOUNT (that is, the arch specific one, not the generic one). > > You're right that ARM's implementation of __ftrace_modify_code() is hidden > within an #ifdef CONFIG_OLD_MCOUNT. > > But, > - its implementation doesn't "need" or depend on OLD_MCOUNT > - and it's true in general that the kernel text must be made writable > before ftrace_modify_all_code() attempts to write to it. > > So, I bet that the set_kernel_text_rw()-calling ARM implementations of > arch_ftrace_update_code() and __ftrace_modify_code() resp. have been > inserted under that CONFIG_OLD_MCOUNT #ifdef by mistake with commit > 80d6b0c2eed2 ("ARM: mm: allow text and rodata sections to be > read-only"). > > In conclusion, I claim that DYNAMIC_FTRACE w/o OLD_MCOUNT had been > broken before your patch already. I didn't explicitly test that though. > That is correct. The DYNAMIC_FTRACE w/o OLD_MCOUNT doesn't work. > I think that should be fixed rather than your DYNAMIC_FTRACE_WITH_REGS > pulling in OLD_MCOUNT in order to repair DYNAMIC_FTRACE. > > Especially since your implementation seems to require !OLD_MCOUNT... > So making arch specific __ftrace_modify_code to be OLD_MCOUNT independent might fix DYNAMIC_FTRACE w/o OLD_MCOUNT and implicitly make DYNAMIC_FTRACE_WITH_REGS not dependent on OLD_MCOUNT.
I will investigate this further today. Probably the whole dependancy between FUNCTION_TRACER and OLD_MCOUNT will need to be changed/updated. > >> Also, at least the ldmia ..., {..., sp, ...} insn needs a !THUMB2_KERNEL. > >> > >> > >> > select HAVE_EFFICIENT_UNALIGNED_ACCESS if (CPU_V6 || CPU_V6K || CPU_V7) && MMU > >> > select HAVE_EXIT_THREAD > >> > select HAVE_FTRACE_MCOUNT_RECORD if (!XIP_KERNEL) > >> > >> <snip> > >> > >> > diff --git a/arch/arm/kernel/entry-ftrace.S b/arch/arm/kernel/entry-ftrace.S > >> > index c73c403..3916dd6 100644 > >> > --- a/arch/arm/kernel/entry-ftrace.S > >> > +++ b/arch/arm/kernel/entry-ftrace.S > >> > @@ -92,12 +92,78 @@ > >> > 2: mcount_exit > >> > .endm > >> > > >> > +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS > >> > + > >> > +.macro __ftrace_regs_caller > >> > + > >> > + sub sp, sp, #8 @ space for CPSR and OLD_R0 (not used) > >> > + > >> > + add ip, sp, #12 @ move in IP the value of SP as it was > >> > + @ before the push {lr} of the mcount mechanism > >> > + stmdb sp!, {ip,lr,pc} > >> > + stmdb sp!, {r0-r11,lr} > >> > + > >> > + @ stack content at this point: > >> > + @ 0 4 48 52 56 60 64 68 72 > >> > + @ R0 | R1 | ... | LR | SP + 4 | LR | PC | PSR | OLD_R0 | previous LR | > >> > >> Being a constant, the saved pc is not very useful, I think. > > So you're saying skip it ? But you still need to leave space for it. So why not > > just save it even if the value is not useful? > > No, no, I don't want to skip it. I'd just prefer to have the pt_regs' > ->ARM_lr and ->ARM_pc slots filled with different, perhaps more useful > values: > > >> > >> Wouldn't it be better (and more consistent with other archs) to have > >> > >> pt_regs->ARM_lr = original lr > >> pt_refs->ARM_pc = current lr > >> > >> instead? > > The stack would look like this then > > @ ... | ARM_ip | ARM_sp | ARM_lr | ARM_pc | ... | > @ 0 4 48 52 56 60 64 68 72 > @ R0 | R1 | ... | LR | SP + 4 | original LR | original PC | PSR | OLD_R0 | original LR | > > I.e. the pt_regs would capture almost the full context of the > instrumented function (except for ip). > So basicly what you are saying is: - instead of current LR save original LR (previous one saved in instrumented function epilog) - instead of current PC save original PC (previous one saved in instrumented function epilog)
I still don't see the point of saving the actual value of PC since nobody will ever restore it. In case of livepatch it will get overwritten anyway. As for LR, I agree, it could be the original one in pt_regs.
I'll look into this sometime today or tomorrow and get back with updates. > Thanks, > > Nicolai
Thanks
| |