This is just an idea I've been trying out for a while now.
Just in case somebody wants to play with it, this applies to linux-arm/for-next.
Also please note that this was only tested in qemu, but I will do some testing
on some real hardware in the following days.
FWICT, on this arch the compiler always generates a function prologue somewhere
between these lines:
e1a0c00d mov ip, sp
e92ddff0 push {r4-r9, sl, fp, ip, lr, pc}
e24cb004 sub fp, ip, #4
e24dd064 sub sp, sp, #100 ; 0x64 <--- local variables
e52de004 push {lr} ; (str lr, [sp, #-4]!)
ebf9c2c9 bl 80110364 <__gnu_mcount_nc>
....
Every function that follows this pattern (the number of registers pushed and the
sp subtraction for the local variables being the only acceptable exception) can
be patched with this mechanism. IIRC, only the inline functions and notrace
functions do not follow this pattern.
Considering that the function is livepatchable, when the time comes to call
ftrace_call, the ftrace_regs_caller is called instead.
Because this arch didn't have a ftrace with regs implementation, the
ftrace_regs_caller was added.
This new function adds the regs saving/restoring part, plus the part necessary
for the livepatch mechanism to work. After the regs are saved and the r3 is set
to contain the sp's value, we're keeping the old pc into r10 in order to be
checked later against the new pc.
Next, the r1 and r0 are set for the ftrace_func, then, the ftrace_stub is called
and the klp_ftrace_handler overwrites the old pc with the new one.
Here comes the tricky part. We're checking if the pc is still the old one, if it
is we jump the whole livepatching and go ahead with restoring the saved regs.
If the pc is modified, it means we're livepatching current function and we need
to pop all regs from r1 through r12, jump over the next two regs saved on stack
(we're not interested in those since we're trying to get the same regs context
as it was at the point the function-to-be-patched was called) and put the new pc
into r11.
Since r12 contains the sp from when the function just got branched to, we need
to set the sp back to that.
Then we need to put the new pc on stack so that when we're popping r11 through
pc, we will actually jump to the first instruction from the new function.
We don't need to worry about the returning phase since the epilogue of the new
function will take care of that and from there on everything goes back to
normal.
The whole advantage of this over adding compiler support is that we're not
introducing nops at the beginning of the function. As a matter of fact, we're
not changing anything between an image with livepatch and an image without it
(except the ftrace_regs_call addition and the livepatch necessary code).
As for the implementation of the ftrace_regs_caller, I still think there might
be some unsafe stack handling since I'm getting some build warnings. Those are
due to pushing/popping of a list of regs in which the sp resides. I'll try to
get around those in a next iteration (if necessary), but first I would like to
hear some opinions about this work and if it's worth going forward.
Everything else should be pretty straightforward, so I'll skip explaining that.
Abel Vesa (7):
arm: Add livepatch arch specific code
arm: ftrace: Add call modify mechanism
arm: module: Add apply_relocate_add
arm: Add ftrace with regs support
arm: ftrace: Add ARCH_SUPPORTS_FTRACE_OPS for ftrace with regs
arm: Add livepatch to build if CONFIG_LIVEPATCH
arm: Add livepatch necessary arch selects into Kconfig
MAINTAINERS | 3 +++
arch/arm/Kconfig | 4 ++++
arch/arm/include/asm/ftrace.h | 4 ++++
arch/arm/include/asm/livepatch.h | 46 +++++++++++++++++++++++++++++++++++++
arch/arm/kernel/Makefile | 1 +
arch/arm/kernel/entry-ftrace.S | 49 ++++++++++++++++++++++++++++++++++++++++
arch/arm/kernel/ftrace.c | 21 +++++++++++++++++
arch/arm/kernel/livepatch.c | 43 +++++++++++++++++++++++++++++++++++
arch/arm/kernel/module.c | 9 ++++++++
9 files changed, 180 insertions(+)
create mode 100644 arch/arm/include/asm/livepatch.h
create mode 100644 arch/arm/kernel/livepatch.c
--
2.7.4
It was only added to fix compiler error. It is not implemented
yet.
Signed-off-by: Abel Vesa <[email protected]>
---
arch/arm/kernel/module.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/arch/arm/kernel/module.c b/arch/arm/kernel/module.c
index 4f14b5c..bf94922 100644
--- a/arch/arm/kernel/module.c
+++ b/arch/arm/kernel/module.c
@@ -52,6 +52,15 @@ void *module_alloc(unsigned long size)
#endif
int
+apply_relocate_add(Elf32_Shdr *sechdrs, const char *strtab,
+ unsigned int symindex, unsigned int relindex,
+ struct module *module)
+{
+ /* Not implemented yet */
+ return 0;
+}
+
+int
apply_relocate(Elf32_Shdr *sechdrs, const char *strtab, unsigned int symindex,
unsigned int relindex, struct module *module)
{
--
2.7.4
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 <[email protected]>
---
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}
+ 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}
+ 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
--
2.7.4
Function ftrace_modify_call provides a way to replace ftrace_stub
with the ftrace function. This helps the klp_ftrace_handler to be
called via ftrace_ops_no_ops, which in turn will set the pc with
the patched function's starting address. This is used for
livepatching.
Signed-off-by: Abel Vesa <[email protected]>
---
arch/arm/kernel/ftrace.c | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/arch/arm/kernel/ftrace.c b/arch/arm/kernel/ftrace.c
index 3f17594..cb4543a 100644
--- a/arch/arm/kernel/ftrace.c
+++ b/arch/arm/kernel/ftrace.c
@@ -139,6 +139,15 @@ int ftrace_update_ftrace_func(ftrace_func_t func)
ret = ftrace_modify_code(pc, 0, new, false);
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+ if (!ret) {
+ pc = (unsigned long)&ftrace_regs_call;
+ new = ftrace_call_replace(pc, (unsigned long)func);
+
+ ret = ftrace_modify_code(pc, 0, new, false);
+ }
+#endif
+
#ifdef CONFIG_OLD_MCOUNT
if (!ret) {
pc = (unsigned long)&ftrace_call_old;
@@ -151,6 +160,18 @@ int ftrace_update_ftrace_func(ftrace_func_t func)
return ret;
}
+int ftrace_modify_call(struct dyn_ftrace *rec, unsigned long old_addr,
+ unsigned long addr)
+{
+ unsigned long pc = rec->ip;
+ u32 old, new;
+
+ old = arm_gen_branch(pc, old_addr);
+ new = arm_gen_branch(pc, addr);
+
+ return ftrace_modify_code(pc, old, new, true);
+}
+
int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
{
unsigned long new, old;
--
2.7.4
Necessary livepatch file added to makefile.
Signed-off-by: Abel Vesa <[email protected]>
---
arch/arm/kernel/Makefile | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
index ad325a8..9e70220 100644
--- a/arch/arm/kernel/Makefile
+++ b/arch/arm/kernel/Makefile
@@ -48,6 +48,7 @@ obj-$(CONFIG_HAVE_ARM_TWD) += smp_twd.o
obj-$(CONFIG_ARM_ARCH_TIMER) += arch_timer.o
obj-$(CONFIG_FUNCTION_TRACER) += entry-ftrace.o
obj-$(CONFIG_DYNAMIC_FTRACE) += ftrace.o insn.o
+obj-$(CONFIG_LIVEPATCH) += livepatch.o
obj-$(CONFIG_FUNCTION_GRAPH_TRACER) += ftrace.o insn.o
obj-$(CONFIG_JUMP_LABEL) += jump_label.o insn.o patch.o
obj-$(CONFIG_KEXEC) += machine_kexec.o relocate_kernel.o
--
2.7.4
ARCH_SUPPORTS_FTRACE_OPS is needed for livepatch if
CONFIG_DYNAMIC_FTRACE_WITH_REGS is defined.
Signed-off-by: Abel Vesa <[email protected]>
---
arch/arm/include/asm/ftrace.h | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/arm/include/asm/ftrace.h b/arch/arm/include/asm/ftrace.h
index bfe2a2f..f434ce9 100644
--- a/arch/arm/include/asm/ftrace.h
+++ b/arch/arm/include/asm/ftrace.h
@@ -1,6 +1,10 @@
#ifndef _ASM_ARM_FTRACE
#define _ASM_ARM_FTRACE
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+#define ARCH_SUPPORTS_FTRACE_OPS 1
+#endif
+
#ifdef CONFIG_FUNCTION_TRACER
#define MCOUNT_ADDR ((unsigned long)(__gnu_mcount_nc))
#define MCOUNT_INSN_SIZE 4 /* sizeof mcount call */
--
2.7.4
This adds HAVE_LIVEPATCH, MODULES_USE_ELF_RELA and HAVE_LIVEPATCH
to arm Kconfig.
Signed-off-by: Abel Vesa <[email protected]>
---
arch/arm/Kconfig | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 186c4c2..f4e9ace 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -50,6 +50,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
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)
@@ -67,6 +68,7 @@ config ARM
select HAVE_KERNEL_XZ
select HAVE_KPROBES if !XIP_KERNEL && !CPU_ENDIAN_BE32 && !CPU_V7M
select HAVE_KRETPROBES if (HAVE_KPROBES)
+ select HAVE_LIVEPATCH
select HAVE_MEMBLOCK
select HAVE_MOD_ARCH_SPECIFIC
select HAVE_NMI
@@ -82,6 +84,7 @@ config ARM
select HAVE_VIRT_CPU_ACCOUNTING_GEN
select IRQ_FORCED_THREADING
select MODULES_USE_ELF_REL
+ select MODULES_USE_ELF_RELA
select NO_BOOTMEM
select OF_EARLY_FLATTREE if OF
select OF_RESERVED_MEM if OF
@@ -1841,6 +1844,7 @@ config XEN
help
Say Y if you want to run Linux in a Virtual Machine on Xen on ARM.
+source "kernel/livepatch/Kconfig"
endmenu
menu "Boot options"
--
2.7.4
klp_get_ftrace_location is used by ftrace to get the entry for a
specific function from the mcount list. klp_arch_set_pc is used
to set the pc from the regs passed as an argument to the
ftrace_ops_no_ops function to the starting address of the patched
function. klp_write_module_reloc is not doing anything at this
moment.
Signed-off-by: Abel Vesa <[email protected]>
---
MAINTAINERS | 3 +++
arch/arm/include/asm/livepatch.h | 46 ++++++++++++++++++++++++++++++++++++++++
arch/arm/kernel/livepatch.c | 43 +++++++++++++++++++++++++++++++++++++
3 files changed, 92 insertions(+)
create mode 100644 arch/arm/include/asm/livepatch.h
create mode 100644 arch/arm/kernel/livepatch.c
diff --git a/MAINTAINERS b/MAINTAINERS
index bd182a1..d43b790 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7466,12 +7466,15 @@ M: Josh Poimboeuf <[email protected]>
M: Jessica Yu <[email protected]>
M: Jiri Kosina <[email protected]>
M: Miroslav Benes <[email protected]>
+M: Abel Vesa <[email protected]>
R: Petr Mladek <[email protected]>
S: Maintained
F: kernel/livepatch/
F: include/linux/livepatch.h
F: arch/x86/include/asm/livepatch.h
F: arch/x86/kernel/livepatch.c
+F: arch/arm/include/asm/livepatch.h
+F: arch/arm/kernel/livepatch.c
F: Documentation/livepatch/
F: Documentation/ABI/testing/sysfs-kernel-livepatch
F: samples/livepatch/
diff --git a/arch/arm/include/asm/livepatch.h b/arch/arm/include/asm/livepatch.h
new file mode 100644
index 0000000..d4e3ff0
--- /dev/null
+++ b/arch/arm/include/asm/livepatch.h
@@ -0,0 +1,46 @@
+/*
+ * livepatch.h - arm specific Kernel Live Patching Core
+ *
+ * Copyright (C) 2016 Abel Vesa <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef _ASM_ARM_LIVEPATCH_H
+#define _ASM_ARM_LIVEPATCH_H
+
+#include <asm/setup.h>
+#include <linux/module.h>
+#include <linux/ftrace.h>
+
+static inline int klp_check_compiler_support(void)
+{
+ return 0;
+}
+
+int klp_write_module_reloc(struct module *mod, unsigned long type,
+ unsigned long loc, unsigned long value);
+
+static inline void klp_arch_set_pc(struct pt_regs *regs, unsigned long ip)
+{
+ regs->uregs[15] = ip;
+}
+
+#define klp_get_ftrace_location klp_get_ftrace_location
+static inline unsigned long klp_get_ftrace_location(unsigned long faddr)
+{
+ return ftrace_location_range(faddr, faddr + 24);
+}
+
+#endif /* _ASM_ARM_LIVEPATCH_H */
diff --git a/arch/arm/kernel/livepatch.c b/arch/arm/kernel/livepatch.c
new file mode 100644
index 0000000..0656cd6
--- /dev/null
+++ b/arch/arm/kernel/livepatch.c
@@ -0,0 +1,43 @@
+/*
+ * livepatch.c - arm specific Kernel Live Patching Core
+ *
+ * Copyright (C) 2016 Abel Vesa <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/module.h>
+#include <linux/uaccess.h>
+#include <linux/ftrace.h>
+#include <asm/elf.h>
+#include <asm/livepatch.h>
+#include <asm/insn.h>
+#include <asm/ftrace.h>
+
+/**
+ * klp_write_module_reloc() - write a relocation in a module
+ * @mod: module in which the section to be modified is found
+ * @type: ELF relocation type (see asm/elf.h)
+ * @loc: address that the relocation should be written to
+ * @value: relocation value (sym address + addend)
+ *
+ * This function writes a relocation to the specified location for
+ * a particular module.
+ */
+int klp_write_module_reloc(struct module *mod, unsigned long type,
+ unsigned long loc, unsigned long value)
+{
+ /* Not implemented yet */
+ return 0;
+}
--
2.7.4
On 2016/12/7 1:06, Abel Vesa wrote:
> This is just an idea I've been trying out for a while now.
>
> Just in case somebody wants to play with it, this applies to linux-arm/for-next.
>
> Also please note that this was only tested in qemu, but I will do some testing
> on some real hardware in the following days.
>
> FWICT, on this arch the compiler always generates a function prologue somewhere
> between these lines:
>
> e1a0c00d mov ip, sp
> e92ddff0 push {r4-r9, sl, fp, ip, lr, pc}
> e24cb004 sub fp, ip, #4
> e24dd064 sub sp, sp, #100 ; 0x64<--- local variables
> e52de004 push {lr} ; (str lr, [sp, #-4]!)
> ebf9c2c9 bl 80110364<__gnu_mcount_nc>
> ....
>
> Every function that follows this pattern (the number of registers pushed and the
> sp subtraction for the local variables being the only acceptable exception) can
> be patched with this mechanism. IIRC, only the inline functions and notrace
> functions do not follow this pattern.
>
> Considering that the function is livepatchable, when the time comes to call
> ftrace_call, the ftrace_regs_caller is called instead.
>
> Because this arch didn't have a ftrace with regs implementation, the
> ftrace_regs_caller was added.
>
> This new function adds the regs saving/restoring part, plus the part necessary
> for the livepatch mechanism to work. After the regs are saved and the r3 is set
> to contain the sp's value, we're keeping the old pc into r10 in order to be
> checked later against the new pc.
>
> Next, the r1 and r0 are set for the ftrace_func, then, the ftrace_stub is called
> and the klp_ftrace_handler overwrites the old pc with the new one.
>
> Here comes the tricky part. We're checking if the pc is still the old one, if it
> is we jump the whole livepatching and go ahead with restoring the saved regs.
>
> If the pc is modified, it means we're livepatching current function and we need
> to pop all regs from r1 through r12, jump over the next two regs saved on stack
> (we're not interested in those since we're trying to get the same regs context
> as it was at the point the function-to-be-patched was called) and put the new pc
> into r11.
>
> Since r12 contains the sp from when the function just got branched to, we need
> to set the sp back to that.
>
> Then we need to put the new pc on stack so that when we're popping r11 through
> pc, we will actually jump to the first instruction from the new function.
>
> We don't need to worry about the returning phase since the epilogue of the new
> function will take care of that and from there on everything goes back to
> normal.
>
> The whole advantage of this over adding compiler support is that we're not
> introducing nops at the beginning of the function. As a matter of fact, we're
> not changing anything between an image with livepatch and an image without it
> (except the ftrace_regs_call addition and the livepatch necessary code).
>
> As for the implementation of the ftrace_regs_caller, I still think there might
> be some unsafe stack handling since I'm getting some build warnings. Those are
> due to pushing/popping of a list of regs in which the sp resides. I'll try to
> get around those in a next iteration (if necessary), but first I would like to
> hear some opinions about this work and if it's worth going forward.
>
Hi, so your idea is that when the pc is modified, we undo the work of the prologue
of the old function, and then jump to the first instruction of the new function.
But I doubt if we can really undo the work of the prologue correctly ? I don't know
about arm, but gcc on arm64 may do some tricky things in prologue. So is there any
chance we may restore a wrong context for the new function ?
Thanks.
> Everything else should be pretty straightforward, so I'll skip explaining that.
>
> Abel Vesa (7):
> arm: Add livepatch arch specific code
> arm: ftrace: Add call modify mechanism
> arm: module: Add apply_relocate_add
> arm: Add ftrace with regs support
> arm: ftrace: Add ARCH_SUPPORTS_FTRACE_OPS for ftrace with regs
> arm: Add livepatch to build if CONFIG_LIVEPATCH
> arm: Add livepatch necessary arch selects into Kconfig
>
> MAINTAINERS | 3 +++
> arch/arm/Kconfig | 4 ++++
> arch/arm/include/asm/ftrace.h | 4 ++++
> arch/arm/include/asm/livepatch.h | 46 +++++++++++++++++++++++++++++++++++++
> arch/arm/kernel/Makefile | 1 +
> arch/arm/kernel/entry-ftrace.S | 49 ++++++++++++++++++++++++++++++++++++++++
> arch/arm/kernel/ftrace.c | 21 +++++++++++++++++
> arch/arm/kernel/livepatch.c | 43 +++++++++++++++++++++++++++++++++++
> arch/arm/kernel/module.c | 9 ++++++++
> 9 files changed, 180 insertions(+)
> create mode 100644 arch/arm/include/asm/livepatch.h
> create mode 100644 arch/arm/kernel/livepatch.c
>
Hi Abel,
[auto build test ERROR on linus/master]
[also build test ERROR on v4.9-rc8 next-20161206]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Abel-Vesa/arm-Add-livepatch-support/20161207-074210
config: arm-simpad_defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm
Note: the linux-review/Abel-Vesa/arm-Add-livepatch-support/20161207-074210 HEAD 49113edc744f38a682a4afa9e904384bb00f2988 builds fine.
It only hurts bisectibility.
All errors (new ones prefixed by >>):
>> arch/arm/kernel/module.c:55:1: error: redefinition of 'apply_relocate_add'
apply_relocate_add(Elf32_Shdr *sechdrs, const char *strtab,
^~~~~~~~~~~~~~~~~~
In file included from arch/arm/kernel/module.c:14:0:
include/linux/moduleloader.h:65:19: note: previous definition of 'apply_relocate_add' was here
static inline int apply_relocate_add(Elf_Shdr *sechdrs,
^~~~~~~~~~~~~~~~~~
vim +/apply_relocate_add +55 arch/arm/kernel/module.c
49 GFP_KERNEL, PAGE_KERNEL_EXEC, 0, NUMA_NO_NODE,
50 __builtin_return_address(0));
51 }
52 #endif
53
54 int
> 55 apply_relocate_add(Elf32_Shdr *sechdrs, const char *strtab,
56 unsigned int symindex, unsigned int relindex,
57 struct module *module)
58 {
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Tue, 6 Dec 2016 17:06:04 +0000
Abel Vesa <[email protected]> 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 <[email protected]>
> ---
> 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}
> + 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
So basically what the below does is to undo the mcount prologue and
recall the new function as the old function is called.
> +#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}
But the above really could do with a lot of comments to explain exactly
what it is doing.
I don't condemn or condone this code. I'm just happy I don't have to
maintain it.
-- Steve
> +#endif
> +ftrace_regs_caller_end\suffix:
> + ldmia sp!, {r0-r14}
> + 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
On Tue, 6 Dec 2016 17:06:07 +0000
Abel Vesa <[email protected]> wrote:
> This adds HAVE_LIVEPATCH, MODULES_USE_ELF_RELA and HAVE_LIVEPATCH
> to arm Kconfig.
>
> Signed-off-by: Abel Vesa <[email protected]>
Patch 5, 6 and 7 really ought to be one patch.
-- Steve
Hi Abel,
[auto build test WARNING on linus/master]
[also build test WARNING on v4.9-rc8 next-20161206]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Abel-Vesa/arm-Add-livepatch-support/20161207-074210
config: arm-allmodconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm
All warnings (new ones prefixed by >>):
arch/arm/kernel/entry-ftrace.S: Assembler messages:
>> arch/arm/kernel/entry-ftrace.S:259: Warning: if writeback register is in list, it must be the lowest reg in the list
>> arch/arm/kernel/entry-ftrace.S:259: Warning: writeback of base register when in register list is UNPREDICTABLE
vim +259 arch/arm/kernel/entry-ftrace.S
82112379 Russell King 2014-10-28 243 #else
82112379 Russell King 2014-10-28 244 __mcount
82112379 Russell King 2014-10-28 245 #endif
82112379 Russell King 2014-10-28 246 UNWIND(.fnend)
82112379 Russell King 2014-10-28 247 ENDPROC(__gnu_mcount_nc)
82112379 Russell King 2014-10-28 248
82112379 Russell King 2014-10-28 249 #ifdef CONFIG_DYNAMIC_FTRACE
82112379 Russell King 2014-10-28 250 ENTRY(ftrace_caller)
82112379 Russell King 2014-10-28 251 UNWIND(.fnstart)
82112379 Russell King 2014-10-28 252 __ftrace_caller
82112379 Russell King 2014-10-28 253 UNWIND(.fnend)
82112379 Russell King 2014-10-28 254 ENDPROC(ftrace_caller)
22cc202f Abel Vesa 2016-12-06 255
22cc202f Abel Vesa 2016-12-06 256 #ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
22cc202f Abel Vesa 2016-12-06 257 ENTRY(ftrace_regs_caller)
22cc202f Abel Vesa 2016-12-06 258 UNWIND(.fnstart)
22cc202f Abel Vesa 2016-12-06 @259 __ftrace_regs_caller
22cc202f Abel Vesa 2016-12-06 260 UNWIND(.fnend)
22cc202f Abel Vesa 2016-12-06 261 ENDPROC(ftrace_regs_caller)
22cc202f Abel Vesa 2016-12-06 262 #endif
22cc202f Abel Vesa 2016-12-06 263
82112379 Russell King 2014-10-28 264 #endif
82112379 Russell King 2014-10-28 265
82112379 Russell King 2014-10-28 266 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
82112379 Russell King 2014-10-28 267 ENTRY(ftrace_graph_caller)
:::::: The code at line 259 was first introduced by commit
:::::: 22cc202f6f168f6987ec2441a19ce1601a296a4e arm: Add ftrace with regs support
:::::: TO: Abel Vesa <[email protected]>
:::::: CC: 0day robot <[email protected]>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Abel,
[auto build test ERROR on linus/master]
[also build test ERROR on v4.9-rc8 next-20161206]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Abel-Vesa/arm-Add-livepatch-support/20161207-074210
config: arm-allmodconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm
Note: the linux-review/Abel-Vesa/arm-Add-livepatch-support/20161207-074210 HEAD 49113edc744f38a682a4afa9e904384bb00f2988 builds fine.
It only hurts bisectibility.
All errors (new ones prefixed by >>):
>> arch/arm/kernel/ftrace.c:163:5: error: redefinition of 'ftrace_modify_call'
int ftrace_modify_call(struct dyn_ftrace *rec, unsigned long old_addr,
^~~~~~~~~~~~~~~~~~
In file included from arch/arm/kernel/ftrace.c:15:0:
include/linux/ftrace.h:595:19: note: previous definition of 'ftrace_modify_call' was here
static inline int ftrace_modify_call(struct dyn_ftrace *rec, unsigned long old_addr,
^~~~~~~~~~~~~~~~~~
vim +/ftrace_modify_call +163 arch/arm/kernel/ftrace.c
157 }
158 #endif
159
160 return ret;
161 }
162
> 163 int ftrace_modify_call(struct dyn_ftrace *rec, unsigned long old_addr,
164 unsigned long addr)
165 {
166 unsigned long pc = rec->ip;
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Tue, Dec 06, 2016 at 05:06:04PM +0000, 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 <[email protected]>
> ---
> 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}
> + 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}
Some comments about the above stack manipulation (in the code) would be
useful - remember, the contents of your cover letter is lost when the
patches are applied.
However, I'm not convinced yet that this doesn't unbalance the stack,
unless the livepatching code pushes some extra registers onto it,
particularly with the following unstacking code after the livepatch
code.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
On Wed, Dec 07, 2016 at 09:38:07AM +0800, zhouchengming wrote:
> On 2016/12/7 1:06, Abel Vesa wrote:
> >This is just an idea I've been trying out for a while now.
> >
> >Just in case somebody wants to play with it, this applies to linux-arm/for-next.
> >
> >Also please note that this was only tested in qemu, but I will do some testing
> >on some real hardware in the following days.
> >
> >FWICT, on this arch the compiler always generates a function prologue somewhere
> >between these lines:
> >
> >e1a0c00d mov ip, sp
> >e92ddff0 push {r4-r9, sl, fp, ip, lr, pc}
> >e24cb004 sub fp, ip, #4
> >e24dd064 sub sp, sp, #100 ; 0x64<--- local variables
> >e52de004 push {lr} ; (str lr, [sp, #-4]!)
> >ebf9c2c9 bl 80110364<__gnu_mcount_nc>
> >....
> >
> >Every function that follows this pattern (the number of registers pushed and the
> >sp subtraction for the local variables being the only acceptable exception) can
> >be patched with this mechanism. IIRC, only the inline functions and notrace
> >functions do not follow this pattern.
> >
> >Considering that the function is livepatchable, when the time comes to call
> >ftrace_call, the ftrace_regs_caller is called instead.
> >
> >Because this arch didn't have a ftrace with regs implementation, the
> >ftrace_regs_caller was added.
> >
> >This new function adds the regs saving/restoring part, plus the part necessary
> >for the livepatch mechanism to work. After the regs are saved and the r3 is set
> >to contain the sp's value, we're keeping the old pc into r10 in order to be
> >checked later against the new pc.
> >
> >Next, the r1 and r0 are set for the ftrace_func, then, the ftrace_stub is called
> >and the klp_ftrace_handler overwrites the old pc with the new one.
> >
> >Here comes the tricky part. We're checking if the pc is still the old one, if it
> >is we jump the whole livepatching and go ahead with restoring the saved regs.
> >
> >If the pc is modified, it means we're livepatching current function and we need
> >to pop all regs from r1 through r12, jump over the next two regs saved on stack
> >(we're not interested in those since we're trying to get the same regs context
> >as it was at the point the function-to-be-patched was called) and put the new pc
> >into r11.
> >
> >Since r12 contains the sp from when the function just got branched to, we need
> >to set the sp back to that.
> >
> >Then we need to put the new pc on stack so that when we're popping r11 through
> >pc, we will actually jump to the first instruction from the new function.
> >
> >We don't need to worry about the returning phase since the epilogue of the new
> >function will take care of that and from there on everything goes back to
> >normal.
> >
> >The whole advantage of this over adding compiler support is that we're not
> >introducing nops at the beginning of the function. As a matter of fact, we're
> >not changing anything between an image with livepatch and an image without it
> >(except the ftrace_regs_call addition and the livepatch necessary code).
> >
> >As for the implementation of the ftrace_regs_caller, I still think there might
> >be some unsafe stack handling since I'm getting some build warnings. Those are
> >due to pushing/popping of a list of regs in which the sp resides. I'll try to
> >get around those in a next iteration (if necessary), but first I would like to
> >hear some opinions about this work and if it's worth going forward.
> >
>
> Hi, so your idea is that when the pc is modified, we undo the work of the prologue
> of the old function, and then jump to the first instruction of the new function.
> But I doubt if we can really undo the work of the prologue correctly ? I don't know
> about arm, but gcc on arm64 may do some tricky things in prologue. So is there any
> chance we may restore a wrong context for the new function ?
>
> Thanks.
>
I forgot to mention that this is actually taking advantage of how this arch deals
with function calling. This mechanism might not be appliable to any other arch
AFAIK. On arm 32bit, as long as the mcount prologue looks like in the shown
example, the function is livepatchable. I will come back with a new version of
this patch today (latest tomorrow) with comments as Russel and Steven mentioned.
I hope that will clarify why this is working on arm 32bit.
> >Everything else should be pretty straightforward, so I'll skip explaining that.
> >
> >Abel Vesa (7):
> > arm: Add livepatch arch specific code
> > arm: ftrace: Add call modify mechanism
> > arm: module: Add apply_relocate_add
> > arm: Add ftrace with regs support
> > arm: ftrace: Add ARCH_SUPPORTS_FTRACE_OPS for ftrace with regs
> > arm: Add livepatch to build if CONFIG_LIVEPATCH
> > arm: Add livepatch necessary arch selects into Kconfig
> >
> > MAINTAINERS | 3 +++
> > arch/arm/Kconfig | 4 ++++
> > arch/arm/include/asm/ftrace.h | 4 ++++
> > arch/arm/include/asm/livepatch.h | 46 +++++++++++++++++++++++++++++++++++++
> > arch/arm/kernel/Makefile | 1 +
> > arch/arm/kernel/entry-ftrace.S | 49 ++++++++++++++++++++++++++++++++++++++++
> > arch/arm/kernel/ftrace.c | 21 +++++++++++++++++
> > arch/arm/kernel/livepatch.c | 43 +++++++++++++++++++++++++++++++++++
> > arch/arm/kernel/module.c | 9 ++++++++
> > 9 files changed, 180 insertions(+)
> > create mode 100644 arch/arm/include/asm/livepatch.h
> > create mode 100644 arch/arm/kernel/livepatch.c
> >
>
>
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 <[email protected]>
> ---
> 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.
> + 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
>
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 <[email protected]>
> > ---
> > 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
> >
>
On Tue 2016-12-06 17:06:06, Abel Vesa wrote:
> Necessary livepatch file added to makefile.
>
> Signed-off-by: Abel Vesa <[email protected]>
> ---
> arch/arm/kernel/Makefile | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
> index ad325a8..9e70220 100644
> --- a/arch/arm/kernel/Makefile
> +++ b/arch/arm/kernel/Makefile
> @@ -48,6 +48,7 @@ obj-$(CONFIG_HAVE_ARM_TWD) += smp_twd.o
> obj-$(CONFIG_ARM_ARCH_TIMER) += arch_timer.o
> obj-$(CONFIG_FUNCTION_TRACER) += entry-ftrace.o
> obj-$(CONFIG_DYNAMIC_FTRACE) += ftrace.o insn.o
> +obj-$(CONFIG_LIVEPATCH) += livepatch.o
It is strange that you add a source file in one patch and make it
build in a much later patch.
I suggest to restructure the entire patchset a bit. Please, first
add support for FTRACE_WITH_REGS. It makes sense on its own.
Then add the livepatch support on top of it.
Otherwise, it is not necessary to send v2 immediately for such
non-trivial code. There might be more people that would want
to look at it and it might take days until they find a time.
It is always better to collect some feedback, think about it
over night(s). Every question often opens many other questions
and it usually takes some time until all settles down into
a good picture again.
Best Regards,
Petr
On Tue 2016-12-06 17:06:00, Abel Vesa wrote:
> This is just an idea I've been trying out for a while now.
>
> Just in case somebody wants to play with it, this applies to linux-arm/for-next.
>
> Also please note that this was only tested in qemu, but I will do some testing
> on some real hardware in the following days.
>
> FWICT, on this arch the compiler always generates a function prologue somewhere
> between these lines:
>
> e1a0c00d mov ip, sp
> e92ddff0 push {r4-r9, sl, fp, ip, lr, pc}
> e24cb004 sub fp, ip, #4
> e24dd064 sub sp, sp, #100 ; 0x64 <--- local variables
> e52de004 push {lr} ; (str lr, [sp, #-4]!)
> ebf9c2c9 bl 80110364 <__gnu_mcount_nc>
> ....
>
> Every function that follows this pattern (the number of registers pushed and the
> sp subtraction for the local variables being the only acceptable exception) can
> be patched with this mechanism. IIRC, only the inline functions and notrace
> functions do not follow this pattern.
Please, where do you check that the given function follows this
pattern? I do not have experience with arm at all. But compiler
is able to do crazy optimizations these days.
I think that this was already mentioned somewhere. But please, put
this detailed explanation also to related patch/code so that it
can later be found in the git commits. It will also help to
better understand/review the particular patches.
Best Regards,
Petr
On Wed, Dec 07, 2016 at 04:05:25PM +0100, Petr Mladek wrote:
> On Tue 2016-12-06 17:06:06, Abel Vesa wrote:
> > Necessary livepatch file added to makefile.
> >
> > Signed-off-by: Abel Vesa <[email protected]>
> > ---
> > arch/arm/kernel/Makefile | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
> > index ad325a8..9e70220 100644
> > --- a/arch/arm/kernel/Makefile
> > +++ b/arch/arm/kernel/Makefile
> > @@ -48,6 +48,7 @@ obj-$(CONFIG_HAVE_ARM_TWD) += smp_twd.o
> > obj-$(CONFIG_ARM_ARCH_TIMER) += arch_timer.o
> > obj-$(CONFIG_FUNCTION_TRACER) += entry-ftrace.o
> > obj-$(CONFIG_DYNAMIC_FTRACE) += ftrace.o insn.o
> > +obj-$(CONFIG_LIVEPATCH) += livepatch.o
>
> It is strange that you add a source file in one patch and make it
> build in a much later patch.
>
> I suggest to restructure the entire patchset a bit. Please, first
> add support for FTRACE_WITH_REGS. It makes sense on its own.
> Then add the livepatch support on top of it.
>
> Otherwise, it is not necessary to send v2 immediately for such
> non-trivial code. There might be more people that would want
> to look at it and it might take days until they find a time.
> It is always better to collect some feedback, think about it
> over night(s). Every question often opens many other questions
> and it usually takes some time until all settles down into
> a good picture again.
>
> Best Regards,
> Petr
You're right, I should send this into two steps. One patchset that
adds FTRACE_WITH_REGS and then a second one that implements the
livepatch and is based on the first one.
Will do that.
Thanks.